Code review comment for lp:~alan-griffiths/mir/surface-states-simplification

> Firstly, needs fixing:
> Text conflict in tests/unit-tests/shell/test_application_session.cpp
> 1 conflicts encountered.

Resolved.

> Secondly, I disagree with the approach. There are no strong "dependencies"
> here. The information in question is all optional to the class receiving it,
> so should not be passed in constructors. The classes can function without
> event sinks or surface IDs. They will work as they already do. Only event
> emission would not happen. So it's optional relative to those classes, and
> therefore not something to enforce in the constructor.

Arguing that a subset of the functionality works without these attributes is an argument that the subset belongs in a class providing just that subset. While we could factor that subset out, I don't think we need to that now.

> Aside from anything else, the degree of coupling introduced here looks very
> high. It's always better to minimize coupling.

I agree that coupling should be reduced - I disagree that this MP increases coupling. For example, the interface used by frontend should not need Session::set_event_sink().

> You can't call it a
> simplification if coupling is increased and the diff is "+205/-148".

You can't measure coupling (or other attributes of the resulting code) by diff size.