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

I like the example - but it does show some gaps in the current architecture:

234 + // TODO: Remove strange dynamic cast. Perhaps this is an indication that SessionManager provides an interface such as
235 + // "FocusController" (and configuration, the_focus_controller())

I agree - focus_next() belongs in an interface (otherwise code that uses it cannot be unit tested).

~~~~

236 + // TODO: We use this strange two phase initialization in order to
237 + // avoid a circular dependency between the_input_filters() and the_session_store(). This seems to indicate that perhaps
238 + // the InputManager should not be the InputFocusSelector

That's the_*event*_filters() right?

Doesn't the potential circular dependency in configuration indicate that we have an ownership cycle and a potential resource leak?

I think deeper thought is required.

~~~~

217 + std::initializer_list<std::shared_ptr<mi::EventFilter> const> the_event_filters() override
218 + {
219 + static std::initializer_list<std::shared_ptr<mi::EventFilter> const> filter_list = { app_switcher };
220 + return filter_list;
221 + }
222 +
223 + std::shared_ptr<me::ApplicationSwitcher> app_switcher;

Initializing a static variable with a member variable?

That means if there ever were a second instance of this class (unlikely I know) then the function would return a reference to the ApplicationSwitcher created by the first instance. (And elsewhere the new instance is used.)