Mir

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

parenting-server-api
Merge into development-branch

Proposed by Daniel van Vugt on 2014-11-06

Status:

Rejected

Rejected by:

Cemil Azizoglu on 2014-11-20

Proposed branch:

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

Merge into:

lp://staging/mir

Diff against target:

498 lines (+191/-10)

21 files modified

client-ABI-sha1sums (+1/-1)
common-ABI-sha1sums (+1/-1)
include/common/mir_toolkit/common.h (+6/-0)
include/server/mir/frontend/session.h (+2/-0)
include/server/mir/frontend/surface.h (+3/-0)
platform-ABI-sha1sums (+1/-1)
server-ABI-sha1sums (+3/-3)
src/server/frontend/session_mediator.cpp (+1/-2)
src/server/scene/application_session.cpp (+17/-0)
src/server/scene/application_session.h (+1/-0)
src/server/scene/basic_surface.cpp (+37/-0)
src/server/scene/basic_surface.h (+5/-0)
tests/include/mir_test_doubles/mock_frontend_surface.h (+3/-0)
tests/include/mir_test_doubles/mock_surface.h (+3/-0)
tests/include/mir_test_doubles/stub_scene_session.h (+4/-0)
tests/include/mir_test_doubles/stub_scene_surface.h (+2/-0)
tests/include/mir_test_doubles/stub_session.h (+4/-0)
tests/unit-tests/client/test_client_mir_surface.cpp (+2/-1)
tests/unit-tests/scene/test_application_session.cpp (+46/-0)
tests/unit-tests/scene/test_basic_surface.cpp (+14/-1)
tests/unit-tests/scene/test_surface_impl.cpp (+35/-0)

To merge this branch:

bzr merge lp://staging/~vanvugt/mir/parenting-server-api

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Cemil Azizoglu (community)			Needs Resubmitting on 2014-11-18
Robert Carr (community)			Disapprove on 2014-11-17
PS Jenkins bot (community)	continuous-integration	2014-11-06	Approve on 2014-11-17
Alberto Aguirre (community)		2014-11-06	Abstain on 2014-11-13
Alan Griffiths		2014-11-06	Needs Resubmitting on 2014-11-11
Review via email: mp+240828@code.staging.launchpad.net

This proposal supersedes a proposal from 2014-11-03.

Commit message

Introduce basic surface parent information in the server API.

Description of the change

Server ABI broken, but we have already bumped it for this series.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-04: Posted in a previous version of this proposal

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-04: Posted in a previous version of this proposal

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.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-05: Posted in a previous version of this proposal

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.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-05: Posted in a previous version of this proposal

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-05: Posted in a previous version of this proposal

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

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-11-05: Posted in a previous version of this proposal

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?

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-11-05: Posted in a previous version of this proposal

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.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-06: Posted in a previous version of this proposal

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-06:

PASSED: Continuous integration, rev:2034
http://jenkins.qa.ubuntu.com/job/mir-ci/2023/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/12
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/12/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-11-06:

Looks good.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-07:

FAILED: Continuous integration, rev:2036
http://jenkins.qa.ubuntu.com/job/mir-ci/2034/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/30
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/9/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/23
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/23/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/9
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/9/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3303/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15538

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-07:

Failure is lp:1390388

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-09:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-10:

I'm not yet sure that this solves the right problem.

review: Abstain

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-10:

> I'm not yet sure that this solves the right problem.

Actually, I've decided that it is clear that deciding this needs resolution of the question about who "owns" the decision about strategy.

On the basis that there is a reasonable school of thought that it isn't owned by the client or frontend code then we should resolve that discussion before landing changes to frontend to support it.

*Needs discussion* at tomorrow's meeting.

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-11:

I completely agree, and have worked to remove as much from the frontend as possible, over multiple iterations.

The key problem is parent "ID" only exists in the frontend. And correlating that with a parent ptr in the frontend appears to be much cleaner than the alternative of duplicating ID information into the scene; BasicSurface (where it's meaningless, hence absent right now). Not only that but pushing all ID responsibility into the lower layer would also require additional lookup functionality get pushed down there. Very ugly. This is cleaner.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-11:

> I completely agree, and have worked to remove as much from the frontend as
> possible, over multiple iterations.
>
> The key problem is parent "ID" only exists in the frontend. And correlating
> that with a parent ptr in the frontend appears to be much cleaner than the
> alternative of duplicating ID information into the scene; BasicSurface (where
> it's meaningless, hence absent right now). Not only that but pushing all ID
> responsibility into the lower layer would also require additional lookup
> functionality get pushed down there. Very ugly. This is cleaner.

I agree that any mapping of ID to ptr belongs in frontend.

However, following Thomas's meeting it is clear that some rework is needed to fit the design philosophy as there will not be a (re)parenting request from the client for an existing surface. C.f.

https://lists.ubuntu.com/archives/mir-devel/2014-November/000961.html

review: Needs Resubmitting

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-12:

I disagree :)

The design you point to is artificially limited, and dramatically overcomplicated. It fails to support shells other than Unity without at least requiring ongoing maintenance and unbounded further expansion in complexity. It's not sensible at all, so I stand by this design.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-12:

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-12:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-11-13:

I guess abstain as another approach will be taken.

review: Abstain

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-14:

PASSED: Continuous integration, rev:2042
http://jenkins.qa.ubuntu.com/job/mir-ci/2103/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/123
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/123
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/103
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/92
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/92/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/103
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/103/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3390
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15823

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

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-11-14:

This one is also at a dead-end.

review: Disapprove

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-17:

It's the only working solution right now for an important feature.

I'll keep the review up till someone proposes a better solution (or lands a worse one :)

lp://staging/~vanvugt/mir/parenting-server-api updated on 2014-11-17

2043. By Daniel van Vugt on 2014-11-17: Merge latest trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-17:

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

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-11-17:

Outcome is clear...registering disapprove to encourage Daniel to save time and not continue updating :)

review: Disapprove

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-18:

I've learned to never expect or assume any branch will land at all :)

Actually I'm surprised this branch is unpopular. Not mentioning anything about client API design here it would appear to support the alternative client APIs being proposed rather neatly. And most importantly, without any protocol modifications.

I've been thinking if this branch lands then I can at least unblock and carry on with the server side logic for relative positioning. Again without mentioning client APIs.

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-11-18:

The objection is that parent should not be an attribute, as was decided in Thomas' meeting. It should be set once and not be changed, which goes against the approach this MP is taking. Hence the Resubmit. This is my understanding of it (Others please jump in, if I'm missing something).

review: Needs Resubmitting

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-19:

I'm also wondering about type morphing, which design wants us to support... What if a surface morphs from type A to type B and only type B requires parenting?

In that case you'd have to reject a simple set_type() request and add more client functions that do an atomic set_type_B(surface, newparent). So there's the scalability issue still, and you do gain atomicity, but I'm not sure that atomicity is more important than the scalability advantage.

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-11-20:

> I'm also wondering about type morphing, which design wants us to support...
> What if a surface morphs from type A to type B and only type B requires
> parenting?
>
> In that case you'd have to reject a simple set_type() request and add more
> client functions that do an atomic set_type_B(surface, newparent). So there's
> the scalability issue still, and you do gain atomicity, but I'm not sure that
> atomicity is more important than the scalability advantage.

Yes the API that has been agreed upon with tvoss would require additional functions to handle this case. I don't think this is the correct venue if you have concerns about this approach. Talking to Thomas directly might be more effective.

Unmerged revisions

2043. By Daniel van Vugt on 2014-11-17: Merge latest trunk
2042. By Daniel van Vugt on 2014-11-14: Merge latest trunk
2041. By Daniel van Vugt on 2014-11-13: Merge latest trunk
2040. By Daniel van Vugt on 2014-11-12: Tidy up ApplicationSession unit-tests' mocks.
2039. By Daniel van Vugt on 2014-11-12: Merge latest trunk and fix conflicts.
2038. By Daniel van Vugt on 2014-11-11: Merge latest trunk
2037. By Daniel van Vugt on 2014-11-10: Merge latest trunk
2036. By Daniel van Vugt on 2014-11-07: Add a dummy expectation to silence gmock (part of LP: #1390312)
2035. By Daniel van Vugt on 2014-11-07: Merge latest trunk
2034. By Daniel van Vugt on 2014-11-06: Also verify that configure_surface() gives the correct shared_ptr to
set_parent().