Mir

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

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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

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.

Aside from anything else, the degree of coupling introduced here looks very high. It's always better to minimize coupling. You can't call it a simplification if coupling is increased and the diff is "+205/-148".

review: Disapprove

« Back to merge proposal