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

PASSED: Continuous integration, rev:579
http://jenkins.qa.ubuntu.com/job/mir-ci/343/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/327
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/210
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/348
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/348/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/148

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/343/rebuild

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.

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.

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/

Thanks both!

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

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.

> 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 :)

I know how to use bzr!

PASSED: Continuous integration, rev:585
http://jenkins.qa.ubuntu.com/job/mir-ci/375/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/367
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/250
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/380
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/380/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/375/rebuild

>> 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.

PASSED: Continuous integration, rev:586
http://jenkins.qa.ubuntu.com/job/mir-ci/377/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/369
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/252
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/382
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/382/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/167

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/377/rebuild

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

Fix headers for real!

PASSED: Continuous integration, rev:588
http://jenkins.qa.ubuntu.com/job/mir-ci/386/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/382
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/265
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/391
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/391/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/174

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/386/rebuild

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.

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

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.)