Mir

Merge lp:~robertcarr/mir/ease-shell-configuration into lp:~mir-team/mir/trunk

Proposed by Robert Carr on 2013-04-09
Status: Rejected
Rejected by: Robert Carr on 2013-04-17
Proposed branch: lp:~robertcarr/mir/ease-shell-configuration
Merge into: lp:~mir-team/mir/trunk
Diff against target: 813 lines (+393/-110)
12 files modified
include/server/mir/default_server_configuration.h (+3/-0)
include/server/mir/shell/default_shell_configuration.h (+71/-0)
include/server/mir/shell/session_manager.h (+2/-4)
include/server/mir/shell/shell_configuration.h (+52/-0)
src/server/default_server_configuration.cpp (+14/-14)
src/server/shell/CMakeLists.txt (+1/-0)
src/server/shell/default_shell_configuration.cpp (+82/-0)
src/server/shell/session_manager.cpp (+8/-11)
tests/acceptance-tests/test_focus_selection.cpp (+33/-12)
tests/death-tests/test_application_manager_death.cpp (+25/-5)
tests/integration-tests/shell/test_session_manager.cpp (+47/-31)
tests/unit-tests/shell/test_session_manager.cpp (+55/-33)
To merge this branch: bzr merge lp:~robertcarr/mir/ease-shell-configuration
Reviewer Review Type Date Requested Status
Alan Griffiths Needs Fixing on 2013-04-17
Daniel van Vugt 2013-04-09 Abstain on 2013-04-17
PS Jenkins bot (community) continuous-integration Approve on 2013-04-15
Alexandros Frantzis (community) Needs Fixing on 2013-04-11
Review via email: mp+157965@code.launchpad.net

Commit message

Encapsulate SessionManager dependencies in a ShellConfiguration object (using the idiom established by input)

Description of the change

I want to write a small demo shell which among other things (largely input filtering), replaces the placement strategy. It's a little cumbersome to do so from the shells perspective though! Also the large number of arguments to SessionManager constructor make refactoring a little painful.

This branch refactors it to use the idiom established by the server configuration and input configuration

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote :

I like the intention. Though I don't claim to fully understand the context of all the changes.

This is a common mistake throughout the Mir code. We should probably stop copying and pasting it...
15 + * GNU General Public License for more details.
115 + * GNU General Public License for more details.
220 + * GNU General Public License for more details.
Need "Lesser" as in LGPL.

review: Needs Fixing
Alexandros Frantzis (afrantzis) wrote :

187 + auto configuration = std::make_shared<msh::DefaultShellConfiguration>(the_display(), the_input_focus_selector(),
188 + the_surface_factory());

I think it would make sense to have DefaultServerConfiguration::the_shell_configuration(), since that's the part that users will want to change by overriding.

140 + virtual ~ShellConfiguration() {}

For new base classes/interfaces it is recommended to use "= default()" for the destructor, too, so that we get the noexcept guarantee. The price for this is that we need to explicitly mark destructors of derived classes as noexcept(true).

Some nits:

242 + : view_area(view_area),
243 + input_focus_selector(focus_selector),
244 + underlying_surface_factory(surface_factory)

':' should be indented 4 spaces (we don't use 2 space indentations in any case).

318 + : surface_factory(config->the_surface_factory()),

Likewise.

370 + : DefaultShellConfiguration(view_area, input_selector, surface_factory)

Likewise.

498 + : sequence(mt::fake_shared(container))

Likewise

648 + : session_manager(mt::fake_shared(config))

Likewise.

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

On 11/04/13 15:59, Alexandros Frantzis wrote:
> For new base classes/interfaces it is recommended to use "= default()" for the destructor, too, so that we get the noexcept guarantee. The price for this is that we need to explicitly mark destructors of derived classes as noexcept(true).

Only true for *some* derived classes: Ones with an explicit destructor
definition, or those where there are members with throwing destructors.

--
Alan Griffiths +44 (0)798 9938 758
Octopull Ltd http://www.octopull.co.uk/

Robert Carr (robertcarr) wrote :

Thanks both!

Copyright headers fixed (r581)
Add the_shell_configuration() to default server configuration (r582)
Use = default for ~ShellConfiguration (r583)
Fix whitespace (r584)

Daniel van Vugt (vanvugt) wrote :

server/*: Need to s/GNU Lesser General/GNU General/g

Stylistically I disagree with a lot of this. Though the sources of all my concerns originate before this proposal and are not really new.

From a high-level perspective, I totally agree that writing a simple dumb shell would be very useful. I was thinking the same. Although I am concerned about the number and complexity of classes we're exposing to shell developers. It feels like too much, but I don't understand them well enough to comment further.

Just need to fix up the licenses for now.

review: Needs Fixing
Alexandros Frantzis (afrantzis) wrote :

> Thanks both!
>
> Copyright headers fixed (r581)
> Add the_shell_configuration() to default server configuration (r582)
> Use = default for ~ShellConfiguration (r583)
> Fix whitespace (r584)

It seems you didn't push the commits :)

580. By Robert Carr on 2013-04-11

Merge trunk

581. By Robert Carr on 2013-04-11

Fix copyright headers

582. By Robert Carr on 2013-04-11

Add the_shell_configuration() to default server configuration

583. By Robert Carr on 2013-04-11

~ShellConfiguration is noexcept

584. By Robert Carr on 2013-04-11

Whitespace

Robert Carr (robertcarr) wrote :

I know how to use bzr!

585. By Robert Carr on 2013-04-12

Merge trunk

Robert Carr (robertcarr) wrote :

>> From a high-level perspective, I totally agree that writing a simple dumb shell would be very useful. I was >> thinking the same. Although I am concerned about the number
>> and complexity of classes we're exposing to shell
>> developers. It feels like too much, but I don't understand them well enough to comment further.

I feel this concern! This is part of the idea with getting a little dumb shell going (and some of the other examples), to give us explicit targets to try and improve.

586. By Robert Carr on 2013-04-12

Trim whitespace

Daniel van Vugt (vanvugt) wrote :

Still need to remove "Lesser" in src/server/* as in:
37 + * under the terms of the GNU Lesser General Public License version 3,

review: Needs Fixing
Robert Carr (robertcarr) wrote :

Fix headers for real!

587. By Robert Carr on 2013-04-15

Merge trunk

588. By Robert Carr on 2013-04-15

Fix headers

Alan Griffiths (alan-griffiths) wrote :

While I think we need to add structure to our configuration process, I'm not convinced that this is is right way forward. (C.f. https://code.launchpad.net/~robertcarr/mir/rebuild-input-focus-selection/+merge/158505/comments/348807)

Lets have a discussion of the pros & cons of different approaches before going too far along any path.

review: Needs Information
Daniel van Vugt (vanvugt) wrote :

OK, licenses are fixed. Though based on Alan's comment I won't do another review just yet.

review: Abstain
Alan Griffiths (alan-griffiths) wrote :

From the email list:

On 16/04/13 22:45, Robert Carr wrote:
> In the case of the shell configuration, the intent was to allow overriding of the session store dependencies constructed in the_session_manager without overriding the entire construction. I felt strange adding many more methods to the already polluted default_server_configuration (the_focus_sequence, the_focus_setter, the_placement_strategy, etc...) so it felt appropriate to build some hierarchy. In this case the dependencies (placement strategy, etc..) live in the same name space and there is no attempt at isolation, only organization.

I can see this approach - the master config having some member config objects with reduced scope. But for this approach to be followed consistently the ShellConfiguration should be held in the DefaultServerConfiguration by a shared_ptr, not a CachedPtr (and it should not strong references to anything that isn't another configuration object.)

review: Needs Fixing

Unmerged revisions

588. By Robert Carr on 2013-04-15

Fix headers

587. By Robert Carr on 2013-04-15

Merge trunk

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/default_server_configuration.h'
2--- include/server/mir/default_server_configuration.h 2013-04-12 04:55:11 +0000
3+++ include/server/mir/default_server_configuration.h 2013-04-15 14:20:37 +0000
4@@ -48,6 +48,7 @@
5
6 namespace shell
7 {
8+class ShellConfiguration;
9 class SessionManager;
10 class SurfaceFactory;
11 class SurfaceSource;
12@@ -108,6 +109,7 @@
13 virtual std::shared_ptr<frontend::SessionMediatorReport> the_session_mediator_report();
14 virtual std::shared_ptr<frontend::MessageProcessorReport> the_message_processor_report();
15
16+ virtual std::shared_ptr<shell::ShellConfiguration> the_shell_configuration();
17 virtual std::shared_ptr<frontend::Shell> the_frontend_shell();
18 virtual std::shared_ptr<shell::SurfaceFactory> the_surface_factory();
19
20@@ -130,6 +132,7 @@
21 virtual std::shared_ptr<input::InputChannelFactory> the_input_channel_factory();
22
23 CachedPtr<frontend::Communicator> communicator;
24+ CachedPtr<shell::ShellConfiguration> shell_configuration;
25 CachedPtr<frontend::Shell> session_manager;
26 CachedPtr<input::InputManager> input_manager;
27 CachedPtr<graphics::Platform> graphics_platform;
28
29=== added file 'include/server/mir/shell/default_shell_configuration.h'
30--- include/server/mir/shell/default_shell_configuration.h 1970-01-01 00:00:00 +0000
31+++ include/server/mir/shell/default_shell_configuration.h 2013-04-15 14:20:37 +0000
32@@ -0,0 +1,71 @@
33+/*
34+ * Copyright © 2013 Canonical Ltd.
35+ *
36+ * This program is free software: you can redistribute it and/or modify it
37+ * under the terms of the GNU General Public License version 3,
38+ * as published by the Free Software Foundation.
39+ *
40+ * This program is distributed in the hope that it will be useful,
41+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
42+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
43+ * GNU General Public License for more details.
44+ *
45+ * You should have received a copy of the GNU General Public License
46+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
47+ *
48+ * Authored by: Robert Carr <robert.carr@canonical.com>
49+ */
50+
51+#ifndef MIR_SHELL_DEFAULT_SHELL_CONFIGURATION_H_
52+#define MIR_SHELL_DEFAULT_SHELL_CONFIGURATION_H_
53+
54+#include "mir/shell/shell_configuration.h"
55+#include "mir/cached_ptr.h"
56+
57+namespace mir
58+{
59+namespace graphics
60+{
61+class ViewableArea;
62+}
63+namespace shell
64+{
65+class InputFocusSelector;
66+class SurfaceFactory;
67+class PlacementStrategy;
68+
69+class DefaultShellConfiguration : public ShellConfiguration
70+{
71+public:
72+ DefaultShellConfiguration(std::shared_ptr<graphics::ViewableArea> const& view_area,
73+ std::shared_ptr<InputFocusSelector> const& focus_selector,
74+ std::shared_ptr<SurfaceFactory> const& surface_factory);
75+ virtual ~DefaultShellConfiguration() noexcept(true) = default;
76+
77+ std::shared_ptr<SurfaceFactory> the_surface_factory();
78+ std::shared_ptr<SessionContainer> the_session_container();
79+ std::shared_ptr<FocusSequence> the_focus_sequence();
80+ std::shared_ptr<FocusSetter> the_focus_setter();
81+
82+ std::shared_ptr<PlacementStrategy> the_placement_strategy();
83+
84+protected:
85+ DefaultShellConfiguration(DefaultShellConfiguration const&) = delete;
86+ DefaultShellConfiguration& operator=(DefaultShellConfiguration const&) = delete;
87+
88+private:
89+ std::shared_ptr<graphics::ViewableArea> const view_area;
90+ std::shared_ptr<InputFocusSelector> const input_focus_selector;
91+ std::shared_ptr<SurfaceFactory> const underlying_surface_factory;
92+
93+ CachedPtr<SurfaceFactory> surface_factory;
94+ CachedPtr<SessionContainer> session_container;
95+ CachedPtr<FocusSequence> focus_sequence;
96+ CachedPtr<FocusSetter> focus_setter;
97+ CachedPtr<PlacementStrategy> placement_strategy;
98+};
99+
100+}
101+} // namespace mir
102+
103+#endif // MIR_SHELL_DEFAULT_SHELL_CONFIGURATION_H_
104
105=== modified file 'include/server/mir/shell/session_manager.h'
106--- include/server/mir/shell/session_manager.h 2013-04-12 04:55:11 +0000
107+++ include/server/mir/shell/session_manager.h 2013-04-15 14:20:37 +0000
108@@ -36,6 +36,7 @@
109 /// Management of sessions and surfaces
110 namespace shell
111 {
112+class ShellConfiguration;
113 class SurfaceFactory;
114 class SessionContainer;
115 class FocusSequence;
116@@ -44,10 +45,7 @@
117 class SessionManager : public frontend::Shell
118 {
119 public:
120- explicit SessionManager(std::shared_ptr<SurfaceFactory> const& surface_factory,
121- std::shared_ptr<SessionContainer> const& session_container,
122- std::shared_ptr<FocusSequence> const& focus_sequence,
123- std::shared_ptr<FocusSetter> const& focus_setter);
124+ explicit SessionManager(std::shared_ptr<ShellConfiguration> const& configuration);
125 virtual ~SessionManager();
126
127 virtual std::shared_ptr<frontend::Session> open_session(std::string const& name);
128
129=== added file 'include/server/mir/shell/shell_configuration.h'
130--- include/server/mir/shell/shell_configuration.h 1970-01-01 00:00:00 +0000
131+++ include/server/mir/shell/shell_configuration.h 2013-04-15 14:20:37 +0000
132@@ -0,0 +1,52 @@
133+/*
134+ * Copyright © 2013 Canonical Ltd.
135+ *
136+ * This program is free software: you can redistribute it and/or modify it
137+ * under the terms of the GNU General Public License version 3,
138+ * as published by the Free Software Foundation.
139+ *
140+ * This program is distributed in the hope that it will be useful,
141+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
142+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
143+ * GNU General Public License for more details.
144+ *
145+ * You should have received a copy of the GNU General Public License
146+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
147+ *
148+ * Authored by: Robert Carr <robert.carr@canonical.com>
149+ */
150+
151+#ifndef MIR_SHELL_SHELL_CONFIGURATION_H_
152+#define MIR_SHELL_SHELL_CONFIGURATION_H_
153+
154+#include <memory>
155+
156+namespace mir
157+{
158+namespace shell
159+{
160+class SurfaceFactory;
161+class SessionContainer;
162+class FocusSequence;
163+class FocusSetter;
164+
165+class ShellConfiguration
166+{
167+public:
168+ virtual ~ShellConfiguration() = default;
169+
170+ virtual std::shared_ptr<SurfaceFactory> the_surface_factory() = 0;
171+ virtual std::shared_ptr<SessionContainer> the_session_container() = 0;
172+ virtual std::shared_ptr<FocusSequence> the_focus_sequence() = 0;
173+ virtual std::shared_ptr<FocusSetter> the_focus_setter() = 0;
174+
175+protected:
176+ ShellConfiguration() = default;
177+ ShellConfiguration(ShellConfiguration const&) = delete;
178+ ShellConfiguration& operator=(ShellConfiguration const&) = delete;
179+};
180+
181+}
182+} // namespace mir
183+
184+#endif // MIR_SHELL_SHELL_CONFIGURATION_H_
185
186=== modified file 'src/server/default_server_configuration.cpp'
187--- src/server/default_server_configuration.cpp 2013-04-12 04:55:11 +0000
188+++ src/server/default_server_configuration.cpp 2013-04-15 14:20:37 +0000
189@@ -32,12 +32,8 @@
190 #include "mir/frontend/null_message_processor_report.h"
191 #include "mir/frontend/session_mediator.h"
192 #include "mir/frontend/resource_cache.h"
193+#include "mir/shell/default_shell_configuration.h"
194 #include "mir/shell/session_manager.h"
195-#include "mir/shell/registration_order_focus_sequence.h"
196-#include "mir/shell/single_visibility_focus_mechanism.h"
197-#include "mir/shell/session_container.h"
198-#include "mir/shell/consuming_placement_strategy.h"
199-#include "mir/shell/organising_surface_factory.h"
200 #include "mir/graphics/display.h"
201 #include "mir/graphics/gl_renderer.h"
202 #include "mir/graphics/renderer.h"
203@@ -292,20 +288,24 @@
204 });
205 }
206
207+std::shared_ptr<msh::ShellConfiguration>
208+mir::DefaultServerConfiguration::the_shell_configuration()
209+{
210+ return shell_configuration(
211+ [this]()
212+ {
213+ return std::make_shared<msh::DefaultShellConfiguration>(the_display(), the_input_focus_selector(),
214+ the_surface_factory());
215+ });
216+}
217+
218 std::shared_ptr<mf::Shell>
219 mir::DefaultServerConfiguration::the_frontend_shell()
220 {
221 return session_manager(
222- [this]() -> std::shared_ptr<msh::SessionManager>
223+ [this]()
224 {
225- auto session_container = std::make_shared<msh::SessionContainer>();
226- auto focus_mechanism = std::make_shared<msh::SingleVisibilityFocusMechanism>(session_container, the_input_focus_selector());
227- auto focus_selection_strategy = std::make_shared<msh::RegistrationOrderFocusSequence>(session_container);
228-
229- auto placement_strategy = std::make_shared<msh::ConsumingPlacementStrategy>(the_display());
230- auto organising_factory = std::make_shared<msh::OrganisingSurfaceFactory>(the_surface_factory(), placement_strategy);
231-
232- return std::make_shared<msh::SessionManager>(organising_factory, session_container, focus_selection_strategy, focus_mechanism);
233+ return std::make_shared<msh::SessionManager>(the_shell_configuration());
234 });
235 }
236
237
238=== modified file 'src/server/shell/CMakeLists.txt'
239--- src/server/shell/CMakeLists.txt 2013-03-21 03:32:59 +0000
240+++ src/server/shell/CMakeLists.txt 2013-04-15 14:20:37 +0000
241@@ -1,6 +1,7 @@
242 set(
243 SHELL_SOURCES
244
245+ default_shell_configuration.cpp
246 application_session.cpp
247 session_container.cpp
248 session_manager.cpp
249
250=== added file 'src/server/shell/default_shell_configuration.cpp'
251--- src/server/shell/default_shell_configuration.cpp 1970-01-01 00:00:00 +0000
252+++ src/server/shell/default_shell_configuration.cpp 2013-04-15 14:20:37 +0000
253@@ -0,0 +1,82 @@
254+/*
255+ * Copyright © 2013 Canonical Ltd.
256+ *
257+ * This program is free software: you can redistribute it and/or modify it
258+ * under the terms of the GNU General Public License version 3,
259+ * as published by the Free Software Foundation.
260+ *
261+ * This program is distributed in the hope that it will be useful,
262+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
263+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
264+ * GNU General Public License for more details.
265+ *
266+ * You should have received a copy of the GNU General Public License
267+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
268+ *
269+ * Authored by: Robert Carr <robert.carr@canonical.com>
270+ */
271+
272+#include "mir/shell/default_shell_configuration.h"
273+
274+#include "mir/shell/organising_surface_factory.h"
275+#include "mir/shell/consuming_placement_strategy.h"
276+#include "mir/shell/session_container.h"
277+#include "mir/shell/registration_order_focus_sequence.h"
278+#include "mir/shell/single_visibility_focus_mechanism.h"
279+
280+namespace msh = mir::shell;
281+namespace mg = mir::graphics;
282+
283+msh::DefaultShellConfiguration::DefaultShellConfiguration(std::shared_ptr<mg::ViewableArea> const& view_area,
284+ std::shared_ptr<msh::InputFocusSelector> const& focus_selector,
285+ std::shared_ptr<msh::SurfaceFactory> const& surface_factory) :
286+ view_area(view_area),
287+ input_focus_selector(focus_selector),
288+ underlying_surface_factory(surface_factory)
289+{
290+}
291+
292+std::shared_ptr<msh::SurfaceFactory> msh::DefaultShellConfiguration::the_surface_factory()
293+{
294+ return surface_factory(
295+ [this]() -> std::shared_ptr<msh::SurfaceFactory>
296+ {
297+ return std::make_shared<msh::OrganisingSurfaceFactory>(underlying_surface_factory, the_placement_strategy());
298+ });
299+}
300+
301+std::shared_ptr<msh::SessionContainer> msh::DefaultShellConfiguration::the_session_container()
302+{
303+ return session_container(
304+ [this]()
305+ {
306+ return std::make_shared<msh::SessionContainer>();
307+ });
308+}
309+
310+std::shared_ptr<msh::FocusSequence> msh::DefaultShellConfiguration::the_focus_sequence()
311+{
312+ return focus_sequence(
313+ [this]()
314+ {
315+ return std::make_shared<msh::RegistrationOrderFocusSequence>(the_session_container());
316+ });
317+}
318+
319+std::shared_ptr<msh::FocusSetter> msh::DefaultShellConfiguration::the_focus_setter()
320+{
321+ return focus_setter(
322+ [this]()
323+ {
324+ return std::make_shared<msh::SingleVisibilityFocusMechanism>(the_session_container(), input_focus_selector);
325+ });
326+}
327+
328+std::shared_ptr<msh::PlacementStrategy> msh::DefaultShellConfiguration::the_placement_strategy()
329+{
330+ return placement_strategy(
331+ [this]()
332+ {
333+ return std::make_shared<msh::ConsumingPlacementStrategy>(view_area);
334+ });
335+}
336
337=== modified file 'src/server/shell/session_manager.cpp'
338--- src/server/shell/session_manager.cpp 2013-04-12 04:55:11 +0000
339+++ src/server/shell/session_manager.cpp 2013-04-15 14:20:37 +0000
340@@ -17,6 +17,7 @@
341 */
342
343 #include "mir/shell/session_manager.h"
344+#include "mir/shell/shell_configuration.h"
345 #include "mir/shell/application_session.h"
346 #include "mir/shell/session_container.h"
347 #include "mir/shell/surface_factory.h"
348@@ -31,19 +32,15 @@
349 namespace mf = mir::frontend;
350 namespace msh = mir::shell;
351
352-msh::SessionManager::SessionManager(
353- std::shared_ptr<msh::SurfaceFactory> const& surface_factory,
354- std::shared_ptr<msh::SessionContainer> const& container,
355- std::shared_ptr<msh::FocusSequence> const& sequence,
356- std::shared_ptr<msh::FocusSetter> const& focus_setter) :
357- surface_factory(surface_factory),
358- app_container(container),
359- focus_sequence(sequence),
360- focus_setter(focus_setter)
361+msh::SessionManager::SessionManager(std::shared_ptr<msh::ShellConfiguration> const& config) :
362+ surface_factory(config->the_surface_factory()),
363+ app_container(config->the_session_container()),
364+ focus_sequence(config->the_focus_sequence()),
365+ focus_setter(config->the_focus_setter())
366 {
367 assert(surface_factory);
368- assert(sequence);
369- assert(container);
370+ assert(focus_sequence);
371+ assert(app_container);
372 assert(focus_setter);
373 }
374
375
376=== modified file 'tests/acceptance-tests/test_focus_selection.cpp'
377--- tests/acceptance-tests/test_focus_selection.cpp 2013-04-08 04:03:40 +0000
378+++ tests/acceptance-tests/test_focus_selection.cpp 2013-04-15 14:20:37 +0000
379@@ -23,12 +23,14 @@
380 #include "mir/shell/consuming_placement_strategy.h"
381 #include "mir/shell/organising_surface_factory.h"
382 #include "mir/shell/session_manager.h"
383+#include "mir/shell/default_shell_configuration.h"
384 #include "mir/graphics/display.h"
385 #include "mir/shell/input_focus_selector.h"
386
387 #include "mir_test_framework/display_server_test_fixture.h"
388 #include "mir_test_doubles/mock_focus_setter.h"
389 #include "mir_test_doubles/mock_input_focus_selector.h"
390+#include "mir_test/fake_shared.h"
391
392 #include <gtest/gtest.h>
393 #include <gmock/gmock.h>
394@@ -36,7 +38,9 @@
395 namespace mf = mir::frontend;
396 namespace msh = mir::shell;
397 namespace mi = mir::input;
398-namespace mtd = mir::test::doubles;
399+namespace mg = mir::graphics;
400+namespace mt = mir::test;
401+namespace mtd = mt::doubles;
402 namespace mtf = mir_test_framework;
403
404 namespace
405@@ -46,6 +50,25 @@
406
407 namespace
408 {
409+struct MockFocusShellConfiguration : public msh::DefaultShellConfiguration
410+{
411+ MockFocusShellConfiguration(std::shared_ptr<mg::ViewableArea> const& view_area,
412+ std::shared_ptr<msh::InputFocusSelector> const& input_selector,
413+ std::shared_ptr<msh::SurfaceFactory> const& surface_factory) :
414+ DefaultShellConfiguration(view_area, input_selector, surface_factory)
415+ {
416+ }
417+
418+ ~MockFocusShellConfiguration() noexcept(true) {}
419+
420+ std::shared_ptr<msh::FocusSetter> the_focus_setter() override
421+ {
422+ return mt::fake_shared(mock_focus_setter);
423+ }
424+
425+ mtd::MockFocusSetter mock_focus_setter;
426+};
427+
428 struct ClientConfigCommon : TestingClientConfiguration
429 {
430 ClientConfigCommon() :
431@@ -139,31 +162,29 @@
432 {
433 struct ServerConfig : TestingServerConfiguration
434 {
435+ std::shared_ptr<MockFocusShellConfiguration> shell_config;
436+
437 std::shared_ptr<mf::Shell>
438 the_frontend_shell()
439 {
440+ if (!shell_config)
441+ shell_config = std::make_shared<MockFocusShellConfiguration>(the_display(),
442+ the_input_focus_selector(),
443+ the_surface_factory());
444 return session_manager(
445 [this]() -> std::shared_ptr<msh::SessionManager>
446 {
447 using namespace ::testing;
448-
449- auto session_container = std::make_shared<msh::SessionContainer>();
450- auto focus_setter = std::make_shared<mtd::MockFocusSetter>();
451- auto focus_selection_strategy = std::make_shared<msh::RegistrationOrderFocusSequence>(session_container);
452-
453- auto placement_strategy = std::make_shared<msh::ConsumingPlacementStrategy>(the_display());
454- auto organising_factory = std::make_shared<msh::OrganisingSurfaceFactory>(the_surface_factory(), placement_strategy);
455-
456 {
457 InSequence seq;
458 // Once on application registration and once on surface creation
459- EXPECT_CALL(*focus_setter, set_focus_to(NonNullSession())).Times(2);
460+ EXPECT_CALL(shell_config->mock_focus_setter, set_focus_to(NonNullSession())).Times(2);
461 // Focus is cleared when the session is closed
462- EXPECT_CALL(*focus_setter, set_focus_to(_)).Times(1);
463+ EXPECT_CALL(shell_config->mock_focus_setter, set_focus_to(_)).Times(1);
464 }
465 // TODO: Counterexample ~racarr
466
467- return std::make_shared<msh::SessionManager>(organising_factory, session_container, focus_selection_strategy, focus_setter);
468+ return std::make_shared<msh::SessionManager>(shell_config);
469 });
470 }
471 } server_config;
472
473=== modified file 'tests/death-tests/test_application_manager_death.cpp'
474--- tests/death-tests/test_application_manager_death.cpp 2013-03-21 03:32:59 +0000
475+++ tests/death-tests/test_application_manager_death.cpp 2013-04-15 14:20:37 +0000
476@@ -17,11 +17,35 @@
477 */
478
479 #include "mir/shell/session_manager.h"
480+#include "mir/shell/shell_configuration.h"
481 #include <gmock/gmock.h>
482 #include <gtest/gtest.h>
483
484 namespace msh = mir::shell;
485
486+namespace
487+{
488+struct NullShellConfiguration : public msh::ShellConfiguration
489+{
490+ std::shared_ptr<msh::SurfaceFactory> the_surface_factory()
491+ {
492+ return std::shared_ptr<msh::SurfaceFactory>();
493+ }
494+ std::shared_ptr<msh::SessionContainer> the_session_container()
495+ {
496+ return std::shared_ptr<msh::SessionContainer>();
497+ }
498+ std::shared_ptr<msh::FocusSequence> the_focus_sequence()
499+ {
500+ return std::shared_ptr<msh::FocusSequence>();
501+ }
502+ std::shared_ptr<msh::FocusSetter> the_focus_setter()
503+ {
504+ return std::shared_ptr<msh::FocusSetter>();
505+ }
506+};
507+}
508+
509 TEST(SessionManagerDeathTest, DISABLED_class_invariants_not_satisfied_triggers_assertion)
510 {
511 // Trying to avoid "[WARNING] /usr/src/gtest/src/gtest-death-test.cc:789::
512@@ -30,11 +54,7 @@
513 // ::testing::FLAGS_gtest_death_test_style = "threadsafe";
514 // leads to the test failing under valgrind
515 EXPECT_EXIT(
516- std::shared_ptr<msh::SurfaceFactory> factory;
517- mir::shell::SessionManager app(factory,
518- std::shared_ptr<msh::SessionContainer>(),
519- std::shared_ptr<msh::FocusSequence>(),
520- std::shared_ptr<msh::FocusSetter>()),
521+ mir::shell::SessionManager app(std::make_shared<NullShellConfiguration>()),
522 ::testing::KilledBySignal(SIGABRT),
523 ".*");
524 }
525
526=== modified file 'tests/integration-tests/shell/test_session_manager.cpp'
527--- tests/integration-tests/shell/test_session_manager.cpp 2013-03-22 16:41:59 +0000
528+++ tests/integration-tests/shell/test_session_manager.cpp 2013-04-15 14:20:37 +0000
529@@ -17,6 +17,7 @@
530 */
531
532 #include "mir/shell/session_manager.h"
533+#include "mir/shell/shell_configuration.h"
534 #include "mir/shell/session.h"
535 #include "mir/shell/focus_sequence.h"
536 #include "mir/shell/focus_setter.h"
537@@ -41,22 +42,45 @@
538 namespace mt = mir::test;
539 namespace mtd = mir::test::doubles;
540
541+struct TestingShellConfiguration : public msh::ShellConfiguration
542+{
543+ TestingShellConfiguration() :
544+ sequence(mt::fake_shared(container))
545+ {
546+ }
547+ ~TestingShellConfiguration() noexcept(true) {}
548+
549+ std::shared_ptr<msh::SurfaceFactory> the_surface_factory()
550+ {
551+ return mt::fake_shared(surface_factory);
552+ }
553+ std::shared_ptr<msh::SessionContainer> the_session_container()
554+ {
555+ return mt::fake_shared(container);
556+ }
557+ std::shared_ptr<msh::FocusSequence> the_focus_sequence()
558+ {
559+ return mt::fake_shared(sequence);
560+ }
561+ std::shared_ptr<msh::FocusSetter> the_focus_setter()
562+ {
563+ return mt::fake_shared(focus_setter);
564+ }
565+
566+ mtd::MockSurfaceFactory surface_factory;
567+ msh::SessionContainer container;
568+ msh::RegistrationOrderFocusSequence sequence;
569+ mtd::MockFocusSetter focus_setter;
570+};
571+
572 TEST(TestSessionManagerAndFocusSelectionStrategy, cycle_focus)
573 {
574 using namespace ::testing;
575- mtd::MockSurfaceFactory surface_factory;
576- std::shared_ptr<msh::SessionContainer> container(new msh::SessionContainer());
577- msh::RegistrationOrderFocusSequence sequence(container);
578- mtd::MockFocusSetter focus_changer;
579- std::shared_ptr<mf::Session> new_session;
580-
581- msh::SessionManager session_manager(
582- mt::fake_shared(surface_factory),
583- container,
584- mt::fake_shared(sequence),
585- mt::fake_shared(focus_changer));
586-
587- EXPECT_CALL(focus_changer, set_focus_to(_)).Times(3);
588+
589+ TestingShellConfiguration config;
590+ msh::SessionManager session_manager(mt::fake_shared(config));
591+
592+ EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(3);
593
594 auto session1 = session_manager.open_session("Visual Basic Studio");
595 auto session2 = session_manager.open_session("Microsoft Access");
596@@ -64,9 +88,9 @@
597
598 {
599 InSequence seq;
600- EXPECT_CALL(focus_changer, set_focus_to(Eq(session1))).Times(1);
601- EXPECT_CALL(focus_changer, set_focus_to(Eq(session2))).Times(1);
602- EXPECT_CALL(focus_changer, set_focus_to(Eq(session3))).Times(1);
603+ EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session1))).Times(1);
604+ EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session2))).Times(1);
605+ EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session3))).Times(1);
606 }
607
608 session_manager.focus_next();
609@@ -77,19 +101,11 @@
610 TEST(TestSessionManagerAndFocusSelectionStrategy, closing_applications_transfers_focus)
611 {
612 using namespace ::testing;
613- mtd::MockSurfaceFactory surface_factory;
614- std::shared_ptr<msh::SessionContainer> model(new msh::SessionContainer());
615- msh::RegistrationOrderFocusSequence sequence(model);
616- mtd::MockFocusSetter focus_changer;
617- std::shared_ptr<mf::Session> new_session;
618-
619- msh::SessionManager session_manager(
620- mt::fake_shared(surface_factory),
621- model,
622- mt::fake_shared(sequence),
623- mt::fake_shared(focus_changer));
624-
625- EXPECT_CALL(focus_changer, set_focus_to(_)).Times(3);
626+
627+ TestingShellConfiguration config;
628+ msh::SessionManager session_manager(mt::fake_shared(config));
629+
630+ EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(3);
631
632 auto session1 = session_manager.open_session("Visual Basic Studio");
633 auto session2 = session_manager.open_session("Microsoft Access");
634@@ -97,8 +113,8 @@
635
636 {
637 InSequence seq;
638- EXPECT_CALL(focus_changer, set_focus_to(Eq(session2))).Times(1);
639- EXPECT_CALL(focus_changer, set_focus_to(Eq(session1))).Times(1);
640+ EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session2))).Times(1);
641+ EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session1))).Times(1);
642 }
643
644 session_manager.close_session(session3);
645
646=== modified file 'tests/unit-tests/shell/test_session_manager.cpp'
647--- tests/unit-tests/shell/test_session_manager.cpp 2013-04-01 16:11:03 +0000
648+++ tests/unit-tests/shell/test_session_manager.cpp 2013-04-15 14:20:37 +0000
649@@ -21,6 +21,7 @@
650 #include "mir/shell/session_manager.h"
651 #include "mir/shell/session_container.h"
652 #include "mir/shell/session.h"
653+#include "mir/shell/shell_configuration.h"
654 #include "mir/frontend/session.h"
655 #include "mir/frontend/surface_creation_parameters.h"
656 #include "mir/surfaces/surface.h"
657@@ -63,22 +64,43 @@
658 MOCK_CONST_METHOD0(default_focus, std::shared_ptr<mf::Session>());
659 };
660
661+struct TestingShellConfiguration : public msh::ShellConfiguration
662+{
663+ TestingShellConfiguration() {}
664+ ~TestingShellConfiguration() noexcept(true) {}
665+
666+ std::shared_ptr<msh::SurfaceFactory> the_surface_factory()
667+ {
668+ return mt::fake_shared(surface_factory);
669+ }
670+ std::shared_ptr<msh::SessionContainer> the_session_container()
671+ {
672+ return mt::fake_shared(container);
673+ }
674+ std::shared_ptr<msh::FocusSequence> the_focus_sequence()
675+ {
676+ return mt::fake_shared(focus_sequence);
677+ }
678+ std::shared_ptr<msh::FocusSetter> the_focus_setter()
679+ {
680+ return mt::fake_shared(focus_setter);
681+ }
682+
683+ mtd::MockSurfaceFactory surface_factory;
684+ MockSessionContainer container;
685+ MockFocusSequence focus_sequence;
686+ ::testing::NiceMock<mtd::MockFocusSetter> focus_setter;
687+};
688+
689 struct SessionManagerSetup : public testing::Test
690 {
691- SessionManagerSetup()
692- : session_manager(mt::fake_shared(surface_factory),
693- mt::fake_shared(container),
694- mt::fake_shared(sequence),
695- mt::fake_shared(focus_setter))
696+ SessionManagerSetup() :
697+ session_manager(mt::fake_shared(config))
698 {
699 }
700
701 mtd::StubSurfaceBuilder surface_builder;
702- mtd::MockSurfaceFactory surface_factory;
703- testing::NiceMock<MockSessionContainer> container; // Inelegant but some tests need a stub
704- MockFocusSequence sequence;
705- testing::NiceMock<mtd::MockFocusSetter> focus_setter; // Inelegant but some tests need a stub
706-
707+ TestingShellConfiguration config;
708 msh::SessionManager session_manager;
709 };
710
711@@ -88,12 +110,12 @@
712 {
713 using namespace ::testing;
714
715- EXPECT_CALL(container, insert_session(_)).Times(1);
716- EXPECT_CALL(container, remove_session(_)).Times(1);
717- EXPECT_CALL(focus_setter, set_focus_to(_));
718- EXPECT_CALL(focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);
719+ EXPECT_CALL(config.container, insert_session(_)).Times(1);
720+ EXPECT_CALL(config.container, remove_session(_)).Times(1);
721+ EXPECT_CALL(config.focus_setter, set_focus_to(_));
722+ EXPECT_CALL(config.focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);
723
724- EXPECT_CALL(sequence, default_focus()).WillOnce(Return((std::shared_ptr<mf::Session>())));
725+ EXPECT_CALL(config.focus_sequence, default_focus()).WillOnce(Return((std::shared_ptr<mf::Session>())));
726
727 auto session = session_manager.open_session("Visual Basic Studio");
728 session_manager.close_session(session);
729@@ -103,22 +125,22 @@
730 {
731 using namespace ::testing;
732
733- EXPECT_CALL(surface_factory, create_surface(_)).Times(1);
734+ EXPECT_CALL(config.surface_factory, create_surface(_)).Times(1);
735
736 std::shared_ptr<mi::InputChannel> null_input_channel;
737- ON_CALL(surface_factory, create_surface(_)).WillByDefault(
738+ ON_CALL(config.surface_factory, create_surface(_)).WillByDefault(
739 Return(std::make_shared<msh::Surface>(
740 mt::fake_shared(surface_builder),
741 mf::a_surface(),
742 null_input_channel)));
743
744- EXPECT_CALL(container, insert_session(_)).Times(1);
745- EXPECT_CALL(container, remove_session(_)).Times(1);
746-
747- EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1);
748- EXPECT_CALL(focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);
749-
750- EXPECT_CALL(sequence, default_focus()).WillOnce(Return((std::shared_ptr<mf::Session>())));
751+ EXPECT_CALL(config.container, insert_session(_)).Times(1);
752+ EXPECT_CALL(config.container, remove_session(_)).Times(1);
753+
754+ EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(1);
755+ EXPECT_CALL(config.focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);
756+
757+ EXPECT_CALL(config.focus_sequence, default_focus()).WillOnce(Return((std::shared_ptr<mf::Session>())));
758
759 auto session = session_manager.open_session("Visual Basic Studio");
760 session->create_surface(mf::a_surface().of_size(geom::Size{geom::Width{1024}, geom::Height{768}}));
761@@ -131,8 +153,8 @@
762 using namespace ::testing;
763 std::shared_ptr<mf::Session> new_session;
764
765- EXPECT_CALL(container, insert_session(_)).Times(1);
766- EXPECT_CALL(focus_setter, set_focus_to(_)).WillOnce(SaveArg<0>(&new_session));
767+ EXPECT_CALL(config.container, insert_session(_)).Times(1);
768+ EXPECT_CALL(config.focus_setter, set_focus_to(_)).WillOnce(SaveArg<0>(&new_session));
769
770 auto session = session_manager.open_session("Visual Basic Studio");
771 EXPECT_EQ(session, new_session);
772@@ -147,7 +169,7 @@
773
774 session_manager.tag_session_with_lightdm_id(session1, 1);
775
776- EXPECT_CALL(focus_setter, set_focus_to(Eq(session1)));
777+ EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session1)));
778 session_manager.focus_session_with_lightdm_id(1);
779 }
780
781@@ -161,8 +183,8 @@
782 session_manager.tag_session_with_lightdm_id(session1, 1);
783 session_manager.focus_session_with_lightdm_id(1);
784
785- EXPECT_CALL(sequence, default_focus()).WillOnce(Return(session2));
786- EXPECT_CALL(focus_setter, set_focus_to(Eq(session2)));
787+ EXPECT_CALL(config.focus_sequence, default_focus()).WillOnce(Return(session2));
788+ EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session2)));
789
790 session_manager.close_session(session1);
791 }
792@@ -171,7 +193,7 @@
793 {
794 using namespace ::testing;
795 std::shared_ptr<mi::InputChannel> null_input_channel;
796- ON_CALL(surface_factory, create_surface(_)).WillByDefault(
797+ ON_CALL(config.surface_factory, create_surface(_)).WillByDefault(
798 Return(std::make_shared<msh::Surface>(
799 mt::fake_shared(surface_builder),
800 mf::a_surface(),
801@@ -181,9 +203,9 @@
802 {
803 InSequence seq;
804
805- EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1); // Session creation
806- EXPECT_CALL(surface_factory, create_surface(_)).Times(1);
807- EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1); // Post Surface creation
808+ EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(1); // Session creation
809+ EXPECT_CALL(config.surface_factory, create_surface(_)).Times(1);
810+ EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(1); // Post Surface creation
811 }
812
813 auto session1 = session_manager.open_session("Weather Report");

Subscribers

People subscribed via source and target branches