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

Proposed by Alan Griffiths on 2016-11-04
Status: Rejected
Rejected by: Alan Griffiths on 2016-11-07
Proposed branch: lp:~alan-griffiths/miral/WindowControllerInterface-isVisible
Merge into: lp:miral
Diff against target: 160 lines (+40/-8)
8 files modified
miral-qt/src/common/windowcontrollerinterface.h (+2/-0)
miral-qt/src/modules/Unity/Application/mirsurface.cpp (+18/-5)
miral-qt/src/platforms/mirserver/windowcontroller.cpp (+5/-0)
miral-qt/src/platforms/mirserver/windowcontroller.h (+2/-0)
miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp (+5/-0)
miral-qt/src/platforms/mirserver/windowmanagementpolicy.h (+1/-0)
miral-qt/tests/framework/stub_windowcontroller.h (+1/-0)
miral-qt/tests/modules/WindowManager/windowmodel_test.cpp (+6/-3)
To merge this branch: bzr merge lp:~alan-griffiths/miral/WindowControllerInterface-isVisible
Reviewer Review Type Date Requested Status
Gerry Boland 2016-11-04 Disapprove on 2016-11-07
Review via email: mp+310039@code.launchpad.net

Commit message

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

To post a comment you must log in.
439. By Alan Griffiths on 2016-11-04

Put const in the Qt place

Daniel d'Andrada (dandrader) wrote :

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

We use Q_ASSERT() in Qt code.

440. By Alan Griffiths on 2016-11-04

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

Alan Griffiths (alan-griffiths) wrote :

> We use Q_ASSERT() in Qt code.

OK

Gerry Boland (gerboland) wrote :

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) wrote :

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) wrote :

> 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) wrote :

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

441. By Alan Griffiths on 2016-11-04

Use StubWindowModelController in tests where nullptr won't do

Gerry Boland (gerboland) wrote :

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) wrote :

> 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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'miral-qt/src/common/windowcontrollerinterface.h'
2--- miral-qt/src/common/windowcontrollerinterface.h 2016-11-03 10:15:24 +0000
3+++ miral-qt/src/common/windowcontrollerinterface.h 2016-11-04 15:54:01 +0000
4@@ -48,6 +48,8 @@
5
6 virtual void requestState(const miral::Window &window, const Mir::State state) = 0;
7
8+ virtual bool isVisible(const miral::Window &window) = 0;
9+
10 virtual void deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event) = 0;
11 virtual void deliverTouchEvent (const miral::Window &window, const MirTouchEvent *event) = 0;
12 virtual void deliverPointerEvent (const miral::Window &window, const MirPointerEvent *event) = 0;
13
14=== modified file 'miral-qt/src/modules/Unity/Application/mirsurface.cpp'
15--- miral-qt/src/modules/Unity/Application/mirsurface.cpp 2016-11-03 17:06:04 +0000
16+++ miral-qt/src/modules/Unity/Application/mirsurface.cpp 2016-11-04 15:54:01 +0000
17@@ -424,11 +424,24 @@
18
19 void MirSurface::updateVisible()
20 {
21- const bool visible = !(m_state == Mir::HiddenState || m_state == Mir::MinimizedState) && m_surface->visible();
22-
23- if (m_visible != visible) {
24- m_visible = visible;
25- Q_EMIT visibleChanged(visible);
26+ try
27+ {
28+ const bool visible = m_controller->isVisible(m_window);
29+
30+ if (m_visible != visible) {
31+ m_visible = visible;
32+ Q_EMIT visibleChanged(visible);
33+ }
34+ }
35+ catch (const std::out_of_range &)
36+ {
37+ // MirAL can not know we hold ownership of the underlying surface and may have forgotten it
38+ Q_ASSERT(m_surface.unique());
39+
40+ if (m_visible) {
41+ m_visible = false;
42+ Q_EMIT visibleChanged(false);
43+ }
44 }
45 }
46
47
48=== modified file 'miral-qt/src/platforms/mirserver/windowcontroller.cpp'
49--- miral-qt/src/platforms/mirserver/windowcontroller.cpp 2016-11-03 10:15:24 +0000
50+++ miral-qt/src/platforms/mirserver/windowcontroller.cpp 2016-11-04 15:54:01 +0000
51@@ -76,6 +76,11 @@
52 }
53 }
54
55+bool WindowController::isVisible(const miral::Window &window)
56+{
57+ return m_policy && m_policy->isVisible(window);
58+}
59+
60 void WindowController::deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event)
61 {
62 if (m_policy) {
63
64=== modified file 'miral-qt/src/platforms/mirserver/windowcontroller.h'
65--- miral-qt/src/platforms/mirserver/windowcontroller.h 2016-11-03 10:15:24 +0000
66+++ miral-qt/src/platforms/mirserver/windowcontroller.h 2016-11-04 15:54:01 +0000
67@@ -39,6 +39,8 @@
68
69 void requestState(const miral::Window &window, const Mir::State state) override;
70
71+ bool isVisible(const miral::Window &window) override;
72+
73 void deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event) override;
74 void deliverTouchEvent (const miral::Window &window, const MirTouchEvent *event) override;
75 void deliverPointerEvent (const miral::Window &window, const MirPointerEvent *event) override;
76
77=== modified file 'miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp'
78--- miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp 2016-11-03 16:03:29 +0000
79+++ miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp 2016-11-04 15:54:01 +0000
80@@ -340,3 +340,8 @@
81 });
82 }
83 }
84+
85+bool WindowManagementPolicy::isVisible(const miral::Window &window)
86+{
87+ return window && m_tools.info_for(window).is_visible();
88+}
89
90=== modified file 'miral-qt/src/platforms/mirserver/windowmanagementpolicy.h'
91--- miral-qt/src/platforms/mirserver/windowmanagementpolicy.h 2016-11-03 10:15:24 +0000
92+++ miral-qt/src/platforms/mirserver/windowmanagementpolicy.h 2016-11-04 15:54:01 +0000
93@@ -80,6 +80,7 @@
94 void move (const miral::Window &window, const Point topLeft);
95 void raise(const miral::Window &window);
96 void requestState(const miral::Window &window, const Mir::State state);
97+ bool isVisible(const miral::Window &window);
98
99 void ask_client_to_close(const miral::Window &window);
100 void forceClose(const miral::Window &window);
101
102=== modified file 'miral-qt/tests/framework/stub_windowcontroller.h'
103--- miral-qt/tests/framework/stub_windowcontroller.h 2016-11-03 10:15:24 +0000
104+++ miral-qt/tests/framework/stub_windowcontroller.h 2016-11-04 15:54:01 +0000
105@@ -33,6 +33,7 @@
106 void forceClose(const miral::Window &/*window*/) override { return; }
107
108 void requestState(const miral::Window &/*window*/, const Mir::State /*state*/) override { return; }
109+ bool isVisible(const miral::Window &) override { return false; }
110
111 void deliverKeyboardEvent(const miral::Window &/*window*/, const MirKeyboardEvent */*event*/) override { return; }
112 void deliverTouchEvent (const miral::Window &/*window*/, const MirTouchEvent */*event*/) override { return; }
113
114=== modified file 'miral-qt/tests/modules/WindowManager/windowmodel_test.cpp'
115--- miral-qt/tests/modules/WindowManager/windowmodel_test.cpp 2016-10-06 20:56:04 +0000
116+++ miral-qt/tests/modules/WindowManager/windowmodel_test.cpp 2016-11-04 15:54:01 +0000
117@@ -25,6 +25,7 @@
118 #include "Unity/Application/mirsurface.h"
119 #include "Unity/Application/windowmodel.h"
120
121+#include "stub_windowcontroller.h"
122 #include <mir/test/doubles/stub_surface.h>
123 #include <mir/test/doubles/stub_session.h>
124
125@@ -123,6 +124,8 @@
126 const std::shared_ptr<StubSession> stubSession{std::make_shared<StubSession>()};
127 const std::shared_ptr<SizedStubSurface> stubSurface{std::make_shared<SizedStubSurface>()};
128 QCoreApplication *qtApp;
129+
130+ StubWindowModelController controller;
131 };
132
133 /*
134@@ -688,7 +691,7 @@
135 TEST_F(WindowModelTest, WindowReadyCausesMirSurfaceToEmitReadySignal)
136 {
137 WindowModelNotifier notifier;
138- WindowModel model(&notifier, nullptr); // no need for controller in this testcase
139+ WindowModel model(&notifier, &controller);
140
141 auto newWindow = createNewWindow();
142 notifier.windowAdded(newWindow);
143@@ -745,7 +748,7 @@
144 auto const& param = GetParam();
145
146 WindowModelNotifier notifier;
147- WindowModel model(&notifier, nullptr); // no need for controller in this testcase
148+ WindowModel model(&notifier, &controller);
149
150 auto newWindow = createNewWindowWithState(Mir::UnknownState);
151 notifier.windowAdded(newWindow);
152@@ -766,7 +769,7 @@
153 auto const& param = GetParam();
154
155 WindowModelNotifier notifier;
156- WindowModel model(&notifier, nullptr); // no need for controller in this testcase
157+ WindowModel model(&notifier, &controller);
158
159 auto newWindow = createNewWindowWithState(Mir::UnknownState);
160 notifier.windowAdded(newWindow);

Subscribers

People subscribed via source and target branches