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

[Alan Griffiths]

[miral-qt] Implement WindowControllerInterface::isVisible(const miral::Window &window)

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

439. By Alan Griffiths on 2016-11-04

Put const in the Qt place

[Daniel d'Andrada (dandrader)]

"""
+ assert(m_surface.unique());
"""

We use Q_ASSERT() in Qt code.

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

440. By Alan Griffiths on 2016-11-04

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

[Alan Griffiths (alan-griffiths)]

> We use Q_ASSERT() in Qt code.

OK

[Gerry Boland (gerboland)]

Is a state getter/interpreter something that belongs in the Controller?
How do we know if the value returned by this changes?

+ const bool visible = m_controller && m_controller->isVisible(m_window);
there should always be a controller in MirSurface, no need to check for its validity.

review: Needs Information

[Daniel d'Andrada (dandrader)]

BTW, shouldn't we move our attention to lp:~unity-team/qtmir/miral-qt-integration ?
miral-qt-integration is what is going to land and actually get used.
If we don't shift our attention, it might not even land.

[Alan Griffiths (alan-griffiths)]

> Is a state getter/interpreter something that belongs in the Controller?
> How do we know if the value returned by this changes?
>
> + const bool visible = m_controller && m_controller->isVisible(m_window);
> there should always be a controller in MirSurface, no need to check for its
> validity.

The tests segfault without the check. But if you want I can do some rework there.

[Gerry Boland (gerboland)]

> The tests segfault without the check. But if you want I can do some rework
> there.
Yes please.

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

441. By Alan Griffiths on 2016-11-04

Use StubWindowModelController in tests where nullptr won't do

[Gerry Boland (gerboland)]

Thanks for this, but we will move all development of miral-qt to lp:~unity-team/qtmir/miral-qt-integration to make it easier to land - can you re-propose this change on top of that branch please

review: Disapprove

[Alan Griffiths (alan-griffiths)]

> Thanks for this, but we will move all development of miral-qt to lp:~unity-
> team/qtmir/miral-qt-integration to make it easier to land - can you re-propose
> this change on top of that branch please

Done: lp:~alan-griffiths/qtmir/WindowControllerInterface-isVisible/+merge/310184

Unmerged revisions

441. By Alan Griffiths on 2016-11-04

Use StubWindowModelController in tests where nullptr won't do

440. By Alan Griffiths on 2016-11-04

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

439. By Alan Griffiths on 2016-11-04

Put const in the Qt place

438. By Alan Griffiths on 2016-11-04

Nasty hack because of existing nasty hack

437. By Alan Griffiths on 2016-11-03

Another null check

436. By Alan Griffiths on 2016-11-03

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

=== modified file 'miral-qt/src/common/windowcontrollerinterface.h'
--- miral-qt/src/common/windowcontrollerinterface.h	2016-11-03 10:15:24 +0000
+++ miral-qt/src/common/windowcontrollerinterface.h	2016-11-04 15:54:01 +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 'miral-qt/src/modules/Unity/Application/mirsurface.cpp'
--- miral-qt/src/modules/Unity/Application/mirsurface.cpp	2016-11-03 17:06:04 +0000
+++ miral-qt/src/modules/Unity/Application/mirsurface.cpp	2016-11-04 15:54:01 +0000
@@ -424,11 +424,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 'miral-qt/src/platforms/mirserver/windowcontroller.cpp'
--- miral-qt/src/platforms/mirserver/windowcontroller.cpp	2016-11-03 10:15:24 +0000
+++ miral-qt/src/platforms/mirserver/windowcontroller.cpp	2016-11-04 15:54:01 +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 'miral-qt/src/platforms/mirserver/windowcontroller.h'
--- miral-qt/src/platforms/mirserver/windowcontroller.h	2016-11-03 10:15:24 +0000
+++ miral-qt/src/platforms/mirserver/windowcontroller.h	2016-11-04 15:54:01 +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 'miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp'
--- miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp	2016-11-03 16:03:29 +0000
+++ miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp	2016-11-04 15:54:01 +0000
@@ -340,3 +340,8 @@
         });
     }
 }
+
+bool WindowManagementPolicy::isVisible(const miral::Window &window)
+{
+    return window && m_tools.info_for(window).is_visible();
+}

=== modified file 'miral-qt/src/platforms/mirserver/windowmanagementpolicy.h'
--- miral-qt/src/platforms/mirserver/windowmanagementpolicy.h	2016-11-03 10:15:24 +0000
+++ miral-qt/src/platforms/mirserver/windowmanagementpolicy.h	2016-11-04 15:54:01 +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 'miral-qt/tests/framework/stub_windowcontroller.h'
--- miral-qt/tests/framework/stub_windowcontroller.h	2016-11-03 10:15:24 +0000
+++ miral-qt/tests/framework/stub_windowcontroller.h	2016-11-04 15:54:01 +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 'miral-qt/tests/modules/WindowManager/windowmodel_test.cpp'
--- miral-qt/tests/modules/WindowManager/windowmodel_test.cpp	2016-10-06 20:56:04 +0000
+++ miral-qt/tests/modules/WindowManager/windowmodel_test.cpp	2016-11-04 15:54:01 +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);