Merge lp:~robert-ancell/mir/pause-status into lp:~mir-team/mir/trunk
- pause-status
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Robert Ancell | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 1072 | ||||
Proposed branch: | lp:~robert-ancell/mir/pause-status | ||||
Merge into: | lp:~mir-team/mir/trunk | ||||
Diff against target: |
380 lines (+245/-0) 8 files modified
include/server/mir/default_pause_resume_listener.h (+40/-0) include/server/mir/default_server_configuration.h (+2/-0) include/server/mir/pause_resume_listener.h (+40/-0) include/server/mir/server_configuration.h (+2/-0) include/test/mir_test_doubles/mock_pause_resume_listener.h (+44/-0) src/server/default_server_configuration.cpp (+11/-0) src/server/display_server.cpp (+7/-0) tests/integration-tests/test_display_server_main_loop_events.cpp (+99/-0) |
||||
To merge this branch: | bzr merge lp:~robert-ancell/mir/pause-status | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Approve | |
Alan Griffiths | Approve | ||
Review via email: mp+186134@code.launchpad.net |
Commit message
Report when paused and resumed via configuration
Description of the change
Robert Ancell (robert-ancell) wrote : | # |
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1062
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1064
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1064
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Alan Griffiths (alan-griffiths) wrote : | # |
I'm not convinced by the naming: The ServerStatus class doesn't represent a status - it is a listener interface monitoring status transitions. Something like StatusChangeTra
It looks as though exceptions have been ignored. What is the correct behavior if, for example "241 + server_
What is the connection with the linked bug?
Robert Ancell (robert-ancell) wrote : | # |
Tracker doesn't seem to be used anywhere else in Mir so I think that's a worse name. The "Status" part of the name is weak, since we don't have a concept of what pause() / resume() means. This is the only interface that relates to the server state, I don't see it being transition specific - it could easily be extended to contain get_state().
If the application using libmirserver throws an exception in paused() or removed() I see no reason why we should ignore that - it should stop the server. Also, we're only notifying that the pause/resume has occurred, not controlling it.
The connection to the linked bug is USC needs to know when it has been VT switched away from (i.e. paused) so that it can tell its children to suspend. In the case of XMir this allows it to know when to stop using the input devices
Alan Griffiths (alan-griffiths) wrote : | # |
> Tracker doesn't seem to be used anywhere else in Mir so I think that's a worse
> name. The "Status" part of the name is weak, since we don't have a concept of
> what pause() / resume() means. This is the only interface that relates to the
> server state, I don't see it being transition specific - it could easily be
> extended to contain get_state().
This interface is used by the display server to notify state transitions - the name ServerState doesn't reflect this role. Does PauseResumeListener sound better to you?
Why would the server want to call get_state()?
> If the application using libmirserver throws an exception in paused() or
> removed() I see no reason why we should ignore that - it should stop the
> server.
It is likely to stop the server in an untidy manner: e.g. it has just restarted all the subsystems and then an exception is thrown that will probably be caught in main().
> Also, we're only notifying that the pause/resume has occurred, not
> controlling it.
So exceptions are, perhaps, not good behaviour?
> The connection to the linked bug is USC needs to know when it has been VT
> switched away from (i.e. paused) so that it can tell its children to suspend.
> In the case of XMir this allows it to know when to stop using the input
> devices
OK
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1066
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Francis Ginther (fginther) wrote : | # |
Amd64 build was hung, re-approving.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
PS Jenkins bot (ps-jenkins) : | # |
Preview Diff
1 | === added file 'include/server/mir/default_pause_resume_listener.h' |
2 | --- include/server/mir/default_pause_resume_listener.h 1970-01-01 00:00:00 +0000 |
3 | +++ include/server/mir/default_pause_resume_listener.h 2013-09-18 21:09:49 +0000 |
4 | @@ -0,0 +1,40 @@ |
5 | +/* |
6 | + * Copyright © 2013 Canonical Ltd. |
7 | + * |
8 | + * This program is free software: you can redistribute it and/or modify it |
9 | + * under the terms of the GNU General Public License version 3, |
10 | + * as published by the Free Software Foundation. |
11 | + * |
12 | + * This program is distributed in the hope that it will be useful, |
13 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
14 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
15 | + * GNU General Public License for more details. |
16 | + * |
17 | + * You should have received a copy of the GNU General Public License |
18 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
19 | + * |
20 | + * Authored by: Robert Ancell <robert.ancell@canonical.com> |
21 | + */ |
22 | + |
23 | +#ifndef MIR_DEFAULT_PAUSE_RESUME_LISTENER_H_ |
24 | +#define MIR_DEFAULT_PAUSE_RESUME_LISTENER_H_ |
25 | + |
26 | +#include "mir/pause_resume_listener.h" |
27 | + |
28 | +namespace mir |
29 | +{ |
30 | +class DefaultPauseResumeListener : public virtual PauseResumeListener |
31 | +{ |
32 | +public: |
33 | + virtual void paused() |
34 | + { |
35 | + } |
36 | + |
37 | + virtual void resumed() |
38 | + { |
39 | + } |
40 | +}; |
41 | + |
42 | +} |
43 | + |
44 | +#endif /* MIR_DEFAULT_PAUSE_RESUME_LISTENER_H_ */ |
45 | |
46 | === modified file 'include/server/mir/default_server_configuration.h' |
47 | --- include/server/mir/default_server_configuration.h 2013-09-18 16:24:32 +0000 |
48 | +++ include/server/mir/default_server_configuration.h 2013-09-18 21:09:49 +0000 |
49 | @@ -123,6 +123,7 @@ |
50 | virtual std::shared_ptr<compositor::Compositor> the_compositor(); |
51 | virtual std::shared_ptr<input::InputManager> the_input_manager(); |
52 | virtual std::shared_ptr<MainLoop> the_main_loop(); |
53 | + virtual std::shared_ptr<PauseResumeListener> the_pause_resume_listener(); |
54 | virtual std::shared_ptr<DisplayChanger> the_display_changer(); |
55 | virtual std::shared_ptr<graphics::Platform> the_graphics_platform(); |
56 | virtual std::shared_ptr<input::InputConfiguration> the_input_configuration(); |
57 | @@ -286,6 +287,7 @@ |
58 | CachedPtr<surfaces::SurfaceController> surface_controller; |
59 | CachedPtr<time::TimeSource> time_source; |
60 | CachedPtr<MainLoop> main_loop; |
61 | + CachedPtr<PauseResumeListener> pause_resume_listener; |
62 | CachedPtr<graphics::DisplayConfigurationPolicy> display_configuration_policy; |
63 | CachedPtr<graphics::nested::HostConnection> host_connection; |
64 | CachedPtr<input::NestedInputRelay> nested_input_relay; |
65 | |
66 | === added file 'include/server/mir/pause_resume_listener.h' |
67 | --- include/server/mir/pause_resume_listener.h 1970-01-01 00:00:00 +0000 |
68 | +++ include/server/mir/pause_resume_listener.h 2013-09-18 21:09:49 +0000 |
69 | @@ -0,0 +1,40 @@ |
70 | +/* |
71 | + * Copyright © 2013 Canonical Ltd. |
72 | + * |
73 | + * This program is free software: you can redistribute it and/or modify it |
74 | + * under the terms of the GNU General Public License version 3, |
75 | + * as published by the Free Software Foundation. |
76 | + * |
77 | + * This program is distributed in the hope that it will be useful, |
78 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
79 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
80 | + * GNU General Public License for more details. |
81 | + * |
82 | + * You should have received a copy of the GNU General Public License |
83 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
84 | + * |
85 | + * Authored by: Robert Ancell <robert.ancell@canonical.com> |
86 | + */ |
87 | + |
88 | +#ifndef MIR_PAUSE_RESUME_LISTENER_H_ |
89 | +#define MIR_PAUSE_RESUME_LISTENER_H_ |
90 | + |
91 | +namespace mir |
92 | +{ |
93 | + |
94 | +class PauseResumeListener |
95 | +{ |
96 | +public: |
97 | + virtual void paused() = 0; |
98 | + virtual void resumed() = 0; |
99 | + |
100 | +protected: |
101 | + PauseResumeListener() = default; |
102 | + virtual ~PauseResumeListener() = default; |
103 | + PauseResumeListener(PauseResumeListener const&) = delete; |
104 | + PauseResumeListener& operator=(PauseResumeListener const&) = delete; |
105 | +}; |
106 | + |
107 | +} |
108 | + |
109 | +#endif /* MIR_PAUSE_RESUME_LISTENER_H_ */ |
110 | |
111 | === modified file 'include/server/mir/server_configuration.h' |
112 | --- include/server/mir/server_configuration.h 2013-08-28 03:41:48 +0000 |
113 | +++ include/server/mir/server_configuration.h 2013-09-18 21:09:49 +0000 |
114 | @@ -49,6 +49,7 @@ |
115 | } |
116 | |
117 | class MainLoop; |
118 | +class PauseResumeListener; |
119 | class DisplayChanger; |
120 | |
121 | class ServerConfiguration |
122 | @@ -61,6 +62,7 @@ |
123 | virtual std::shared_ptr<compositor::Compositor> the_compositor() = 0; |
124 | virtual std::shared_ptr<input::InputManager> the_input_manager() = 0; |
125 | virtual std::shared_ptr<MainLoop> the_main_loop() = 0; |
126 | + virtual std::shared_ptr<PauseResumeListener> the_pause_resume_listener() = 0; |
127 | virtual std::shared_ptr<DisplayChanger> the_display_changer() = 0; |
128 | virtual std::shared_ptr<graphics::Platform> the_graphics_platform() = 0; |
129 | virtual std::shared_ptr<input::InputConfiguration> the_input_configuration() = 0; |
130 | |
131 | === added file 'include/test/mir_test_doubles/mock_pause_resume_listener.h' |
132 | --- include/test/mir_test_doubles/mock_pause_resume_listener.h 1970-01-01 00:00:00 +0000 |
133 | +++ include/test/mir_test_doubles/mock_pause_resume_listener.h 2013-09-18 21:09:49 +0000 |
134 | @@ -0,0 +1,44 @@ |
135 | +/* |
136 | + * Copyright © 2013 Canonical Ltd. |
137 | + * |
138 | + * This program is free software: you can redistribute it and/or modify it |
139 | + * under the terms of the GNU General Public License version 3, |
140 | + * as published by the Free Software Foundation. |
141 | + * |
142 | + * This program is distributed in the hope that it will be useful, |
143 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
144 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
145 | + * GNU General Public License for more details. |
146 | + * |
147 | + * You should have received a copy of the GNU General Public License |
148 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
149 | + * |
150 | + * Authored by: Robert Ancell <robert.ancell@canonical.com> |
151 | + */ |
152 | + |
153 | +#ifndef MIR_TEST_DOUBLES_MOCK_PAUSE_RESUME_LISTENER_H_ |
154 | +#define MIR_TEST_DOUBLES_MOCK_PAUSE_RESUME_LISTENER_H_ |
155 | + |
156 | +#include "mir/pause_resume_listener.h" |
157 | + |
158 | +#include <gmock/gmock.h> |
159 | + |
160 | +namespace mir |
161 | +{ |
162 | +namespace test |
163 | +{ |
164 | +namespace doubles |
165 | +{ |
166 | + |
167 | +class MockPauseResumeListener : public mir::PauseResumeListener |
168 | +{ |
169 | +public: |
170 | + MOCK_METHOD0(paused, void()); |
171 | + MOCK_METHOD0(resumed, void()); |
172 | +}; |
173 | + |
174 | +} |
175 | +} |
176 | +} |
177 | + |
178 | +#endif /* MIR_TEST_DOUBLES_MOCK_PAUSE_RESUME_LISTENER_H_ */ |
179 | |
180 | === modified file 'src/server/default_server_configuration.cpp' |
181 | --- src/server/default_server_configuration.cpp 2013-09-18 16:24:32 +0000 |
182 | +++ src/server/default_server_configuration.cpp 2013-09-18 21:09:49 +0000 |
183 | @@ -19,6 +19,7 @@ |
184 | #include "mir/default_server_configuration.h" |
185 | #include "mir/abnormal_exit.h" |
186 | #include "mir/asio_main_loop.h" |
187 | +#include "mir/default_pause_resume_listener.h" |
188 | #include "mir/shared_library.h" |
189 | |
190 | #include "mir/options/program_option.h" |
191 | @@ -936,6 +937,16 @@ |
192 | }); |
193 | } |
194 | |
195 | + |
196 | +std::shared_ptr<mir::PauseResumeListener> mir::DefaultServerConfiguration::the_pause_resume_listener() |
197 | +{ |
198 | + return pause_resume_listener( |
199 | + []() |
200 | + { |
201 | + return std::make_shared<mir::DefaultPauseResumeListener>(); |
202 | + }); |
203 | +} |
204 | + |
205 | std::shared_ptr<mg::DisplayConfigurationPolicy> |
206 | mir::DefaultServerConfiguration::the_display_configuration_policy() |
207 | { |
208 | |
209 | === modified file 'src/server/display_server.cpp' |
210 | --- src/server/display_server.cpp 2013-08-28 03:41:48 +0000 |
211 | +++ src/server/display_server.cpp 2013-09-18 21:09:49 +0000 |
212 | @@ -21,6 +21,7 @@ |
213 | #include "mir/display_server.h" |
214 | #include "mir/server_configuration.h" |
215 | #include "mir/main_loop.h" |
216 | +#include "mir/pause_resume_listener.h" |
217 | #include "mir/display_changer.h" |
218 | |
219 | #include "mir/compositor/compositor.h" |
220 | @@ -74,6 +75,7 @@ |
221 | communicator{config.the_communicator()}, |
222 | input_manager{config.the_input_manager()}, |
223 | main_loop{config.the_main_loop()}, |
224 | + pause_resume_listener{config.the_pause_resume_listener()}, |
225 | display_changer{config.the_display_changer()}, |
226 | paused{false}, |
227 | configure_display_on_resume{false} |
228 | @@ -113,6 +115,8 @@ |
229 | return false; |
230 | } |
231 | |
232 | + pause_resume_listener->paused(); |
233 | + |
234 | return true; |
235 | } |
236 | |
237 | @@ -149,6 +153,8 @@ |
238 | return false; |
239 | } |
240 | |
241 | + pause_resume_listener->resumed(); |
242 | + |
243 | return true; |
244 | } |
245 | |
246 | @@ -173,6 +179,7 @@ |
247 | std::shared_ptr<mf::Communicator> const communicator; |
248 | std::shared_ptr<mi::InputManager> const input_manager; |
249 | std::shared_ptr<mir::MainLoop> const main_loop; |
250 | + std::shared_ptr<mir::PauseResumeListener> const pause_resume_listener; |
251 | std::shared_ptr<mir::DisplayChanger> const display_changer; |
252 | bool paused; |
253 | bool configure_display_on_resume; |
254 | |
255 | === modified file 'tests/integration-tests/test_display_server_main_loop_events.cpp' |
256 | --- tests/integration-tests/test_display_server_main_loop_events.cpp 2013-08-28 03:41:48 +0000 |
257 | +++ tests/integration-tests/test_display_server_main_loop_events.cpp 2013-09-18 21:09:49 +0000 |
258 | @@ -22,12 +22,14 @@ |
259 | #include "mir/graphics/display_configuration_policy.h" |
260 | #include "mir/main_loop.h" |
261 | #include "mir/display_changer.h" |
262 | +#include "mir/pause_resume_listener.h" |
263 | |
264 | #include "mir_test/pipe.h" |
265 | #include "mir_test_framework/testing_server_configuration.h" |
266 | #include "mir_test_doubles/mock_input_manager.h" |
267 | #include "mir_test_doubles/mock_compositor.h" |
268 | #include "mir_test_doubles/null_display.h" |
269 | +#include "mir_test_doubles/mock_pause_resume_listener.h" |
270 | #include "mir/run_mir.h" |
271 | |
272 | #include <gtest/gtest.h> |
273 | @@ -311,6 +313,71 @@ |
274 | int const resume_signal; |
275 | }; |
276 | |
277 | +class TestPauseResumeListenerConfig : public mtf::TestingServerConfiguration |
278 | +{ |
279 | +public: |
280 | + TestPauseResumeListenerConfig() |
281 | + : pause_signal{SIGUSR1}, resume_signal{SIGUSR2} |
282 | + { |
283 | + } |
284 | + |
285 | + std::shared_ptr<mg::Display> the_display() override |
286 | + { |
287 | + if (!mock_display) |
288 | + { |
289 | + auto display = mtf::TestingServerConfiguration::the_display(); |
290 | + mock_display = std::make_shared<MockDisplay>(display, |
291 | + pause_signal, |
292 | + resume_signal, |
293 | + p.read_fd()); |
294 | + } |
295 | + |
296 | + return mock_display; |
297 | + } |
298 | + |
299 | + std::shared_ptr<mir::PauseResumeListener> the_pause_resume_listener() override |
300 | + { |
301 | + if (!mock_pause_resume_listener) |
302 | + mock_pause_resume_listener = std::make_shared<mtd::MockPauseResumeListener>(); |
303 | + |
304 | + return mock_pause_resume_listener; |
305 | + } |
306 | + |
307 | + std::shared_ptr<MockDisplay> the_mock_display() |
308 | + { |
309 | + the_display(); |
310 | + return mock_display; |
311 | + } |
312 | + |
313 | + std::shared_ptr<mtd::MockPauseResumeListener> the_mock_pause_resume_listener() |
314 | + { |
315 | + the_pause_resume_listener(); |
316 | + return mock_pause_resume_listener; |
317 | + } |
318 | + |
319 | + void emit_pause_event_and_wait_for_handler() |
320 | + { |
321 | + kill(getpid(), pause_signal); |
322 | + while (!mock_display->pause_handler_invoked()) |
323 | + std::this_thread::sleep_for(std::chrono::microseconds{500}); |
324 | + } |
325 | + |
326 | + void emit_resume_event_and_wait_for_handler() |
327 | + { |
328 | + kill(getpid(), resume_signal); |
329 | + while (!mock_display->resume_handler_invoked()) |
330 | + std::this_thread::sleep_for(std::chrono::microseconds{500}); |
331 | + } |
332 | + |
333 | +private: |
334 | + std::shared_ptr<MockDisplay> mock_display; |
335 | + std::shared_ptr<mtd::MockPauseResumeListener> mock_pause_resume_listener; |
336 | + |
337 | + mt::Pipe p; |
338 | + int const pause_signal; |
339 | + int const resume_signal; |
340 | +}; |
341 | + |
342 | } |
343 | |
344 | TEST(DisplayServerMainLoopEvents, display_server_shuts_down_properly_on_sigint) |
345 | @@ -582,3 +649,35 @@ |
346 | t.detach(); |
347 | }); |
348 | } |
349 | + |
350 | +TEST(DisplayServerMainLoopEvents, pause_resume_listener) |
351 | +{ |
352 | + using namespace testing; |
353 | + |
354 | + TestPauseResumeListenerConfig server_config; |
355 | + |
356 | + auto mock_display = server_config.the_mock_display(); |
357 | + auto mock_pause_resume_listener = server_config.the_mock_pause_resume_listener(); |
358 | + |
359 | + { |
360 | + InSequence s; |
361 | + |
362 | + EXPECT_CALL(*mock_display, pause()).Times(1); |
363 | + EXPECT_CALL(*mock_pause_resume_listener, paused()).Times(1); |
364 | + EXPECT_CALL(*mock_display, resume()).Times(1); |
365 | + EXPECT_CALL(*mock_pause_resume_listener, resumed()).Times(1); |
366 | + } |
367 | + |
368 | + mir::run_mir(server_config, |
369 | + [&server_config](mir::DisplayServer&) |
370 | + { |
371 | + std::thread t{ |
372 | + [&] |
373 | + { |
374 | + server_config.emit_pause_event_and_wait_for_handler(); |
375 | + server_config.emit_resume_event_and_wait_for_handler(); |
376 | + kill(getpid(), SIGTERM); |
377 | + }}; |
378 | + t.detach(); |
379 | + }); |
380 | +} |
Required for unity-system- compositor to disable XMir input on VT/session switch: https:/ /code.launchpad .net/~robert- ancell/ unity-system- compositor/ app-lifecycle/ +merge/ 186135