QtMir

Merge lp:~alan-griffiths/qtmir/small-refactoring-of-MirWindowManager into lp:qtmir

  1. small-refactoring-of-MirWindowManager
  2. Merge into trunk
Proposed by Alan Griffiths on 2015-08-03
Status: Merged
Approved by: Gerry Boland on 2015-09-29
Approved revision: 358
Merged at revision: 392
Proposed branch: lp:~alan-griffiths/qtmir/small-refactoring-of-MirWindowManager
Merge into: lp:qtmir
Prerequisite: lp:~alan-griffiths/qtmir/use-WindowManager
Diff against target: 261 lines (+99/-79)
4 files modified
CMakeLists.txt (+1/-1)
src/platforms/mirserver/mirserver.cpp (+2/-2)
src/platforms/mirserver/mirwindowmanager.cpp (+92/-41)
src/platforms/mirserver/mirwindowmanager.h (+4/-35)
To merge this branch: bzr merge lp:~alan-griffiths/qtmir/small-refactoring-of-MirWindowManager
Reviewer Review Type Date Requested Status
Gerry Boland 2015-08-03 Approve on 2015-09-29
PS Jenkins bot continuous-integration Approve on 2015-09-28
Review via email: mp+266698@code.launchpad.net

Commit Message

Opaquify MirWindowManager to control visibility of upcoming Window Management work

Description of the Change

Opaquify MirWindowManager to control visibility of upcoming Window Management work

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
lp:~alan-griffiths/qtmir/small-refactoring-of-MirWindowManager updated on 2015-08-03
352. By Alan Griffiths on 2015-08-03

Also pass the focus controller (will need it later)

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Gerry Boland (gerboland) wrote :

+auto MirWindowManagerImpl::add_surface(
am not a fan of using "auto" unless the type is completely unmistakeably obvious. Here I can't quite deduce, is it a bool to say surface was added successfully? Or a shared/unique pointer to that new surface? Or a list of surfaces?

The C++ committee and I don't agree on some things just yet ;)

+auto MirWindowManager::create() -> std::unique_ptr<MirWindowManager>
For consistency I'd prefer you avoid this new C++ syntax. Few are comfortable with it at the moment IMO.

Are those the only reasons for using C++14?

I'm open to a good argument saying why it'll be good to use, but the only reason I can really see for using auto in function definitions is if we'd like to use duck-typing in C++, which isn't necessary here IMO.

review: Needs Fixing
Alan Griffiths (alan-griffiths) wrote :

> +auto MirWindowManagerImpl::add_surface(
> am not a fan of using "auto" unless the type is completely unmistakeably
> obvious. Here I can't quite deduce, is it a bool to say surface was added
> successfully? Or a shared/unique pointer to that new surface? Or a list of
> surfaces?
>
> The C++ committee and I don't agree on some things just yet ;)
>
> +auto MirWindowManager::create() -> std::unique_ptr<MirWindowManager>
> For consistency I'd prefer you avoid this new C++ syntax. Few are comfortable
> with it at the moment IMO.
>
> Are those the only reasons for using C++14?

No, those are all C++11. The use of C++14 is:

+ return std::make_unique<MirWindowManagerImpl>(displayLayout);

Alan Griffiths (alan-griffiths) wrote :

> +auto MirWindowManagerImpl::add_surface(
> am not a fan of using "auto" unless the type is completely unmistakeably
> obvious. Here I can't quite deduce, is it a bool to say surface was added
> successfully? Or a shared/unique pointer to that new surface? Or a list of
> surfaces?

FWIW That's preexisting - see line 98

lp:~alan-griffiths/qtmir/small-refactoring-of-MirWindowManager updated on 2015-09-04
353. By Alan Griffiths on 2015-09-04

merge --weave lp:qtmir

354. By Alan Griffiths on 2015-09-04

Old fashioned function definitions for Gerry

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~alan-griffiths/qtmir/small-refactoring-of-MirWindowManager updated on 2015-09-07
355. By Alan Griffiths on 2015-09-07

No change for Jenkns

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

The following packages have unmet dependencies:
 udev : Depends: libudev1 (= 225-1ubuntu1) but 225-1ubuntu2 is to be installed.

Gerry Boland (gerboland) wrote :

+ static auto create(
+ mir::shell::FocusController* focus_controller,
+ const std::shared_ptr<mir::shell::DisplayLayout> &displayLayout)
+ -> std::unique_ptr<MirWindowManager>;
We still have one new-fangled definition here.

review: Needs Fixing
lp:~alan-griffiths/qtmir/small-refactoring-of-MirWindowManager updated on 2015-09-09
356. By Alan Griffiths on 2015-09-09

If at first Gerry's not happy, try again...

Alan Griffiths (alan-griffiths) wrote :

> + static auto create(
> + mir::shell::FocusController* focus_controller,
> + const std::shared_ptr<mir::shell::DisplayLayout> &displayLayout)
> + -> std::unique_ptr<MirWindowManager>;
> We still have one new-fangled definition here.

that's a declaration. :^)

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Gerry Boland (gerboland) wrote :

Gerry happy :)

review: Approve
lp:~alan-griffiths/qtmir/small-refactoring-of-MirWindowManager updated on 2015-09-28
357. By Alan Griffiths on 2015-09-24

merge lp:qtmir

358. By Alan Griffiths on 2015-09-28

merge lp:qtmir

PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:358
http://jenkins.qa.ubuntu.com/job/qtmir-ci/445/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/141
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/141
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/141/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/23
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/178
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/178
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/178/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/23

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/445/rebuild

review: Approve (continuous-integration)
Gerry Boland (gerboland) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2015-09-17 20:17:17 +0000
3+++ CMakeLists.txt 2015-09-28 14:52:47 +0000
4@@ -20,7 +20,7 @@
5 set(CMAKE_AUTOMOC ON)
6
7 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=c99 -Wall -Wextra -Werror")
8-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -fno-strict-aliasing -Werror -Wextra")
9+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++14 -Wall -fno-strict-aliasing -Werror -Wextra")
10 set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--no-undefined")
11
12
13
14=== modified file 'src/platforms/mirserver/mirserver.cpp'
15--- src/platforms/mirserver/mirserver.cpp 2015-08-11 12:08:32 +0000
16+++ src/platforms/mirserver/mirserver.cpp 2015-09-28 14:52:47 +0000
17@@ -86,10 +86,10 @@
18 return std::make_shared<MirServerStatusListener>();
19 });
20
21- override_the_window_manager_builder([this](mir::shell::FocusController* /*focus_controller*/)
22+ override_the_window_manager_builder([this](mir::shell::FocusController* focus_controller)
23 -> std::shared_ptr<mir::shell::WindowManager>
24 {
25- return std::make_shared<MirWindowManager>(the_shell_display_layout());
26+ return {MirWindowManager::create(focus_controller, the_shell_display_layout())};
27 });
28
29 set_terminator([&](int)
30
31=== modified file 'src/platforms/mirserver/mirwindowmanager.cpp'
32--- src/platforms/mirserver/mirwindowmanager.cpp 2015-08-11 12:08:32 +0000
33+++ src/platforms/mirserver/mirwindowmanager.cpp 2015-09-28 14:52:47 +0000
34@@ -26,25 +26,69 @@
35
36 namespace ms = mir::scene;
37
38-MirWindowManager::MirWindowManager(const std::shared_ptr<mir::shell::DisplayLayout> &displayLayout) :
39+namespace
40+{
41+class MirWindowManagerImpl : public MirWindowManager
42+{
43+public:
44+
45+ MirWindowManagerImpl(const std::shared_ptr<mir::shell::DisplayLayout> &displayLayout);
46+
47+ void add_session(std::shared_ptr<mir::scene::Session> const& session) override;
48+
49+ void remove_session(std::shared_ptr<mir::scene::Session> const& session) override;
50+
51+ mir::frontend::SurfaceId add_surface(
52+ std::shared_ptr<mir::scene::Session> const& session,
53+ mir::scene::SurfaceCreationParameters const& params,
54+ std::function<mir::frontend::SurfaceId(std::shared_ptr<mir::scene::Session> const& session, mir::scene::SurfaceCreationParameters const& params)> const& build) override;
55+
56+ void remove_surface(
57+ std::shared_ptr<mir::scene::Session> const& session,
58+ std::weak_ptr<mir::scene::Surface> const& surface) override;
59+
60+ void add_display(mir::geometry::Rectangle const& area) override;
61+
62+ void remove_display(mir::geometry::Rectangle const& area) override;
63+
64+ bool handle_keyboard_event(MirKeyboardEvent const* event) override;
65+
66+ bool handle_touch_event(MirTouchEvent const* event) override;
67+
68+ bool handle_pointer_event(MirPointerEvent const* event) override;
69+
70+ int set_surface_attribute(
71+ std::shared_ptr<mir::scene::Session> const& session,
72+ std::shared_ptr<mir::scene::Surface> const& surface,
73+ MirSurfaceAttrib attrib,
74+ int value) override;
75+
76+ void modify_surface(const std::shared_ptr<mir::scene::Session>&, const std::shared_ptr<mir::scene::Surface>&, const mir::shell::SurfaceSpecification&);
77+
78+private:
79+ std::shared_ptr<mir::shell::DisplayLayout> const m_displayLayout;
80+};
81+
82+}
83+
84+MirWindowManagerImpl::MirWindowManagerImpl(const std::shared_ptr<mir::shell::DisplayLayout> &displayLayout) :
85 m_displayLayout{displayLayout}
86 {
87- qCDebug(QTMIR_MIR_MESSAGES) << "MirWindowManager::MirWindowManager";
88-}
89-
90-void MirWindowManager::add_session(std::shared_ptr<ms::Session> const& /*session*/)
91-{
92-}
93-
94-void MirWindowManager::remove_session(std::shared_ptr<ms::Session> const& /*session*/)
95-{
96-}
97-
98-auto MirWindowManager::add_surface(
99+ qCDebug(QTMIR_MIR_MESSAGES) << "MirWindowManagerImpl::MirWindowManagerImpl";
100+}
101+
102+void MirWindowManagerImpl::add_session(std::shared_ptr<ms::Session> const& /*session*/)
103+{
104+}
105+
106+void MirWindowManagerImpl::remove_session(std::shared_ptr<ms::Session> const& /*session*/)
107+{
108+}
109+
110+mir::frontend::SurfaceId MirWindowManagerImpl::add_surface(
111 std::shared_ptr<ms::Session> const& session,
112 ms::SurfaceCreationParameters const& requestParameters,
113 std::function<mir::frontend::SurfaceId(std::shared_ptr<ms::Session> const& session, ms::SurfaceCreationParameters const& params)> const& build)
114--> mir::frontend::SurfaceId
115 {
116 tracepoint(qtmirserver, surfacePlacementStart);
117
118@@ -59,7 +103,7 @@
119 m_displayLayout->size_to_output(rect);
120 placedParameters.size = rect.size;
121
122- qCDebug(QTMIR_MIR_MESSAGES) << "MirWindowManager::add_surface(): size requested ("
123+ qCDebug(QTMIR_MIR_MESSAGES) << "MirWindowManagerImpl::add_surface(): size requested ("
124 << requestParameters.size.width.as_int() << "," << requestParameters.size.height.as_int() << ") and placed ("
125 << placedParameters.size.width.as_int() << "," << placedParameters.size.height.as_int() << ")";
126
127@@ -68,36 +112,36 @@
128 return build(session, placedParameters);
129 }
130
131-void MirWindowManager::remove_surface(
132+void MirWindowManagerImpl::remove_surface(
133 std::shared_ptr<ms::Session> const& /*session*/,
134 std::weak_ptr<ms::Surface> const& /*surface*/)
135 {
136 }
137
138-void MirWindowManager::add_display(mir::geometry::Rectangle const& /*area*/)
139-{
140-}
141-
142-void MirWindowManager::remove_display(mir::geometry::Rectangle const& /*area*/)
143-{
144-}
145-
146-bool MirWindowManager::handle_keyboard_event(MirKeyboardEvent const* /*event*/)
147-{
148- return false;
149-}
150-
151-bool MirWindowManager::handle_touch_event(MirTouchEvent const* /*event*/)
152-{
153- return false;
154-}
155-
156-bool MirWindowManager::handle_pointer_event(MirPointerEvent const* /*event*/)
157-{
158- return false;
159-}
160-
161-int MirWindowManager::set_surface_attribute(
162+void MirWindowManagerImpl::add_display(mir::geometry::Rectangle const& /*area*/)
163+{
164+}
165+
166+void MirWindowManagerImpl::remove_display(mir::geometry::Rectangle const& /*area*/)
167+{
168+}
169+
170+bool MirWindowManagerImpl::handle_keyboard_event(MirKeyboardEvent const* /*event*/)
171+{
172+ return false;
173+}
174+
175+bool MirWindowManagerImpl::handle_touch_event(MirTouchEvent const* /*event*/)
176+{
177+ return false;
178+}
179+
180+bool MirWindowManagerImpl::handle_pointer_event(MirPointerEvent const* /*event*/)
181+{
182+ return false;
183+}
184+
185+int MirWindowManagerImpl::set_surface_attribute(
186 std::shared_ptr<ms::Session> const& /*session*/,
187 std::shared_ptr<ms::Surface> const& surface,
188 MirSurfaceAttrib attrib,
189@@ -106,7 +150,14 @@
190 return surface->configure(attrib, value);
191 }
192
193-void MirWindowManager::modify_surface(const std::shared_ptr<mir::scene::Session>&, const std::shared_ptr<mir::scene::Surface>&, const mir::shell::SurfaceSpecification&)
194+void MirWindowManagerImpl::modify_surface(const std::shared_ptr<mir::scene::Session>&, const std::shared_ptr<mir::scene::Surface>&, const mir::shell::SurfaceSpecification&)
195 {
196 // TODO support surface modifications
197 }
198+
199+std::unique_ptr<MirWindowManager> MirWindowManager::create(
200+ mir::shell::FocusController* /*focus_controller*/,
201+ const std::shared_ptr<mir::shell::DisplayLayout> &displayLayout)
202+{
203+ return std::make_unique<MirWindowManagerImpl>(displayLayout);
204+}
205
206=== modified file 'src/platforms/mirserver/mirwindowmanager.h'
207--- src/platforms/mirserver/mirwindowmanager.h 2015-08-11 12:08:32 +0000
208+++ src/platforms/mirserver/mirwindowmanager.h 2015-09-28 14:52:47 +0000
209@@ -24,6 +24,7 @@
210 namespace mir {
211 namespace shell {
212 class DisplayLayout;
213+ class FocusController;
214 }
215 }
216
217@@ -33,41 +34,9 @@
218
219 public:
220
221- MirWindowManager(const std::shared_ptr<mir::shell::DisplayLayout> &displayLayout);
222-
223- void add_session(std::shared_ptr<mir::scene::Session> const& session) override;
224-
225- void remove_session(std::shared_ptr<mir::scene::Session> const& session) override;
226-
227- mir::frontend::SurfaceId add_surface(
228- std::shared_ptr<mir::scene::Session> const& session,
229- mir::scene::SurfaceCreationParameters const& params,
230- std::function<mir::frontend::SurfaceId(std::shared_ptr<mir::scene::Session> const& session, mir::scene::SurfaceCreationParameters const& params)> const& build) override;
231-
232- void remove_surface(
233- std::shared_ptr<mir::scene::Session> const& session,
234- std::weak_ptr<mir::scene::Surface> const& surface) override;
235-
236- void add_display(mir::geometry::Rectangle const& area) override;
237-
238- void remove_display(mir::geometry::Rectangle const& area) override;
239-
240- bool handle_keyboard_event(MirKeyboardEvent const* event) override;
241-
242- bool handle_touch_event(MirTouchEvent const* event) override;
243-
244- bool handle_pointer_event(MirPointerEvent const* event) override;
245-
246- int set_surface_attribute(
247- std::shared_ptr<mir::scene::Session> const& session,
248- std::shared_ptr<mir::scene::Surface> const& surface,
249- MirSurfaceAttrib attrib,
250- int value) override;
251-
252- void modify_surface(const std::shared_ptr<mir::scene::Session>&, const std::shared_ptr<mir::scene::Surface>&, const mir::shell::SurfaceSpecification&);
253-
254-private:
255- std::shared_ptr<mir::shell::DisplayLayout> const m_displayLayout;
256+ static std::unique_ptr<MirWindowManager> create(
257+ mir::shell::FocusController* focus_controller,
258+ const std::shared_ptr<mir::shell::DisplayLayout> &displayLayout);
259 };
260
261 #endif /* QPAMIRSERVER_WINDOW_MANAGER_H */

Subscribers

People subscribed via source and target branches