Mir

Code review comment for lp:~robertcarr/mir/ease-shell-configuration

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

« Back to merge proposal