Merge lp:~nick-dedekind/qtmir/lp1475678.surface-occlude into lp:qtmir

[Nick Dedekind]

Support server->client visibility change to stop rendering (lp:#1475678)

Support server->client visibility change to stop rendering (lp:#1475678)

 * Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~nick-dedekind/unity8/lp1475678.surface-occlude/+merge/273427
https://code.launchpad.net/~nick-dedekind/unity-api/lp1475678.surface-occlude/+merge/273425
https://code.launchpad.net/~nick-dedekind/qtubuntu/lp1475678.surface-occlude/+merge/273424

 * Did you perform an exploratory manual test run of your code change and any related functionality?
 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:379
http://jenkins.qa.ubuntu.com/job/qtmir-ci/481/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/177/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/177/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/59/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/214/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/214/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/59/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/481/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

One thing to look at is to what happens when MirSurfaceItem::setSurface() is called and the MirSurfaceItem in question is already hidden. You have to synce the visibility of MirSurface with the one of its MirSurfaceItem inside setSurface().

Another thing to consider is what happens when there are multiple MirSurfaceItems displaying the same MirSurface? It will only get occluded if *all* items showing it are hidden. In other words: the MirSurface will be exposed as long as there's some visible MirSurfaceItem in the scene displaying it.

review: Needs Fixing

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

380. By Nick Dedekind on 2015-10-12

Aggregate the surface visibility

[Nick Dedekind (nick-dedekind)]

> One thing to look at is to what happens when MirSurfaceItem::setSurface() is
> called and the MirSurfaceItem in question is already hidden. You have to synce
> the visibility of MirSurface with the one of its MirSurfaceItem inside
> setSurface().

It did already, but I've updated since (view registration).

>
> Another thing to consider is what happens when there are multiple
> MirSurfaceItems displaying the same MirSurface? It will only get occluded if
> *all* items showing it are hidden. In other words: the MirSurface will be
> exposed as long as there's some visible MirSurfaceItem in the scene displaying
> it.

I've added surface visibility aggregation through view registration.

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

381. By Nick Dedekind on 2015-10-12

only log on change

382. By Nick Dedekind on 2015-10-12

use isEmpty

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:381
http://jenkins.qa.ubuntu.com/job/qtmir-ci/495/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/191/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/191/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/73/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/228/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/228/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/73/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/495/rebuild

review: Needs Fixing (continuous-integration)

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

383. By Nick Dedekind on 2015-10-12

use isEmpty

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:383
http://jenkins.qa.ubuntu.com/job/qtmir-ci/496/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/192/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/192/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/74/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/229/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/229/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/74/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/496/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

Please remove the cyclic dependency between MirSurfaceItem and MirSurface.

review: Needs Fixing

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

384. By Nick Dedekind on 2015-10-13

remove circ dep

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:384
http://jenkins.qa.ubuntu.com/job/qtmir-ci/509/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/205/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/205/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/87/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/242/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/242/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/87/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/509/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

"""
+int MirSurface::registerView()
"""

It would be better if it took the view id as a parameter, like this:
void MirSurface::registerView(qintptr viewId)

That way MirSurfaceItem could simply do:
m_surface->registerView((qintptr)this);

and not have to bother storing any id at all. Afterall, MirSurface::m_views is already storing them. So no need to store on both ends.

Sorry, I should have clarified that when you sent me http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked the missing parameter in that draft (and I didn't notice it was returning an int).

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

I think you can remove that from tests/modules/common/fake_mirsurface.h

"""
+#include <QDebug>
"""

[Nick Dedekind (nick-dedekind)]

> """
> +int MirSurface::registerView()
> """
>
> It would be better if it took the view id as a parameter, like this:
> void MirSurface::registerView(qintptr viewId)
>
>
> That way MirSurfaceItem could simply do:
> m_surface->registerView((qintptr)this);
>
> and not have to bother storing any id at all. Afterall, MirSurface::m_views is
> already storing them. So no need to store on both ends.
>
> Sorry, I should have clarified that when you sent me
> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked the
> missing parameter in that draft (and I didn't notice it was returning an int).

I don't think that's very good coding practice. It requires the caller to be responsible for choosing a value unique to another class (even if it's its mem address, it's still the assumption). Most registration mechanisms do it my way, for good reason.

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

385. By Nick Dedekind on 2015-10-14

removed debug

[Nick Dedekind (nick-dedekind)]

> I think you can remove that from tests/modules/common/fake_mirsurface.h
>
> """
> +#include <QDebug>
> """

Done.

[Daniel d'Andrada (dandrader)]

On 14/10/2015 11:57, Nick Dedekind wrote:
>> """
>> +int MirSurface::registerView()
>> """
>>
>> It would be better if it took the view id as a parameter, like this:
>> void MirSurface::registerView(qintptr viewId)
>>
>>
>> That way MirSurfaceItem could simply do:
>> m_surface->registerView((qintptr)this);
>>
>> and not have to bother storing any id at all. Afterall, MirSurface::m_views is
>> already storing them. So no need to store on both ends.
>>
>> Sorry, I should have clarified that when you sent me
>> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked the
>> missing parameter in that draft (and I didn't notice it was returning an int).
> I don't think that's very good coding practice. It requires the caller to be responsible for choosing a value unique to another class (even if it's its mem address, it's still the assumption). Most registration mechanisms do it my way, for good reason.

mir::graphics::RenderableList
generate_renderables(compositor::CompositorID id) uses the same approach
(CompositorID being "void const*") as well, if you want an exmaple.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:385
http://jenkins.qa.ubuntu.com/job/qtmir-ci/516/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/212/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/212/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/94/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/249/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/249/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/94/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/516/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

> > """
> > +int MirSurface::registerView()
> > """
> >
> > It would be better if it took the view id as a parameter, like this:
> > void MirSurface::registerView(qintptr viewId)
> >
> >
> > That way MirSurfaceItem could simply do:
> > m_surface->registerView((qintptr)this);
> >
> > and not have to bother storing any id at all. Afterall, MirSurface::m_views
> is
> > already storing them. So no need to store on both ends.
> >
> > Sorry, I should have clarified that when you sent me
> > http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked
> the
> > missing parameter in that draft (and I didn't notice it was returning an
> int).
>
> I don't think that's very good coding practice. It requires the caller to be
> responsible for choosing a value unique to another class (even if it's its mem
> address, it's still the assumption). Most registration mechanisms do it my
> way, for good reason.

You're right in that this way is strictly better, especially if you don't have control over the clients registering. For now, we do have control over the clients, so we can ensure they behave correctly.

Re-using "this" memory address as unique identifier means there's one less variable to worry about.

Since we control the clients, I don't think we need the complexity of the correct approach just now. If third parties start using qtmir MirSurface directly, then we will need to rethink it.

[Daniel d'Andrada (dandrader)]

On 14/10/2015 13:41, Gerry Boland wrote:
>>> """
>>> +int MirSurface::registerView()
>>> """
>>>
>>> It would be better if it took the view id as a parameter, like this:
>>> void MirSurface::registerView(qintptr viewId)
>>>
>>>
>>> That way MirSurfaceItem could simply do:
>>> m_surface->registerView((qintptr)this);
>>>
>>> and not have to bother storing any id at all. Afterall, MirSurface::m_views
>> is
>>> already storing them. So no need to store on both ends.
>>>
>>> Sorry, I should have clarified that when you sent me
>>> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked
>> the
>>> missing parameter in that draft (and I didn't notice it was returning an
>> int).
>>
>> I don't think that's very good coding practice. It requires the caller to be
>> responsible for choosing a value unique to another class (even if it's its mem
>> address, it's still the assumption). Most registration mechanisms do it my
>> way, for good reason.
> You're right in that this way is strictly better, especially if you don't have control over the clients registering. For now, we do have control over the clients, so we can ensure they behave correctly.
>
> Re-using "this" memory address as unique identifier means there's one less variable to worry about.
>
> Since we control the clients, I don't think we need the complexity of the correct approach just now. If third parties start using qtmir MirSurface directly, then we will need to rethink it.

qtmir::MirSurfaceInterface (where registerView() belongs to) is an
internal qtmir API.

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

386. By Nick Dedekind on 2015-10-19

use qintptr for viewId

[Nick Dedekind (nick-dedekind)]

> On 14/10/2015 13:41, Gerry Boland wrote:
> >>> """
> >>> +int MirSurface::registerView()
> >>> """
> >>>
> >>> It would be better if it took the view id as a parameter, like this:
> >>> void MirSurface::registerView(qintptr viewId)
> >>>
> >>>
> >>> That way MirSurfaceItem could simply do:
> >>> m_surface->registerView((qintptr)this);
> >>>
> >>> and not have to bother storing any id at all. Afterall,
> MirSurface::m_views
> >> is
> >>> already storing them. So no need to store on both ends.
> >>>
> >>> Sorry, I should have clarified that when you sent me
> >>> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked
> >> the
> >>> missing parameter in that draft (and I didn't notice it was returning an
> >> int).
> >>
> >> I don't think that's very good coding practice. It requires the caller to
> be
> >> responsible for choosing a value unique to another class (even if it's its
> mem
> >> address, it's still the assumption). Most registration mechanisms do it my
> >> way, for good reason.
> > You're right in that this way is strictly better, especially if you don't
> have control over the clients registering. For now, we do have control over
> the clients, so we can ensure they behave correctly.
> >
> > Re-using "this" memory address as unique identifier means there's one less
> variable to worry about.
> >
> > Since we control the clients, I don't think we need the complexity of the
> correct approach just now. If third parties start using qtmir MirSurface
> directly, then we will need to rethink it.
>
> qtmir::MirSurfaceInterface (where registerView() belongs to) is an
> internal qtmir API.

Done.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:386
http://jenkins.qa.ubuntu.com/job/qtmir-ci/525/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/221/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/221/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/103/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/258/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/258/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/103/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/525/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

MirSurface should call deleteLater() if the last MirSurfaceItem unregisters from it and it's no longer live() or pointed to by a session.

Ie, this code should be kept:

"""
if (m_viewCount == 0) {
    [..]
    if (m_session.isNull() || !m_live) {
        deleteLater();
    }
}
"""

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

Kept in MirSurface::unregisterView() I mean.

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

387. By Nick Dedekind on 2015-10-20

Re-added deleteLater

388. By Nick Dedekind on 2015-10-20

Added test for Surface deleteLater on deregister

[Nick Dedekind (nick-dedekind)]

> MirSurface should call deleteLater() if the last MirSurfaceItem unregisters
> from it and it's no longer live() or pointed to by a session.
>
> Ie, this code should be kept:
>
> """
> if (m_viewCount == 0) {
> [..]
> if (m_session.isNull() || !m_live) {
> deleteLater();
> }
> }
> """

Yikes, my bad. don't know how that happened.
re-added.

I also added a MirSurface test (to test the delete on unregister) and moved the surface related tests to a "SurfaceManager" test folder. Most other things are hosted under their Management related folders (App & Session tests). We should be adding SurfaceManager tests anyway. Guess this will come with multi-window support.

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

389. By Nick Dedekind on 2015-10-20

added missing file

390. By Nick Dedekind on 2015-10-20

removed line

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:388
http://jenkins.qa.ubuntu.com/job/qtmir-ci/526/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/222/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/222/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/104/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/259/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/259/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/104/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/526/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:390
http://jenkins.qa.ubuntu.com/job/qtmir-ci/527/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/223/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/223/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/105/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/260/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/260/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/105/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/527/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

Code looks good. Didn't test yet.

review: Approve (code)

[Daniel d'Andrada (dandrader)]

review: Approve

[lp:~nick-dedekind/qtmir/lp1475678.surface-occlude]

391. By Nick Dedekind on 2015-10-21

merged with trunk

392. By Nick Dedekind on 2015-10-26

merged with trunk

393. By Nick Dedekind on 2015-10-26

bump libunity-api version

=== modified file 'debian/control'
--- debian/control	2015-10-21 11:46:54 +0000
+++ debian/control	2015-10-27 16:23:25 +0000
@@ -23,7 +23,7 @@
                libubuntu-app-launch2-dev,
                libubuntu-application-api-dev (>= 2.1.0),
                libudev-dev,
-               libunity-api-dev (>= 7.101),
+               libunity-api-dev (>= 7.102),
                liburl-dispatcher1-dev,
                libxkbcommon-dev,
                libxrender-dev,

=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
--- src/modules/Unity/Application/mirsurface.cpp	2015-10-21 11:47:03 +0000
+++ src/modules/Unity/Application/mirsurface.cpp	2015-10-27 16:23:25 +0000
@@ -176,7 +176,6 @@
     , m_textureUpdated(false)
     , m_currentFrameNumber(0)
     , m_live(true)
-    , m_viewCount(0)
 {
     m_surfaceObserver = observer;
     if (observer) {
@@ -208,9 +207,9 @@
 
 MirSurface::~MirSurface()
 {
-    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::~MirSurface this=" << this << " viewCount=" << m_viewCount;
+    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::~MirSurface this=" << this << " viewCount=" << m_views.count();
 
-    Q_ASSERT(m_viewCount == 0);
+    Q_ASSERT(m_views.isEmpty());
 
     if (m_session) {
         m_session->setSurface(nullptr);
@@ -242,6 +241,9 @@
     case mir_surface_attrib_state:
         Q_EMIT stateChanged(state());
         break;
+    case mir_surface_attrib_visibility:
+        Q_EMIT visibleChanged(visible());
+        break;
     default:
         break;
     }
@@ -555,6 +557,11 @@
     return m_live;
 }
 
+bool MirSurface::visible() const
+{
+    return m_surface->query(mir_surface_attrib_visibility) == mir_surface_visibility_exposed;
+}
+
 void MirSurface::mousePressEvent(QMouseEvent *event)
 {
     auto ev = makeMirEvent(event, mir_pointer_action_button_down);
@@ -638,29 +645,56 @@
 
 bool MirSurface::isBeingDisplayed() const
 {
-    return m_viewCount > 0;
+    return !m_views.isEmpty();
 }
 
-void MirSurface::incrementViewCount()
+void MirSurface::registerView(qintptr viewId)
 {
-    ++m_viewCount;
-    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::incrementViewCount after=" << m_viewCount;
-    if (m_viewCount == 1) {
+    m_views.insert(viewId, MirSurface::View{false});
+    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::registerView(" << viewId << ")"
+                                      << " after=" << m_views.count();
+    if (m_views.count() == 1) {
         Q_EMIT isBeingDisplayedChanged();
     }
 }
 
-void MirSurface::decrementViewCount()
+void MirSurface::unregisterView(qintptr viewId)
 {
-    Q_ASSERT(m_viewCount > 0);
-    --m_viewCount;
-    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::decrementViewCount after=" << m_viewCount;
-    if (m_viewCount == 0) {
+    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::unregisterView(" << viewId << ")"
+                                      << " after=" << m_views.count() << " live=" << m_live;
+    m_views.remove(viewId);
+    if (m_views.count() == 0) {
         Q_EMIT isBeingDisplayedChanged();
         if (m_session.isNull() || !m_live) {
             deleteLater();
         }
     }
+    updateVisibility();
+}
+
+void MirSurface::setViewVisibility(qintptr viewId, bool visible)
+{
+    if (!m_views.contains(viewId)) return;
+
+    m_views[viewId].visible = visible;
+    updateVisibility();
+}
+
+void MirSurface::updateVisibility()
+{
+    bool newVisible = false;
+    QHashIterator<qintptr, View> i(m_views);
+    while (i.hasNext()) {
+        i.next();
+        newVisible |= i.value().visible;
+    }
+
+    if (newVisible != visible()) {
+        qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::updateVisibility(" << newVisible << ")";
+
+        m_surface->configure(mir_surface_attrib_visibility,
+                             newVisible ? mir_surface_visibility_exposed : mir_surface_visibility_occluded);
+    }
 }
 
 unsigned int MirSurface::currentFrameNumber() const
@@ -670,7 +704,7 @@
 
 void MirSurface::onSessionDestroyed()
 {
-    if (m_viewCount == 0) {
+    if (m_views.isEmpty()) {
         deleteLater();
     }
 }

=== modified file 'src/modules/Unity/Application/mirsurface.h'
--- src/modules/Unity/Application/mirsurface.h	2015-10-21 11:47:03 +0000
+++ src/modules/Unity/Application/mirsurface.h	2015-10-27 16:23:25 +0000
@@ -67,6 +67,8 @@
 
     bool live() const override;
 
+    bool visible() const override;
+
     Mir::OrientationAngle orientationAngle() const override;
     void setOrientationAngle(Mir::OrientationAngle angle) override;
 
@@ -81,8 +83,10 @@
     void startFrameDropper() override;
 
     bool isBeingDisplayed() const override;
-    void incrementViewCount() override;
-    void decrementViewCount() override;
+
+    void registerView(qintptr viewId) override;
+    void unregisterView(qintptr viewId) override;
+    void setViewVisibility(qintptr viewId, bool visible) override;
 
     // methods called from the rendering (scene graph) thread:
     QSharedPointer<QSGTexture> texture() override;
@@ -125,6 +129,7 @@
 private:
     void syncSurfaceSizeWithItemSize();
     bool clientIsRunning() const;
+    void updateVisibility();
 
     std::shared_ptr<mir::scene::Surface> m_surface;
     QPointer<SessionInterface> m_session;
@@ -144,7 +149,10 @@
     unsigned int m_currentFrameNumber;
 
     bool m_live;
-    int m_viewCount;
+    struct View {
+        bool visible;
+    };
+    QHash<qintptr, View> m_views;
 
     std::shared_ptr<SurfaceObserver> m_surfaceObserver;
 

=== modified file 'src/modules/Unity/Application/mirsurfaceinterface.h'
--- src/modules/Unity/Application/mirsurfaceinterface.h	2015-10-13 18:29:13 +0000
+++ src/modules/Unity/Application/mirsurfaceinterface.h	2015-10-27 16:23:25 +0000
@@ -48,8 +48,10 @@
     virtual void startFrameDropper() = 0;
 
     virtual bool isBeingDisplayed() const = 0;
-    virtual void incrementViewCount() = 0;
-    virtual void decrementViewCount() = 0;
+
+    virtual void registerView(qintptr viewId) = 0;
+    virtual void unregisterView(qintptr viewId) = 0;
+    virtual void setViewVisibility(qintptr viewId, bool visible) = 0;
 
     // methods called from the rendering (scene graph) thread:
     virtual QSharedPointer<QSGTexture> texture() = 0;

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
--- src/modules/Unity/Application/mirsurfaceitem.cpp	2015-10-13 18:29:13 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.cpp	2015-10-27 16:23:25 +0000
@@ -106,6 +106,7 @@
     connect(&m_updateMirSurfaceSizeTimer, &QTimer::timeout, this, &MirSurfaceItem::updateMirSurfaceSize);
 
     connect(this, &QQuickItem::activeFocusChanged, this, &MirSurfaceItem::updateMirSurfaceFocus);
+    connect(this, &QQuickItem::visibleChanged, this, &MirSurfaceItem::updateMirSurfaceVisibility);
 }
 
 MirSurfaceItem::~MirSurfaceItem()
@@ -524,6 +525,15 @@
     m_surface->resize(width, height);
 }
 
+void MirSurfaceItem::updateMirSurfaceVisibility()
+{
+    if (!m_surface || !m_surface->live()) {
+        return;
+    }
+
+    m_surface->setViewVisibility((qintptr)this, isVisible());
+}
+
 void MirSurfaceItem::updateMirSurfaceFocus(bool focused)
 {
     if (m_surface && m_consumesInput && m_surface->live()) {
@@ -603,7 +613,7 @@
             m_surface->setFocus(false);
         }
 
-        m_surface->decrementViewCount();
+        m_surface->unregisterView((qintptr)this);
 
         if (!m_surface->isBeingDisplayed() && window()) {
             disconnect(window(), nullptr, m_surface, nullptr);
@@ -613,7 +623,7 @@
     m_surface = surface;
 
     if (m_surface) {
-        m_surface->incrementViewCount();
+        m_surface->registerView((qintptr)this);
 
         // When a new mir frame gets posted we notify the QML engine that this item needs redrawing,
         // schedules call to updatePaintNode() from the rendering thread
@@ -623,8 +633,10 @@
         connect(m_surface, &MirSurfaceInterface::liveChanged, this, &MirSurfaceItem::liveChanged);
         connect(m_surface, &MirSurfaceInterface::sizeChanged, this, &MirSurfaceItem::onActualSurfaceSizeChanged);
 
-        connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurfaceInterface::onCompositorSwappedBuffers,
-            (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));
+        if (window()) {
+            connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurfaceInterface::onCompositorSwappedBuffers,
+                (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));
+        }
 
         Q_EMIT typeChanged(m_surface->type());
         Q_EMIT liveChanged(true);
@@ -632,6 +644,7 @@
 
         updateMirSurfaceSize();
         setImplicitSize(m_surface->size().width(), m_surface->size().height());
+        updateMirSurfaceVisibility();
 
         if (m_orientationAngle) {
             m_surface->setOrientationAngle(*m_orientationAngle);

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.h'
--- src/modules/Unity/Application/mirsurfaceitem.h	2015-09-18 20:11:50 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.h	2015-10-27 16:23:25 +0000
@@ -112,6 +112,7 @@
     void updateMirSurfaceSize();
 
     void updateMirSurfaceFocus(bool focused);
+    void updateMirSurfaceVisibility();
 
     void onActualSurfaceSizeChanged(const QSize &size);
 

=== modified file 'tests/modules/CMakeLists.txt'
--- tests/modules/CMakeLists.txt	2015-01-09 11:25:13 +0000
+++ tests/modules/CMakeLists.txt	2015-10-27 16:23:25 +0000
@@ -2,7 +2,7 @@
 add_subdirectory(ApplicationManager)
 add_subdirectory(DesktopFileReader)
 add_subdirectory(General)
-add_subdirectory(MirSurfaceItem)
+add_subdirectory(SharedWakelock)
 add_subdirectory(SessionManager)
-add_subdirectory(SharedWakelock)
+add_subdirectory(SurfaceManager)
 add_subdirectory(TaskController)

=== renamed directory 'tests/modules/MirSurfaceItem' => 'tests/modules/SurfaceManager'
=== modified file 'tests/modules/SurfaceManager/CMakeLists.txt'
--- tests/modules/MirSurfaceItem/CMakeLists.txt	2015-08-31 09:51:28 +0000
+++ tests/modules/SurfaceManager/CMakeLists.txt	2015-10-27 16:23:25 +0000
@@ -1,8 +1,10 @@
 set(
-  MIR_SURFACE_ITEM_TEST_SOURCES
+  MIR_SURFACE_MANAGER_TEST_SOURCES
   mirsurfaceitem_test.cpp
+  mirsurface_test.cpp
   ${CMAKE_SOURCE_DIR}/src/common/debughelpers.cpp
   ${CMAKE_SOURCE_DIR}/tests/modules/common/fake_mirsurface.h
+  ${CMAKE_SOURCE_DIR}/tests/modules/common/fake_session.h
 )
 
 include_directories(
@@ -11,10 +13,10 @@
   ${MIRSERVER_INCLUDE_DIRS}
 )
 
-add_executable(mirsurfaceitem_test ${MIR_SURFACE_ITEM_TEST_SOURCES})
+add_executable(surfacemanager_test ${MIR_SURFACE_MANAGER_TEST_SOURCES})
 
 target_link_libraries(
-  mirsurfaceitem_test
+  surfacemanager_test
 
   qpa-mirserver
   unityapplicationplugin
@@ -25,4 +27,4 @@
   ${GMOCK_LIBRARIES}
 )
 
-add_test(MirSurfaceItem mirsurfaceitem_test)
+add_test(SurfaceManager surfacemanager_test)

=== added file 'tests/modules/SurfaceManager/mirsurface_test.cpp'
--- tests/modules/SurfaceManager/mirsurface_test.cpp	1970-01-01 00:00:00 +0000
+++ tests/modules/SurfaceManager/mirsurface_test.cpp	2015-10-27 16:23:25 +0000
@@ -0,0 +1,107 @@
+/*
+ * Copyright (C) 2014-2015 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License version 3, as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define MIR_INCLUDE_DEPRECATED_EVENT_HEADER
+
+#include <gtest/gtest.h>
+
+#include <QLoggingCategory>
+#include <QTest>
+
+// the test subject
+#include <Unity/Application/mirsurface.h>
+
+// tests/modules/common
+#include <fake_session.h>
+#include <mock_surface.h>
+
+using namespace qtmir;
+
+namespace ms = mir::scene;
+
+class MirSurfaceTest : public ::testing::Test
+{
+public:
+    MirSurfaceTest()
+    {
+        // We don't want the logging spam cluttering the test results
+        QLoggingCategory::setFilterRules(QStringLiteral("qtmir.surfaces=false"));        
+    }
+};
+
+TEST_F(MirSurfaceTest, DeleteMirSurfaceOnLastNonLiveUnregisterView)
+{
+    using namespace testing;
+
+    int argc = 0;
+    char* argv[0];
+    QCoreApplication qtApp(argc, argv); // app for deleteLater event
+
+    auto fakeSession = new FakeSession();
+    auto mockSurface = std::make_shared<NiceMock<ms::MockSurface>>();
+
+    MirSurface *surface = new MirSurface(mockSurface, fakeSession, nullptr, nullptr);
+    bool surfaceDeleted = false;
+    QObject::connect(surface, &QObject::destroyed, surface, [&surfaceDeleted](){ surfaceDeleted = true; });
+
+    qintptr view1 = (qintptr)1;
+    qintptr view2 = (qintptr)2;
+
+    surface->registerView(view1);
+    surface->registerView(view2);
+    surface->setLive(false);
+    EXPECT_FALSE(surfaceDeleted);
+
+    surface->unregisterView(view1);
+    surface->unregisterView(view2);
+
+    QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
+    EXPECT_TRUE(surfaceDeleted);
+
+    delete fakeSession;
+}
+
+
+TEST_F(MirSurfaceTest, DoNotDeleteMirSurfaceOnLastLiveUnregisterView)
+{
+    using namespace testing;
+
+    int argc = 0;
+    char* argv[0];
+    QCoreApplication qtApp(argc, argv); // app for deleteLater event
+
+    auto fakeSession = new FakeSession();
+    auto mockSurface = std::make_shared<NiceMock<ms::MockSurface>>();
+
+    MirSurface *surface = new MirSurface(mockSurface, fakeSession, nullptr, nullptr);
+    bool surfaceDeleted = false;
+    QObject::connect(surface, &QObject::destroyed, surface, [&surfaceDeleted](){ surfaceDeleted = true; });
+
+    qintptr view1 = (qintptr)1;
+    qintptr view2 = (qintptr)2;
+
+    surface->registerView(view1);
+    surface->registerView(view2);
+
+    surface->unregisterView(view1);
+    surface->unregisterView(view2);
+
+    QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
+    EXPECT_FALSE(surfaceDeleted);
+
+    delete fakeSession;
+    delete surface;
+}

=== modified file 'tests/modules/SurfaceManager/mirsurfaceitem_test.cpp'
--- tests/modules/MirSurfaceItem/mirsurfaceitem_test.cpp	2015-08-31 09:51:28 +0000
+++ tests/modules/SurfaceManager/mirsurfaceitem_test.cpp	2015-10-27 16:23:25 +0000
@@ -29,22 +29,28 @@
 
 using namespace qtmir;
 
+class MirSurfaceItemTest : public ::testing::Test
+{
+public:
+    MirSurfaceItemTest()
+    {
+        // We don't want the logging spam cluttering the test results
+        QLoggingCategory::setFilterRules(QStringLiteral("qtmir.surfaces=false"));        
+    }
+};
+
 /*
   Tests that even if Qt fails to finish a touch sequence, MirSurfaceItem will
   properly finish it when forwarding it to its mir::input::surface. So
   mir::input::surface will still consume a proper sequence of touch events
   (comprised of a begin, zero or more updates and an end).
  */
-TEST(MirSurfaceItemTest, MissingTouchEnd)
+TEST_F(MirSurfaceItemTest, MissingTouchEnd)
 {
-    // We don't want the logging spam cluttering the test results
-    QLoggingCategory::setFilterRules(QStringLiteral("qtmir*=false"));
-
     MirSurfaceItem *surfaceItem = new MirSurfaceItem;
 
     FakeMirSurface *fakeSurface = new FakeMirSurface;
 
-
     surfaceItem->setSurface(fakeSurface);
     surfaceItem->setConsumesInput(true);
 
@@ -93,3 +99,46 @@
     delete surfaceItem;
     delete fakeSurface;
 }
+
+TEST_F(MirSurfaceItemTest, SetSurfaceInitializesVisiblity)
+{
+    MirSurfaceItem *surfaceItem = new MirSurfaceItem;
+    surfaceItem->setVisible(false);
+    
+    FakeMirSurface *fakeSurface = new FakeMirSurface;
+    surfaceItem->setSurface(fakeSurface);
+
+    EXPECT_FALSE(fakeSurface->visible());
+
+    delete surfaceItem;
+    delete fakeSurface;
+}
+
+TEST_F(MirSurfaceItemTest, AggregateSurfaceVisibility)
+{
+    MirSurfaceItem *surfaceItem1 = new MirSurfaceItem;
+    surfaceItem1->setVisible(true);
+    MirSurfaceItem *surfaceItem2 = new MirSurfaceItem;
+    surfaceItem1->setVisible(true);
+    
+    FakeMirSurface *fakeSurface = new FakeMirSurface;
+    surfaceItem1->setSurface(fakeSurface);
+    surfaceItem2->setSurface(fakeSurface);
+
+    EXPECT_TRUE(fakeSurface->visible());
+
+    surfaceItem1->setVisible(false);
+    EXPECT_TRUE(fakeSurface->visible());
+
+    surfaceItem2->setVisible(false);
+    EXPECT_FALSE(fakeSurface->visible());
+
+    surfaceItem1->setVisible(true);
+    EXPECT_TRUE(fakeSurface->visible());
+
+    delete surfaceItem1;
+    EXPECT_FALSE(fakeSurface->visible());
+
+    delete surfaceItem2;
+    delete fakeSurface;
+}

=== modified file 'tests/modules/common/fake_mirsurface.h'
--- tests/modules/common/fake_mirsurface.h	2015-10-13 18:29:13 +0000
+++ tests/modules/common/fake_mirsurface.h	2015-10-27 16:23:25 +0000
@@ -21,6 +21,7 @@
 
 #include <QSharedPointer>
 #include <QSGTexture>
+#include <QPointer>
 
 namespace qtmir {
 
@@ -56,7 +57,7 @@
         , m_live(true)
         , m_state(Mir::RestoredState)
         , m_orientationAngle(Mir::Angle0)
-        , m_viewCount(0)
+        , m_visible(true)
         , m_focused(false)
     {}
 
@@ -87,6 +88,8 @@
 
     bool live() const override { return m_live; }
 
+    bool visible() const override { return m_visible; }
+
     Mir::OrientationAngle orientationAngle() const override { return m_orientationAngle; }
     void setOrientationAngle(Mir::OrientationAngle angle) override {
         if (m_orientationAngle != angle) {
@@ -116,18 +119,28 @@
         }
     }
 
-    bool isBeingDisplayed() const override { return m_viewCount > 0; }
-    void incrementViewCount() override {
-        ++m_viewCount;
-        if (m_viewCount == 1) {
-            Q_EMIT isBeingDisplayedChanged();
-        }
-    }
-    void decrementViewCount() override {
-        --m_viewCount;
-        if (m_viewCount == 0) {
-            Q_EMIT isBeingDisplayedChanged();
-        }
+    void setViewVisibility(qintptr viewId, bool visible) override {
+        if (!m_views.contains(viewId)) return;
+
+        m_views[viewId] = visible;
+        updateVisibility();
+    }
+
+    bool isBeingDisplayed() const override { return !m_views.isEmpty(); }
+
+    void registerView(qintptr viewId) override {
+        m_views.insert(viewId, false);
+        if (m_views.count() == 1) {
+            Q_EMIT isBeingDisplayedChanged();
+        }
+    }
+
+    void unregisterView(qintptr viewId) override {
+        m_views.remove(viewId);
+        if (m_views.count() == 0) {
+            Q_EMIT isBeingDisplayedChanged();
+        }
+        updateVisibility();
     }
 
     // methods called from the rendering (scene graph) thread:
@@ -182,6 +195,20 @@
     QList<TouchEvent> &touchesReceived() { return m_touchesReceived; }
 
 private:
+    void updateVisibility() {
+        bool newVisible = false;
+        QHashIterator<int, bool> i(m_views);
+        while (i.hasNext()) {
+            i.next();
+            newVisible |= i.value();
+        }
+
+        if (m_visible != newVisible) {
+            m_visible = newVisible;
+            Q_EMIT visibleChanged(newVisible);
+        }
+    }
+
 
     bool m_isFirstFrameDrawn;
     SessionInterface *m_session;
@@ -189,8 +216,9 @@
     bool m_live;
     Mir::State m_state;
     Mir::OrientationAngle m_orientationAngle;
+    bool m_visible;
     QSize m_size;
-    int m_viewCount;
+    QHash<int, bool> m_views;
     bool m_focused;
 
     QList<TouchEvent> m_touchesReceived;

=== modified file 'tests/modules/common/fake_session.h'
--- tests/modules/common/fake_session.h	2015-08-31 09:51:28 +0000
+++ tests/modules/common/fake_session.h	2015-10-27 16:23:25 +0000
@@ -28,6 +28,7 @@
 public:
     FakeSession()
         : SessionInterface(0)
+        , m_application(nullptr)
         , m_state(Starting)
     {
     }