Mir

Merge lp:~robertcarr/mir/notify-surface-destruction into lp:~mir-team/mir/trunk

Proposed by Robert Carr
Status: Merged
Approved by: Robert Carr
Approved revision: no longer in the source branch.
Merged at revision: 881
Proposed branch: lp:~robertcarr/mir/notify-surface-destruction
Merge into: lp:~mir-team/mir/trunk
Diff against target: 380 lines (+103/-23)
11 files modified
include/server/mir/shell/application_session.h (+5/-1)
include/server/mir/shell/null_session_listener.h (+3/-0)
include/server/mir/shell/session_listener.h (+4/-0)
include/test/mir_test_doubles/mock_session_listener.h (+50/-0)
src/server/shell/application_session.cpp (+11/-2)
src/server/shell/session_manager.cpp (+1/-1)
tests/integration-tests/shell/test_session.cpp (+3/-1)
tests/unit-tests/shell/test_application_session.cpp (+19/-6)
tests/unit-tests/shell/test_registration_order_focus_sequence.cpp (+2/-1)
tests/unit-tests/shell/test_session_manager.cpp (+2/-10)
tests/unit-tests/shell/test_the_session_container_implementation.cpp (+3/-1)
To merge this branch: bzr merge lp:~robertcarr/mir/notify-surface-destruction
Reviewer Review Type Date Requested Status
Alan Griffiths Needs Information
PS Jenkins bot (community) continuous-integration Approve
Alexandros Frantzis (community) Approve
Robert Ancell Approve
Gerry Boland (community) Approve
Review via email: mp+174834@code.launchpad.net

Commit message

Add surface lifecycle notification to the session listener.

Description of the change

Enable notification of surface destruction for Unity 8.

To post a comment you must log in.
Revision history for this message
Robert Carr (robertcarr) wrote :

Not sure if this should be part of the_session_listener, but tbh everything else feels pretty clumsy (passing unused listeners through objects) and I think there is a reasonable association here.

Revision history for this message
Robert Carr (robertcarr) wrote :

The clumsiness of other methods kind of points out that maybe the SessionManager needs to use a factory for creating sessions

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

Is what I need, +1

review: Approve
Revision history for this message
Robert Ancell (robert-ancell) wrote :

I'd prefer having surface_created() and surface_destroyed() so they're more symmetrical. In that case you'd remove the surface from the container and destroy it after passing it to the listener. But it's not major and it depends on how you're consuming these events.

Other than the missing file, this seems OK.

review: Approve
Revision history for this message
Robert Ancell (robert-ancell) wrote :

In that case you'd remove the surface from the container, call the listener then destroy it

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Looks good.

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

There seems to be a mismatch between names and roles here.

On trunk SessionListener is the reporting interface for SessionManager. (There are only null and mock implementations of this interface however.)

On the basis of trunk code it really ought to be SessionManagerReport, but in view of this MP SessionReport might be better.

All that is a preexisting problem, so feel free to ignore.

While I don't see any strong reason for not to use this interface report other session related events, I do feel that it should be identifying the session for which a surface is created when it does so. Vis:

virtual void surface_created(Session& session, std::shared_ptr<Surface> const& surface) = 0;
virtual void destroying_surface(Session& session, std::shared_ptr<Surface> const& surface) = 0;

I also think Robert has a point about the asymmetric names.

review: Needs Fixing
Revision history for this message
Robert Carr (robertcarr) wrote :

I've added the session arguments.

I think SessionListener was the wrong name before. It is in use by unity 8. I think given the changes SessionListener is a good name.

The asymmetry in the names was intentional, if we work like

surface_destroyed(std::shared_ptr<msh::Surface>)

I think the API is a little bit of a trap! as any method you can call on msh::Surface will throw.

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've added the session arguments.
>
> I think SessionListener was the wrong name before. It is in use by unity 8. I
> think given the changes SessionListener is a good name.

This is the Mir codebase, and reporting interfaces are called *Report here.

Or do we have a confusion over the purpose of this interface?

> The asymmetry in the names was intentional, if we work like
>
> surface_destroyed(std::shared_ptr<msh::Surface>)
>
> I think the API is a little bit of a trap! as any method you can call on
> msh::Surface will throw.

I think using the perfect tense is the trap.

void surface_create(Session&, Surface&)
void surface_destroy(Session&, Surface&)

(Or are we intentionally sharing ownership with the listener?)

review: Needs Information
Revision history for this message
Robert Carr (robertcarr) wrote :

I guess I thought we were only using report interfaces for log style interfaces, where as this is more of a signalling style pattern. I think we are intentionally sharing ownership with the listener (of the msh::Surface), as listener is notified surface_created, it may want to store a shared_ptr to this surface in its internal models

Revision history for this message
Robert Carr (robertcarr) wrote :

Alan is on vacation and this branch is useful for Gerry so if no one has any objections I am going to merge it at EOD!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/shell/application_session.h'
2--- include/server/mir/shell/application_session.h 2013-06-25 13:19:15 +0000
3+++ include/server/mir/shell/application_session.h 2013-07-18 22:53:24 +0000
4@@ -34,6 +34,7 @@
5 class SurfaceFactory;
6 class Surface;
7 class SnapshotStrategy;
8+class SessionListener;
9
10 class ApplicationSession : public Session
11 {
12@@ -41,12 +42,14 @@
13 ApplicationSession(
14 std::shared_ptr<SurfaceFactory> const& surface_factory,
15 std::string const& session_name,
16- std::shared_ptr<SnapshotStrategy> const& snapshot_strategy);
17+ std::shared_ptr<SnapshotStrategy> const& snapshot_strategy,
18+ std::shared_ptr<SessionListener> const& session_listener);
19
20 ApplicationSession(
21 std::shared_ptr<SurfaceFactory> const& surface_factory,
22 std::string const& session_name,
23 std::shared_ptr<SnapshotStrategy> const& snapshot_strategy,
24+ std::shared_ptr<SessionListener> const& session_listener,
25 std::shared_ptr<events::EventSink> const& sink);
26
27 ~ApplicationSession();
28@@ -75,6 +78,7 @@
29 std::shared_ptr<SurfaceFactory> const surface_factory;
30 std::string const session_name;
31 std::shared_ptr<SnapshotStrategy> const snapshot_strategy;
32+ std::shared_ptr<SessionListener> const session_listener;
33 std::shared_ptr<events::EventSink> const event_sink;
34
35 frontend::SurfaceId next_id();
36
37=== modified file 'include/server/mir/shell/null_session_listener.h'
38--- include/server/mir/shell/null_session_listener.h 2013-06-18 05:30:43 +0000
39+++ include/server/mir/shell/null_session_listener.h 2013-07-18 22:53:24 +0000
40@@ -38,6 +38,9 @@
41 void focused(std::shared_ptr<Session> const&) override {}
42 void unfocused() override {}
43
44+ void surface_created(Session&, std::shared_ptr<Surface> const&) override {}
45+ void destroying_surface(Session&, std::shared_ptr<Surface> const&) override {}
46+
47 protected:
48 NullSessionListener(const NullSessionListener&) = delete;
49 NullSessionListener& operator=(const NullSessionListener&) = delete;
50
51=== modified file 'include/server/mir/shell/session_listener.h'
52--- include/server/mir/shell/session_listener.h 2013-06-18 05:30:43 +0000
53+++ include/server/mir/shell/session_listener.h 2013-07-18 22:53:24 +0000
54@@ -26,6 +26,7 @@
55 namespace shell
56 {
57 class Session;
58+class Surface;
59
60 class SessionListener
61 {
62@@ -34,6 +35,9 @@
63 virtual void stopping(std::shared_ptr<Session> const& session) = 0;
64 virtual void focused(std::shared_ptr<Session> const& session) = 0;
65 virtual void unfocused() = 0;
66+
67+ virtual void surface_created(Session& session, std::shared_ptr<Surface> const& surface) = 0;
68+ virtual void destroying_surface(Session& session, std::shared_ptr<Surface> const& surface) = 0;
69
70 protected:
71 SessionListener() = default;
72
73=== added file 'include/test/mir_test_doubles/mock_session_listener.h'
74--- include/test/mir_test_doubles/mock_session_listener.h 1970-01-01 00:00:00 +0000
75+++ include/test/mir_test_doubles/mock_session_listener.h 2013-07-18 22:53:24 +0000
76@@ -0,0 +1,50 @@
77+/*
78+ * Copyright © 2013 Canonical Ltd.
79+ *
80+ * This program is free software: you can redistribute it and/or modify it
81+ * under the terms of the GNU General Public License version 3,
82+ * as published by the Free Software Foundation.
83+ *
84+ * This program is distributed in the hope that it will be useful,
85+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
86+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
87+ * GNU General Public License for more details.
88+ *
89+ * You should have received a copy of the GNU General Public License
90+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
91+ *
92+ * Authored by: Robert Carr <robert.carr@canonical.com>
93+ */
94+
95+#ifndef MIR_TEST_DOUBLES_MOCK_SESSION_LISTENER_H_
96+#define MIR_TEST_DOUBLES_MOCK_SESSION_LISTENER_H_
97+
98+#include "mir/shell/session_listener.h"
99+
100+#include <gmock/gmock.h>
101+
102+namespace mir
103+{
104+namespace test
105+{
106+namespace doubles
107+{
108+
109+struct MockSessionListener : public shell::SessionListener
110+{
111+ virtual ~MockSessionListener() noexcept(true) {}
112+
113+ MOCK_METHOD1(starting, void(std::shared_ptr<shell::Session> const&));
114+ MOCK_METHOD1(stopping, void(std::shared_ptr<shell::Session> const&));
115+ MOCK_METHOD1(focused, void(std::shared_ptr<shell::Session> const&));
116+ MOCK_METHOD0(unfocused, void());
117+
118+ MOCK_METHOD2(surface_created, void(shell::Session&, std::shared_ptr<shell::Surface> const&));
119+ MOCK_METHOD2(destroying_surface, void(shell::Session&, std::shared_ptr<shell::Surface> const&));
120+};
121+
122+}
123+}
124+} // namespace mir
125+
126+#endif // MIR_TEST_DOUBLES_MOCK_SESSION_LISTENER_H_
127
128=== modified file 'src/server/shell/application_session.cpp'
129--- src/server/shell/application_session.cpp 2013-06-25 13:19:15 +0000
130+++ src/server/shell/application_session.cpp 2013-07-18 22:53:24 +0000
131@@ -20,6 +20,7 @@
132 #include "mir/shell/surface.h"
133 #include "mir/shell/surface_factory.h"
134 #include "mir/shell/snapshot_strategy.h"
135+#include "mir/shell/session_listener.h"
136
137 #include <boost/throw_exception.hpp>
138
139@@ -36,10 +37,12 @@
140 std::shared_ptr<SurfaceFactory> const& surface_factory,
141 std::string const& session_name,
142 std::shared_ptr<SnapshotStrategy> const& snapshot_strategy,
143+ std::shared_ptr<msh::SessionListener> const& session_listener,
144 std::shared_ptr<me::EventSink> const& sink) :
145 surface_factory(surface_factory),
146 session_name(session_name),
147 snapshot_strategy(snapshot_strategy),
148+ session_listener(session_listener),
149 event_sink(sink),
150 next_surface_id(0)
151 {
152@@ -49,8 +52,9 @@
153 msh::ApplicationSession::ApplicationSession(
154 std::shared_ptr<SurfaceFactory> const& surface_factory,
155 std::string const& session_name,
156- std::shared_ptr<SnapshotStrategy> const& snapshot_strategy) :
157- ApplicationSession(surface_factory, session_name, snapshot_strategy, std::shared_ptr<me::EventSink>())
158+ std::shared_ptr<SnapshotStrategy> const& snapshot_strategy,
159+ std::shared_ptr<msh::SessionListener> const& session_listener) :
160+ ApplicationSession(surface_factory, session_name, snapshot_strategy, session_listener, std::shared_ptr<me::EventSink>())
161 {
162 }
163
164@@ -75,6 +79,9 @@
165
166 std::unique_lock<std::mutex> lock(surfaces_mutex);
167 surfaces[id] = surf;
168+
169+ session_listener->surface_created(*this, surf);
170+
171 return id;
172 }
173
174@@ -114,6 +121,8 @@
175 {
176 std::unique_lock<std::mutex> lock(surfaces_mutex);
177 auto p = checked_find(id);
178+
179+ session_listener->destroying_surface(*this, p->second);
180
181 p->second->destroy();
182 surfaces.erase(p);
183
184=== modified file 'src/server/shell/session_manager.cpp'
185--- src/server/shell/session_manager.cpp 2013-07-04 04:53:57 +0000
186+++ src/server/shell/session_manager.cpp 2013-07-18 22:53:24 +0000
187@@ -79,7 +79,7 @@
188 {
189 std::shared_ptr<msh::Session> new_session =
190 std::make_shared<msh::ApplicationSession>(
191- surface_factory, name, snapshot_strategy, sink);
192+ surface_factory, name, snapshot_strategy, session_listener, sink);
193
194 app_container->insert_session(new_session);
195
196
197=== modified file 'tests/integration-tests/shell/test_session.cpp'
198--- tests/integration-tests/shell/test_session.cpp 2013-07-17 16:31:45 +0000
199+++ tests/integration-tests/shell/test_session.cpp 2013-07-18 22:53:24 +0000
200@@ -25,6 +25,7 @@
201 #include "mir/shell/placement_strategy.h"
202 #include "mir/shell/surface.h"
203 #include "mir/shell/surface_creation_parameters.h"
204+#include "mir/shell/null_session_listener.h"
205 #include "mir/surfaces/buffer_stream.h"
206 #include "mir/graphics/renderer.h"
207 #include "mir/frontend/communicator.h"
208@@ -157,7 +158,8 @@
209 msh::ApplicationSession session{
210 conf.the_shell_surface_factory(),
211 "stress",
212- conf.the_shell_snapshot_strategy()};
213+ conf.the_shell_snapshot_strategy(),
214+ std::make_shared<msh::NullSessionListener>()};
215 session.create_surface(msh::a_surface());
216
217 auto compositor = conf.the_compositor();
218
219=== modified file 'tests/unit-tests/shell/test_application_session.cpp'
220--- tests/unit-tests/shell/test_application_session.cpp 2013-07-17 16:38:25 +0000
221+++ tests/unit-tests/shell/test_application_session.cpp 2013-07-18 22:53:24 +0000
222@@ -19,9 +19,11 @@
223 #include "mir/shell/application_session.h"
224 #include "mir/graphics/buffer.h"
225 #include "mir/shell/surface_creation_parameters.h"
226+#include "mir/shell/null_session_listener.h"
227 #include "mir_test/fake_shared.h"
228 #include "mir_test_doubles/mock_surface_factory.h"
229 #include "mir_test_doubles/mock_surface.h"
230+#include "mir_test_doubles/mock_session_listener.h"
231 #include "mir_test_doubles/stub_surface_builder.h"
232 #include "mir_test_doubles/stub_surface.h"
233 #include "mir_test_doubles/null_snapshot_strategy.h"
234@@ -52,9 +54,14 @@
235
236 EXPECT_CALL(surface_factory, create_surface(_, _, _));
237 EXPECT_CALL(*mock_surface, destroy());
238+
239+ mtd::MockSessionListener listener;
240+ EXPECT_CALL(listener, surface_created(_, _)).Times(1);
241+ EXPECT_CALL(listener, destroying_surface(_, _)).Times(1);
242
243 msh::ApplicationSession session(mt::fake_shared(surface_factory), "Foo",
244- std::make_shared<mtd::NullSnapshotStrategy>());
245+ std::make_shared<mtd::NullSnapshotStrategy>(),
246+ mt::fake_shared(listener));
247
248 msh::SurfaceCreationParameters params;
249 auto surf = session.create_surface(params);
250@@ -79,7 +86,9 @@
251 }
252
253 msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo",
254- std::make_shared<mtd::NullSnapshotStrategy>());
255+ std::make_shared<mtd::NullSnapshotStrategy>(),
256+ std::make_shared<msh::NullSessionListener>());
257+
258
259 msh::SurfaceCreationParameters params;
260 auto id1 = app_session.create_surface(params);
261@@ -110,7 +119,8 @@
262 ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(Return(mock_surface));
263
264 msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo",
265- std::make_shared<mtd::NullSnapshotStrategy>());
266+ std::make_shared<mtd::NullSnapshotStrategy>(),
267+ std::make_shared<msh::NullSessionListener>());
268
269 EXPECT_CALL(surface_factory, create_surface(_, _, _));
270
271@@ -136,7 +146,8 @@
272
273 mtd::MockSurfaceFactory surface_factory;
274 msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo",
275- std::make_shared<mtd::NullSnapshotStrategy>());
276+ std::make_shared<mtd::NullSnapshotStrategy>(),
277+ std::make_shared<msh::NullSessionListener>());
278 mf::SurfaceId invalid_surface_id(1);
279
280 EXPECT_THROW({
281@@ -150,7 +161,8 @@
282
283 mtd::MockSurfaceFactory surface_factory;
284 msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo",
285- std::make_shared<mtd::NullSnapshotStrategy>());
286+ std::make_shared<mtd::NullSnapshotStrategy>(),
287+ std::make_shared<msh::NullSessionListener>());
288 mf::SurfaceId invalid_surface_id(1);
289
290 EXPECT_THROW({
291@@ -176,7 +188,8 @@
292 auto snapshot_strategy = std::make_shared<MockSnapshotStrategy>();
293 mtd::MockSurfaceFactory surface_factory;
294 msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo",
295- snapshot_strategy);
296+ snapshot_strategy,
297+ std::make_shared<msh::NullSessionListener>());
298
299 EXPECT_CALL(*snapshot_strategy, take_snapshot_of(_,_));
300
301
302=== modified file 'tests/unit-tests/shell/test_registration_order_focus_sequence.cpp'
303--- tests/unit-tests/shell/test_registration_order_focus_sequence.cpp 2013-06-25 13:19:15 +0000
304+++ tests/unit-tests/shell/test_registration_order_focus_sequence.cpp 2013-07-18 22:53:24 +0000
305@@ -20,6 +20,7 @@
306 #include "mir/shell/default_session_container.h"
307 #include "mir/shell/registration_order_focus_sequence.h"
308 #include "mir/shell/surface_creation_parameters.h"
309+#include "mir/shell/null_session_listener.h"
310 #include "mir/surfaces/surface.h"
311
312 #include "mir_test_doubles/mock_buffer_stream.h"
313@@ -52,7 +53,7 @@
314 return std::make_shared<msh::ApplicationSession>(
315 factory, name,
316 std::shared_ptr<msh::SnapshotStrategy>(),
317- std::shared_ptr<me::EventSink>());
318+ std::make_shared<msh::NullSessionListener>());
319 }
320
321 std::shared_ptr<mtd::MockSurfaceFactory> const factory;
322
323=== modified file 'tests/unit-tests/shell/test_session_manager.cpp'
324--- tests/unit-tests/shell/test_session_manager.cpp 2013-06-25 13:19:15 +0000
325+++ tests/unit-tests/shell/test_session_manager.cpp 2013-07-18 22:53:24 +0000
326@@ -31,6 +31,7 @@
327 #include "mir_test_doubles/mock_buffer_stream.h"
328 #include "mir_test_doubles/mock_surface_factory.h"
329 #include "mir_test_doubles/mock_focus_setter.h"
330+#include "mir_test_doubles/mock_session_listener.h"
331 #include "mir_test_doubles/stub_surface_builder.h"
332 #include "mir_test_doubles/null_snapshot_strategy.h"
333
334@@ -165,15 +166,6 @@
335 namespace
336 {
337
338-struct MockSessionListener : public msh::SessionListener
339-{
340- virtual ~MockSessionListener() noexcept(true) {}
341- MOCK_METHOD1(starting, void(std::shared_ptr<msh::Session> const&));
342- MOCK_METHOD1(stopping, void(std::shared_ptr<msh::Session> const&));
343- MOCK_METHOD1(focused, void(std::shared_ptr<msh::Session> const&));
344- MOCK_METHOD0(unfocused, void());
345-};
346-
347 struct SessionManagerSessionListenerSetup : public testing::Test
348 {
349 SessionManagerSessionListenerSetup()
350@@ -191,7 +183,7 @@
351 testing::NiceMock<MockSessionContainer> container; // Inelegant but some tests need a stub
352 testing::NiceMock<MockFocusSequence> focus_sequence;
353 testing::NiceMock<mtd::MockFocusSetter> focus_setter; // Inelegant but some tests need a stub
354- MockSessionListener session_listener;
355+ mtd::MockSessionListener session_listener;
356
357 msh::SessionManager session_manager;
358 };
359
360=== modified file 'tests/unit-tests/shell/test_the_session_container_implementation.cpp'
361--- tests/unit-tests/shell/test_the_session_container_implementation.cpp 2013-06-25 13:19:15 +0000
362+++ tests/unit-tests/shell/test_the_session_container_implementation.cpp 2013-07-18 22:53:24 +0000
363@@ -19,6 +19,7 @@
364 #include "mir/shell/application_session.h"
365 #include "mir/shell/default_session_container.h"
366 #include "mir/shell/surface_creation_parameters.h"
367+#include "mir/shell/null_session_listener.h"
368 #include "mir/surfaces/surface.h"
369 #include "mir_test_doubles/mock_buffer_stream.h"
370 #include "mir_test_doubles/mock_surface_factory.h"
371@@ -42,7 +43,8 @@
372 {
373 return std::make_shared<msh::ApplicationSession>(
374 factory, session_name,
375- std::make_shared<mtd::NullSnapshotStrategy>());
376+ std::make_shared<mtd::NullSnapshotStrategy>(),
377+ std::make_shared<msh::NullSessionListener>());
378 }
379
380 }

Subscribers

People subscribed via source and target branches