Merge lp://staging/~alan-griffiths/mir/surface-states-simplification into lp://staging/~mir-team/mir/trunk

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

Also, here's an example use-case for changing the event sink of a surface:

Windows (like the one Flash video uses in a web browser) are implemented using reparenting. If Mir was to support such a thing in future then it would be handy to be able to re-assign a surface to a different session. Hence you'd also need to be able to change a surface's event target (its session).

> Secondly, I disagree with the approach. There are no strong "dependencies" here.

As mentioned in an older comment [1], they are effectively dependencies because of how Mir uses the class. That is, in all cases in production code we need to set both the id and the sink for our features to work properly.

> If Mir was to support such a thing in future then it would be handy to be able to re-assign
> a surface to a different session

The key words are "if ... in the future ...". If we need this in the future, we are free to change the code to suit our needs. Right now the values are meant to be set only once at creation time, and accepting them in the constructor enforces this restriction (and, as mentioned above, also enforces that we always get a fully working object).

[1] https://code.launchpad.net/~vanvugt/mir/surface-states/+merge/158289/comments/348068

There is a difference between a local dependency and a system dependency. On the local scale, the classes do not depend on EventSink to work. So on a local scale, it should not affect the constructor.

A surface can emit events. A surface can have an ID. However those are not required attributes for the surface to be created and work as a surface.

No amount of "modern C++ tradition" supersedes the basic programming ideal of low coupling.

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

I have stated my opinions strongly. I disagree with this proposal as it significantly decreases readability and maintainability. Those are the traits of high coupling, and things I would like to avoid.

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

I think we're using the term 'coupling' too broadly to make much progress arguing :) I'll propose re-framing the argument, and then give my 2 cents

I think we can agree on this though:
1) Its better if a function call doesn't have any hidden prerequisites, like "you must call function A before you call function B". This is sort of 'coupling of calling requirements".
2) A class with a lot of constructor parameters has a lot of dependencies and might be trying to do too much. This is sort of 'coupling of object dependencies'

my 2cents...
so we have to find a way to balance.

I think that, with regards to notify_change/set_id, set_id() is a dependency that has to be called before notify_change() is called, so the id may as well be in the constructor. Another possibility (if we really don't want any more construction parameters for Surface) is to have another object with expanded functionality that takes

We've tried to mitigate the problem of #2 by having a lot of mocks and stubs and nulls that are easy to plug in and make the object easy to manipulate in test. Since its easier to read a constructor than it is to trace what the call order is, I'd rather plug in mocks/stubs to the constructor than have to trace a call order to figure out if we've fulfilled our usage requirements.

You are free to propose and land this directly to lp:mir.

I just don't want code like this landing in a branch with my name on it.

FAILED: Continuous integration, rev:669
http://jenkins.qa.ubuntu.com/job/mir-ci/480/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/516/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/399
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/485
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/485/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/234

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/480/rebuild

PASSED: Continuous integration, rev:670
http://jenkins.qa.ubuntu.com/job/mir-ci/482/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/518
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/401
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/487
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/487/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/482/rebuild

might be the first time there's been 2 rival reviews going on ;-)

again, seems the biggest point of contention is about 2 phase initialization and what has more or less coupling. maybe a new day will let me rephrase a bit better. :) Seems to me that (for example) in msh::Surface msh::Surface's notify_change function is coupled to having the ID, so whether you put it in the constructor or require a set_ function to be called, both depend on having an id to use notify_change() in a way. I'd rather come down on the side of putting it all dependencies in the constructor, because it allows us to see pretty quickly what a class needs, and gives us an easy-to-read signal about if a class is doing too much and needs to refactor. So, msh::Shell, might just be on the point of needing a refactor. (if its constructor is getting "too bloated" of course)

I know we had a debate similar to this one at an earlier point in the project, I hope we have another one next week :)

This doesn't immediately *look* like a simplification to me.

My understanding is that this is a simplification for clients of this code - ie: consumers of this class no longer have to check if the EventSink or SurfaceID is set.

My question is then - do consumers of this class need to check if the EventSink or SurfaceID is set currently? It looks like consumers of this class generally won't care - if they configure() a surface which doesn't have an EventSink set then nothing will get the event but everything else will work as expected?

From a high-level perspective I can see two problems here:

1. It's 1000 lines bigger than my proposal. Even if you agree that's not a simplification, but for other purposes, then I think it's still a bit much to review all in one go.

2. This does not solve the problem of code I disagree with ending up in a commit with my name on it. Because once merged it would just show one commit with both our names as authors.

To keep code reviews smaller and simpler, please propose your changes separately (after) surface-states to lp:mir. I won't agree with them, but I won't block them there either.

Just further to Kevin's comment:

"We've tried to mitigate the problem of #2 by having a lot of mocks and stubs and nulls that are easy to plug in and make the object easy to manipulate in test. Since its easier to read a constructor than it is to trace what the call order is, I'd rather plug in mocks/stubs to the constructor than have to trace a call order to figure out if we've fulfilled our usage requirements."

This is false: "lot of mocks and stubs and nulls that are easy to plug in and make the object easy to manipulate in test". You define "easy" by how much effort it requires for someone like you guys (who have been at it for over a year) to modify the code. I define "easy" by how much effort a newcomer requires (with no prior knowledge of the code).

This is false: "Since its easier to read a constructor than it is to trace what the call order is". Constructors do not explicitly state what they're doing, unlike "set_id" or "set_event_sink". And if you did make a mistake somewhere and dereference a null shared_ptr, then the trace would be a call stack. Automatically generated and easier to read.

If you resubmit and prerequisite the surface-states branch, I will close my eyes and happily Abstain.

> My question is then - do consumers of this class need to check if the
> EventSink or SurfaceID is set currently? It looks like consumers of this class
> generally won't care - if they configure() a surface which doesn't have an
> EventSink set then nothing will get the event but everything else will work as
> expected?

On this point is that consumers shouldn't even know about them - they shouldn't see these functions in the interface they use (i.e. they don't belong in frontend::Surface). Unnecessarily exposing these functions *is* increasing coupling.

PASSED: Continuous integration, rev:671
http://jenkins.qa.ubuntu.com/job/mir-ci/491/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/533
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/416
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/496
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/496/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/242

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/491/rebuild