Mir

Merge lp:~robert-ancell/mir/pause-status into lp:~mir-team/mir/trunk

Proposed by Robert Ancell
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
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

To post a comment you must log in.
Revision history for this message
Robert Ancell (robert-ancell) wrote :

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
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 StatusChangeTracker.

It looks as though exceptions have been ignored. What is the correct behavior if, for example "241 + server_status->resumed();" throws. The client code expects a true or false return - not an exception.

What is the connection with the linked bug?

review: Needs Fixing
Revision history for this message
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

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

OK

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Francis Ginther (fginther) wrote :

Amd64 build was hung, re-approving.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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+}

Subscribers

People subscribed via source and target branches