Merge lp:~macslow/qtmir/fix-1475678 into lp:qtmir

[Mirco Müller]

Added hook to allow MirSurfaceManager to mark MirSurfaceItems visible/non-visible.

Added hook to allow MirSurfaceManager to mark MirSurfaceItems visible/non-visible.

[Gerry Boland (gerboland)]

m_surface->visible()
this isn't doing what you'd expect. Looking into the Mir code: src/server/scene/basic_surface.cpp:

bool ms::BasicSurface::visible() const
{
    std::unique_lock<std::mutex> lk(guard);
    return visible(lk);
}

bool ms::BasicSurface::visible(std::unique_lock<std::mutex>&) const
{
    bool visible{false};
    for (auto const& info : layers)
        visible |= info.stream->has_submitted_buffer();
    return !hidden && visible;
}

This is a place where the integration of QtMir and Mir isn't so good. Mir kinda assumes that we're using its Scene, and thus its compositor. But we're not. Our Scene is defined in QML, and our compositor is the Qt scenegraph renderer.

BasicSurface::visible() defines if the surface is visible in Mir's scene. It doesn't notify the client that its surface is exposed or not.

So instead you should ignore this, and read the surface attribute to learn if the client thinks it is exposed or not.

[Gerry Boland (gerboland)]

I'm unclear why you're introducing "visibility" - do you want to distinguish QML's visible state from the client's exposure state?

I also don't see the point of an enum for it, it's just true/false.

[Gerry Boland (gerboland)]

void MirSurfaceManager::displayOff()
void MirSurfaceManager::displayOn()

I disapprove of this design. A surface manager should not care about display state. It's the shell's decision if a client surface should be marked invisible when the display is off.

Also this throws away any visibility state that the shell may have set on surfaces.

A remote desktop would be an example where we don't care about the actual display state.

[lp:~macslow/qtmir/fix-1475678]

349. By Mirco Müller on 2015-07-31

Just use visible as a term instead of visibility for MirSurfaceItem and map that to mir::scene::surface's visibility-attribute.

[Mirco Müller (macslow)]

> ...
> So instead you should ignore this, and read the surface attribute to learn if
> the client thinks it is exposed or not.

This has been addressed.

[Mirco Müller (macslow)]

> I'm unclear why you're introducing "visibility" - do you want to distinguish
> QML's visible state from the client's exposure state?
>
> I also don't see the point of an enum for it, it's just true/false.

I got rid of visibility.

[Mirco Müller (macslow)]

> void MirSurfaceManager::displayOff()
> void MirSurfaceManager::displayOn()
>
> I disapprove of this design. A surface manager should not care about display
> state. It's the shell's decision if a client surface should be marked
> invisible when the display is off.
>
> Also this throws away any visibility state that the shell may have set on
> surfaces.
>
> A remote desktop would be an example where we don't care about the actual
> display state.

displayOn()/displayOff() are just temp. methods for quicker testing of the correct working of the visible-flag propagation. They will not stay... it's a WIP-branch after all :)

Unmerged revisions

349. By Mirco Müller on 2015-07-31

Just use visible as a term instead of visibility for MirSurfaceItem and map that to mir::scene::surface's visibility-attribute.

348. By Mirco Müller on 2015-07-30

Added hooks to allow MirSurfaceManager to mark MirSurfaceItems visible/non-visible.

347. By Mirco Müller on 2015-07-30

Merge with trunk.

346. By Mirco Müller on 2015-07-21

Expose visibility property of MirSurfaceItem.

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
--- src/modules/Unity/Application/mirsurfaceitem.cpp	2015-07-16 06:53:53 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.cpp	2015-07-31 13:26:26 +0000
@@ -260,6 +260,8 @@
     //m_surface->configure(mir_surface_attrib_focus, mir_surface_unfocused);
     connect(this, &QQuickItem::activeFocusChanged, this, &MirSurfaceItem::updateMirSurfaceFocus);
 
+    connect(this, &QQuickItem::visibleChanged, this, &MirSurfaceItem::updateMirSurfaceVisibility);
+
     if (m_session) {
         connect(m_session.data(), &Session::stateChanged, this, &MirSurfaceItem::onSessionStateChanged);
     }
@@ -306,6 +308,11 @@
     return static_cast<MirSurfaceItem::State>(m_surface->state());
 }
 
+bool MirSurfaceItem::visible() const
+{
+    return m_surface->query(mir_surface_attrib_visibility) == mir_surface_visibility_exposed;
+}
+
 MirSurfaceItem::OrientationAngle MirSurfaceItem::orientationAngle() const
 {
     return m_orientationAngle;
@@ -672,6 +679,15 @@
     }
 }
 
+void MirSurfaceItem::setVisible(const bool visible)
+{
+    qDebug() << "MirSurfaceItem::setVisible(" << (visible ? "true" : "false") << ")";
+    if (this->visible() != visible) {
+        m_shell->set_surface_attribute(m_session->session(), m_surface, mir_surface_attrib_visibility, static_cast<int>(visible));
+        Q_EMIT visibleChanged();
+    }
+}
+
 void MirSurfaceItem::setLive(const bool live)
 {
     if (m_live != live) {
@@ -689,6 +705,9 @@
     case mir_surface_attrib_state:
         Q_EMIT stateChanged();
         break;
+    case mir_surface_attrib_visibility:
+        Q_EMIT visibleChanged();
+        break;
     default:
         break;
     }
@@ -735,6 +754,16 @@
     }
 }
 
+void MirSurfaceItem::updateMirSurfaceVisibility()
+{
+    qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::updateMirSurfaceVisibility";
+    if (visible()) {
+        m_shell->set_surface_attribute(m_session->session(), m_surface, mir_surface_attrib_visibility, mir_surface_visibility_exposed);
+    } else {
+        m_shell->set_surface_attribute(m_session->session(), m_surface, mir_surface_attrib_visibility, mir_surface_visibility_occluded);
+    }
+}
+
 void MirSurfaceItem::dropPendingBuffers()
 {
     QMutexLocker locker(&m_mutex);

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.h'
--- src/modules/Unity/Application/mirsurfaceitem.h	2015-05-13 17:18:45 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.h	2015-07-31 13:26:26 +0000
@@ -52,6 +52,7 @@
 
     Q_PROPERTY(Type type READ type NOTIFY typeChanged)
     Q_PROPERTY(State state READ state NOTIFY stateChanged)
+    Q_PROPERTY(bool visible READ visible NOTIFY visibleChanged)
     Q_PROPERTY(QString name READ name NOTIFY nameChanged)
     Q_PROPERTY(bool live READ live NOTIFY liveChanged)
 
@@ -98,6 +99,7 @@
     //getters
     Type type() const;
     State state() const;
+    bool visible() const;
     QString name() const;
     bool live() const;
     SessionInterface *session() const;
@@ -128,6 +130,7 @@
 Q_SIGNALS:
     void typeChanged();
     void stateChanged();
+    void visibleChanged();
     void nameChanged();
     void orientationAngleChanged(OrientationAngle angle);
     void liveChanged(bool live);
@@ -161,6 +164,7 @@
     void updateMirSurfaceSize();
 
     void updateMirSurfaceFocus(bool focused);
+    void updateMirSurfaceVisibility();
     void onAttributeChanged(const MirSurfaceAttrib, const int);
 
 private:
@@ -169,6 +173,7 @@
 
     void setType(const Type&);
     void setState(const State&);
+    void setVisible(const bool);
     void setLive(const bool);
 
     // called by MirSurfaceManager

=== modified file 'src/modules/Unity/Application/mirsurfacemanager.cpp'
--- src/modules/Unity/Application/mirsurfacemanager.cpp	2015-03-11 10:10:49 +0000
+++ src/modules/Unity/Application/mirsurfacemanager.cpp	2015-07-31 13:26:26 +0000
@@ -17,6 +17,7 @@
 // Qt
 #include <QGuiApplication>
 #include <QMutexLocker>
+#include <QDebug>
 
 // local
 #include "mirsurfacemanager.h"
@@ -94,6 +95,34 @@
     m_mirSurfaceToItemHash.clear();
 }
 
+void MirSurfaceManager::displayOff()
+{
+    qDebug() << "MirSurfaceManager::displayOff() - surfaces: " << m_mirSurfaceToItemHash.size();
+
+    MirSurfaceItem* item = nullptr;
+    QMutexLocker lock(&m_mutex);
+    auto it = m_mirSurfaceToItemHash.begin();
+    while (it != m_mirSurfaceToItemHash.end()) {
+        item = it.value();
+        item->setVisible(false);
+        it++;
+    }
+}
+
+void MirSurfaceManager::displayOn()
+{
+    qDebug() << "MirSurfaceManager::displayOn() - surfaces: " << m_mirSurfaceToItemHash.size();
+
+    MirSurfaceItem* item = nullptr;
+    QMutexLocker lock(&m_mutex);
+    auto it = m_mirSurfaceToItemHash.begin();
+    while (it != m_mirSurfaceToItemHash.end()) {
+        item = it.value();
+        item->setVisible(true);
+        it++;
+    }
+}
+
 void MirSurfaceManager::onSessionCreatedSurface(const mir::scene::Session *mirSession,
                                                 const std::shared_ptr<mir::scene::Surface> &surface,
                                                 const std::shared_ptr<SurfaceObserver> &observer)

=== modified file 'src/modules/Unity/Application/mirsurfacemanager.h'
--- src/modules/Unity/Application/mirsurfacemanager.h	2015-03-11 10:10:49 +0000
+++ src/modules/Unity/Application/mirsurfacemanager.h	2015-07-31 13:26:26 +0000
@@ -63,6 +63,9 @@
 
     static MirSurfaceManager* singleton();
 
+    Q_INVOKABLE void displayOff();
+    Q_INVOKABLE void displayOn();
+
 Q_SIGNALS:
     void surfaceCreated(MirSurfaceItem* surface);
     void surfaceDestroyed(MirSurfaceItem* surface);