Subject: Configuration

From: Alan Griffiths <[email-masked]>
Date: Tue, 16 Apr 2013 15:28:48 +0100

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

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.

However, I've thought myself around in a circle trying to improve
things. (Sharing because someone out there will contribute the insight
I'm lacking.)

~~~~

What I'd like to see is that these inter-dependencies are built up
within the ServerConfiguration class hierarchy in a way that allows
alternative implementations to be substituted.

What I'm not sure is possible (without exposing lots of implementation
as template code) is how flexible such a solution is.

E.g. (Approach #1)

class ServerConfiguration { /* interface supplying dependencies of
DisplayServer */ };

class DefaultSubsystem1Dependencies : public virtual ServerConfiguration
  { /* interface supplying dependencies of Subsystem1 */ };
...
class DefaultSubsystem5Dependencies : public virtual ServerConfiguration
  { /* interface supplying dependencies of Subsystem5 */ };

class DefaultDependencies :
    public virtual DefaultSubsystem1Dependencies,
    ...
    public virtual DefaultSubsystem5Dependencies
{ /* all the dependencies */ };

class DefaultSubsystem1 : public virtual ServerConfiguration, virtual
DefaultDependencies
  { /* implementation building Subsystem1 */ };
...
class DefaultSubsystem5 : public virtual ServerConfiguration, virtual
DefaultDependencies
  { /* implementation building Subsystem5 */ };

class DefaultServerConfiguration :
    public virtual ServerConfiguration,
    virtual DefaultSubsystem1,
    ...
    virtual DefaultSubsystem5
{ /* combines the implementations */ };

The inflexibility exists because the DefaultSubsystemX classes have to
derive from any DefaultSubsystemY interfaces that contain functions they
implement. Some flexibility could be attained using typelists, but not
without making the implementation classes templates. (Which pulls all
the implementation knowledge into a header - even worse than the current
all the implementation knowledge into a translation unit.)

Another issue with this approach is that there is nothing keeping the
dependencies in DefaultSubsystem1Dependencies correct (as long as a
needed function is declared in some interface everything compiles).

So, with this approach, there seems to be little value in the
DefaultSubsystemXDependencies interfaces - everything could be placed
directly into DefaultDependencies. Hence "Approach #2"...

~~~~

Approach #2

class ServerConfiguration { /* interface supplying dependencies of
DisplayServer */ };

class DefaultDependencies : public  virtual ServerConfiguration
{ /* all the dependencies */ };

class DefaultSubsystem1 : public virtual ServerConfiguration, virtual
DefaultDependencies
  { /* implementation building Subsystem1 */ };
...
class DefaultSubsystem5 : public virtual ServerConfiguration, virtual
DefaultDependencies
  { /* implementation building Subsystem5 */ };

class DefaultServerConfiguration :
    public virtual ServerConfiguration,
    virtual DefaultSubsystem1,
    ...
    virtual DefaultSubsystem5
{ /* combines the implementations */ };

Now this allows someone to come along with a:

class NonDefaultSubsystem3 : public virtual ServerConfiguration, virtual
DefaultDependencies
  { /* implementation building Subsystem3 */ };

class NonDefaultServerConfiguration :
    public virtual ServerConfiguration,
    virtual DefaultSubsystem1,
    ...
    NonDefaultSubsystem3,
    ...
    virtual DefaultSubsystem5
{ /* combines the implementations */ };

But this doesn't gain much over simply splitting up the implementation
file. (Approach #3)

~~~~

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

---

From: Alan Griffiths <[email-masked]>
Date: Tue, 16 Apr 2013 17:25:19 +0100

> mess. (At least racarr and I have.)

A related issue is the use of graphics::Platform.

This is often passed like a dependency to constructors using a
shared_ptr, but is really only a factory object. (Which means that the
shared pointer can be released and the Platform destroyed if the
constructor doesn't keep a copy.)

---

From: Robert Carr <[email-masked]>
Date: Tue, 16 Apr 2013 14:45:53 -0700

> agree that concealing dependencies line this is bad, the shared_ptr is

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

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

---

From: Daniel van Vugt <[email-masked]>
Date: Wed, 17 Apr 2013 09:37:53 +0800

I could see the configuration stuff becoming unwieldy, though have not 
had to use it yet so am not aware of the intent or requirements.

My only request is to try and keep any new solution simple. Reconsider 
if and where "configuration" needs to be a class at all.

Also consider using the abbreviation "Config". The reasoning being that 
humans see words as symbols and frequently recognize them by shape 
alone. We do not look at individual letters, so try not to have 
SomeLongClassNameConfiguration. It hinders readability.

---

From: Alan Griffiths <[email-masked]>
Date: Wed, 17 Apr 2013 13:08:48 +0100

> live in the same name space and there is no attempt at isolation, only

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