Merge lp:~vanvugt/mir/parenting-server-api into lp:mir
- parenting-server-api
- Merge into development-branch
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 |
Related bugs: |
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:
|
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
59 + virtual void set_parent(
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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_
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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_
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
PASSED: Continuous integration, rev:2027
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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(
is solving?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alberto Aguirre (albaguirre) wrote : Posted in a previous version of this proposal | # |
59 + virtual void set_parent(
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2034
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alberto Aguirre (albaguirre) wrote : | # |
Looks good.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2036
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alan Griffiths (alan-griffiths) wrote : | # |
Failure is lp:1390388
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2036
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alan Griffiths (alan-griffiths) wrote : | # |
I'm not yet sure that this solves the right problem.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2039
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2040
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alberto Aguirre (albaguirre) wrote : | # |
I guess abstain as another approach will be taken.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2042
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Cemil Azizoglu (cemil-azizoglu) wrote : | # |
This one is also at a dead-end.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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 :)
- 2043. By Daniel van Vugt
-
Merge latest trunk
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2043
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Robert Carr (robertcarr) wrote : | # |
Outcome is clear...registering disapprove to encourage Daniel to save time and not continue updating :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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).
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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
1 | === modified file 'client-ABI-sha1sums' |
2 | --- client-ABI-sha1sums 2014-11-12 05:01:53 +0000 |
3 | +++ client-ABI-sha1sums 2014-11-17 01:55:23 +0000 |
4 | @@ -8,7 +8,7 @@ |
5 | 4f85e3d00314a7df869e56c3701a45310909fae2 include/client/mir_toolkit/mir_surface.h |
6 | b141c4d79802ad626d969249c0004744e5c2a525 include/client/mir_toolkit/mir_wait.h |
7 | 9907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h |
8 | -2100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h |
9 | +77a5955e53e1901766372b4fb29e34872ef62667 include/common/mir_toolkit/common.h |
10 | fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h |
11 | bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h |
12 | 4975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h |
13 | |
14 | === modified file 'common-ABI-sha1sums' |
15 | --- common-ABI-sha1sums 2014-11-11 07:34:31 +0000 |
16 | +++ common-ABI-sha1sums 2014-11-17 01:55:23 +0000 |
17 | @@ -11,7 +11,7 @@ |
18 | be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h |
19 | 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h |
20 | 9907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h |
21 | -2100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h |
22 | +77a5955e53e1901766372b4fb29e34872ef62667 include/common/mir_toolkit/common.h |
23 | fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h |
24 | bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h |
25 | 4975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h |
26 | |
27 | === modified file 'include/common/mir_toolkit/common.h' |
28 | --- include/common/mir_toolkit/common.h 2014-11-11 07:34:31 +0000 |
29 | +++ include/common/mir_toolkit/common.h 2014-11-17 01:55:23 +0000 |
30 | @@ -42,6 +42,7 @@ |
31 | mir_surface_attrib_focus, |
32 | mir_surface_attrib_dpi, |
33 | mir_surface_attrib_visibility, |
34 | + mir_surface_attrib_parent, |
35 | /* Must be last */ |
36 | mir_surface_attribs |
37 | } MirSurfaceAttrib; |
38 | @@ -90,6 +91,11 @@ |
39 | mir_surface_visibility_exposed |
40 | } MirSurfaceVisibility; |
41 | |
42 | +enum MirSurfaceParentNone |
43 | +{ |
44 | + mir_surface_parent_none = -1 |
45 | +}; |
46 | + |
47 | typedef enum MirLifecycleState |
48 | { |
49 | mir_lifecycle_state_will_suspend, |
50 | |
51 | === modified file 'include/server/mir/frontend/session.h' |
52 | --- include/server/mir/frontend/session.h 2014-10-01 06:25:56 +0000 |
53 | +++ include/server/mir/frontend/session.h 2014-11-17 01:55:23 +0000 |
54 | @@ -49,6 +49,8 @@ |
55 | virtual SurfaceId create_surface(scene::SurfaceCreationParameters const& params) = 0; |
56 | virtual void destroy_surface(SurfaceId surface) = 0; |
57 | virtual std::shared_ptr<Surface> get_surface(SurfaceId surface) const = 0; |
58 | + virtual int configure_surface(SurfaceId id, MirSurfaceAttrib at, |
59 | + int value) = 0; |
60 | |
61 | virtual std::string name() const = 0; |
62 | |
63 | |
64 | === modified file 'include/server/mir/frontend/surface.h' |
65 | --- include/server/mir/frontend/surface.h 2014-10-01 06:25:56 +0000 |
66 | +++ include/server/mir/frontend/surface.h 2014-11-17 01:55:23 +0000 |
67 | @@ -52,6 +52,9 @@ |
68 | virtual bool supports_input() const = 0; |
69 | virtual int client_input_fd() const = 0; |
70 | |
71 | + virtual std::shared_ptr<Surface> parent() const = 0; |
72 | + virtual void set_parent(std::weak_ptr<Surface> const&) = 0; |
73 | + |
74 | virtual int configure(MirSurfaceAttrib attrib, int value) = 0; |
75 | virtual int query(MirSurfaceAttrib attrib) = 0; |
76 | |
77 | |
78 | === modified file 'platform-ABI-sha1sums' |
79 | --- platform-ABI-sha1sums 2014-11-14 17:46:49 +0000 |
80 | +++ platform-ABI-sha1sums 2014-11-17 01:55:23 +0000 |
81 | @@ -11,7 +11,7 @@ |
82 | be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h |
83 | 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h |
84 | 9907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h |
85 | -2100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h |
86 | +77a5955e53e1901766372b4fb29e34872ef62667 include/common/mir_toolkit/common.h |
87 | fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h |
88 | bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h |
89 | 4975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h |
90 | |
91 | === modified file 'server-ABI-sha1sums' |
92 | --- server-ABI-sha1sums 2014-11-14 17:46:49 +0000 |
93 | +++ server-ABI-sha1sums 2014-11-17 01:55:23 +0000 |
94 | @@ -11,7 +11,7 @@ |
95 | be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h |
96 | 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h |
97 | 9907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h |
98 | -2100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h |
99 | +77a5955e53e1901766372b4fb29e34872ef62667 include/common/mir_toolkit/common.h |
100 | fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h |
101 | bdaceadd56e41d2cb708f7c4da97acf84dfc75b7 include/common/mir_toolkit/event.h |
102 | 4975998aa1056ed0d39dcc538127453e516ad8e9 include/common/mir_toolkit/mesa/native_display.h |
103 | @@ -59,10 +59,10 @@ |
104 | fae0008af826abbc4b4324d49e9c03d43b08765a include/server/mir/frontend/screencast.h |
105 | b729a7710c37d9f1fba56a6f8e8eae1c2559f57a include/server/mir/frontend/session_authorizer.h |
106 | 34ce482df448fd2fc5f0c4ae5ac8b7fecbd228c9 include/server/mir/frontend/session_credentials.h |
107 | -e5ea465ed7e05f0e1d6a837d0d6b3a04c2d7fc19 include/server/mir/frontend/session.h |
108 | +1ae5b9c89ead7529b3c20301caf5869c4a52f75f include/server/mir/frontend/session.h |
109 | d754e5bb057f3018d7228170cf9a0c66d75d8cc2 include/server/mir/frontend/session_mediator_report.h |
110 | 68468aa2298c4e2cdc1bbb7cb5f250a914ae16c9 include/server/mir/frontend/shell.h |
111 | -f67b9788943a4b9121b3891dfbe979d57922ea78 include/server/mir/frontend/surface.h |
112 | +d33a31128bcb6908ec769bfdb9897781748d2472 include/server/mir/frontend/surface.h |
113 | 618b43a84cce0ad671ed68fe2ba796fbc7b79e31 include/server/mir/frontend/surface_id.h |
114 | f95c2bddf13d15993ef5d6a0ad7b9106ae550b87 include/server/mir/input/composite_event_filter.h |
115 | 67719acb03b35d383dfefd65e8dfb872c42bcc11 include/server/mir/input/cursor_images.h |
116 | |
117 | === modified file 'src/server/frontend/session_mediator.cpp' |
118 | --- src/server/frontend/session_mediator.cpp 2014-11-13 16:41:26 +0000 |
119 | +++ src/server/frontend/session_mediator.cpp 2014-11-17 01:55:23 +0000 |
120 | @@ -359,8 +359,7 @@ |
121 | |
122 | auto const id = mf::SurfaceId(request->surfaceid().value()); |
123 | int value = request->ivalue(); |
124 | - auto const surface = session->get_surface(id); |
125 | - int newvalue = surface->configure(attrib, value); |
126 | + int newvalue = session->configure_surface(id, attrib, value); |
127 | |
128 | response->set_ivalue(newvalue); |
129 | } |
130 | |
131 | === modified file 'src/server/scene/application_session.cpp' |
132 | --- src/server/scene/application_session.cpp 2014-10-01 06:25:56 +0000 |
133 | +++ src/server/scene/application_session.cpp 2014-11-17 01:55:23 +0000 |
134 | @@ -105,6 +105,23 @@ |
135 | return checked_find(id)->second; |
136 | } |
137 | |
138 | +int ms::ApplicationSession::configure_surface(mf::SurfaceId id, |
139 | + MirSurfaceAttrib attrib, |
140 | + int value) |
141 | +{ |
142 | + auto surface = get_surface(id); |
143 | + |
144 | + if (attrib == mir_surface_attrib_parent) |
145 | + { |
146 | + std::shared_ptr<mf::Surface> parent; |
147 | + if (value != mir_surface_parent_none) |
148 | + parent = get_surface(mf::SurfaceId(value)); |
149 | + surface->set_parent(parent); |
150 | + } |
151 | + |
152 | + return surface->configure(attrib, value); |
153 | +} |
154 | + |
155 | void ms::ApplicationSession::take_snapshot(SnapshotCallback const& snapshot_taken) |
156 | { |
157 | if (auto surface = default_surface()) |
158 | |
159 | === modified file 'src/server/scene/application_session.h' |
160 | --- src/server/scene/application_session.h 2014-11-03 06:51:26 +0000 |
161 | +++ src/server/scene/application_session.h 2014-11-17 01:55:23 +0000 |
162 | @@ -55,6 +55,7 @@ |
163 | frontend::SurfaceId create_surface(SurfaceCreationParameters const& params) override; |
164 | void destroy_surface(frontend::SurfaceId surface) override; |
165 | std::shared_ptr<frontend::Surface> get_surface(frontend::SurfaceId surface) const override; |
166 | + int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int) override; |
167 | |
168 | void take_snapshot(SnapshotCallback const& snapshot_taken) override; |
169 | std::shared_ptr<Surface> default_surface() const override; |
170 | |
171 | === modified file 'src/server/scene/basic_surface.cpp' |
172 | --- src/server/scene/basic_surface.cpp 2014-10-21 16:21:14 +0000 |
173 | +++ src/server/scene/basic_surface.cpp 2014-11-17 01:55:23 +0000 |
174 | @@ -35,6 +35,7 @@ |
175 | #include <algorithm> |
176 | |
177 | namespace mc = mir::compositor; |
178 | +namespace mf = mir::frontend; |
179 | namespace ms = mir::scene; |
180 | namespace msh = mir::shell; |
181 | namespace mg = mir::graphics; |
182 | @@ -141,6 +142,7 @@ |
183 | attrib_values[mir_surface_attrib_focus] = mir_surface_unfocused; |
184 | attrib_values[mir_surface_attrib_dpi] = 0; |
185 | attrib_values[mir_surface_attrib_visibility] = mir_surface_visibility_exposed; |
186 | + attrib_values[mir_surface_attrib_parent] = mir_surface_parent_none; |
187 | } |
188 | |
189 | void ms::BasicSurface::force_requests_to_complete() |
190 | @@ -166,6 +168,38 @@ |
191 | return surface_name; |
192 | } |
193 | |
194 | +void ms::BasicSurface::set_parent(std::weak_ptr<mf::Surface> const& p) |
195 | +{ |
196 | + std::unique_lock<std::mutex> lk(guard); |
197 | + weak_parent = p; |
198 | +} |
199 | + |
200 | +int ms::BasicSurface::set_parent_id(int id) |
201 | +{ |
202 | + std::unique_lock<std::mutex> lk(guard); |
203 | + if (attrib_values[mir_surface_attrib_parent] != id) |
204 | + { |
205 | + attrib_values[mir_surface_attrib_parent] = id; |
206 | + lk.unlock(); |
207 | + observers.attrib_changed(mir_surface_attrib_parent, id); |
208 | + } |
209 | + return id; |
210 | +} |
211 | + |
212 | +std::shared_ptr<mf::Surface> ms::BasicSurface::parent() const |
213 | +{ |
214 | + std::unique_lock<std::mutex> lk(guard); |
215 | + return weak_parent.lock(); |
216 | +} |
217 | + |
218 | +/* Coming soon: Parent-relative placement. Something like... |
219 | +void ms::BasicSurface::place(geometry::Displacement const& disp) |
220 | +{ |
221 | + if (auto p = parent()) |
222 | + move_to(p->screen_position().top_left() + disp); |
223 | +} |
224 | +*/ |
225 | + |
226 | void ms::BasicSurface::move_to(geometry::Point const& top_left) |
227 | { |
228 | { |
229 | @@ -500,6 +534,9 @@ |
230 | case mir_surface_attrib_visibility: |
231 | result = set_visibility(static_cast<MirSurfaceVisibility>(result)); |
232 | break; |
233 | + case mir_surface_attrib_parent: |
234 | + result = set_parent_id(result); |
235 | + break; |
236 | default: |
237 | BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface " |
238 | "attribute.")); |
239 | |
240 | === modified file 'src/server/scene/basic_surface.h' |
241 | --- src/server/scene/basic_surface.h 2014-11-03 06:51:26 +0000 |
242 | +++ src/server/scene/basic_surface.h 2014-11-17 01:55:23 +0000 |
243 | @@ -147,6 +147,9 @@ |
244 | |
245 | int dpi() const; |
246 | |
247 | + void set_parent(std::weak_ptr<frontend::Surface> const& parent) override; |
248 | + std::shared_ptr<frontend::Surface> parent() const override; |
249 | + |
250 | private: |
251 | bool visible(std::unique_lock<std::mutex>&) const; |
252 | MirSurfaceType set_type(MirSurfaceType t); // Use configure() to make public changes |
253 | @@ -155,6 +158,7 @@ |
254 | MirSurfaceVisibility set_visibility(MirSurfaceVisibility v); |
255 | int set_swap_interval(int); |
256 | MirSurfaceFocusState set_focus_state(MirSurfaceFocusState f); |
257 | + int set_parent_id(int); |
258 | |
259 | SurfaceObservers observers; |
260 | std::mutex mutable guard; |
261 | @@ -173,6 +177,7 @@ |
262 | std::shared_ptr<SurfaceConfigurator> const configurator; |
263 | std::shared_ptr<graphics::CursorImage> cursor_image_; |
264 | std::shared_ptr<SceneReport> const report; |
265 | + std::weak_ptr<frontend::Surface> weak_parent; |
266 | |
267 | void initialize_attributes(); |
268 | int attrib_values[mir_surface_attribs]; |
269 | |
270 | === modified file 'tests/include/mir_test_doubles/mock_frontend_surface.h' |
271 | --- tests/include/mir_test_doubles/mock_frontend_surface.h 2014-10-01 06:25:56 +0000 |
272 | +++ tests/include/mir_test_doubles/mock_frontend_surface.h 2014-11-17 01:55:23 +0000 |
273 | @@ -62,6 +62,9 @@ |
274 | |
275 | MOCK_CONST_METHOD0(supports_input, bool()); |
276 | MOCK_CONST_METHOD0(client_input_fd, int()); |
277 | + |
278 | + MOCK_CONST_METHOD0(parent, std::shared_ptr<frontend::Surface>()); |
279 | + MOCK_METHOD1(set_parent, void(std::weak_ptr<Surface> const&)); |
280 | |
281 | MOCK_METHOD1(set_cursor_image, void(std::shared_ptr<graphics::CursorImage> const&)); |
282 | |
283 | |
284 | === modified file 'tests/include/mir_test_doubles/mock_surface.h' |
285 | --- tests/include/mir_test_doubles/mock_surface.h 2014-10-27 22:31:16 +0000 |
286 | +++ tests/include/mir_test_doubles/mock_surface.h 2014-11-17 01:55:23 +0000 |
287 | @@ -67,6 +67,9 @@ |
288 | MOCK_METHOD1(take_input_focus, void(std::shared_ptr<shell::InputTargeter> const&)); |
289 | MOCK_METHOD1(add_observer, void(std::shared_ptr<scene::SurfaceObserver> const&)); |
290 | MOCK_METHOD1(remove_observer, void(std::weak_ptr<scene::SurfaceObserver> const&)); |
291 | + |
292 | + MOCK_METHOD1(set_parent, void(std::weak_ptr<frontend::Surface> const&)); |
293 | + MOCK_METHOD0(parent, std::shared_ptr<frontend::Surface>()); |
294 | }; |
295 | |
296 | } |
297 | |
298 | === modified file 'tests/include/mir_test_doubles/stub_scene_session.h' |
299 | --- tests/include/mir_test_doubles/stub_scene_session.h 2014-10-01 06:25:56 +0000 |
300 | +++ tests/include/mir_test_doubles/stub_scene_session.h 2014-11-17 01:55:23 +0000 |
301 | @@ -43,6 +43,10 @@ |
302 | { |
303 | return std::shared_ptr<frontend::Surface>(); |
304 | } |
305 | + int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int value) override |
306 | + { |
307 | + return value; |
308 | + } |
309 | std::string name() const override |
310 | { |
311 | return std::string(); |
312 | |
313 | === modified file 'tests/include/mir_test_doubles/stub_scene_surface.h' |
314 | --- tests/include/mir_test_doubles/stub_scene_surface.h 2014-10-01 06:25:56 +0000 |
315 | +++ tests/include/mir_test_doubles/stub_scene_surface.h 2014-11-17 01:55:23 +0000 |
316 | @@ -95,6 +95,8 @@ |
317 | |
318 | bool supports_input() const override { return true;} |
319 | int client_input_fd() const override { return fd;} |
320 | + std::shared_ptr<frontend::Surface> parent() const override { return {}; } |
321 | + void set_parent(std::weak_ptr<frontend::Surface> const&) override { } |
322 | int configure(MirSurfaceAttrib, int) override { return 0; } |
323 | int query(MirSurfaceAttrib) override { return 0; } |
324 | void with_most_recent_buffer_do(std::function<void(graphics::Buffer&)> const& ) override {} |
325 | |
326 | === modified file 'tests/include/mir_test_doubles/stub_session.h' |
327 | --- tests/include/mir_test_doubles/stub_session.h 2014-10-01 06:25:56 +0000 |
328 | +++ tests/include/mir_test_doubles/stub_session.h 2014-11-17 01:55:23 +0000 |
329 | @@ -41,6 +41,10 @@ |
330 | { |
331 | return std::shared_ptr<frontend::Surface>(); |
332 | } |
333 | + int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int value) override |
334 | + { |
335 | + return value; |
336 | + } |
337 | std::string name() const override |
338 | { |
339 | return std::string(); |
340 | |
341 | === modified file 'tests/unit-tests/client/test_client_mir_surface.cpp' |
342 | --- tests/unit-tests/client/test_client_mir_surface.cpp 2014-10-28 13:59:51 +0000 |
343 | +++ tests/unit-tests/client/test_client_mir_surface.cpp 2014-11-17 01:55:23 +0000 |
344 | @@ -186,7 +186,8 @@ |
345 | { mir_surface_attrib_swapinterval, 1 }, |
346 | { mir_surface_attrib_focus, mir_surface_focused }, |
347 | { mir_surface_attrib_dpi, 19 }, |
348 | - { mir_surface_attrib_visibility, mir_surface_visibility_exposed } |
349 | + { mir_surface_attrib_visibility, mir_surface_visibility_exposed }, |
350 | + { mir_surface_attrib_parent, 123 } |
351 | }; |
352 | |
353 | struct MockBuffer : public mcl::ClientBuffer |
354 | |
355 | === modified file 'tests/unit-tests/scene/test_application_session.cpp' |
356 | --- tests/unit-tests/scene/test_application_session.cpp 2014-11-11 04:02:18 +0000 |
357 | +++ tests/unit-tests/scene/test_application_session.cpp 2014-11-17 01:55:23 +0000 |
358 | @@ -109,6 +109,52 @@ |
359 | session.destroy_surface(surf); |
360 | } |
361 | |
362 | +TEST(ApplicationSession, configures_surface) |
363 | +{ |
364 | + using namespace ::testing; |
365 | + |
366 | + auto mock_surface = make_mock_surface(); |
367 | + auto mock_parent_surface = make_mock_surface(); |
368 | + |
369 | + mtd::NullEventSink sender; |
370 | + NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator; |
371 | + |
372 | + EXPECT_CALL(surface_coordinator, add_surface(_, _)) |
373 | + .WillOnce(Return(mock_parent_surface)) |
374 | + .WillOnce(Return(mock_surface)); |
375 | + |
376 | + ms::ApplicationSession session( |
377 | + mt::fake_shared(surface_coordinator), |
378 | + __LINE__, |
379 | + "Foo", |
380 | + std::make_shared<mtd::NullSnapshotStrategy>(), |
381 | + std::make_shared<ms::NullSessionListener>(), |
382 | + mt::fake_shared(sender)); |
383 | + |
384 | + ms::SurfaceCreationParameters params; |
385 | + auto parent_id = session.create_surface(params); |
386 | + auto child_id = session.create_surface(params); |
387 | + |
388 | + EXPECT_CALL(*mock_surface, configure(mir_surface_attrib_type, |
389 | + mir_surface_type_dialog)) |
390 | + .WillOnce(Return(mir_surface_type_dialog)); |
391 | + std::weak_ptr<mf::Surface> parent_that_was_set; |
392 | + EXPECT_CALL(*mock_surface, set_parent(_)) |
393 | + .WillOnce(SaveArg<0>(&parent_that_was_set)); |
394 | + EXPECT_CALL(*mock_surface, configure(mir_surface_attrib_parent, |
395 | + parent_id.as_value())) |
396 | + .WillOnce(Return(parent_id.as_value())); |
397 | + |
398 | + session.configure_surface(child_id, mir_surface_attrib_type, |
399 | + mir_surface_type_dialog); |
400 | + session.configure_surface(child_id, mir_surface_attrib_parent, |
401 | + parent_id.as_value()); |
402 | + |
403 | + EXPECT_EQ(mock_parent_surface, parent_that_was_set.lock()); |
404 | + session.destroy_surface(child_id); |
405 | + session.destroy_surface(parent_id); |
406 | +} |
407 | + |
408 | TEST(ApplicationSession, listener_notified_of_surface_destruction_on_session_destruction) |
409 | { |
410 | using namespace ::testing; |
411 | |
412 | === modified file 'tests/unit-tests/scene/test_basic_surface.cpp' |
413 | --- tests/unit-tests/scene/test_basic_surface.cpp 2014-10-01 06:25:56 +0000 |
414 | +++ tests/unit-tests/scene/test_basic_surface.cpp 2014-11-17 01:55:23 +0000 |
415 | @@ -528,6 +528,13 @@ |
416 | -1 |
417 | }; |
418 | |
419 | +AttributeTestParameters const surface_parent_test_parameters{ |
420 | + mir_surface_attrib_parent, |
421 | + mir_surface_parent_none, |
422 | + 123, |
423 | + mir_surface_parent_none |
424 | +}; |
425 | + |
426 | } |
427 | |
428 | TEST_P(BasicSurfaceAttributeTest, default_value) |
429 | @@ -591,9 +598,12 @@ |
430 | auto const& attribute = params.attribute; |
431 | auto const& invalid_value = params.an_invalid_value; |
432 | |
433 | - EXPECT_THROW({ |
434 | + if (invalid_value != params.default_value) |
435 | + { |
436 | + EXPECT_THROW({ |
437 | surface.configure(attribute, invalid_value); |
438 | }, std::logic_error); |
439 | + } |
440 | } |
441 | |
442 | INSTANTIATE_TEST_CASE_P(SurfaceTypeAttributeTest, BasicSurfaceAttributeTest, |
443 | @@ -614,6 +624,9 @@ |
444 | INSTANTIATE_TEST_CASE_P(SurfaceFocusAttributeTest, BasicSurfaceAttributeTest, |
445 | ::testing::Values(surface_focus_test_parameters)); |
446 | |
447 | +INSTANTIATE_TEST_CASE_P(SurfaceParentAttributeTest, BasicSurfaceAttributeTest, |
448 | + ::testing::Values(surface_parent_test_parameters)); |
449 | + |
450 | TEST_F(BasicSurfaceTest, configure_returns_value_set_by_configurator) |
451 | { |
452 | using namespace testing; |
453 | |
454 | === modified file 'tests/unit-tests/scene/test_surface_impl.cpp' |
455 | --- tests/unit-tests/scene/test_surface_impl.cpp 2014-10-01 06:25:56 +0000 |
456 | +++ tests/unit-tests/scene/test_surface_impl.cpp 2014-11-17 01:55:23 +0000 |
457 | @@ -327,6 +327,41 @@ |
458 | EXPECT_FLOAT_EQ(1.0f, surf.alpha()); |
459 | } |
460 | |
461 | +TEST_F(Surface, remembers_parent) |
462 | +{ |
463 | + auto parent = std::make_shared<ms::BasicSurface>( |
464 | + std::string("stub"), |
465 | + geom::Rectangle{{},{}}, |
466 | + false, |
467 | + buffer_stream, |
468 | + std::shared_ptr<mi::InputChannel>(), |
469 | + stub_input_sender, |
470 | + null_configurator, |
471 | + std::shared_ptr<mg::CursorImage>(), |
472 | + report); |
473 | + |
474 | + ms::BasicSurface child( |
475 | + std::string("stub"), |
476 | + geom::Rectangle{{},{}}, |
477 | + false, |
478 | + buffer_stream, |
479 | + std::shared_ptr<mi::InputChannel>(), |
480 | + stub_input_sender, |
481 | + null_configurator, |
482 | + std::shared_ptr<mg::CursorImage>(), |
483 | + report); |
484 | + |
485 | + EXPECT_FALSE(child.parent()); |
486 | + |
487 | + child.set_parent(parent); |
488 | + EXPECT_EQ(parent, child.parent()); |
489 | + EXPECT_TRUE(!!child.parent()); |
490 | + |
491 | + child.set_parent({}); |
492 | + EXPECT_NE(parent, child.parent()); |
493 | + EXPECT_FALSE(child.parent()); |
494 | +} |
495 | + |
496 | TEST_F(Surface, sends_focus_notifications_when_focus_gained_and_lost) |
497 | { |
498 | using namespace testing; |
PASSED: Continuous integration, rev:2026 jenkins. qa.ubuntu. com/job/ mir-ci/ 1985/ jenkins. qa.ubuntu. com/job/ mir-android- utopic- i386-build/ 2352 jenkins. qa.ubuntu. com/job/ mir-clang- utopic- amd64-build/ 2362 jenkins. qa.ubuntu. com/job/ mir-mediumtests -utopic- touch/2287 jenkins. qa.ubuntu. com/job/ mir-utopic- amd64-ci/ 331 jenkins. qa.ubuntu. com/job/ mir-utopic- amd64-ci/ 331/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ mir-mediumtests -builder- utopic- armhf/1224 jenkins. qa.ubuntu. com/job/ mir-mediumtests -builder- utopic- armhf/1224/ artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ mir-mediumtests -runner- mako/3259 s-jenkins. ubuntu- ci:8080/ job/touch- flash-device/ 15400
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/mir- ci/1985/ rebuild
http://