Merge lp:~alan-griffiths/qtmir/WindowControllerInterface-isVisible into lp:qtmir

[Alan Griffiths]

Implement WindowControllerInterface::isVisible(const miral::Window &window)

[Daniel d'Andrada (dandrader)]

I don't get why we need the try{}catch{} in MirSurface::updateVisible()

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> I don't get why we need the try{}catch{} in MirSurface::updateVisible()

The m_surface member is an awkward hack that extends the life of a ms:Surface object beyond the time it is known to MirAL and Mir. (I hope, eventually, to make this unnecessary.)

This lifetime extension makes it difficult to detect a race between a surface closing (as far as the rest of the system is concerned) and MirSurface::updateVisible() being called.

The only convenient point at which to detect this race is in miral::BasicWindowManager::info_for() which throws out_of_range{}. The try...catch detects this, asserts that the cause is m_surface extending the lifetime and suppresses the "error".

One scenario I've found to reliably reproduce this problem is:

qml-demo-shell --startup gnome-terminal

and then close the terminal through the file menu

[Daniel d'Andrada (dandrader)]

ok

review: Abstain

[Daniel d'Andrada (dandrader)]

This would leave the code in a inconsistent state. We update MirSurface::visible when state changes but MirSurface::visible itself is defined by an opaque function from an external controller. So no guarantees that controller::isVisible indeed changes when surface state changes and, most importantly, *only* when surface state changes.

If controller::isVisible changes due to thins other than surface state changes, this MirSurface::visible won't get update and will, therefore, be broken.

I think this change should be accompanied by a notification miral::WindowInfo::visible changes from miral.

Which makes me think that, while the miral notification doesn't come up in the API, we either drop the "&& m_surface->visible()" from updateVisible or also track changes to it if possible (which I think it's not)

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> This would leave the code in a inconsistent state. We update
> MirSurface::visible when state changes but MirSurface::visible itself is
> defined by an opaque function from an external controller. So no guarantees
> that controller::isVisible indeed changes when surface state changes and, most
> importantly, *only* when surface state changes.

This MP does not effect any of that.

> If controller::isVisible changes due to thins other than surface state
> changes, this MirSurface::visible won't get update and will, therefore, be
> broken.
>
> I think this change should be accompanied by a notification
> miral::WindowInfo::visible changes from miral.
>
> Which makes me think that, while the miral notification doesn't come up in the
> API, we either drop the "&& m_surface->visible()" from updateVisible or also
> track changes to it if possible (which I think it's not)

That would change the meaning of MirSurface::visible - I don't know enough of the purpose of this property to know if that's a good idea.

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

PASSED: Continuous integration, rev:587
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/438/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/3703
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/3731
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/3575
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/3575/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/3575
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/3575/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/3575
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/3575/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/3575
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/3575/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/3575
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/3575/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/3575
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/3575/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel d'Andrada (dandrader)]

This must be accompanied by miral notifying us of changes in miral::WindowInfo::is_visible().

review: Needs Fixing

Unmerged revisions

587. By Alan Griffiths on 2017-01-05

  Use StubWindowModelController in tests where nullptr won't do

586. By Alan Griffiths on 2017-01-05

  Don't use the standard library when Qt has an alternative

585. By Alan Griffiths on 2017-01-05

  Put const in the Qt place

584. By Alan Griffiths on 2017-01-05

  Nasty hack because of existing nasty hack

583. By Alan Griffiths on 2017-01-05

  Another null check

582. By Alan Griffiths on 2017-01-05

  Implement WindowControllerInterface::isVisible(const miral::Window &window)

=== modified file 'src/common/windowcontrollerinterface.h'
--- src/common/windowcontrollerinterface.h	2016-11-03 20:17:46 +0000
+++ src/common/windowcontrollerinterface.h	2017-01-05 10:19:23 +0000
@@ -48,6 +48,8 @@
 
     virtual void requestState(const miral::Window &window, const Mir::State state) = 0;
 
+    virtual bool isVisible(const miral::Window &window) = 0;
+
     virtual void deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event) = 0;
     virtual void deliverTouchEvent   (const miral::Window &window, const MirTouchEvent *event) = 0;
     virtual void deliverPointerEvent (const miral::Window &window, const MirPointerEvent *event) = 0;
 
=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
--- src/modules/Unity/Application/mirsurface.cpp	2016-11-25 15:23:36 +0000
+++ src/modules/Unity/Application/mirsurface.cpp	2017-01-05 10:19:23 +0000
@@ -427,11 +427,24 @@
 
 void MirSurface::updateVisible()
 {
-    const bool visible = !(m_state == Mir::HiddenState || m_state == Mir::MinimizedState) && m_surface->visible();
-
-    if (m_visible != visible) {
-        m_visible = visible;
-        Q_EMIT visibleChanged(visible);
+    try
+    {
+        const bool visible = m_controller->isVisible(m_window);
+
+        if (m_visible != visible) {
+            m_visible = visible;
+            Q_EMIT visibleChanged(visible);
+        }
+    }
+    catch (const std::out_of_range &)
+    {
+        // MirAL can not know we hold ownership of the underlying surface and may have forgotten it
+        Q_ASSERT(m_surface.unique());
+
+        if (m_visible) {
+            m_visible = false;
+            Q_EMIT visibleChanged(false);
+        }
     }
 }
 
 
=== modified file 'src/platforms/mirserver/windowcontroller.cpp'
--- src/platforms/mirserver/windowcontroller.cpp	2016-11-03 20:17:46 +0000
+++ src/platforms/mirserver/windowcontroller.cpp	2017-01-05 10:19:23 +0000
@@ -76,6 +76,11 @@
     }
 }
 
+bool WindowController::isVisible(const miral::Window &window)
+{
+    return  m_policy && m_policy->isVisible(window);
+}
+
 void WindowController::deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event)
 {
     if (m_policy) {
 
=== modified file 'src/platforms/mirserver/windowcontroller.h'
--- src/platforms/mirserver/windowcontroller.h	2016-11-03 20:17:46 +0000
+++ src/platforms/mirserver/windowcontroller.h	2017-01-05 10:19:23 +0000
@@ -39,6 +39,8 @@
 
     void requestState(const miral::Window &window, const Mir::State state) override;
 
+    bool isVisible(const miral::Window &window) override;
+
     void deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event) override;
     void deliverTouchEvent   (const miral::Window &window, const MirTouchEvent *event) override;
     void deliverPointerEvent (const miral::Window &window, const MirPointerEvent *event) override;
 
=== modified file 'src/platforms/mirserver/windowmanagementpolicy.cpp'
--- src/platforms/mirserver/windowmanagementpolicy.cpp	2016-11-22 18:01:11 +0000
+++ src/platforms/mirserver/windowmanagementpolicy.cpp	2017-01-05 10:19:23 +0000
@@ -333,3 +333,8 @@
         });
     }
 }
+
+bool WindowManagementPolicy::isVisible(const miral::Window &window)
+{
+    return window && m_tools.info_for(window).is_visible();
+}
 
=== modified file 'src/platforms/mirserver/windowmanagementpolicy.h'
--- src/platforms/mirserver/windowmanagementpolicy.h	2016-11-03 20:17:46 +0000
+++ src/platforms/mirserver/windowmanagementpolicy.h	2017-01-05 10:19:23 +0000
@@ -80,6 +80,7 @@
     void move  (const miral::Window &window, const Point topLeft);
     void raise(const miral::Window &window);
     void requestState(const miral::Window &window, const Mir::State state);
+    bool isVisible(const miral::Window &window);
 
     void ask_client_to_close(const miral::Window &window);
     void forceClose(const miral::Window &window);
 
=== modified file 'tests/framework/stub_windowcontroller.h'
--- tests/framework/stub_windowcontroller.h	2016-11-03 20:17:46 +0000
+++ tests/framework/stub_windowcontroller.h	2017-01-05 10:19:23 +0000
@@ -33,6 +33,7 @@
     void forceClose(const miral::Window &/*window*/) override { return; }
 
     void requestState(const miral::Window &/*window*/, const Mir::State /*state*/) override { return; }
+    bool isVisible(const miral::Window &) override { return false; }
 
     void deliverKeyboardEvent(const miral::Window &/*window*/, const MirKeyboardEvent */*event*/) override { return; }
     void deliverTouchEvent   (const miral::Window &/*window*/, const MirTouchEvent */*event*/)    override { return; }
 
=== modified file 'tests/modules/WindowManager/windowmodel_test.cpp'
--- tests/modules/WindowManager/windowmodel_test.cpp	2016-11-03 20:17:46 +0000
+++ tests/modules/WindowManager/windowmodel_test.cpp	2017-01-05 10:19:23 +0000
@@ -25,6 +25,7 @@
 #include "Unity/Application/mirsurface.h"
 #include "Unity/Application/windowmodel.h"
 
+#include "stub_windowcontroller.h"
 #include <mir/test/doubles/stub_surface.h>
 #include <mir/test/doubles/stub_session.h>
 
@@ -123,6 +124,8 @@
     const std::shared_ptr<StubSession> stubSession{std::make_shared<StubSession>()};
     const std::shared_ptr<SizedStubSurface> stubSurface{std::make_shared<SizedStubSurface>()};
     QCoreApplication *qtApp;
+
+    StubWindowModelController controller;
 };
 
 /*
@@ -688,7 +691,7 @@
 TEST_F(WindowModelTest, WindowReadyCausesMirSurfaceToEmitReadySignal)
 {
     WindowModelNotifier notifier;
-    WindowModel model(&notifier, nullptr); // no need for controller in this testcase
+    WindowModel model(&notifier, &controller);
 
     auto newWindow = createNewWindow();
     notifier.windowAdded(newWindow);
@@ -745,7 +748,7 @@
     auto const& param = GetParam();
 
     WindowModelNotifier notifier;
-    WindowModel model(&notifier, nullptr); // no need for controller in this testcase
+    WindowModel model(&notifier, &controller);
 
     auto newWindow = createNewWindowWithState(Mir::UnknownState);
     notifier.windowAdded(newWindow);
@@ -766,7 +769,7 @@
     auto const& param = GetParam();
 
     WindowModelNotifier notifier;
-    WindowModel model(&notifier, nullptr); // no need for controller in this testcase
+    WindowModel model(&notifier, &controller);
 
     auto newWindow = createNewWindowWithState(Mir::UnknownState);
     notifier.windowAdded(newWindow);