Mir

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

Proposed by Daniel van Vugt
Status: Rejected
Rejected by: Cemil Azizoglu
Proposed branch: lp:~vanvugt/mir/parenting-server-api
Merge into: lp: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:~vanvugt/mir/parenting-server-api
Reviewer Review Type Date Requested Status
Cemil Azizoglu (community) Needs Resubmitting
Robert Carr (community) Disapprove
PS Jenkins bot (community) continuous-integration Approve
Alberto Aguirre (community) Abstain
Alan Griffiths Needs Resubmitting
Review via email: mp+240828@code.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.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : 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 : 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 : 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 : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
Chris Halse Rogers (raof) wrote : 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 : 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 : 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 :
review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Looks good.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Failure is lp:1390388

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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

review: Abstain
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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

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 :

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

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 :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

I guess abstain as another approach will be taken.

review: Abstain
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

This one is also at a dead-end.

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

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:~vanvugt/mir/parenting-server-api updated
2043. By Daniel van Vugt

Merge latest trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote :

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 :

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 :

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 :

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 :

> 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

Merge latest trunk

2042. By Daniel van Vugt

Merge latest trunk

2041. By Daniel van Vugt

Merge latest trunk

2040. By Daniel van Vugt

Tidy up ApplicationSession unit-tests' mocks.

2039. By Daniel van Vugt

Merge latest trunk and fix conflicts.

2038. By Daniel van Vugt

Merge latest trunk

2037. By Daniel van Vugt

Merge latest trunk

2036. By Daniel van Vugt

Add a dummy expectation to silence gmock (part of LP: #1390312)

2035. By Daniel van Vugt

Merge latest trunk

2034. By Daniel van Vugt

Also verify that configure_surface() gives the correct shared_ptr to
set_parent().

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'client-ABI-sha1sums'
--- client-ABI-sha1sums 2014-11-12 05:01:53 +0000
+++ client-ABI-sha1sums 2014-11-17 01:55:23 +0000
@@ -8,7 +8,7 @@
84f85e3d00314a7df869e56c3701a45310909fae2 include/client/mir_toolkit/mir_surface.h84f85e3d00314a7df869e56c3701a45310909fae2 include/client/mir_toolkit/mir_surface.h
9b141c4d79802ad626d969249c0004744e5c2a525 include/client/mir_toolkit/mir_wait.h9b141c4d79802ad626d969249c0004744e5c2a525 include/client/mir_toolkit/mir_wait.h
109907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h109907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h
112100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h1177a5955e53e1901766372b4fb29e34872ef62667 include/common/mir_toolkit/common.h
12fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h12fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h
13bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h13bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h
144975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h144975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h
1515
=== modified file 'common-ABI-sha1sums'
--- common-ABI-sha1sums 2014-11-11 07:34:31 +0000
+++ common-ABI-sha1sums 2014-11-17 01:55:23 +0000
@@ -11,7 +11,7 @@
11be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h11be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h
129ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h129ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h
139907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h139907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h
142100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h1477a5955e53e1901766372b4fb29e34872ef62667 include/common/mir_toolkit/common.h
15fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h15fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h
16bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h16bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h
174975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h174975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h
1818
=== modified file 'include/common/mir_toolkit/common.h'
--- include/common/mir_toolkit/common.h 2014-11-11 07:34:31 +0000
+++ include/common/mir_toolkit/common.h 2014-11-17 01:55:23 +0000
@@ -42,6 +42,7 @@
42 mir_surface_attrib_focus,42 mir_surface_attrib_focus,
43 mir_surface_attrib_dpi,43 mir_surface_attrib_dpi,
44 mir_surface_attrib_visibility,44 mir_surface_attrib_visibility,
45 mir_surface_attrib_parent,
45 /* Must be last */46 /* Must be last */
46 mir_surface_attribs47 mir_surface_attribs
47} MirSurfaceAttrib;48} MirSurfaceAttrib;
@@ -90,6 +91,11 @@
90 mir_surface_visibility_exposed91 mir_surface_visibility_exposed
91} MirSurfaceVisibility;92} MirSurfaceVisibility;
9293
94enum MirSurfaceParentNone
95{
96 mir_surface_parent_none = -1
97};
98
93typedef enum MirLifecycleState99typedef enum MirLifecycleState
94{100{
95 mir_lifecycle_state_will_suspend,101 mir_lifecycle_state_will_suspend,
96102
=== modified file 'include/server/mir/frontend/session.h'
--- include/server/mir/frontend/session.h 2014-10-01 06:25:56 +0000
+++ include/server/mir/frontend/session.h 2014-11-17 01:55:23 +0000
@@ -49,6 +49,8 @@
49 virtual SurfaceId create_surface(scene::SurfaceCreationParameters const& params) = 0;49 virtual SurfaceId create_surface(scene::SurfaceCreationParameters const& params) = 0;
50 virtual void destroy_surface(SurfaceId surface) = 0;50 virtual void destroy_surface(SurfaceId surface) = 0;
51 virtual std::shared_ptr<Surface> get_surface(SurfaceId surface) const = 0;51 virtual std::shared_ptr<Surface> get_surface(SurfaceId surface) const = 0;
52 virtual int configure_surface(SurfaceId id, MirSurfaceAttrib at,
53 int value) = 0;
5254
53 virtual std::string name() const = 0;55 virtual std::string name() const = 0;
5456
5557
=== modified file 'include/server/mir/frontend/surface.h'
--- include/server/mir/frontend/surface.h 2014-10-01 06:25:56 +0000
+++ include/server/mir/frontend/surface.h 2014-11-17 01:55:23 +0000
@@ -52,6 +52,9 @@
52 virtual bool supports_input() const = 0;52 virtual bool supports_input() const = 0;
53 virtual int client_input_fd() const = 0;53 virtual int client_input_fd() const = 0;
5454
55 virtual std::shared_ptr<Surface> parent() const = 0;
56 virtual void set_parent(std::weak_ptr<Surface> const&) = 0;
57
55 virtual int configure(MirSurfaceAttrib attrib, int value) = 0;58 virtual int configure(MirSurfaceAttrib attrib, int value) = 0;
56 virtual int query(MirSurfaceAttrib attrib) = 0;59 virtual int query(MirSurfaceAttrib attrib) = 0;
5760
5861
=== modified file 'platform-ABI-sha1sums'
--- platform-ABI-sha1sums 2014-11-14 17:46:49 +0000
+++ platform-ABI-sha1sums 2014-11-17 01:55:23 +0000
@@ -11,7 +11,7 @@
11be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h11be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h
129ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h129ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h
139907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h139907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h
142100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h1477a5955e53e1901766372b4fb29e34872ef62667 include/common/mir_toolkit/common.h
15fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h15fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h
16bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h16bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h
174975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h174975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h
1818
=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums 2014-11-14 17:46:49 +0000
+++ server-ABI-sha1sums 2014-11-17 01:55:23 +0000
@@ -11,7 +11,7 @@
11be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h11be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h
129ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h129ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h
139907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h139907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h
142100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h1477a5955e53e1901766372b4fb29e34872ef62667 include/common/mir_toolkit/common.h
15fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h15fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h
16bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h16bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h
174975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h174975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h
@@ -59,10 +59,10 @@
59fae0008af826abbc4b4324d49e9c03d43b08765a include/server/mir/frontend/screencast.h59fae0008af826abbc4b4324d49e9c03d43b08765a include/server/mir/frontend/screencast.h
60b729a7710c37d9f1fba56a6f8e8eae1c2559f57a include/server/mir/frontend/session_authorizer.h60b729a7710c37d9f1fba56a6f8e8eae1c2559f57a include/server/mir/frontend/session_authorizer.h
6134ce482df448fd2fc5f0c4ae5ac8b7fecbd228c9 include/server/mir/frontend/session_credentials.h6134ce482df448fd2fc5f0c4ae5ac8b7fecbd228c9 include/server/mir/frontend/session_credentials.h
62e5ea465ed7e05f0e1d6a837d0d6b3a04c2d7fc19 include/server/mir/frontend/session.h621ae5b9c89ead7529b3c20301caf5869c4a52f75f include/server/mir/frontend/session.h
63d754e5bb057f3018d7228170cf9a0c66d75d8cc2 include/server/mir/frontend/session_mediator_report.h63d754e5bb057f3018d7228170cf9a0c66d75d8cc2 include/server/mir/frontend/session_mediator_report.h
6468468aa2298c4e2cdc1bbb7cb5f250a914ae16c9 include/server/mir/frontend/shell.h6468468aa2298c4e2cdc1bbb7cb5f250a914ae16c9 include/server/mir/frontend/shell.h
65f67b9788943a4b9121b3891dfbe979d57922ea78 include/server/mir/frontend/surface.h65d33a31128bcb6908ec769bfdb9897781748d2472 include/server/mir/frontend/surface.h
66618b43a84cce0ad671ed68fe2ba796fbc7b79e31 include/server/mir/frontend/surface_id.h66618b43a84cce0ad671ed68fe2ba796fbc7b79e31 include/server/mir/frontend/surface_id.h
67f95c2bddf13d15993ef5d6a0ad7b9106ae550b87 include/server/mir/input/composite_event_filter.h67f95c2bddf13d15993ef5d6a0ad7b9106ae550b87 include/server/mir/input/composite_event_filter.h
6867719acb03b35d383dfefd65e8dfb872c42bcc11 include/server/mir/input/cursor_images.h6867719acb03b35d383dfefd65e8dfb872c42bcc11 include/server/mir/input/cursor_images.h
6969
=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp 2014-11-13 16:41:26 +0000
+++ src/server/frontend/session_mediator.cpp 2014-11-17 01:55:23 +0000
@@ -359,8 +359,7 @@
359359
360 auto const id = mf::SurfaceId(request->surfaceid().value());360 auto const id = mf::SurfaceId(request->surfaceid().value());
361 int value = request->ivalue();361 int value = request->ivalue();
362 auto const surface = session->get_surface(id);362 int newvalue = session->configure_surface(id, attrib, value);
363 int newvalue = surface->configure(attrib, value);
364363
365 response->set_ivalue(newvalue);364 response->set_ivalue(newvalue);
366 }365 }
367366
=== modified file 'src/server/scene/application_session.cpp'
--- src/server/scene/application_session.cpp 2014-10-01 06:25:56 +0000
+++ src/server/scene/application_session.cpp 2014-11-17 01:55:23 +0000
@@ -105,6 +105,23 @@
105 return checked_find(id)->second;105 return checked_find(id)->second;
106}106}
107107
108int ms::ApplicationSession::configure_surface(mf::SurfaceId id,
109 MirSurfaceAttrib attrib,
110 int value)
111{
112 auto surface = get_surface(id);
113
114 if (attrib == mir_surface_attrib_parent)
115 {
116 std::shared_ptr<mf::Surface> parent;
117 if (value != mir_surface_parent_none)
118 parent = get_surface(mf::SurfaceId(value));
119 surface->set_parent(parent);
120 }
121
122 return surface->configure(attrib, value);
123}
124
108void ms::ApplicationSession::take_snapshot(SnapshotCallback const& snapshot_taken)125void ms::ApplicationSession::take_snapshot(SnapshotCallback const& snapshot_taken)
109{126{
110 if (auto surface = default_surface())127 if (auto surface = default_surface())
111128
=== modified file 'src/server/scene/application_session.h'
--- src/server/scene/application_session.h 2014-11-03 06:51:26 +0000
+++ src/server/scene/application_session.h 2014-11-17 01:55:23 +0000
@@ -55,6 +55,7 @@
55 frontend::SurfaceId create_surface(SurfaceCreationParameters const& params) override;55 frontend::SurfaceId create_surface(SurfaceCreationParameters const& params) override;
56 void destroy_surface(frontend::SurfaceId surface) override;56 void destroy_surface(frontend::SurfaceId surface) override;
57 std::shared_ptr<frontend::Surface> get_surface(frontend::SurfaceId surface) const override;57 std::shared_ptr<frontend::Surface> get_surface(frontend::SurfaceId surface) const override;
58 int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int) override;
5859
59 void take_snapshot(SnapshotCallback const& snapshot_taken) override;60 void take_snapshot(SnapshotCallback const& snapshot_taken) override;
60 std::shared_ptr<Surface> default_surface() const override;61 std::shared_ptr<Surface> default_surface() const override;
6162
=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp 2014-10-21 16:21:14 +0000
+++ src/server/scene/basic_surface.cpp 2014-11-17 01:55:23 +0000
@@ -35,6 +35,7 @@
35#include <algorithm>35#include <algorithm>
3636
37namespace mc = mir::compositor;37namespace mc = mir::compositor;
38namespace mf = mir::frontend;
38namespace ms = mir::scene;39namespace ms = mir::scene;
39namespace msh = mir::shell;40namespace msh = mir::shell;
40namespace mg = mir::graphics;41namespace mg = mir::graphics;
@@ -141,6 +142,7 @@
141 attrib_values[mir_surface_attrib_focus] = mir_surface_unfocused;142 attrib_values[mir_surface_attrib_focus] = mir_surface_unfocused;
142 attrib_values[mir_surface_attrib_dpi] = 0;143 attrib_values[mir_surface_attrib_dpi] = 0;
143 attrib_values[mir_surface_attrib_visibility] = mir_surface_visibility_exposed;144 attrib_values[mir_surface_attrib_visibility] = mir_surface_visibility_exposed;
145 attrib_values[mir_surface_attrib_parent] = mir_surface_parent_none;
144}146}
145147
146void ms::BasicSurface::force_requests_to_complete()148void ms::BasicSurface::force_requests_to_complete()
@@ -166,6 +168,38 @@
166 return surface_name;168 return surface_name;
167}169}
168170
171void ms::BasicSurface::set_parent(std::weak_ptr<mf::Surface> const& p)
172{
173 std::unique_lock<std::mutex> lk(guard);
174 weak_parent = p;
175}
176
177int ms::BasicSurface::set_parent_id(int id)
178{
179 std::unique_lock<std::mutex> lk(guard);
180 if (attrib_values[mir_surface_attrib_parent] != id)
181 {
182 attrib_values[mir_surface_attrib_parent] = id;
183 lk.unlock();
184 observers.attrib_changed(mir_surface_attrib_parent, id);
185 }
186 return id;
187}
188
189std::shared_ptr<mf::Surface> ms::BasicSurface::parent() const
190{
191 std::unique_lock<std::mutex> lk(guard);
192 return weak_parent.lock();
193}
194
195/* Coming soon: Parent-relative placement. Something like...
196void ms::BasicSurface::place(geometry::Displacement const& disp)
197{
198 if (auto p = parent())
199 move_to(p->screen_position().top_left() + disp);
200}
201*/
202
169void ms::BasicSurface::move_to(geometry::Point const& top_left)203void ms::BasicSurface::move_to(geometry::Point const& top_left)
170{204{
171 {205 {
@@ -500,6 +534,9 @@
500 case mir_surface_attrib_visibility:534 case mir_surface_attrib_visibility:
501 result = set_visibility(static_cast<MirSurfaceVisibility>(result));535 result = set_visibility(static_cast<MirSurfaceVisibility>(result));
502 break;536 break;
537 case mir_surface_attrib_parent:
538 result = set_parent_id(result);
539 break;
503 default:540 default:
504 BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "541 BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
505 "attribute."));542 "attribute."));
506543
=== modified file 'src/server/scene/basic_surface.h'
--- src/server/scene/basic_surface.h 2014-11-03 06:51:26 +0000
+++ src/server/scene/basic_surface.h 2014-11-17 01:55:23 +0000
@@ -147,6 +147,9 @@
147147
148 int dpi() const;148 int dpi() const;
149149
150 void set_parent(std::weak_ptr<frontend::Surface> const& parent) override;
151 std::shared_ptr<frontend::Surface> parent() const override;
152
150private:153private:
151 bool visible(std::unique_lock<std::mutex>&) const;154 bool visible(std::unique_lock<std::mutex>&) const;
152 MirSurfaceType set_type(MirSurfaceType t); // Use configure() to make public changes155 MirSurfaceType set_type(MirSurfaceType t); // Use configure() to make public changes
@@ -155,6 +158,7 @@
155 MirSurfaceVisibility set_visibility(MirSurfaceVisibility v);158 MirSurfaceVisibility set_visibility(MirSurfaceVisibility v);
156 int set_swap_interval(int);159 int set_swap_interval(int);
157 MirSurfaceFocusState set_focus_state(MirSurfaceFocusState f);160 MirSurfaceFocusState set_focus_state(MirSurfaceFocusState f);
161 int set_parent_id(int);
158162
159 SurfaceObservers observers;163 SurfaceObservers observers;
160 std::mutex mutable guard;164 std::mutex mutable guard;
@@ -173,6 +177,7 @@
173 std::shared_ptr<SurfaceConfigurator> const configurator;177 std::shared_ptr<SurfaceConfigurator> const configurator;
174 std::shared_ptr<graphics::CursorImage> cursor_image_;178 std::shared_ptr<graphics::CursorImage> cursor_image_;
175 std::shared_ptr<SceneReport> const report;179 std::shared_ptr<SceneReport> const report;
180 std::weak_ptr<frontend::Surface> weak_parent;
176181
177 void initialize_attributes();182 void initialize_attributes();
178 int attrib_values[mir_surface_attribs];183 int attrib_values[mir_surface_attribs];
179184
=== modified file 'tests/include/mir_test_doubles/mock_frontend_surface.h'
--- tests/include/mir_test_doubles/mock_frontend_surface.h 2014-10-01 06:25:56 +0000
+++ tests/include/mir_test_doubles/mock_frontend_surface.h 2014-11-17 01:55:23 +0000
@@ -62,6 +62,9 @@
6262
63 MOCK_CONST_METHOD0(supports_input, bool());63 MOCK_CONST_METHOD0(supports_input, bool());
64 MOCK_CONST_METHOD0(client_input_fd, int());64 MOCK_CONST_METHOD0(client_input_fd, int());
65
66 MOCK_CONST_METHOD0(parent, std::shared_ptr<frontend::Surface>());
67 MOCK_METHOD1(set_parent, void(std::weak_ptr<Surface> const&));
65 68
66 MOCK_METHOD1(set_cursor_image, void(std::shared_ptr<graphics::CursorImage> const&));69 MOCK_METHOD1(set_cursor_image, void(std::shared_ptr<graphics::CursorImage> const&));
6770
6871
=== modified file 'tests/include/mir_test_doubles/mock_surface.h'
--- tests/include/mir_test_doubles/mock_surface.h 2014-10-27 22:31:16 +0000
+++ tests/include/mir_test_doubles/mock_surface.h 2014-11-17 01:55:23 +0000
@@ -67,6 +67,9 @@
67 MOCK_METHOD1(take_input_focus, void(std::shared_ptr<shell::InputTargeter> const&));67 MOCK_METHOD1(take_input_focus, void(std::shared_ptr<shell::InputTargeter> const&));
68 MOCK_METHOD1(add_observer, void(std::shared_ptr<scene::SurfaceObserver> const&));68 MOCK_METHOD1(add_observer, void(std::shared_ptr<scene::SurfaceObserver> const&));
69 MOCK_METHOD1(remove_observer, void(std::weak_ptr<scene::SurfaceObserver> const&));69 MOCK_METHOD1(remove_observer, void(std::weak_ptr<scene::SurfaceObserver> const&));
70
71 MOCK_METHOD1(set_parent, void(std::weak_ptr<frontend::Surface> const&));
72 MOCK_METHOD0(parent, std::shared_ptr<frontend::Surface>());
70};73};
7174
72}75}
7376
=== modified file 'tests/include/mir_test_doubles/stub_scene_session.h'
--- tests/include/mir_test_doubles/stub_scene_session.h 2014-10-01 06:25:56 +0000
+++ tests/include/mir_test_doubles/stub_scene_session.h 2014-11-17 01:55:23 +0000
@@ -43,6 +43,10 @@
43 {43 {
44 return std::shared_ptr<frontend::Surface>();44 return std::shared_ptr<frontend::Surface>();
45 }45 }
46 int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int value) override
47 {
48 return value;
49 }
46 std::string name() const override50 std::string name() const override
47 {51 {
48 return std::string();52 return std::string();
4953
=== modified file 'tests/include/mir_test_doubles/stub_scene_surface.h'
--- tests/include/mir_test_doubles/stub_scene_surface.h 2014-10-01 06:25:56 +0000
+++ tests/include/mir_test_doubles/stub_scene_surface.h 2014-11-17 01:55:23 +0000
@@ -95,6 +95,8 @@
9595
96 bool supports_input() const override { return true;}96 bool supports_input() const override { return true;}
97 int client_input_fd() const override { return fd;}97 int client_input_fd() const override { return fd;}
98 std::shared_ptr<frontend::Surface> parent() const override { return {}; }
99 void set_parent(std::weak_ptr<frontend::Surface> const&) override { }
98 int configure(MirSurfaceAttrib, int) override { return 0; }100 int configure(MirSurfaceAttrib, int) override { return 0; }
99 int query(MirSurfaceAttrib) override { return 0; }101 int query(MirSurfaceAttrib) override { return 0; }
100 void with_most_recent_buffer_do(std::function<void(graphics::Buffer&)> const& ) override {}102 void with_most_recent_buffer_do(std::function<void(graphics::Buffer&)> const& ) override {}
101103
=== modified file 'tests/include/mir_test_doubles/stub_session.h'
--- tests/include/mir_test_doubles/stub_session.h 2014-10-01 06:25:56 +0000
+++ tests/include/mir_test_doubles/stub_session.h 2014-11-17 01:55:23 +0000
@@ -41,6 +41,10 @@
41 {41 {
42 return std::shared_ptr<frontend::Surface>();42 return std::shared_ptr<frontend::Surface>();
43 }43 }
44 int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int value) override
45 {
46 return value;
47 }
44 std::string name() const override48 std::string name() const override
45 {49 {
46 return std::string();50 return std::string();
4751
=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp 2014-10-28 13:59:51 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp 2014-11-17 01:55:23 +0000
@@ -186,7 +186,8 @@
186 { mir_surface_attrib_swapinterval, 1 },186 { mir_surface_attrib_swapinterval, 1 },
187 { mir_surface_attrib_focus, mir_surface_focused },187 { mir_surface_attrib_focus, mir_surface_focused },
188 { mir_surface_attrib_dpi, 19 },188 { mir_surface_attrib_dpi, 19 },
189 { mir_surface_attrib_visibility, mir_surface_visibility_exposed }189 { mir_surface_attrib_visibility, mir_surface_visibility_exposed },
190 { mir_surface_attrib_parent, 123 }
190};191};
191192
192struct MockBuffer : public mcl::ClientBuffer193struct MockBuffer : public mcl::ClientBuffer
193194
=== modified file 'tests/unit-tests/scene/test_application_session.cpp'
--- tests/unit-tests/scene/test_application_session.cpp 2014-11-11 04:02:18 +0000
+++ tests/unit-tests/scene/test_application_session.cpp 2014-11-17 01:55:23 +0000
@@ -109,6 +109,52 @@
109 session.destroy_surface(surf);109 session.destroy_surface(surf);
110}110}
111111
112TEST(ApplicationSession, configures_surface)
113{
114 using namespace ::testing;
115
116 auto mock_surface = make_mock_surface();
117 auto mock_parent_surface = make_mock_surface();
118
119 mtd::NullEventSink sender;
120 NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
121
122 EXPECT_CALL(surface_coordinator, add_surface(_, _))
123 .WillOnce(Return(mock_parent_surface))
124 .WillOnce(Return(mock_surface));
125
126 ms::ApplicationSession session(
127 mt::fake_shared(surface_coordinator),
128 __LINE__,
129 "Foo",
130 std::make_shared<mtd::NullSnapshotStrategy>(),
131 std::make_shared<ms::NullSessionListener>(),
132 mt::fake_shared(sender));
133
134 ms::SurfaceCreationParameters params;
135 auto parent_id = session.create_surface(params);
136 auto child_id = session.create_surface(params);
137
138 EXPECT_CALL(*mock_surface, configure(mir_surface_attrib_type,
139 mir_surface_type_dialog))
140 .WillOnce(Return(mir_surface_type_dialog));
141 std::weak_ptr<mf::Surface> parent_that_was_set;
142 EXPECT_CALL(*mock_surface, set_parent(_))
143 .WillOnce(SaveArg<0>(&parent_that_was_set));
144 EXPECT_CALL(*mock_surface, configure(mir_surface_attrib_parent,
145 parent_id.as_value()))
146 .WillOnce(Return(parent_id.as_value()));
147
148 session.configure_surface(child_id, mir_surface_attrib_type,
149 mir_surface_type_dialog);
150 session.configure_surface(child_id, mir_surface_attrib_parent,
151 parent_id.as_value());
152
153 EXPECT_EQ(mock_parent_surface, parent_that_was_set.lock());
154 session.destroy_surface(child_id);
155 session.destroy_surface(parent_id);
156}
157
112TEST(ApplicationSession, listener_notified_of_surface_destruction_on_session_destruction)158TEST(ApplicationSession, listener_notified_of_surface_destruction_on_session_destruction)
113{159{
114 using namespace ::testing;160 using namespace ::testing;
115161
=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
--- tests/unit-tests/scene/test_basic_surface.cpp 2014-10-01 06:25:56 +0000
+++ tests/unit-tests/scene/test_basic_surface.cpp 2014-11-17 01:55:23 +0000
@@ -528,6 +528,13 @@
528 -1528 -1
529};529};
530530
531AttributeTestParameters const surface_parent_test_parameters{
532 mir_surface_attrib_parent,
533 mir_surface_parent_none,
534 123,
535 mir_surface_parent_none
536};
537
531}538}
532539
533TEST_P(BasicSurfaceAttributeTest, default_value)540TEST_P(BasicSurfaceAttributeTest, default_value)
@@ -591,9 +598,12 @@
591 auto const& attribute = params.attribute;598 auto const& attribute = params.attribute;
592 auto const& invalid_value = params.an_invalid_value;599 auto const& invalid_value = params.an_invalid_value;
593 600
594 EXPECT_THROW({601 if (invalid_value != params.default_value)
602 {
603 EXPECT_THROW({
595 surface.configure(attribute, invalid_value);604 surface.configure(attribute, invalid_value);
596 }, std::logic_error);605 }, std::logic_error);
606 }
597}607}
598608
599INSTANTIATE_TEST_CASE_P(SurfaceTypeAttributeTest, BasicSurfaceAttributeTest,609INSTANTIATE_TEST_CASE_P(SurfaceTypeAttributeTest, BasicSurfaceAttributeTest,
@@ -614,6 +624,9 @@
614INSTANTIATE_TEST_CASE_P(SurfaceFocusAttributeTest, BasicSurfaceAttributeTest,624INSTANTIATE_TEST_CASE_P(SurfaceFocusAttributeTest, BasicSurfaceAttributeTest,
615 ::testing::Values(surface_focus_test_parameters));625 ::testing::Values(surface_focus_test_parameters));
616626
627INSTANTIATE_TEST_CASE_P(SurfaceParentAttributeTest, BasicSurfaceAttributeTest,
628 ::testing::Values(surface_parent_test_parameters));
629
617TEST_F(BasicSurfaceTest, configure_returns_value_set_by_configurator)630TEST_F(BasicSurfaceTest, configure_returns_value_set_by_configurator)
618{631{
619 using namespace testing;632 using namespace testing;
620633
=== modified file 'tests/unit-tests/scene/test_surface_impl.cpp'
--- tests/unit-tests/scene/test_surface_impl.cpp 2014-10-01 06:25:56 +0000
+++ tests/unit-tests/scene/test_surface_impl.cpp 2014-11-17 01:55:23 +0000
@@ -327,6 +327,41 @@
327 EXPECT_FLOAT_EQ(1.0f, surf.alpha());327 EXPECT_FLOAT_EQ(1.0f, surf.alpha());
328}328}
329329
330TEST_F(Surface, remembers_parent)
331{
332 auto parent = std::make_shared<ms::BasicSurface>(
333 std::string("stub"),
334 geom::Rectangle{{},{}},
335 false,
336 buffer_stream,
337 std::shared_ptr<mi::InputChannel>(),
338 stub_input_sender,
339 null_configurator,
340 std::shared_ptr<mg::CursorImage>(),
341 report);
342
343 ms::BasicSurface child(
344 std::string("stub"),
345 geom::Rectangle{{},{}},
346 false,
347 buffer_stream,
348 std::shared_ptr<mi::InputChannel>(),
349 stub_input_sender,
350 null_configurator,
351 std::shared_ptr<mg::CursorImage>(),
352 report);
353
354 EXPECT_FALSE(child.parent());
355
356 child.set_parent(parent);
357 EXPECT_EQ(parent, child.parent());
358 EXPECT_TRUE(!!child.parent());
359
360 child.set_parent({});
361 EXPECT_NE(parent, child.parent());
362 EXPECT_FALSE(child.parent());
363}
364
330TEST_F(Surface, sends_focus_notifications_when_focus_gained_and_lost)365TEST_F(Surface, sends_focus_notifications_when_focus_gained_and_lost)
331{366{
332 using namespace testing;367 using namespace testing;

Subscribers

People subscribed via source and target branches