Configuration

[Robert Carr]

On 04/16/2013 07:28 AM, Alan Griffiths wrote:
> We've agreed that the DefaultServerConfiguration is becoming an unwieldy
> mess. (At least racarr and I have.)
>
> Before coding anything up I want to propose a few principles to see if
> there's any agreement.
>
> With a few exceptions the structural classes that are build by
> DefaultServerConfiguration take explicit shared pointers to their
> dependencies. This approach makes dependencies explicit - which, for
> example, makes use of test doubles clear when testing.
>
> The exceptions are DisplayServer (which takes ServerConfiguration by
> reference) and InputManager & DispatcherController (which take
> InputConfiguration by shared_ptr). There's also an MP under review that
> has SessionManager take ShellConfiguration by shared_ptr. (I think the
> use of shared_ptr in these three exceptions is misleading but, if we
> agree that concealing dependencies line this is bad, the shared_ptr is
> probably moot.)

I think the cases with Input and Shell are a little different. Or at 
least my intent was,

in the case of InputConfiguration, the intent is to allow InputManager 
and DispatcherController to share dependencies,
without leaking droidinput::sp in to default_server_configuration.h. 
Perhaps it should be protected. I think it's also a useful abstraction 
for tests, i.e. FakeEventHubInputConfiguration is more reusable than 
FakeEventHubServerConfiguration. More so than leaking droidinput::sp, it 
attempts to not leak droidinput concepts such as the InputDispatcher or 
EventHub in to the server configuration.

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.
>
> My concern with this latter approach is that it couples the dependencies
> of the class to an interface - and makes reuse of mir by supplying
> alternative implementations a little harder - as it is no longer always
> a matter of overriding a factory method in DefaultServerConfiguration.

I don't think this is necessarily true compared to the current code. I 
think it is easier to override the_placement_strategy and 
the_shell_configuration than the entire session store construction.

> ~~~~
>
> class ServerConfiguration { /* interface supplying dependencies of
> DisplayServer */ };
>
> class DefaultServerConfiguration :
>      public ServerConfiguration
> { /* declare the factory functions */ };
>
> // implentation file 1
> DefaultServerConfiguration methods to build Subsystem1
> ...
> // implentation file 5
> DefaultServerConfiguration methods to build Subsystem5
>
> And someone can come along with a:
>
> class NonDefaultServerConfiguration :
>      public DefaultServerConfiguration
> { /* override some of the factory functions */ };
>
> And apart from splitting up a monster .cpp we're back where we started.
> (Splitting up the implementation file isn't a bad idea though.)
>
My thought is that the shell configuration was a misguided attempt on my 
part and instead the dependencies should be added to 
DefaultServerConfiguration. I think the input configuration is a special 
case caused by the awkwardness around the droidinput boundary...I'm not 
sure what to do about it, it certainly seems like not a public member. I 
think it's useful to split the files!

Thanks,
Robert