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

Proposed by Alan Griffiths
Status: Rejected
Rejected by: Alan Griffiths
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 (community) Disapprove
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

Put const in the Qt place

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

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

We use Q_ASSERT() in Qt code.

440. By Alan Griffiths

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

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> We use Q_ASSERT() in Qt code.

OK

Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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

Use StubWindowModelController in tests where nullptr won't do

Revision history for this message
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
Revision history for this message
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

Use StubWindowModelController in tests where nullptr won't do

440. By Alan Griffiths

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

439. By Alan Griffiths

Put const in the Qt place

438. By Alan Griffiths

Nasty hack because of existing nasty hack

437. By Alan Griffiths

Another null check

436. By Alan Griffiths

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
4848
49 virtual void requestState(const miral::Window &window, const Mir::State state) = 0;49 virtual void requestState(const miral::Window &window, const Mir::State state) = 0;
5050
51 virtual bool isVisible(const miral::Window &window) = 0;
52
51 virtual void deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event) = 0;53 virtual void deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event) = 0;
52 virtual void deliverTouchEvent (const miral::Window &window, const MirTouchEvent *event) = 0;54 virtual void deliverTouchEvent (const miral::Window &window, const MirTouchEvent *event) = 0;
53 virtual void deliverPointerEvent (const miral::Window &window, const MirPointerEvent *event) = 0;55 virtual void deliverPointerEvent (const miral::Window &window, const MirPointerEvent *event) = 0;
5456
=== 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 @@
424424
425void MirSurface::updateVisible()425void MirSurface::updateVisible()
426{426{
427 const bool visible = !(m_state == Mir::HiddenState || m_state == Mir::MinimizedState) && m_surface->visible();427 try
428428 {
429 if (m_visible != visible) {429 const bool visible = m_controller->isVisible(m_window);
430 m_visible = visible;430
431 Q_EMIT visibleChanged(visible);431 if (m_visible != visible) {
432 m_visible = visible;
433 Q_EMIT visibleChanged(visible);
434 }
435 }
436 catch (const std::out_of_range &)
437 {
438 // MirAL can not know we hold ownership of the underlying surface and may have forgotten it
439 Q_ASSERT(m_surface.unique());
440
441 if (m_visible) {
442 m_visible = false;
443 Q_EMIT visibleChanged(false);
444 }
432 }445 }
433}446}
434447
435448
=== 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 @@
76 }76 }
77}77}
7878
79bool WindowController::isVisible(const miral::Window &window)
80{
81 return m_policy && m_policy->isVisible(window);
82}
83
79void WindowController::deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event)84void WindowController::deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event)
80{85{
81 if (m_policy) {86 if (m_policy) {
8287
=== 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 @@
3939
40 void requestState(const miral::Window &window, const Mir::State state) override;40 void requestState(const miral::Window &window, const Mir::State state) override;
4141
42 bool isVisible(const miral::Window &window) override;
43
42 void deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event) override;44 void deliverKeyboardEvent(const miral::Window &window, const MirKeyboardEvent *event) override;
43 void deliverTouchEvent (const miral::Window &window, const MirTouchEvent *event) override;45 void deliverTouchEvent (const miral::Window &window, const MirTouchEvent *event) override;
44 void deliverPointerEvent (const miral::Window &window, const MirPointerEvent *event) override;46 void deliverPointerEvent (const miral::Window &window, const MirPointerEvent *event) override;
4547
=== 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 @@
340 });340 });
341 }341 }
342}342}
343
344bool WindowManagementPolicy::isVisible(const miral::Window &window)
345{
346 return window && m_tools.info_for(window).is_visible();
347}
343348
=== 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 @@
80 void move (const miral::Window &window, const Point topLeft);80 void move (const miral::Window &window, const Point topLeft);
81 void raise(const miral::Window &window);81 void raise(const miral::Window &window);
82 void requestState(const miral::Window &window, const Mir::State state);82 void requestState(const miral::Window &window, const Mir::State state);
83 bool isVisible(const miral::Window &window);
8384
84 void ask_client_to_close(const miral::Window &window);85 void ask_client_to_close(const miral::Window &window);
85 void forceClose(const miral::Window &window);86 void forceClose(const miral::Window &window);
8687
=== 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 @@
33 void forceClose(const miral::Window &/*window*/) override { return; }33 void forceClose(const miral::Window &/*window*/) override { return; }
3434
35 void requestState(const miral::Window &/*window*/, const Mir::State /*state*/) override { return; }35 void requestState(const miral::Window &/*window*/, const Mir::State /*state*/) override { return; }
36 bool isVisible(const miral::Window &) override { return false; }
3637
37 void deliverKeyboardEvent(const miral::Window &/*window*/, const MirKeyboardEvent */*event*/) override { return; }38 void deliverKeyboardEvent(const miral::Window &/*window*/, const MirKeyboardEvent */*event*/) override { return; }
38 void deliverTouchEvent (const miral::Window &/*window*/, const MirTouchEvent */*event*/) override { return; }39 void deliverTouchEvent (const miral::Window &/*window*/, const MirTouchEvent */*event*/) override { return; }
3940
=== 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 @@
25#include "Unity/Application/mirsurface.h"25#include "Unity/Application/mirsurface.h"
26#include "Unity/Application/windowmodel.h"26#include "Unity/Application/windowmodel.h"
2727
28#include "stub_windowcontroller.h"
28#include <mir/test/doubles/stub_surface.h>29#include <mir/test/doubles/stub_surface.h>
29#include <mir/test/doubles/stub_session.h>30#include <mir/test/doubles/stub_session.h>
3031
@@ -123,6 +124,8 @@
123 const std::shared_ptr<StubSession> stubSession{std::make_shared<StubSession>()};124 const std::shared_ptr<StubSession> stubSession{std::make_shared<StubSession>()};
124 const std::shared_ptr<SizedStubSurface> stubSurface{std::make_shared<SizedStubSurface>()};125 const std::shared_ptr<SizedStubSurface> stubSurface{std::make_shared<SizedStubSurface>()};
125 QCoreApplication *qtApp;126 QCoreApplication *qtApp;
127
128 StubWindowModelController controller;
126};129};
127130
128/*131/*
@@ -688,7 +691,7 @@
688TEST_F(WindowModelTest, WindowReadyCausesMirSurfaceToEmitReadySignal)691TEST_F(WindowModelTest, WindowReadyCausesMirSurfaceToEmitReadySignal)
689{692{
690 WindowModelNotifier notifier;693 WindowModelNotifier notifier;
691 WindowModel model(&notifier, nullptr); // no need for controller in this testcase694 WindowModel model(&notifier, &controller);
692695
693 auto newWindow = createNewWindow();696 auto newWindow = createNewWindow();
694 notifier.windowAdded(newWindow);697 notifier.windowAdded(newWindow);
@@ -745,7 +748,7 @@
745 auto const& param = GetParam();748 auto const& param = GetParam();
746749
747 WindowModelNotifier notifier;750 WindowModelNotifier notifier;
748 WindowModel model(&notifier, nullptr); // no need for controller in this testcase751 WindowModel model(&notifier, &controller);
749752
750 auto newWindow = createNewWindowWithState(Mir::UnknownState);753 auto newWindow = createNewWindowWithState(Mir::UnknownState);
751 notifier.windowAdded(newWindow);754 notifier.windowAdded(newWindow);
@@ -766,7 +769,7 @@
766 auto const& param = GetParam();769 auto const& param = GetParam();
767770
768 WindowModelNotifier notifier;771 WindowModelNotifier notifier;
769 WindowModel model(&notifier, nullptr); // no need for controller in this testcase772 WindowModel model(&notifier, &controller);
770773
771 auto newWindow = createNewWindowWithState(Mir::UnknownState);774 auto newWindow = createNewWindowWithState(Mir::UnknownState);
772 notifier.windowAdded(newWindow);775 notifier.windowAdded(newWindow);

Subscribers

People subscribed via source and target branches