Merge lp:~alan-griffiths/qtmir/WindowControllerInterface-isVisible into lp:~unity-team/qtmir/miral-qt-integration

[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.

[lp:~alan-griffiths/qtmir/WindowControllerInterface-isVisible]

570. By Gerry Boland on 2016-11-23

Fix FTBFS due to new googletest framework release (1.8)

- needed to update the gmock-fixes.h to add Unwrap() support
- small signed/unsigned comparison fix flagged by GTest

Approved by: Daniel d'Andrada

571. By Gerry Boland on 2016-11-23

Revert Lttng test-crash workaround from rev 557

Approved by: Daniel d'Andrada, Unity8 CI Bot

572. By Jonas G. Drange on 2016-11-23

relax auth of clients to allow USS to set base mir display config

Approved by: Gerry Boland

573. By Albert Astals Cid on 2016-11-23

Build with Qt 5.7 (LP: #1642608, #1642954)

Approved by: Gerry Boland, Unity8 CI Bot

574. By CI Train Bot Account on 2016-11-23

Releasing 0.5.0+17.04.20161123.3-0ubuntu1

575. By Brandon Schaefer on 2016-12-03

Mir 0.25 compat

576. By CI Train Bot Account on 2016-12-03

Releasing 0.5.0+17.04.20161203-0ubuntu1

577. By CI Train Bot Account on 2016-12-16

First release using MirAL

578. By Daniel d'Andrada on 2016-12-16

Get rid of the ApplicationManager::Factory class

It's useless

Approved by: Gerry Boland, Unity8 CI Bot

579. By Daniel d'Andrada on 2016-12-16

Enable tests in Xenial

This workaround was needed only for the glib version present in Vivid

Approved by: Gerry Boland, Unity8 CI Bot

580. By CI Train Bot Account on 2016-12-16

Releasing 0.5.1+17.04.20161216-0ubuntu1

581. By Łukasz Zemczak on 2016-12-22

No-change rebuild against latest miral.

582. By Alan Griffiths on 2017-01-05

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

583. By Alan Griffiths on 2017-01-05

  Another null check

584. By Alan Griffiths on 2017-01-05

  Nasty hack because of existing nasty hack

585. By Alan Griffiths on 2017-01-05

  Put const in the Qt place

586. By Alan Griffiths on 2017-01-05

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

587. By Alan Griffiths on 2017-01-05

  Use StubWindowModelController in tests where nullptr won't do

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)

581. By Łukasz Zemczak on 2016-12-22

No-change rebuild against latest miral.

580. By CI Train Bot Account on 2016-12-16

Releasing 0.5.1+17.04.20161216-0ubuntu1

579. By Daniel d'Andrada on 2016-12-16

Enable tests in Xenial

This workaround was needed only for the glib version present in Vivid

Approved by: Gerry Boland, Unity8 CI Bot

578. By Daniel d'Andrada on 2016-12-16

Get rid of the ApplicationManager::Factory class

It's useless

Approved by: Gerry Boland, Unity8 CI Bot

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2016-12-13 15:31:26 +0000
+++ CMakeLists.txt	2017-01-05 10:18:12 +0000
@@ -166,10 +166,6 @@
 # Tests
 if (NO_TESTS)
     message(STATUS "Tests disabled")
-# glibc on Vivid+Overlay has a bug which causes LTTng-enabled tests to hang. It is hard to get the glibc version
-# in cmake, so it is easier to check for the LTTng on V+O and gate on that instead.
-elseif (LTTNG_VERSION VERSION_EQUAL 2.7.1)
-    message(STATUS "Tests disabled due to bug with glibc causing lttng to hang (lp:1618201)")
 else()
     include(CTest)
     enable_testing()

=== modified file 'debian/changelog'
--- debian/changelog	2016-12-13 15:31:26 +0000
+++ debian/changelog	2017-01-05 10:18:12 +0000
@@ -1,8 +1,19 @@
-qtmir (0.5.1) UNRELEASED; urgency=medium
-
+qtmir (0.5.1+17.04.20161216-0ubuntu2) zesty; urgency=medium
+
+  * No-change rebuild against latest miral.
+
+ -- Ĺukasz 'sil2100' Zemczak <[e-mail]>  Wed, 21 Dec 2016 10:07:36 +0100
+
+qtmir (0.5.1+17.04.20161216-0ubuntu1) zesty; urgency=medium
+
+  [ Gerry Boland ]
   * First release using MirAL
 
- -- Gerry Boland <[e-mail]>  Fri, 14 Oct 2016 16:51:26 +0100
+  [ Daniel d'Andrada ]
+  * Get rid of the ApplicationManager::Factory class
+  * Enable tests in Xenial
+
+ -- Daniel d'Andrada <[e-mail]>  Fri, 16 Dec 2016 08:25:00 +0000
 
 qtmir (0.5.0+17.04.20161203-0ubuntu1) zesty; urgency=medium
 

=== 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:18:12 +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/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2016-12-07 11:17:13 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2017-01-05 10:18:12 +0000
@@ -97,7 +97,7 @@
 
 } // namespace
 
-ApplicationManager* ApplicationManager::Factory::Factory::create()
+ApplicationManager* ApplicationManager::create()
 {
     NativeInterface *nativeInterface = dynamic_cast<NativeInterface*>(QGuiApplication::platformNativeInterface());
 
@@ -150,8 +150,7 @@
 {
     static ApplicationManager* instance;
     if (!instance) {
-        Factory appFactory;
-        instance = appFactory.create();
+        instance = create();
     }
     return instance;
 }

=== modified file 'src/modules/Unity/Application/application_manager.h'
--- src/modules/Unity/Application/application_manager.h	2016-12-07 11:17:13 +0000
+++ src/modules/Unity/Application/application_manager.h	2017-01-05 10:18:12 +0000
@@ -63,12 +63,7 @@
     Q_PROPERTY(bool empty READ isEmpty NOTIFY emptyChanged)
 
 public:
-    class Factory
-    {
-    public:
-        ApplicationManager* create();
-    };
-
+    static ApplicationManager* create();
     static ApplicationManager* singleton();
 
     explicit ApplicationManager(

=== 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:18:12 +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:18:12 +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:18:12 +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:18:12 +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:18:12 +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:18:12 +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:18:12 +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);