Merge lp://staging/~vanvugt/mir/parenting-server-api into lp://staging/mir

PASSED: Continuous integration, rev:2026
http://jenkins.qa.ubuntu.com/job/mir-ci/1985/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2352
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2362
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/331
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/331/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1224
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1224/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3259
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15400

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

59 + virtual void set_parent(std::weak_ptr<Surface> const&, int id) = 0;

It can't be right that both a pointer and an int are passed here.

Presumably they are both ways of referencing the same parent and passing them separately makes the interface easy to abuse.

Alan: Actually yes that's the messy part, but is the lesser evil.

The problem is that surface ID is only meaningful in the upper layer (SessionMediator) where it's a key into the map. Down in the scene::Surface and scene::BasicSurface it's meaningless to mention IDs. So the obvious answer then is don't mention any surface ID in the lower layers (set_parent). You can indeed omit the "int id", but that creates two problems:
  1. query() doesn't work at all, because the surface implementation doesn't known its parent ID.
  2. Any future use of query(mir_surface_attrib_parent) in the upper layers would require bespoke hacks in the SessionMediator (return parent() and linear search for the surface pointer to get the ID).

So the options are either live with issues #1 & #2, or the current design to pass in the surface ID separately for storage in the BasicSurface's attribute list (which costs nothing as the memory is already reserved).

That said, making query() return no useful result looks like it could suffice for now. And it would simplify the interface as you point out. I might try it.

OK, I tried making set_parent a one-parameter function. It got really ugly. I then had to document that query(mir_surface_attrib_parent) is non-functional, also lost parent-changed event support, and the resulting hacks required to make tests work again were too ugly. Because "parent" then became effectively not a surface attribute any more.

set_parent with two parameters is certainly cleaner, and more functional.

Alternatively, we could clean it up by adding an id() to scene::Surface, but I think that's unnecessary, more complex, and would be equally easy to abuse. The current proposal seems best.

PASSED: Continuous integration, rev:2027
http://jenkins.qa.ubuntu.com/job/mir-ci/1994/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2365
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2375
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2300
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/340
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/340/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1237
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1237/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3272
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15445

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

Hm. As mentioned on the client-side of this proposal I suspect it would make more sense for parent to _not_ be a surface attribute, but a create-time option; that should neatly resolve the problem that the int parameter of
59 + virtual void set_parent(std::weak_ptr<Surface> const&, int id) = 0;
is solving?

59 + virtual void set_parent(std::weak_ptr<Surface> const&, int id) = 0;

The int id needs to go away.

"Alternatively, we could clean it up by adding an id() to scene::Surface, but I think that's unnecessary, more complex, and would be equally easy to abuse."

Why would that be equally easy to abuse? Ids for a surface would be created at construction time and never change.

" Because "parent" then became effectively not a surface attribute any more."

Perhaps that's a strong hint that it isn't then.

So I think the path to resolve this "needs fixing" is indeed to ad an id to Surface.

It's ugly because:
  * id() to a Surface object is meaningless. Only external users of the surface need the ID.
  * You either have to make the constructor take an ID parameter (more annoying coupling), or add a set_id() function which is the part that would be equally easy to abuse.

But if it unblocks us, I might give it a go.