Mir

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

Proposed by Robert Carr
Status: Rejected
Rejected by: Robert Carr
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
Daniel van Vugt Abstain
PS Jenkins bot (community) continuous-integration Approve
Alexandros Frantzis (community) Needs Fixing
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.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
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
Revision history for this message
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
Revision history for this message
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/

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

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

Merge trunk

581. By Robert Carr

Fix copyright headers

582. By Robert Carr

Add the_shell_configuration() to default server configuration

583. By Robert Carr

~ShellConfiguration is noexcept

584. By Robert Carr

Whitespace

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

I know how to use bzr!

585. By Robert Carr

Merge trunk

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

Trim whitespace

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
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
Revision history for this message
Robert Carr (robertcarr) wrote :

Fix headers for real!

587. By Robert Carr

Merge trunk

588. By Robert Carr

Fix headers

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 :

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

Fix headers

587. By Robert Carr

Merge trunk

586. By Robert Carr

Trim whitespace

585. By Robert Carr

Merge trunk

584. By Robert Carr

Whitespace

583. By Robert Carr

~ShellConfiguration is noexcept

582. By Robert Carr

Add the_shell_configuration() to default server configuration

581. By Robert Carr

Fix copyright headers

580. By Robert Carr

Merge trunk

579. By Robert Carr

Factor placement strategy out to a default shell configuration method

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'include/server/mir/default_server_configuration.h'
--- include/server/mir/default_server_configuration.h 2013-04-12 04:55:11 +0000
+++ include/server/mir/default_server_configuration.h 2013-04-15 14:20:37 +0000
@@ -48,6 +48,7 @@
4848
49namespace shell49namespace shell
50{50{
51class ShellConfiguration;
51class SessionManager;52class SessionManager;
52class SurfaceFactory;53class SurfaceFactory;
53class SurfaceSource;54class SurfaceSource;
@@ -108,6 +109,7 @@
108 virtual std::shared_ptr<frontend::SessionMediatorReport> the_session_mediator_report();109 virtual std::shared_ptr<frontend::SessionMediatorReport> the_session_mediator_report();
109 virtual std::shared_ptr<frontend::MessageProcessorReport> the_message_processor_report();110 virtual std::shared_ptr<frontend::MessageProcessorReport> the_message_processor_report();
110111
112 virtual std::shared_ptr<shell::ShellConfiguration> the_shell_configuration();
111 virtual std::shared_ptr<frontend::Shell> the_frontend_shell();113 virtual std::shared_ptr<frontend::Shell> the_frontend_shell();
112 virtual std::shared_ptr<shell::SurfaceFactory> the_surface_factory();114 virtual std::shared_ptr<shell::SurfaceFactory> the_surface_factory();
113115
@@ -130,6 +132,7 @@
130 virtual std::shared_ptr<input::InputChannelFactory> the_input_channel_factory();132 virtual std::shared_ptr<input::InputChannelFactory> the_input_channel_factory();
131133
132 CachedPtr<frontend::Communicator> communicator;134 CachedPtr<frontend::Communicator> communicator;
135 CachedPtr<shell::ShellConfiguration> shell_configuration;
133 CachedPtr<frontend::Shell> session_manager;136 CachedPtr<frontend::Shell> session_manager;
134 CachedPtr<input::InputManager> input_manager;137 CachedPtr<input::InputManager> input_manager;
135 CachedPtr<graphics::Platform> graphics_platform;138 CachedPtr<graphics::Platform> graphics_platform;
136139
=== added file 'include/server/mir/shell/default_shell_configuration.h'
--- include/server/mir/shell/default_shell_configuration.h 1970-01-01 00:00:00 +0000
+++ include/server/mir/shell/default_shell_configuration.h 2013-04-15 14:20:37 +0000
@@ -0,0 +1,71 @@
1/*
2 * Copyright © 2013 Canonical Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU General Public License version 3,
6 * as published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Authored by: Robert Carr <robert.carr@canonical.com>
17 */
18
19#ifndef MIR_SHELL_DEFAULT_SHELL_CONFIGURATION_H_
20#define MIR_SHELL_DEFAULT_SHELL_CONFIGURATION_H_
21
22#include "mir/shell/shell_configuration.h"
23#include "mir/cached_ptr.h"
24
25namespace mir
26{
27namespace graphics
28{
29class ViewableArea;
30}
31namespace shell
32{
33class InputFocusSelector;
34class SurfaceFactory;
35class PlacementStrategy;
36
37class DefaultShellConfiguration : public ShellConfiguration
38{
39public:
40 DefaultShellConfiguration(std::shared_ptr<graphics::ViewableArea> const& view_area,
41 std::shared_ptr<InputFocusSelector> const& focus_selector,
42 std::shared_ptr<SurfaceFactory> const& surface_factory);
43 virtual ~DefaultShellConfiguration() noexcept(true) = default;
44
45 std::shared_ptr<SurfaceFactory> the_surface_factory();
46 std::shared_ptr<SessionContainer> the_session_container();
47 std::shared_ptr<FocusSequence> the_focus_sequence();
48 std::shared_ptr<FocusSetter> the_focus_setter();
49
50 std::shared_ptr<PlacementStrategy> the_placement_strategy();
51
52protected:
53 DefaultShellConfiguration(DefaultShellConfiguration const&) = delete;
54 DefaultShellConfiguration& operator=(DefaultShellConfiguration const&) = delete;
55
56private:
57 std::shared_ptr<graphics::ViewableArea> const view_area;
58 std::shared_ptr<InputFocusSelector> const input_focus_selector;
59 std::shared_ptr<SurfaceFactory> const underlying_surface_factory;
60
61 CachedPtr<SurfaceFactory> surface_factory;
62 CachedPtr<SessionContainer> session_container;
63 CachedPtr<FocusSequence> focus_sequence;
64 CachedPtr<FocusSetter> focus_setter;
65 CachedPtr<PlacementStrategy> placement_strategy;
66};
67
68}
69} // namespace mir
70
71#endif // MIR_SHELL_DEFAULT_SHELL_CONFIGURATION_H_
072
=== modified file 'include/server/mir/shell/session_manager.h'
--- include/server/mir/shell/session_manager.h 2013-04-12 04:55:11 +0000
+++ include/server/mir/shell/session_manager.h 2013-04-15 14:20:37 +0000
@@ -36,6 +36,7 @@
36/// Management of sessions and surfaces36/// Management of sessions and surfaces
37namespace shell37namespace shell
38{38{
39class ShellConfiguration;
39class SurfaceFactory;40class SurfaceFactory;
40class SessionContainer;41class SessionContainer;
41class FocusSequence;42class FocusSequence;
@@ -44,10 +45,7 @@
44class SessionManager : public frontend::Shell45class SessionManager : public frontend::Shell
45{46{
46public:47public:
47 explicit SessionManager(std::shared_ptr<SurfaceFactory> const& surface_factory,48 explicit SessionManager(std::shared_ptr<ShellConfiguration> const& configuration);
48 std::shared_ptr<SessionContainer> const& session_container,
49 std::shared_ptr<FocusSequence> const& focus_sequence,
50 std::shared_ptr<FocusSetter> const& focus_setter);
51 virtual ~SessionManager();49 virtual ~SessionManager();
5250
53 virtual std::shared_ptr<frontend::Session> open_session(std::string const& name);51 virtual std::shared_ptr<frontend::Session> open_session(std::string const& name);
5452
=== added file 'include/server/mir/shell/shell_configuration.h'
--- include/server/mir/shell/shell_configuration.h 1970-01-01 00:00:00 +0000
+++ include/server/mir/shell/shell_configuration.h 2013-04-15 14:20:37 +0000
@@ -0,0 +1,52 @@
1/*
2 * Copyright © 2013 Canonical Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU General Public License version 3,
6 * as published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Authored by: Robert Carr <robert.carr@canonical.com>
17 */
18
19#ifndef MIR_SHELL_SHELL_CONFIGURATION_H_
20#define MIR_SHELL_SHELL_CONFIGURATION_H_
21
22#include <memory>
23
24namespace mir
25{
26namespace shell
27{
28class SurfaceFactory;
29class SessionContainer;
30class FocusSequence;
31class FocusSetter;
32
33class ShellConfiguration
34{
35public:
36 virtual ~ShellConfiguration() = default;
37
38 virtual std::shared_ptr<SurfaceFactory> the_surface_factory() = 0;
39 virtual std::shared_ptr<SessionContainer> the_session_container() = 0;
40 virtual std::shared_ptr<FocusSequence> the_focus_sequence() = 0;
41 virtual std::shared_ptr<FocusSetter> the_focus_setter() = 0;
42
43protected:
44 ShellConfiguration() = default;
45 ShellConfiguration(ShellConfiguration const&) = delete;
46 ShellConfiguration& operator=(ShellConfiguration const&) = delete;
47};
48
49}
50} // namespace mir
51
52#endif // MIR_SHELL_SHELL_CONFIGURATION_H_
053
=== modified file 'src/server/default_server_configuration.cpp'
--- src/server/default_server_configuration.cpp 2013-04-12 04:55:11 +0000
+++ src/server/default_server_configuration.cpp 2013-04-15 14:20:37 +0000
@@ -32,12 +32,8 @@
32#include "mir/frontend/null_message_processor_report.h"32#include "mir/frontend/null_message_processor_report.h"
33#include "mir/frontend/session_mediator.h"33#include "mir/frontend/session_mediator.h"
34#include "mir/frontend/resource_cache.h"34#include "mir/frontend/resource_cache.h"
35#include "mir/shell/default_shell_configuration.h"
35#include "mir/shell/session_manager.h"36#include "mir/shell/session_manager.h"
36#include "mir/shell/registration_order_focus_sequence.h"
37#include "mir/shell/single_visibility_focus_mechanism.h"
38#include "mir/shell/session_container.h"
39#include "mir/shell/consuming_placement_strategy.h"
40#include "mir/shell/organising_surface_factory.h"
41#include "mir/graphics/display.h"37#include "mir/graphics/display.h"
42#include "mir/graphics/gl_renderer.h"38#include "mir/graphics/gl_renderer.h"
43#include "mir/graphics/renderer.h"39#include "mir/graphics/renderer.h"
@@ -292,20 +288,24 @@
292 });288 });
293}289}
294290
291std::shared_ptr<msh::ShellConfiguration>
292mir::DefaultServerConfiguration::the_shell_configuration()
293{
294 return shell_configuration(
295 [this]()
296 {
297 return std::make_shared<msh::DefaultShellConfiguration>(the_display(), the_input_focus_selector(),
298 the_surface_factory());
299 });
300}
301
295std::shared_ptr<mf::Shell>302std::shared_ptr<mf::Shell>
296mir::DefaultServerConfiguration::the_frontend_shell()303mir::DefaultServerConfiguration::the_frontend_shell()
297{304{
298 return session_manager(305 return session_manager(
299 [this]() -> std::shared_ptr<msh::SessionManager>306 [this]()
300 {307 {
301 auto session_container = std::make_shared<msh::SessionContainer>();308 return std::make_shared<msh::SessionManager>(the_shell_configuration());
302 auto focus_mechanism = std::make_shared<msh::SingleVisibilityFocusMechanism>(session_container, the_input_focus_selector());
303 auto focus_selection_strategy = std::make_shared<msh::RegistrationOrderFocusSequence>(session_container);
304
305 auto placement_strategy = std::make_shared<msh::ConsumingPlacementStrategy>(the_display());
306 auto organising_factory = std::make_shared<msh::OrganisingSurfaceFactory>(the_surface_factory(), placement_strategy);
307
308 return std::make_shared<msh::SessionManager>(organising_factory, session_container, focus_selection_strategy, focus_mechanism);
309 });309 });
310}310}
311311
312312
=== modified file 'src/server/shell/CMakeLists.txt'
--- src/server/shell/CMakeLists.txt 2013-03-21 03:32:59 +0000
+++ src/server/shell/CMakeLists.txt 2013-04-15 14:20:37 +0000
@@ -1,6 +1,7 @@
1set(1set(
2 SHELL_SOURCES2 SHELL_SOURCES
33
4 default_shell_configuration.cpp
4 application_session.cpp5 application_session.cpp
5 session_container.cpp6 session_container.cpp
6 session_manager.cpp7 session_manager.cpp
78
=== added file 'src/server/shell/default_shell_configuration.cpp'
--- src/server/shell/default_shell_configuration.cpp 1970-01-01 00:00:00 +0000
+++ src/server/shell/default_shell_configuration.cpp 2013-04-15 14:20:37 +0000
@@ -0,0 +1,82 @@
1/*
2 * Copyright © 2013 Canonical Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU General Public License version 3,
6 * as published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Authored by: Robert Carr <robert.carr@canonical.com>
17 */
18
19#include "mir/shell/default_shell_configuration.h"
20
21#include "mir/shell/organising_surface_factory.h"
22#include "mir/shell/consuming_placement_strategy.h"
23#include "mir/shell/session_container.h"
24#include "mir/shell/registration_order_focus_sequence.h"
25#include "mir/shell/single_visibility_focus_mechanism.h"
26
27namespace msh = mir::shell;
28namespace mg = mir::graphics;
29
30msh::DefaultShellConfiguration::DefaultShellConfiguration(std::shared_ptr<mg::ViewableArea> const& view_area,
31 std::shared_ptr<msh::InputFocusSelector> const& focus_selector,
32 std::shared_ptr<msh::SurfaceFactory> const& surface_factory) :
33 view_area(view_area),
34 input_focus_selector(focus_selector),
35 underlying_surface_factory(surface_factory)
36{
37}
38
39std::shared_ptr<msh::SurfaceFactory> msh::DefaultShellConfiguration::the_surface_factory()
40{
41 return surface_factory(
42 [this]() -> std::shared_ptr<msh::SurfaceFactory>
43 {
44 return std::make_shared<msh::OrganisingSurfaceFactory>(underlying_surface_factory, the_placement_strategy());
45 });
46}
47
48std::shared_ptr<msh::SessionContainer> msh::DefaultShellConfiguration::the_session_container()
49{
50 return session_container(
51 [this]()
52 {
53 return std::make_shared<msh::SessionContainer>();
54 });
55}
56
57std::shared_ptr<msh::FocusSequence> msh::DefaultShellConfiguration::the_focus_sequence()
58{
59 return focus_sequence(
60 [this]()
61 {
62 return std::make_shared<msh::RegistrationOrderFocusSequence>(the_session_container());
63 });
64}
65
66std::shared_ptr<msh::FocusSetter> msh::DefaultShellConfiguration::the_focus_setter()
67{
68 return focus_setter(
69 [this]()
70 {
71 return std::make_shared<msh::SingleVisibilityFocusMechanism>(the_session_container(), input_focus_selector);
72 });
73}
74
75std::shared_ptr<msh::PlacementStrategy> msh::DefaultShellConfiguration::the_placement_strategy()
76{
77 return placement_strategy(
78 [this]()
79 {
80 return std::make_shared<msh::ConsumingPlacementStrategy>(view_area);
81 });
82}
083
=== modified file 'src/server/shell/session_manager.cpp'
--- src/server/shell/session_manager.cpp 2013-04-12 04:55:11 +0000
+++ src/server/shell/session_manager.cpp 2013-04-15 14:20:37 +0000
@@ -17,6 +17,7 @@
17 */17 */
1818
19#include "mir/shell/session_manager.h"19#include "mir/shell/session_manager.h"
20#include "mir/shell/shell_configuration.h"
20#include "mir/shell/application_session.h"21#include "mir/shell/application_session.h"
21#include "mir/shell/session_container.h"22#include "mir/shell/session_container.h"
22#include "mir/shell/surface_factory.h"23#include "mir/shell/surface_factory.h"
@@ -31,19 +32,15 @@
31namespace mf = mir::frontend;32namespace mf = mir::frontend;
32namespace msh = mir::shell;33namespace msh = mir::shell;
3334
34msh::SessionManager::SessionManager(35msh::SessionManager::SessionManager(std::shared_ptr<msh::ShellConfiguration> const& config) :
35 std::shared_ptr<msh::SurfaceFactory> const& surface_factory,36 surface_factory(config->the_surface_factory()),
36 std::shared_ptr<msh::SessionContainer> const& container,37 app_container(config->the_session_container()),
37 std::shared_ptr<msh::FocusSequence> const& sequence,38 focus_sequence(config->the_focus_sequence()),
38 std::shared_ptr<msh::FocusSetter> const& focus_setter) :39 focus_setter(config->the_focus_setter())
39 surface_factory(surface_factory),
40 app_container(container),
41 focus_sequence(sequence),
42 focus_setter(focus_setter)
43{40{
44 assert(surface_factory);41 assert(surface_factory);
45 assert(sequence);42 assert(focus_sequence);
46 assert(container);43 assert(app_container);
47 assert(focus_setter);44 assert(focus_setter);
48}45}
4946
5047
=== modified file 'tests/acceptance-tests/test_focus_selection.cpp'
--- tests/acceptance-tests/test_focus_selection.cpp 2013-04-08 04:03:40 +0000
+++ tests/acceptance-tests/test_focus_selection.cpp 2013-04-15 14:20:37 +0000
@@ -23,12 +23,14 @@
23#include "mir/shell/consuming_placement_strategy.h"23#include "mir/shell/consuming_placement_strategy.h"
24#include "mir/shell/organising_surface_factory.h"24#include "mir/shell/organising_surface_factory.h"
25#include "mir/shell/session_manager.h"25#include "mir/shell/session_manager.h"
26#include "mir/shell/default_shell_configuration.h"
26#include "mir/graphics/display.h"27#include "mir/graphics/display.h"
27#include "mir/shell/input_focus_selector.h"28#include "mir/shell/input_focus_selector.h"
2829
29#include "mir_test_framework/display_server_test_fixture.h"30#include "mir_test_framework/display_server_test_fixture.h"
30#include "mir_test_doubles/mock_focus_setter.h"31#include "mir_test_doubles/mock_focus_setter.h"
31#include "mir_test_doubles/mock_input_focus_selector.h"32#include "mir_test_doubles/mock_input_focus_selector.h"
33#include "mir_test/fake_shared.h"
3234
33#include <gtest/gtest.h>35#include <gtest/gtest.h>
34#include <gmock/gmock.h>36#include <gmock/gmock.h>
@@ -36,7 +38,9 @@
36namespace mf = mir::frontend;38namespace mf = mir::frontend;
37namespace msh = mir::shell;39namespace msh = mir::shell;
38namespace mi = mir::input;40namespace mi = mir::input;
39namespace mtd = mir::test::doubles;41namespace mg = mir::graphics;
42namespace mt = mir::test;
43namespace mtd = mt::doubles;
40namespace mtf = mir_test_framework;44namespace mtf = mir_test_framework;
4145
42namespace46namespace
@@ -46,6 +50,25 @@
4650
47namespace51namespace
48{52{
53struct MockFocusShellConfiguration : public msh::DefaultShellConfiguration
54{
55 MockFocusShellConfiguration(std::shared_ptr<mg::ViewableArea> const& view_area,
56 std::shared_ptr<msh::InputFocusSelector> const& input_selector,
57 std::shared_ptr<msh::SurfaceFactory> const& surface_factory) :
58 DefaultShellConfiguration(view_area, input_selector, surface_factory)
59 {
60 }
61
62 ~MockFocusShellConfiguration() noexcept(true) {}
63
64 std::shared_ptr<msh::FocusSetter> the_focus_setter() override
65 {
66 return mt::fake_shared(mock_focus_setter);
67 }
68
69 mtd::MockFocusSetter mock_focus_setter;
70};
71
49struct ClientConfigCommon : TestingClientConfiguration72struct ClientConfigCommon : TestingClientConfiguration
50{73{
51 ClientConfigCommon() :74 ClientConfigCommon() :
@@ -139,31 +162,29 @@
139{162{
140 struct ServerConfig : TestingServerConfiguration163 struct ServerConfig : TestingServerConfiguration
141 {164 {
165 std::shared_ptr<MockFocusShellConfiguration> shell_config;
166
142 std::shared_ptr<mf::Shell>167 std::shared_ptr<mf::Shell>
143 the_frontend_shell()168 the_frontend_shell()
144 {169 {
170 if (!shell_config)
171 shell_config = std::make_shared<MockFocusShellConfiguration>(the_display(),
172 the_input_focus_selector(),
173 the_surface_factory());
145 return session_manager(174 return session_manager(
146 [this]() -> std::shared_ptr<msh::SessionManager>175 [this]() -> std::shared_ptr<msh::SessionManager>
147 {176 {
148 using namespace ::testing;177 using namespace ::testing;
149
150 auto session_container = std::make_shared<msh::SessionContainer>();
151 auto focus_setter = std::make_shared<mtd::MockFocusSetter>();
152 auto focus_selection_strategy = std::make_shared<msh::RegistrationOrderFocusSequence>(session_container);
153
154 auto placement_strategy = std::make_shared<msh::ConsumingPlacementStrategy>(the_display());
155 auto organising_factory = std::make_shared<msh::OrganisingSurfaceFactory>(the_surface_factory(), placement_strategy);
156
157 {178 {
158 InSequence seq;179 InSequence seq;
159 // Once on application registration and once on surface creation180 // Once on application registration and once on surface creation
160 EXPECT_CALL(*focus_setter, set_focus_to(NonNullSession())).Times(2);181 EXPECT_CALL(shell_config->mock_focus_setter, set_focus_to(NonNullSession())).Times(2);
161 // Focus is cleared when the session is closed182 // Focus is cleared when the session is closed
162 EXPECT_CALL(*focus_setter, set_focus_to(_)).Times(1);183 EXPECT_CALL(shell_config->mock_focus_setter, set_focus_to(_)).Times(1);
163 }184 }
164 // TODO: Counterexample ~racarr185 // TODO: Counterexample ~racarr
165186
166 return std::make_shared<msh::SessionManager>(organising_factory, session_container, focus_selection_strategy, focus_setter);187 return std::make_shared<msh::SessionManager>(shell_config);
167 });188 });
168 }189 }
169 } server_config;190 } server_config;
170191
=== modified file 'tests/death-tests/test_application_manager_death.cpp'
--- tests/death-tests/test_application_manager_death.cpp 2013-03-21 03:32:59 +0000
+++ tests/death-tests/test_application_manager_death.cpp 2013-04-15 14:20:37 +0000
@@ -17,11 +17,35 @@
17 */17 */
1818
19#include "mir/shell/session_manager.h"19#include "mir/shell/session_manager.h"
20#include "mir/shell/shell_configuration.h"
20#include <gmock/gmock.h>21#include <gmock/gmock.h>
21#include <gtest/gtest.h>22#include <gtest/gtest.h>
2223
23namespace msh = mir::shell;24namespace msh = mir::shell;
2425
26namespace
27{
28struct NullShellConfiguration : public msh::ShellConfiguration
29{
30 std::shared_ptr<msh::SurfaceFactory> the_surface_factory()
31 {
32 return std::shared_ptr<msh::SurfaceFactory>();
33 }
34 std::shared_ptr<msh::SessionContainer> the_session_container()
35 {
36 return std::shared_ptr<msh::SessionContainer>();
37 }
38 std::shared_ptr<msh::FocusSequence> the_focus_sequence()
39 {
40 return std::shared_ptr<msh::FocusSequence>();
41 }
42 std::shared_ptr<msh::FocusSetter> the_focus_setter()
43 {
44 return std::shared_ptr<msh::FocusSetter>();
45 }
46};
47}
48
25TEST(SessionManagerDeathTest, DISABLED_class_invariants_not_satisfied_triggers_assertion)49TEST(SessionManagerDeathTest, DISABLED_class_invariants_not_satisfied_triggers_assertion)
26{50{
27// Trying to avoid "[WARNING] /usr/src/gtest/src/gtest-death-test.cc:789::51// Trying to avoid "[WARNING] /usr/src/gtest/src/gtest-death-test.cc:789::
@@ -30,11 +54,7 @@
30// ::testing::FLAGS_gtest_death_test_style = "threadsafe";54// ::testing::FLAGS_gtest_death_test_style = "threadsafe";
31// leads to the test failing under valgrind55// leads to the test failing under valgrind
32 EXPECT_EXIT(56 EXPECT_EXIT(
33 std::shared_ptr<msh::SurfaceFactory> factory;57 mir::shell::SessionManager app(std::make_shared<NullShellConfiguration>()),
34 mir::shell::SessionManager app(factory,
35 std::shared_ptr<msh::SessionContainer>(),
36 std::shared_ptr<msh::FocusSequence>(),
37 std::shared_ptr<msh::FocusSetter>()),
38 ::testing::KilledBySignal(SIGABRT),58 ::testing::KilledBySignal(SIGABRT),
39 ".*");59 ".*");
40}60}
4161
=== modified file 'tests/integration-tests/shell/test_session_manager.cpp'
--- tests/integration-tests/shell/test_session_manager.cpp 2013-03-22 16:41:59 +0000
+++ tests/integration-tests/shell/test_session_manager.cpp 2013-04-15 14:20:37 +0000
@@ -17,6 +17,7 @@
17 */17 */
1818
19#include "mir/shell/session_manager.h"19#include "mir/shell/session_manager.h"
20#include "mir/shell/shell_configuration.h"
20#include "mir/shell/session.h"21#include "mir/shell/session.h"
21#include "mir/shell/focus_sequence.h"22#include "mir/shell/focus_sequence.h"
22#include "mir/shell/focus_setter.h"23#include "mir/shell/focus_setter.h"
@@ -41,22 +42,45 @@
41namespace mt = mir::test;42namespace mt = mir::test;
42namespace mtd = mir::test::doubles;43namespace mtd = mir::test::doubles;
4344
45struct TestingShellConfiguration : public msh::ShellConfiguration
46{
47 TestingShellConfiguration() :
48 sequence(mt::fake_shared(container))
49 {
50 }
51 ~TestingShellConfiguration() noexcept(true) {}
52
53 std::shared_ptr<msh::SurfaceFactory> the_surface_factory()
54 {
55 return mt::fake_shared(surface_factory);
56 }
57 std::shared_ptr<msh::SessionContainer> the_session_container()
58 {
59 return mt::fake_shared(container);
60 }
61 std::shared_ptr<msh::FocusSequence> the_focus_sequence()
62 {
63 return mt::fake_shared(sequence);
64 }
65 std::shared_ptr<msh::FocusSetter> the_focus_setter()
66 {
67 return mt::fake_shared(focus_setter);
68 }
69
70 mtd::MockSurfaceFactory surface_factory;
71 msh::SessionContainer container;
72 msh::RegistrationOrderFocusSequence sequence;
73 mtd::MockFocusSetter focus_setter;
74};
75
44TEST(TestSessionManagerAndFocusSelectionStrategy, cycle_focus)76TEST(TestSessionManagerAndFocusSelectionStrategy, cycle_focus)
45{77{
46 using namespace ::testing;78 using namespace ::testing;
47 mtd::MockSurfaceFactory surface_factory;79
48 std::shared_ptr<msh::SessionContainer> container(new msh::SessionContainer());80 TestingShellConfiguration config;
49 msh::RegistrationOrderFocusSequence sequence(container);81 msh::SessionManager session_manager(mt::fake_shared(config));
50 mtd::MockFocusSetter focus_changer;82
51 std::shared_ptr<mf::Session> new_session;83 EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(3);
52
53 msh::SessionManager session_manager(
54 mt::fake_shared(surface_factory),
55 container,
56 mt::fake_shared(sequence),
57 mt::fake_shared(focus_changer));
58
59 EXPECT_CALL(focus_changer, set_focus_to(_)).Times(3);
6084
61 auto session1 = session_manager.open_session("Visual Basic Studio");85 auto session1 = session_manager.open_session("Visual Basic Studio");
62 auto session2 = session_manager.open_session("Microsoft Access");86 auto session2 = session_manager.open_session("Microsoft Access");
@@ -64,9 +88,9 @@
6488
65 {89 {
66 InSequence seq;90 InSequence seq;
67 EXPECT_CALL(focus_changer, set_focus_to(Eq(session1))).Times(1);91 EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session1))).Times(1);
68 EXPECT_CALL(focus_changer, set_focus_to(Eq(session2))).Times(1);92 EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session2))).Times(1);
69 EXPECT_CALL(focus_changer, set_focus_to(Eq(session3))).Times(1);93 EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session3))).Times(1);
70 }94 }
7195
72 session_manager.focus_next();96 session_manager.focus_next();
@@ -77,19 +101,11 @@
77TEST(TestSessionManagerAndFocusSelectionStrategy, closing_applications_transfers_focus)101TEST(TestSessionManagerAndFocusSelectionStrategy, closing_applications_transfers_focus)
78{102{
79 using namespace ::testing;103 using namespace ::testing;
80 mtd::MockSurfaceFactory surface_factory;104
81 std::shared_ptr<msh::SessionContainer> model(new msh::SessionContainer());105 TestingShellConfiguration config;
82 msh::RegistrationOrderFocusSequence sequence(model);106 msh::SessionManager session_manager(mt::fake_shared(config));
83 mtd::MockFocusSetter focus_changer;107
84 std::shared_ptr<mf::Session> new_session;108 EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(3);
85
86 msh::SessionManager session_manager(
87 mt::fake_shared(surface_factory),
88 model,
89 mt::fake_shared(sequence),
90 mt::fake_shared(focus_changer));
91
92 EXPECT_CALL(focus_changer, set_focus_to(_)).Times(3);
93109
94 auto session1 = session_manager.open_session("Visual Basic Studio");110 auto session1 = session_manager.open_session("Visual Basic Studio");
95 auto session2 = session_manager.open_session("Microsoft Access");111 auto session2 = session_manager.open_session("Microsoft Access");
@@ -97,8 +113,8 @@
97113
98 {114 {
99 InSequence seq;115 InSequence seq;
100 EXPECT_CALL(focus_changer, set_focus_to(Eq(session2))).Times(1);116 EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session2))).Times(1);
101 EXPECT_CALL(focus_changer, set_focus_to(Eq(session1))).Times(1);117 EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session1))).Times(1);
102 }118 }
103119
104 session_manager.close_session(session3);120 session_manager.close_session(session3);
105121
=== modified file 'tests/unit-tests/shell/test_session_manager.cpp'
--- tests/unit-tests/shell/test_session_manager.cpp 2013-04-01 16:11:03 +0000
+++ tests/unit-tests/shell/test_session_manager.cpp 2013-04-15 14:20:37 +0000
@@ -21,6 +21,7 @@
21#include "mir/shell/session_manager.h"21#include "mir/shell/session_manager.h"
22#include "mir/shell/session_container.h"22#include "mir/shell/session_container.h"
23#include "mir/shell/session.h"23#include "mir/shell/session.h"
24#include "mir/shell/shell_configuration.h"
24#include "mir/frontend/session.h"25#include "mir/frontend/session.h"
25#include "mir/frontend/surface_creation_parameters.h"26#include "mir/frontend/surface_creation_parameters.h"
26#include "mir/surfaces/surface.h"27#include "mir/surfaces/surface.h"
@@ -63,22 +64,43 @@
63 MOCK_CONST_METHOD0(default_focus, std::shared_ptr<mf::Session>());64 MOCK_CONST_METHOD0(default_focus, std::shared_ptr<mf::Session>());
64};65};
6566
67struct TestingShellConfiguration : public msh::ShellConfiguration
68{
69 TestingShellConfiguration() {}
70 ~TestingShellConfiguration() noexcept(true) {}
71
72 std::shared_ptr<msh::SurfaceFactory> the_surface_factory()
73 {
74 return mt::fake_shared(surface_factory);
75 }
76 std::shared_ptr<msh::SessionContainer> the_session_container()
77 {
78 return mt::fake_shared(container);
79 }
80 std::shared_ptr<msh::FocusSequence> the_focus_sequence()
81 {
82 return mt::fake_shared(focus_sequence);
83 }
84 std::shared_ptr<msh::FocusSetter> the_focus_setter()
85 {
86 return mt::fake_shared(focus_setter);
87 }
88
89 mtd::MockSurfaceFactory surface_factory;
90 MockSessionContainer container;
91 MockFocusSequence focus_sequence;
92 ::testing::NiceMock<mtd::MockFocusSetter> focus_setter;
93};
94
66struct SessionManagerSetup : public testing::Test95struct SessionManagerSetup : public testing::Test
67{96{
68 SessionManagerSetup()97 SessionManagerSetup() :
69 : session_manager(mt::fake_shared(surface_factory),98 session_manager(mt::fake_shared(config))
70 mt::fake_shared(container),
71 mt::fake_shared(sequence),
72 mt::fake_shared(focus_setter))
73 {99 {
74 }100 }
75101
76 mtd::StubSurfaceBuilder surface_builder;102 mtd::StubSurfaceBuilder surface_builder;
77 mtd::MockSurfaceFactory surface_factory;103 TestingShellConfiguration config;
78 testing::NiceMock<MockSessionContainer> container; // Inelegant but some tests need a stub
79 MockFocusSequence sequence;
80 testing::NiceMock<mtd::MockFocusSetter> focus_setter; // Inelegant but some tests need a stub
81
82 msh::SessionManager session_manager;104 msh::SessionManager session_manager;
83};105};
84106
@@ -88,12 +110,12 @@
88{110{
89 using namespace ::testing;111 using namespace ::testing;
90112
91 EXPECT_CALL(container, insert_session(_)).Times(1);113 EXPECT_CALL(config.container, insert_session(_)).Times(1);
92 EXPECT_CALL(container, remove_session(_)).Times(1);114 EXPECT_CALL(config.container, remove_session(_)).Times(1);
93 EXPECT_CALL(focus_setter, set_focus_to(_));115 EXPECT_CALL(config.focus_setter, set_focus_to(_));
94 EXPECT_CALL(focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);116 EXPECT_CALL(config.focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);
95117
96 EXPECT_CALL(sequence, default_focus()).WillOnce(Return((std::shared_ptr<mf::Session>())));118 EXPECT_CALL(config.focus_sequence, default_focus()).WillOnce(Return((std::shared_ptr<mf::Session>())));
97119
98 auto session = session_manager.open_session("Visual Basic Studio");120 auto session = session_manager.open_session("Visual Basic Studio");
99 session_manager.close_session(session);121 session_manager.close_session(session);
@@ -103,22 +125,22 @@
103{125{
104 using namespace ::testing;126 using namespace ::testing;
105127
106 EXPECT_CALL(surface_factory, create_surface(_)).Times(1);128 EXPECT_CALL(config.surface_factory, create_surface(_)).Times(1);
107129
108 std::shared_ptr<mi::InputChannel> null_input_channel;130 std::shared_ptr<mi::InputChannel> null_input_channel;
109 ON_CALL(surface_factory, create_surface(_)).WillByDefault(131 ON_CALL(config.surface_factory, create_surface(_)).WillByDefault(
110 Return(std::make_shared<msh::Surface>(132 Return(std::make_shared<msh::Surface>(
111 mt::fake_shared(surface_builder),133 mt::fake_shared(surface_builder),
112 mf::a_surface(),134 mf::a_surface(),
113 null_input_channel)));135 null_input_channel)));
114136
115 EXPECT_CALL(container, insert_session(_)).Times(1);137 EXPECT_CALL(config.container, insert_session(_)).Times(1);
116 EXPECT_CALL(container, remove_session(_)).Times(1);138 EXPECT_CALL(config.container, remove_session(_)).Times(1);
117139
118 EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1);140 EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(1);
119 EXPECT_CALL(focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);141 EXPECT_CALL(config.focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);
120142
121 EXPECT_CALL(sequence, default_focus()).WillOnce(Return((std::shared_ptr<mf::Session>())));143 EXPECT_CALL(config.focus_sequence, default_focus()).WillOnce(Return((std::shared_ptr<mf::Session>())));
122144
123 auto session = session_manager.open_session("Visual Basic Studio");145 auto session = session_manager.open_session("Visual Basic Studio");
124 session->create_surface(mf::a_surface().of_size(geom::Size{geom::Width{1024}, geom::Height{768}}));146 session->create_surface(mf::a_surface().of_size(geom::Size{geom::Width{1024}, geom::Height{768}}));
@@ -131,8 +153,8 @@
131 using namespace ::testing;153 using namespace ::testing;
132 std::shared_ptr<mf::Session> new_session;154 std::shared_ptr<mf::Session> new_session;
133155
134 EXPECT_CALL(container, insert_session(_)).Times(1);156 EXPECT_CALL(config.container, insert_session(_)).Times(1);
135 EXPECT_CALL(focus_setter, set_focus_to(_)).WillOnce(SaveArg<0>(&new_session));157 EXPECT_CALL(config.focus_setter, set_focus_to(_)).WillOnce(SaveArg<0>(&new_session));
136158
137 auto session = session_manager.open_session("Visual Basic Studio");159 auto session = session_manager.open_session("Visual Basic Studio");
138 EXPECT_EQ(session, new_session);160 EXPECT_EQ(session, new_session);
@@ -147,7 +169,7 @@
147169
148 session_manager.tag_session_with_lightdm_id(session1, 1);170 session_manager.tag_session_with_lightdm_id(session1, 1);
149171
150 EXPECT_CALL(focus_setter, set_focus_to(Eq(session1)));172 EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session1)));
151 session_manager.focus_session_with_lightdm_id(1);173 session_manager.focus_session_with_lightdm_id(1);
152}174}
153175
@@ -161,8 +183,8 @@
161 session_manager.tag_session_with_lightdm_id(session1, 1);183 session_manager.tag_session_with_lightdm_id(session1, 1);
162 session_manager.focus_session_with_lightdm_id(1);184 session_manager.focus_session_with_lightdm_id(1);
163185
164 EXPECT_CALL(sequence, default_focus()).WillOnce(Return(session2));186 EXPECT_CALL(config.focus_sequence, default_focus()).WillOnce(Return(session2));
165 EXPECT_CALL(focus_setter, set_focus_to(Eq(session2)));187 EXPECT_CALL(config.focus_setter, set_focus_to(Eq(session2)));
166188
167 session_manager.close_session(session1);189 session_manager.close_session(session1);
168}190}
@@ -171,7 +193,7 @@
171{193{
172 using namespace ::testing;194 using namespace ::testing;
173 std::shared_ptr<mi::InputChannel> null_input_channel;195 std::shared_ptr<mi::InputChannel> null_input_channel;
174 ON_CALL(surface_factory, create_surface(_)).WillByDefault(196 ON_CALL(config.surface_factory, create_surface(_)).WillByDefault(
175 Return(std::make_shared<msh::Surface>(197 Return(std::make_shared<msh::Surface>(
176 mt::fake_shared(surface_builder),198 mt::fake_shared(surface_builder),
177 mf::a_surface(),199 mf::a_surface(),
@@ -181,9 +203,9 @@
181 {203 {
182 InSequence seq;204 InSequence seq;
183205
184 EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1); // Session creation206 EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(1); // Session creation
185 EXPECT_CALL(surface_factory, create_surface(_)).Times(1);207 EXPECT_CALL(config.surface_factory, create_surface(_)).Times(1);
186 EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1); // Post Surface creation208 EXPECT_CALL(config.focus_setter, set_focus_to(_)).Times(1); // Post Surface creation
187 }209 }
188210
189 auto session1 = session_manager.open_session("Weather Report");211 auto session1 = session_manager.open_session("Weather Report");

Subscribers

People subscribed via source and target branches