Merge lp:~dandrader/qtmir/promptBeforeSurfaceDraws into lp:qtmir

[Daniel d'Andrada]

Session: Add a blank surface to the public list if it already has child prompt surfaces

A prompt session may come in too early, before its parent session got a chance
to draw its surface's first frame.

Normally that wouldn't be a problem and we could withhold that parent surface until
it's finally drawn to. But unfortunately the application process might be blocked,
unable to draw anything, until its child prompt session gets dismissed.

Because of that we have no option but to expose this blank surface to shell so that
it can display it along with the prompt surface on top of it, so that the user can
interact with it right away and finally unblock the application.

* Are there any related MPs required for this MP to build/function as expected? Please list.
No.

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Not applicable.

[Unity8 CI Bot (unity8-ci-bot)]

FAILED: Continuous integration, rev:485
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/211/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1574
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1531
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1531
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1531
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1531/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1531
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1531/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1531/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1531/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1531/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1531/console

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/211/rebuild

review: Needs Fixing (continuous-integration)

[lp:~dandrader/qtmir/promptBeforeSurfaceDraws]

486. By Daniel d'Andrada on 2016-05-10

Fix MirSurfaceListModel clean up

[Unity8 CI Bot (unity8-ci-bot)]

PASSED: Continuous integration, rev:486
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/212/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1575
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1532
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1532
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1532
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1532/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1532
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1532/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1532
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1532/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1532
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1532/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1532
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1532/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1532
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1532/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/212/rebuild

review: Approve (continuous-integration)

[Nick Dedekind (nick-dedekind)]

This seems to be a cheeky hack around the fact that the surface hasn't had it's first frame drawn and is therefore not "visible". After hacking in the fact that at the moment the prompts are a child of a session, not a surface.

What happens if the surface hadn't been drawn, we create the prompt and append to the blank surface and then suddenly the surface is drawn. The prompt will still be childed to the blank surface which I think is wrong.

I think the fact that the surface "isn't ready" to be shown should be dealt with in unity8. Maybe use the MirSurface::visible property?

bool MirSurface::visible() {
    return m_firstFrameDrawn &&
        m_surface->query(mir_surface_attrib_visibility) == mir_surface_visibility_exposed;
}

SurfaceContainer {
    MirSurfaceItem {
        visible: surface.visible
    }
}

+ add in checks for visibility in animations.

IMO it's all just a bit wrong at the moment.

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

On 11/05/2016 06:17, Nick Dedekind wrote:
> The prompt will still be childed to the blank surface which I think is wrong.

So you think a prompt surface should be treated as a top-level
application surface? Or is it the problem just that the parent surface
is still blank?

[Daniel d'Andrada (dandrader)]

On 11/05/2016 06:17, Nick Dedekind wrote:
> bool MirSurface::visible() {
> return m_firstFrameDrawn &&
> m_surface->query(mir_surface_attrib_visibility) == mir_surface_visibility_exposed;
> }
>
> SurfaceContainer {
> MirSurfaceItem {
> visible: surface.visible
> }
> }
MirSurfaceItem already sets surface visibility based on its own visible
property. And here you make the latter also set the former. Like A sets
B and B sets A, a cyclic data-flow/control.

Besides MirSurface::visible is already used to convey surface
exposure/occlusion information. Here you want to overload that property
to include another meaning. I don't think the two combine well.

[Gerry Boland (gerboland)]

I suspect we should abandon the "only notify qml of new surface after its first frame" thing - and instead have a flag on the MirSurface{,Item} saying "notDrawnToYet" - instead of this blank surface idea.

Like Nick, I'm not a big fan of the blank surface idea either, but I see why you did it. It is the least complex change to fix the issue.

But the "only notify qml of new surface after its first frame" policy is not a Mir policy, just a QtMir policy, and it seems to hurt us here.

Dropping that policy causes us new problems: how does QML draw a MirSurfaceItem which is backed by a mirSurface which was never drawn to. I suppose we do as you do here: make it blank. unity8 will need work to reflect QtMir's behaviour change too.

This what I envision as the proper fix is a much more involved change, not so easily done if it needs to land quickly. We might need to land this (with a FIXME), but we should investigate a proper fix afterwards.

[Nick Dedekind (nick-dedekind)]

> On 11/05/2016 06:17, Nick Dedekind wrote:
> > The prompt will still be childed to the blank surface which I think is
> wrong.
>
> So you think a prompt surface should be treated as a top-level
> application surface? Or is it the problem just that the parent surface
> is still blank?

The Mir implementation of prompts has not got surfaces in mind, they're properties of a session. We've just forced it to be a child the first surface.
I'm not saying the current mir impl is correct; and we'll have trouble as soon as 2 top level surfaces require prompts at the same time...

Sorry, I misunderstood the code, but it seems that you might be prepending the surface twice. Once if you add a prompt when it's blank, and again when it's drawn it's first frame.

[Nick Dedekind (nick-dedekind)]

> On 11/05/2016 06:17, Nick Dedekind wrote:
> > bool MirSurface::visible() {
> > return m_firstFrameDrawn &&
> > m_surface->query(mir_surface_attrib_visibility) ==
> mir_surface_visibility_exposed;
> > }
> >
> > SurfaceContainer {
> > MirSurfaceItem {
> > visible: surface.visible
> > }
> > }
> MirSurfaceItem already sets surface visibility based on its own visible
> property. And here you make the latter also set the former. Like A sets
> B and B sets A, a cyclic data-flow/control.

Sure. opacity, occluded by spinner, or whatever to not show the actual surface.

>
> Besides MirSurface::visible is already used to convey surface
> exposure/occlusion information. Here you want to overload that property
> to include another meaning. I don't think the two combine well.

Well, it's up to us what a exposed property means. Whether it's "occluded" or "should be visible". A separate property would be better but require API.

[Daniel d'Andrada (dandrader)]

Will start working on a overhauling of surface.drawnFirstFrame usage and policy.
If people get anxious they can ship this fix/work-around before the afore-mentioned overhauling is ready.

[Daniel d'Andrada (dandrader)]

On 11/05/2016 09:54, Nick Dedekind wrote:
> Sorry, I misunderstood the code, but it seems that you might be prepending the surface twice. Once if you add a prompt when it's blank, and again when it's drawn it's first frame.
prompt session surfaces are added to its parent session's
promptSurfaceList only in Session::addChildSession. And that's called
only once per prompt session.

Once the once-blank has drawn its first frame it's moved to the
Session's surface list. But that move doesn't affect the surfaces own
promptSurfaceList.

[Nick Dedekind (nick-dedekind)]

ok, looks good. (for now ;)

review: Approve

[Daniel d'Andrada (dandrader)]

https://code.launchpad.net/~dandrader/unity8/surfaceDrawn/+merge/294427 is a better fix for this bug.

=== modified file 'src/modules/Unity/Application/mirsurfacelistmodel.cpp'
--- src/modules/Unity/Application/mirsurfacelistmodel.cpp	2016-04-05 18:58:38 +0000
+++ src/modules/Unity/Application/mirsurfacelistmodel.cpp	2016-05-10 21:06:43 +0000
@@ -28,6 +28,11 @@
 {
 }
 
+MirSurfaceListModel::~MirSurfaceListModel()
+{
+    Q_EMIT destroyed(this); // Early warning, while MirSurfaceListModel methods can still be accessed.
+}
+
 int MirSurfaceListModel::rowCount(const QModelIndex &parent) const
 {
     return !parent.isValid() ? m_surfaceList.size() : 0;

=== modified file 'src/modules/Unity/Application/mirsurfacelistmodel.h'
--- src/modules/Unity/Application/mirsurfacelistmodel.h	2016-04-05 18:58:38 +0000
+++ src/modules/Unity/Application/mirsurfacelistmodel.h	2016-05-10 21:06:43 +0000
@@ -41,6 +41,7 @@
     Q_OBJECT
 public:
     explicit MirSurfaceListModel(QObject *parent = 0);
+    virtual ~MirSurfaceListModel();
 
     Q_INVOKABLE unity::shell::application::MirSurfaceInterface *get(int index) override;
     const unity::shell::application::MirSurfaceInterface *get(int index) const;

=== modified file 'src/modules/Unity/Application/session.cpp'
--- src/modules/Unity/Application/session.cpp	2016-04-06 16:52:49 +0000
+++ src/modules/Unity/Application/session.cpp	2016-05-10 21:06:43 +0000
@@ -199,8 +199,17 @@
     if (newSurface->isFirstFrameDrawn()) {
         prependSurface(newSurface);
     } else {
-        connect(newSurface, &MirSurfaceInterface::firstFrameDrawn,
-                this, [this, newSurface]() { this->prependSurface(newSurface); });
+        m_blankSurfaces.append(newSurface);
+        connect(newSurface, &QObject::destroyed, this, [this, newSurface]()
+            {
+                this->m_blankSurfaces.removeAll(newSurface);
+            });
+        connect(newSurface, &MirSurfaceInterface::firstFrameDrawn, this, [this, newSurface]()
+            {
+                this->m_blankSurfaces.removeAll(newSurface);
+                newSurface->disconnect(this);
+                this->prependSurface(newSurface);
+            });
     }
 }
 
@@ -389,6 +398,18 @@
         // we assume that the top-most surface is the one that caused the prompt session to show up.
         auto promptSurfaceList = static_cast<MirSurfaceListModel*>(m_surfaceList.get(0)->promptSurfaceList());
         promptSurfaceList->addSurfaceList(session->surfaceList());
+    } else if (m_blankSurfaces.count() > 0) {
+        // Prompt session came in too early.
+        // Parent session might be blocked until the prompt session is dismissed.
+        // So we cannot wait anymore and must put that blank surface on the surface list so that the
+        // user can see the prompt surface on top of that it and interact.
+        auto blankSurface = m_blankSurfaces.takeFirst();
+
+        auto promptSurfaceList = static_cast<MirSurfaceListModel*>(blankSurface->promptSurfaceList());
+        promptSurfaceList->addSurfaceList(session->surfaceList());
+
+        blankSurface->disconnect(this);
+        prependSurface(blankSurface);
     }
 }
 

=== modified file 'src/modules/Unity/Application/session.h'
--- src/modules/Unity/Application/session.h	2016-03-28 18:02:26 +0000
+++ src/modules/Unity/Application/session.h	2016-05-10 21:06:43 +0000
@@ -107,6 +107,12 @@
     std::shared_ptr<mir::scene::Session> m_session;
     Application* m_application;
     MirSurfaceListModel m_surfaceList;
+
+    // Registered surfaces that haven't yet drawn their first frames.
+    // Once they do, they get moved to m_surfaceList.
+    // So this list is just a temporary holder and most of the time it's gonna be empty.
+    QList<MirSurfaceInterface*> m_blankSurfaces;
+
     SessionModel* m_children;
     bool m_fullscreen;
     State m_state;

=== modified file 'tests/modules/SessionManager/session_test.cpp'
--- tests/modules/SessionManager/session_test.cpp	2016-03-28 18:02:26 +0000
+++ tests/modules/SessionManager/session_test.cpp	2016-05-10 21:06:43 +0000
@@ -311,3 +311,46 @@
     EXPECT_EQ(Session::Stopped, session->state());
     EXPECT_FALSE(session->m_suspendTimer->isRunning());
 }
+
+/*
+    Regression test for https://bugs.launchpad.net/ubuntu/+source/qtmir/+bug/1578665
+
+    A child prompt session was arriving before the application surface got a chance to draw
+    its first frame. And on top of that, the application was blocked from drawing anything
+    until that prompt session gone away.
+ */
+TEST_F(SessionTests, ChildSessionArrivesBeforeSurfaceDrawnFirstFrame)
+{
+    using namespace testing;
+
+    const QString appId("test-app");
+    const pid_t procId = 5551;
+
+    auto mirSession = std::make_shared<MockSession>(appId.toStdString(), procId);
+
+    auto session = std::make_shared<qtmir::Session>(mirSession, promptSessionManager);
+
+    FakeMirSurface *blankSurface = new FakeMirSurface;
+    session->registerSurface(blankSurface);
+
+    // Surface doesn't appear in this list as it has not drawn its first frame yet
+    EXPECT_EQ(0, session->surfaceList()->count());
+
+    Session* childSession = new Session(mirSession, promptSessionManager);
+    session->addChildSession(childSession);
+
+    FakeMirSurface *promptSurface = new FakeMirSurface;
+    childSession->registerSurface(promptSurface);
+    promptSurface->drawFirstFrame();
+
+    // Even though blankSurface still hasn't drawn its first frame (ie, it's still blank),
+    // it should now appear in this list as it now has a child prompt surface, which the
+    // user must interact with.
+    auto sessionSurfaceList = static_cast<MirSurfaceListModel*>(session->surfaceList());
+    EXPECT_EQ(1, sessionSurfaceList->count());
+    EXPECT_TRUE(sessionSurfaceList->contains(blankSurface));
+
+    auto promptSurfaceList = static_cast<MirSurfaceListModel*>(blankSurface->promptSurfaceList());
+    EXPECT_EQ(1, promptSurfaceList->count());
+    EXPECT_TRUE(promptSurfaceList->contains(promptSurface));
+}