Merge lp:~albaguirre/mir/add-menu-api into lp:mir
- add-menu-api
- Merge into development-branch
Status: | Merged |
---|---|
Approved by: | Chris Halse Rogers |
Approved revision: | no longer in the source branch. |
Merged at revision: | 2219 |
Proposed branch: | lp:~albaguirre/mir/add-menu-api |
Merge into: | lp:mir |
Prerequisite: | lp:~albaguirre/mir/pref-orientation-at-create-time |
Diff against target: |
479 lines (+190/-46) 17 files modified
client-ABI-sha1sums (+2/-2) common-ABI-sha1sums (+1/-1) include/client/mir_toolkit/mir_surface.h (+34/-0) include/common/mir_toolkit/common.h (+7/-0) include/server/mir/scene/surface_creation_parameters.h (+6/-0) platform-ABI-sha1sums (+1/-1) server-ABI-sha1sums (+2/-2) src/client/mir_surface.cpp (+8/-0) src/client/mir_surface.h (+2/-0) src/client/mir_surface_api.cpp (+19/-0) src/client/symbols.map (+5/-1) src/include/common/mir/require.h (+0/-32) src/protobuf/mir_protobuf.proto (+3/-0) src/server/frontend/session_mediator.cpp (+11/-0) src/server/scene/surface_creation_parameters.cpp (+12/-0) tests/acceptance-tests/test_client_surfaces.cpp (+22/-2) tests/integration-tests/client/test_mirsurface.cpp (+55/-5) |
To merge this branch: | bzr merge lp:~albaguirre/mir/add-menu-api |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Chris Halse Rogers | Approve | ||
PS Jenkins bot (community) | continuous-integration | Approve | |
Alan Griffiths | Approve | ||
Robert Carr (community) | Approve | ||
Daniel van Vugt | Abstain | ||
Cemil Azizoglu (community) | Approve | ||
Review via email: mp+244632@code.launchpad.net |
Commit message
Add API to specify client menu surfaces (LP: #1324101)
Description of the change
Add API to specify client menu surfaces
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2150
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://
Gerry Boland (gerboland) wrote : | # |
Weighing in as an external perspective, I must agree with Daniel. Having a method for each surface type will result in a huge number of methods. Plus if/when we change surface type names, we'll be carrying around the old names for some time.
We have an enum with surface types already. Why can't that be passed as a parameter to a mir_connection_
Also why pass parent, rect & format as arguments? Why not just ask for a surface type, be returned a MirSurfaceSpec, which then we can set parent,rect,format upon (if possible)
I ask because I expect we'll later want to specify extra properties when creating a surface, things like surface state (fullscreen/
So as opposed to adding more parameters to that function to set properties of MirSurfaceSpec, instead allow those properties to be set once we've got a MirSurfaceSpec.
Gerry Boland (gerboland) wrote : | # |
https:/
The API design does makes sense, but I'm glad I'm not maintaining such an API. Objection withdrawn
Cemil Azizoglu (cemil-azizoglu) wrote : | # |
Looks good.
Alan Griffiths (alan-griffiths) wrote : | # |
53 + if (!mir_surface_
54 + if (!rect) abort();
I think we need mir::require(
Alberto Aguirre (albaguirre) wrote : | # |
> 53 + if (!mir_surface_
> 54 + if (!rect) abort();
>
> I think we need mir::require(
> MIR_REQUIRE(
Done.
Alberto Aguirre (albaguirre) wrote : | # |
> (1) Headers containing documentation that will be read by users as such should
> be wrapped within 80 columns so they display in users' terminals correctly:
> include/
>
> We don't impose this standard on our own internal code, but the public stuff
> needs to be more consumable.
Fixed - but we'll need to address the rest of the file as well.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2153
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://
Chris Halse Rogers (raof) wrote : | # |
I think we need to guarantee a bit more semantics to the client for this. Specifically, I think we want to guarantee a rectangle that the surface will share an edge with.
If you check out how GTK currently handles menus (for example, try GTK_MODULES= gnome-calculator) the mode menu pops up so that one of its edges is against the “mode” label. It would seem that this API is insufficiently expressive to support that?
Even if we don't want to guarantee that - maybe phone shells want to ignore that? - I think we need to support that.
Maybe the API needs to be something like:
mir_connection_
Daniel van Vugt (vanvugt) : | # |
Daniel van Vugt (vanvugt) wrote : | # |
Yes, this is the shell-specific semantics being baked into the client API I was warning about a while back :(
Alberto Aguirre (albaguirre) wrote : | # |
> I think we need to guarantee a bit more semantics to the client for this.
> Specifically, I think we want to guarantee a rectangle that the surface will
> share an edge with.
>
> If you check out how GTK currently handles menus (for example, try
> GTK_MODULES= gnome-calculator) the mode menu pops up so that one of its edges
> is against the “mode” label. It would seem that this API is insufficiently
> expressive to support that?
>
> Even if we don't want to guarantee that - maybe phone shells want to ignore
> that? - I think we need to support that.
>
> Maybe the API needs to be something like:
> mir_connection_
> adjacency_rect, format)?
I don't understand what adjacency rect is supposed to represent, could you elaborate?
Chris Halse Rogers (raof) wrote : | # |
Normal case:
+----------------+
| |
| Adjacency Rect |
| |
+----------------+
+------
| |
| |
| |
| |
| Valid menu placement |
| |
| |
| |
| |
+------
When the placing the menu there would make it partially offscreen, we have:
+------
| |
| |
| |
| Valid menu placement |
| |
| |
+------
+----------------+
| |
| Adjacency Rect |
| |
+----------------+
-------
Chris Halse Rogers (raof) wrote : | # |
Boo. Launchpad messes up the ASCII art.
The adjacency rect is a rectangle that the menu shares an edge (and most commonly a corner) with.
Think of how menus are presented at the moment - there's a box around the text label, and the menu's top-left corner normally matches with the bottom-left corner of the label-box.
However, if this placement would leave the menu partially off-screen then the menu is displayed so that its bottom left-corner matches the top-left corner of the label-box.
If *that* would result in the menu being partially off-screen, then the menu is displayed so that it's bottom *edge* matches the top edge of the label-box.
Hm. Observing the behaviour, we also need an edge-affinity - context menus do right-edge/
Robert Carr (robertcarr) wrote : | # |
Seems like Chris is correct...
Alberto Aguirre (albaguirre) wrote : | # |
@Chris,
I see so adjacency rectangle is about the client hinting the four preferred corners.
On edge affinity - maybe:
enum EdgeDirection
{
none,
vertical,
horizontal
};
So the api would be something like
mir_connection_
Robert Carr (robertcarr) wrote : | # |
This is strange but I would expect the c'tor arguments to be ordered otherwise:
create_
Feel free to ignore that :p
>>
>> + * \param [in] rect A rectangle that specifies four edges this surface
>> 49 + * can attach to depending on placement constraints in
>> 50 + * the server due to screen size or other requirements.
>> 51 + * The server is not guaranteed to create a surface at
>> 52 + * the requested location
"four edges this surface can attach to depending.
I think this might be hard to read if you don't know you are looking for a definition of an adjacency rectangle.
Maybe we can clarify it by introducing the adjacency term e.g. in the top level comment:
/**
39 + * Create a surface specification for a menu surface.
* Positioning of the surface may be specified with respect to the parent surface via an
* rectangle. The server will attempt to choose an edge of the adjacency rectangle on \
* which to place the surface (taking in to account screen-edge proximity
* and similar constraints)
Then:
>> + * \param [in] rect The adjacency rectangle.
Robert Carr (robertcarr) wrote : | # |
I kind of think that SessionMediator should throw if for example adjacency rect is set and parent is not set. Maybe we already had this discussion and you convinced me otherwise though? Having trouble remembering but have some vague notion.
Any looks good please consider introducing the adjacency terminology though.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2160
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://
Chris Halse Rogers (raof) wrote : | # |
Tests have some
+ ASSERT_
this would be better with mir_test/
+ ASSERT_THAT(menu, IsValid())
(this gets you a helpful message when the assertion fails
What are the semantics of mir_edge_
I agree with Robert; having an adjacency rect set but not a parent or edge_attachment should throw from SessionMediator. As long as our client library isn't buggy it will never send such a message, but it's cheap and useful to check.
Hm. That actually suggests a MessageValidator type interface, dispatching based on surface_type. Probably not for this MP, though.
I also like Robert's rewording of the documentation comment (although I'd replace ‘may be specified’ with ‘is specified’).
I'm only blocking on “what does mir_edge_
Daniel van Vugt (vanvugt) wrote : | # |
Attached bug 1324101. Note however that bug needs a solution for tooltips too. So if this lands, please keep the bug marked In Progress.
Alberto Aguirre (albaguirre) wrote : | # |
> This is strange but I would expect the c'tor arguments to be ordered
> otherwise:
> create_
> height, format)
>
> Feel free to ignore that :p
>
I don't feel strongly one way or the other, but I thought I would make it consistent with mir_connection_
> I think this might be hard to read if you don't know you are looking for a
> definition of an adjacency rectangle.
>
> Maybe we can clarify it by introducing the adjacency term e.g. in the top
> level comment
Done.
Alan Griffiths (alan-griffiths) wrote : | # |
358 + ASSERT_TRUE(spec != nullptr);
Could be ASSERT_THAT(spec, NotNull());
Alberto Aguirre (albaguirre) wrote : | # |
> I kind of think that SessionMediator should throw if for example adjacency
> rect is set and parent is not set. Maybe we already had this discussion and
> you convinced me otherwise though? Having trouble remembering but have some
> vague notion.
>
I think that should be the responsibility of the implementation of session-
Alan Griffiths (alan-griffiths) wrote : | # |
> > I kind of think that SessionMediator should throw if for example adjacency
> > rect is set and parent is not set. Maybe we already had this discussion and
> > you convinced me otherwise though? Having trouble remembering but have some
> > vague notion.
> >
> I think that should be the responsibility of the implementation of
> session-
> now possible since optional_values are used in SurfaceCreation
I agree it shouldn't be down to the frontend (i.e. SessionMediator) to impose window management rules like this.
I know it is before the time of many current team members but we originally envisaged an MVC arrangement where changes to the model (like creating a surface) would be routed through a controller (like a window manager) that is part of the shell and not go directly to the model (in this case the session).
Were we in that hypothetical world then the window manager would validate this policy.
Robert Carr (robertcarr) wrote : | # |
>> I think that should be the responsibility of the implementation of session-
>> SurfaceCreation
Ah right :).
I guess I am still not 100% convinced, because I think of this as a sort of "protocol validation condition" which I view the session mediator as responsible for. Still...not particularly concerned :)
Alberto Aguirre (albaguirre) wrote : | # |
> Tests have some
> + ASSERT_
> this would be better with mir_test/
> + ASSERT_THAT(menu, IsValid())
> (this gets you a helpful message when the assertion fails
>
>
Done.
> What are the semantics of mir_edge_
> makes sense?
Changed the name to any - meaning both horizonal and vertical edge attachment is ok.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2165
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Chris Halse Rogers (raof) wrote : | # |
Um, now mir_edge_
I think it should either be removed, or specified as equal to mir_edge_
@Alan:
> I know it is before the time of many current team members but we originally envisaged an MVC arrangement
> where changes to the model (like creating a surface) would be routed through a controller
> (like a window manager) that is part of the shell and not go directly to the model (in this case the session).
>
> Were we in that hypothetical world then the window manager would validate this policy.
Hm. Whereas I think this would be a part of the contract with the window manager - “when we ask you to create a window, we guarantee that it'll have all the mandatory information”. The client library needs to specify what the mandatory information is, because that's the only place where we can specify client semantics.
> Attached bug 1324101. Note however that bug needs a solution for tooltips too. So if this lands,
> please keep the bug marked In Progress.
This actually *is* a solution for tooltips - with a small adjacency rectangle and mir_edge_
But that reminds me - we need to specify the input semantics of these surfaces. Something like “the new surface has input focus, and will not lose input focus while it is alive and the topmost menu surface. Any tap¹ outside the bounds of the surface will result in the surface being immediately closed and receive a mir_surface_closed event. (Without that event being otherwise delivered?)”
We also need to specify what happens with stacked menus - something like “While a menu is alive, any request to create a menu surface that does *not* specify the existing menu as its parent will fail.”
It might be sensible to require the parent of a new menu to have focus, or the request fail?
¹: This needs to be better defined.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2166
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://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2167
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://
Alberto Aguirre (albaguirre) wrote : | # |
> Um, now mir_edge_
> mir_edge_
>
> I think it should either be removed, or specified as equal to
> mir_edge_
> towards the latter.
>
Fixed.
> @Alan:
> > I know it is before the time of many current team members but we originally
> envisaged an MVC arrangement
> > where changes to the model (like creating a surface) would be routed through
> a controller
> > (like a window manager) that is part of the shell and not go directly to the
> model (in this case the session).
> >
> > Were we in that hypothetical world then the window manager would validate
> this policy.
>
> Hm. Whereas I think this would be a part of the contract with the window
> manager - “when we ask you to create a window, we guarantee that it'll have
> all the mandatory information”. The client library needs to specify what the
> mandatory information is, because that's the only place where we can specify
> client semantics.
Sure, but we are talking about malformed requests, which need to be validated by something. That something for me is not the frontend as that would couple knowledge of window management semantics (The implementation of the window management policy is the one that knows what combination of arguments are correct for a surface type for example).
> But that reminds me - we need to specify the input semantics of these
> surfaces. Something like “the new surface has input focus, and will not lose
> input focus while it is alive and the topmost menu surface. Any tap¹ outside
> the bounds of the surface will result in the surface being immediately closed
> and receive a mir_surface_closed event. (Without that event being otherwise
> delivered?)”
>
> We also need to specify what happens with stacked menus - something like
> “While a menu is alive, any request to create a menu surface that does *not*
> specify the existing menu as its parent will fail.”
>
> It might be sensible to require the parent of a new menu to have focus, or the
> request fail?
>
> ¹: This needs to be better defined.
Yes we need the details on focusing semantics, but I think that goes in hand with the window management story on the server side, i.e. outside of the scope of this particular MP.
Chris Halse Rogers (raof) wrote : | # |
On Wed, Jan 14, 2015 at 9:27 AM, Alberto Aguirre
<email address hidden> wrote:
>> @Alan:
>> > I know it is before the time of many current team members but we
>> originally
>> envisaged an MVC arrangement
>> > where changes to the model (like creating a surface) would be
>> routed through
>> a controller
>> > (like a window manager) that is part of the shell and not go
>> directly to the
>> model (in this case the session).
>> >
>> > Were we in that hypothetical world then the window manager would
>> validate
>> this policy.
>>
>> Hm. Whereas I think this would be a part of the contract with the
>> window
>> manager - “when we ask you to create a window, we guarantee that
>> it'll have
>> all the mandatory information”. The client library needs to
>> specify what the
>> mandatory information is, because that's the only place where we
>> can specify
>> client semantics.
>
> Sure, but we are talking about malformed requests, which need to be
> validated by something. That something for me is not the frontend as
> that would couple knowledge of window management semantics (The
> implementation of the window management policy is the one that knows
> what combination of arguments are correct for a surface type for
> example).
We're (by necessity - there's no where else to put it) encoding window
management semantics into the client library and protocol. The client
library already knows what properties are mandatory for a given surface
type - we encode them in the surface_spec constructor.
The shell's window management policy may (but almost always
*shouldn't*) impose additional constraints.
I think that malformed requests are exactly the sort of thing the
protocol layer should be filtering out. Surface creation isn't a
hot-path, so a defence-in-depth, validate whatever you can at this
stage and let the next stage validate some more, is reasonable.
Note that this doesn't necessarily mean validation is baked into
SessionManager - we could have a separate MessageValidator interface
SessionManager delegates to, or (possibly a better idea) we could
specialise the SurfaceCreate protocol message for each surface type so
it's not possible to represent an invalid request.
That's not necessarily in-scope for this MP, though.
Chris Halse Rogers (raof) wrote : | # |
Looks acceptable to me.
Ideally I'd like to have <parent, adjacency_rect, edge_affinity> be a single optional tuple, but (a) I'm not sure that we have consensus for that, and (b) that'd probably be better handled by splitting SurfaceCreation
Could you update the mir_connection_
Preview Diff
1 | === modified file 'client-ABI-sha1sums' |
2 | --- client-ABI-sha1sums 2015-01-06 03:29:48 +0000 |
3 | +++ client-ABI-sha1sums 2015-01-13 16:38:19 +0000 |
4 | @@ -6,11 +6,11 @@ |
5 | d739af6e64db6b314a727b5fb00be662b98ccd57 include/client/mir_toolkit/mir_platform_message.h |
6 | 9d50df5a141ca03ee8a79f7e844ed4b8b3b7d5d3 include/client/mir_toolkit/mir_prompt_session.h |
7 | 21d07e655e85eeec8a3523e1c6f9c2252176ec01 include/client/mir_toolkit/mir_screencast.h |
8 | -82a2a3d219747a778284811ba0c01e0fdf167b30 include/client/mir_toolkit/mir_surface.h |
9 | +8ea06d0a56ef400ae7813312fd4edb8114a4632a include/client/mir_toolkit/mir_surface.h |
10 | b141c4d79802ad626d969249c0004744e5c2a525 include/client/mir_toolkit/mir_wait.h |
11 | 6f7b4ecc22afba923806ed2bd7d8244be90b0cfd include/client/mir_toolkit/version.h |
12 | 3350dac884d6006753de2a955bc7a05663cd9449 include/common/mir_toolkit/client_types.h |
13 | -c7c708734715a6d1b6fb2652584adb912071a518 include/common/mir_toolkit/common.h |
14 | +20086d47f8747ba34f8d2c4aba75503adb347247 include/common/mir_toolkit/common.h |
15 | fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h |
16 | f4d39e9893ce6308bddd83a49b90f0051f565323 include/common/mir_toolkit/event.h |
17 | 2507f2929415aa423f9551d3c595c439fe1c6efd include/common/mir_toolkit/events/event_deprecated.h |
18 | |
19 | === modified file 'common-ABI-sha1sums' |
20 | --- common-ABI-sha1sums 2015-01-06 03:29:48 +0000 |
21 | +++ common-ABI-sha1sums 2015-01-13 16:38:19 +0000 |
22 | @@ -17,7 +17,7 @@ |
23 | 31b9c24e2ce7194aeea6694e81c160354033d28a include/common/mir/optional_value.h |
24 | 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h |
25 | 3350dac884d6006753de2a955bc7a05663cd9449 include/common/mir_toolkit/client_types.h |
26 | -c7c708734715a6d1b6fb2652584adb912071a518 include/common/mir_toolkit/common.h |
27 | +20086d47f8747ba34f8d2c4aba75503adb347247 include/common/mir_toolkit/common.h |
28 | fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h |
29 | f4d39e9893ce6308bddd83a49b90f0051f565323 include/common/mir_toolkit/event.h |
30 | 2507f2929415aa423f9551d3c595c439fe1c6efd include/common/mir_toolkit/events/event_deprecated.h |
31 | |
32 | === modified file 'include/client/mir_toolkit/mir_surface.h' |
33 | --- include/client/mir_toolkit/mir_surface.h 2014-12-12 14:51:33 +0000 |
34 | +++ include/client/mir_toolkit/mir_surface.h 2015-01-13 16:38:19 +0000 |
35 | @@ -51,6 +51,40 @@ |
36 | MirPixelFormat format); |
37 | |
38 | /** |
39 | + * Create a surface specification for a menu surface. |
40 | + * |
41 | + * Positioning of the surface is specified with respect to the parent surface |
42 | + * via an adjacency rectangle. The server will attempt to choose an edge of the |
43 | + * adjacency rectangle on which to place the surface taking in to account |
44 | + * screen-edge proximity or similar constraints. In addition, the server can use |
45 | + * the edge affinity hint to consider only horizontal or only vertical adjacency |
46 | + * edges in the given rectangle. |
47 | + * |
48 | + * \param [in] connection Connection the surface will be created on |
49 | + * \param [in] width Requested width. The server is not guaranteed to |
50 | + * return a surface of this width. |
51 | + * \param [in] height Requested height. The server is not guaranteed to |
52 | + * return a surface of this height. |
53 | + * \param [in] format Pixel format for the surface. |
54 | + * \param [in] parent A valid parent surface for this menu. |
55 | + * \param [in] rect The adjacency rectangle. The server is not |
56 | + * guaranteed to create a surface at the requested |
57 | + * location. |
58 | + * \param [in] edge The preferred edge direction to attach to. Use |
59 | + * mir_edge_attachment_any for no preference. |
60 | + * \return A handle that can be passed to mir_surface_create() |
61 | + * to complete construction. |
62 | + */ |
63 | +MirSurfaceSpec* |
64 | +mir_connection_create_spec_for_menu_surface(MirConnection* connection, |
65 | + int width, |
66 | + int height, |
67 | + MirPixelFormat format, |
68 | + MirSurface* parent, |
69 | + MirRectangle* rect, |
70 | + MirEdgeAttachment edge); |
71 | + |
72 | +/** |
73 | * Create a surface from a given specification |
74 | * |
75 | * |
76 | |
77 | === modified file 'include/common/mir_toolkit/common.h' |
78 | --- include/common/mir_toolkit/common.h 2014-12-19 02:31:34 +0000 |
79 | +++ include/common/mir_toolkit/common.h 2015-01-13 16:38:19 +0000 |
80 | @@ -159,6 +159,13 @@ |
81 | mir_orientation_mode_landscape_any |
82 | } MirOrientationMode; |
83 | |
84 | +typedef enum MirEdgeAttachment |
85 | +{ |
86 | + mir_edge_attachment_vertical = 1 << 0, |
87 | + mir_edge_attachment_horizontal = 1 << 1, |
88 | + mir_edge_attachment_any = mir_edge_attachment_vertical | |
89 | + mir_edge_attachment_horizontal |
90 | +} MirEdgeAttachment; |
91 | /**@}*/ |
92 | |
93 | #endif |
94 | |
95 | === modified file 'include/server/mir/scene/surface_creation_parameters.h' |
96 | --- include/server/mir/scene/surface_creation_parameters.h 2014-12-19 04:40:45 +0000 |
97 | +++ include/server/mir/scene/surface_creation_parameters.h 2015-01-13 16:38:19 +0000 |
98 | @@ -67,6 +67,10 @@ |
99 | |
100 | SurfaceCreationParameters& with_parent_id(frontend::SurfaceId const& id); |
101 | |
102 | + SurfaceCreationParameters& with_attachment_rect(geometry::Rectangle const& rect); |
103 | + |
104 | + SurfaceCreationParameters& with_edge_attachment(MirEdgeAttachment edge); |
105 | + |
106 | std::string name; |
107 | geometry::Size size; |
108 | geometry::Point top_left; |
109 | @@ -80,6 +84,8 @@ |
110 | mir::optional_value<MirSurfaceType> type; |
111 | mir::optional_value<MirOrientationMode> preferred_orientation; |
112 | mir::optional_value<frontend::SurfaceId> parent_id; |
113 | + mir::optional_value<geometry::Rectangle> attachment_rect; |
114 | + mir::optional_value<MirEdgeAttachment> edge_attachment; |
115 | }; |
116 | |
117 | bool operator==(const SurfaceCreationParameters& lhs, const SurfaceCreationParameters& rhs); |
118 | |
119 | === modified file 'platform-ABI-sha1sums' |
120 | --- platform-ABI-sha1sums 2015-01-08 03:16:37 +0000 |
121 | +++ platform-ABI-sha1sums 2015-01-13 16:38:19 +0000 |
122 | @@ -17,7 +17,7 @@ |
123 | 31b9c24e2ce7194aeea6694e81c160354033d28a include/common/mir/optional_value.h |
124 | 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h |
125 | 3350dac884d6006753de2a955bc7a05663cd9449 include/common/mir_toolkit/client_types.h |
126 | -c7c708734715a6d1b6fb2652584adb912071a518 include/common/mir_toolkit/common.h |
127 | +20086d47f8747ba34f8d2c4aba75503adb347247 include/common/mir_toolkit/common.h |
128 | fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h |
129 | f4d39e9893ce6308bddd83a49b90f0051f565323 include/common/mir_toolkit/event.h |
130 | 2507f2929415aa423f9551d3c595c439fe1c6efd include/common/mir_toolkit/events/event_deprecated.h |
131 | |
132 | === modified file 'server-ABI-sha1sums' |
133 | --- server-ABI-sha1sums 2015-01-08 03:16:37 +0000 |
134 | +++ server-ABI-sha1sums 2015-01-13 16:38:19 +0000 |
135 | @@ -17,7 +17,7 @@ |
136 | 31b9c24e2ce7194aeea6694e81c160354033d28a include/common/mir/optional_value.h |
137 | 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h |
138 | 3350dac884d6006753de2a955bc7a05663cd9449 include/common/mir_toolkit/client_types.h |
139 | -c7c708734715a6d1b6fb2652584adb912071a518 include/common/mir_toolkit/common.h |
140 | +20086d47f8747ba34f8d2c4aba75503adb347247 include/common/mir_toolkit/common.h |
141 | fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c include/common/mir_toolkit/cursors.h |
142 | f4d39e9893ce6308bddd83a49b90f0051f565323 include/common/mir_toolkit/event.h |
143 | 2507f2929415aa423f9551d3c595c439fe1c6efd include/common/mir_toolkit/events/event_deprecated.h |
144 | @@ -96,7 +96,7 @@ |
145 | 5bab4dc0a6488b8b9d47e958c6bab94f1dcf2c57 include/server/mir/scene/surface_buffer_access.h |
146 | bbc9e2e2bb71634cd6f1c5c0430093e10e74fa23 include/server/mir/scene/surface_configurator.h |
147 | e5e4dd7bcaf186810043fa0f05be42d7e49b0843 include/server/mir/scene/surface_coordinator.h |
148 | -6d669f4235c2237fb35578671405db882b6aafba include/server/mir/scene/surface_creation_parameters.h |
149 | +8e3636b736700d72520709c8a61f5e85d9231d4c include/server/mir/scene/surface_creation_parameters.h |
150 | 7da48dbedb4430ce322a50ef8f4e93ce08bdf147 include/server/mir/scene/surface.h |
151 | 587e22d751656ce2d9536afdf5659276ff9bbc46 include/server/mir/scene/surface_observer.h |
152 | 7ef3e99901168cda296d74d05a979f47bf9c3ff1 include/server/mir/server_action_queue.h |
153 | |
154 | === modified file 'src/client/mir_surface.cpp' |
155 | --- src/client/mir_surface.cpp 2014-12-19 04:37:16 +0000 |
156 | +++ src/client/mir_surface.cpp 2015-01-13 16:38:19 +0000 |
157 | @@ -89,6 +89,14 @@ |
158 | SERIALIZE_OPTION_IF_SET(pref_orientation, message); |
159 | if (parent.is_set() && parent.value() != nullptr) |
160 | message.set_parent_id(parent.value()->id()); |
161 | + if (attachment_rect.is_set()) |
162 | + { |
163 | + message.mutable_attachment_rect()->set_left(attachment_rect.value().left); |
164 | + message.mutable_attachment_rect()->set_top(attachment_rect.value().top); |
165 | + message.mutable_attachment_rect()->set_width(attachment_rect.value().width); |
166 | + message.mutable_attachment_rect()->set_height(attachment_rect.value().height); |
167 | + } |
168 | + SERIALIZE_OPTION_IF_SET(edge_attachment, message); |
169 | |
170 | return message; |
171 | } |
172 | |
173 | === modified file 'src/client/mir_surface.h' |
174 | --- src/client/mir_surface.h 2014-12-19 04:37:16 +0000 |
175 | +++ src/client/mir_surface.h 2015-01-13 16:38:19 +0000 |
176 | @@ -78,6 +78,8 @@ |
177 | mir::optional_value<MirOrientationMode> pref_orientation; |
178 | |
179 | mir::optional_value<MirSurface*> parent; |
180 | + mir::optional_value<MirRectangle> attachment_rect; |
181 | + mir::optional_value<MirEdgeAttachment> edge_attachment; |
182 | }; |
183 | |
184 | struct MirSurface : public mir::client::ClientSurface |
185 | |
186 | === modified file 'src/client/mir_surface_api.cpp' |
187 | --- src/client/mir_surface_api.cpp 2015-01-06 03:29:48 +0000 |
188 | +++ src/client/mir_surface_api.cpp 2015-01-13 16:38:19 +0000 |
189 | @@ -51,6 +51,25 @@ |
190 | return new MirSurfaceSpec{connection, width, height, format}; |
191 | } |
192 | |
193 | +MirSurfaceSpec* mir_connection_create_spec_for_menu_surface(MirConnection* connection, |
194 | + int width, |
195 | + int height, |
196 | + MirPixelFormat format, |
197 | + MirSurface* parent, |
198 | + MirRectangle* rect, |
199 | + MirEdgeAttachment edge) |
200 | +{ |
201 | + mir::require(mir_surface_is_valid(parent)); |
202 | + mir::require(rect != nullptr); |
203 | + |
204 | + auto spec = new MirSurfaceSpec{connection, width, height, format}; |
205 | + spec->type = mir_surface_type_menu; |
206 | + spec->parent = parent; |
207 | + spec->attachment_rect = *rect; |
208 | + spec->edge_attachment = edge; |
209 | + return spec; |
210 | +} |
211 | + |
212 | MirSurface* mir_surface_create_sync(MirSurfaceSpec* requested_specification) |
213 | { |
214 | MirSurface* surface = nullptr; |
215 | |
216 | === modified file 'src/client/symbols.map' |
217 | --- src/client/symbols.map 2014-12-12 14:51:33 +0000 |
218 | +++ src/client/symbols.map 2015-01-13 16:38:19 +0000 |
219 | @@ -33,4 +33,8 @@ |
220 | mir_surface_set_preferred_orientation; |
221 | mir_surface_get_preferred_orientation; |
222 | mir_surface_spec_set_preferred_orientation; |
223 | -} MIR_CLIENT_8.1; |
224 | \ No newline at end of file |
225 | +} MIR_CLIENT_8.1; |
226 | + |
227 | +MIR_CLIENT_8.3 { |
228 | + mir_connection_create_spec_for_menu_surface; |
229 | +} MIR_CLIENT_8.2; |
230 | \ No newline at end of file |
231 | |
232 | === removed file 'src/include/common/mir/require.h' |
233 | --- src/include/common/mir/require.h 2014-12-19 05:01:58 +0000 |
234 | +++ src/include/common/mir/require.h 1970-01-01 00:00:00 +0000 |
235 | @@ -1,32 +0,0 @@ |
236 | -/* |
237 | - * Copyright © 2014 Canonical Ltd. |
238 | - * |
239 | - * This program is free software: you can redistribute it and/or modify |
240 | - * it under the terms of the GNU Lesser General Public License version 3 as |
241 | - * published by the Free Software Foundation. |
242 | - * |
243 | - * This program is distributed in the hope that it will be useful, |
244 | - * but WITHOUT ANY WARRANTY; without even the implied warranty of |
245 | - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
246 | - * GNU Lesser General Public License for more details. |
247 | - * |
248 | - * You should have received a copy of the GNU Lesser General Public License |
249 | - * along with this program. If not, see <http://www.gnu.org/licenses/>. |
250 | - * |
251 | - * Authored by: Alberto Aguirre <alberto.aguirre@canonical.com> |
252 | - */ |
253 | - |
254 | -#ifndef MIR_REQUIRE_H_ |
255 | -#define MIR_REQUIRE_H_ |
256 | - |
257 | -#include <cstdlib> |
258 | - |
259 | -namespace mir |
260 | -{ |
261 | -inline void require(bool const expr) |
262 | -{ |
263 | - if (!expr) std::abort(); |
264 | -} |
265 | -} |
266 | - |
267 | -#endif /* MIR_REQUIRE_H_ */ |
268 | |
269 | === modified file 'src/protobuf/mir_protobuf.proto' |
270 | --- src/protobuf/mir_protobuf.proto 2014-12-19 04:38:08 +0000 |
271 | +++ src/protobuf/mir_protobuf.proto 2015-01-13 16:38:19 +0000 |
272 | @@ -24,6 +24,9 @@ |
273 | optional int32 state = 8; |
274 | optional int32 pref_orientation = 9; |
275 | optional int32 parent_id = 10; |
276 | + |
277 | + optional Rectangle attachment_rect = 11; |
278 | + optional int32 edge_attachment = 12; |
279 | } |
280 | |
281 | message SurfaceId { |
282 | |
283 | === modified file 'src/server/frontend/session_mediator.cpp' |
284 | --- src/server/frontend/session_mediator.cpp 2014-12-19 04:40:45 +0000 |
285 | +++ src/server/frontend/session_mediator.cpp 2015-01-13 16:38:19 +0000 |
286 | @@ -198,6 +198,17 @@ |
287 | if (request->has_parent_id()) |
288 | params.with_parent_id(SurfaceId{request->parent_id()}); |
289 | |
290 | + if (request->has_attachment_rect()) |
291 | + { |
292 | + params.with_attachment_rect(geom::Rectangle{ |
293 | + {request->attachment_rect().left(), request->attachment_rect().top()}, |
294 | + {request->attachment_rect().width(), request->attachment_rect().height()} |
295 | + }); |
296 | + } |
297 | + |
298 | + if (request->has_edge_attachment()) |
299 | + params.with_edge_attachment(static_cast<MirEdgeAttachment>(request->edge_attachment())); |
300 | + |
301 | auto const surf_id = session->create_surface(params); |
302 | |
303 | auto surface = session->get_surface(surf_id); |
304 | |
305 | === modified file 'src/server/scene/surface_creation_parameters.cpp' |
306 | --- src/server/scene/surface_creation_parameters.cpp 2014-12-19 04:40:45 +0000 |
307 | +++ src/server/scene/surface_creation_parameters.cpp 2015-01-13 16:38:19 +0000 |
308 | @@ -124,6 +124,18 @@ |
309 | return *this; |
310 | } |
311 | |
312 | +ms::SurfaceCreationParameters& ms::SurfaceCreationParameters::with_attachment_rect(geometry::Rectangle const& rect) |
313 | +{ |
314 | + attachment_rect = rect; |
315 | + return *this; |
316 | +} |
317 | + |
318 | +ms::SurfaceCreationParameters& ms::SurfaceCreationParameters::with_edge_attachment(MirEdgeAttachment edge) |
319 | +{ |
320 | + edge_attachment = edge; |
321 | + return *this; |
322 | +} |
323 | + |
324 | bool ms::operator==( |
325 | const SurfaceCreationParameters& lhs, |
326 | const ms::SurfaceCreationParameters& rhs) |
327 | |
328 | === modified file 'tests/acceptance-tests/test_client_surfaces.cpp' |
329 | --- tests/acceptance-tests/test_client_surfaces.cpp 2014-12-17 15:20:40 +0000 |
330 | +++ tests/acceptance-tests/test_client_surfaces.cpp 2015-01-13 16:38:19 +0000 |
331 | @@ -21,6 +21,7 @@ |
332 | |
333 | #include "mir_test_framework/connected_client_headless_server.h" |
334 | #include "mir_test_framework/any_surface.h" |
335 | +#include "mir_test/validity_matchers.h" |
336 | |
337 | #include <gmock/gmock.h> |
338 | #include <gtest/gtest.h> |
339 | @@ -190,7 +191,7 @@ |
340 | TEST_P(WithOrientation, have_requested_preferred_orientation) |
341 | { |
342 | auto spec = mir_connection_create_spec_for_normal_surface(connection, 1, 1, mir_pixel_format_abgr_8888); |
343 | - ASSERT_TRUE(spec != nullptr); |
344 | + ASSERT_THAT(spec, NotNull()); |
345 | |
346 | MirOrientationMode mode{GetParam()}; |
347 | mir_surface_spec_set_preferred_orientation(spec, mode); |
348 | @@ -198,7 +199,7 @@ |
349 | auto surface = mir_surface_create_sync(spec); |
350 | mir_surface_spec_release(spec); |
351 | |
352 | - ASSERT_TRUE(mir_surface_is_valid(surface)); |
353 | + ASSERT_THAT(surface, IsValid()); |
354 | EXPECT_EQ(mir_surface_get_preferred_orientation(surface), mode); |
355 | |
356 | mir_surface_release_sync(surface); |
357 | @@ -211,3 +212,22 @@ |
358 | mir_orientation_mode_portrait_any, mir_orientation_mode_landscape_any, |
359 | mir_orientation_mode_any)); |
360 | |
361 | +TEST_F(ClientSurfaces, can_be_menus) |
362 | +{ |
363 | + auto parent = mtf::make_any_surface(connection); |
364 | + MirRectangle attachment_rect{100, 200, 100, 100}; |
365 | + |
366 | + auto spec = mir_connection_create_spec_for_menu_surface(connection, 640, 480, |
367 | + mir_pixel_format_abgr_8888, parent, &attachment_rect, mir_edge_attachment_vertical); |
368 | + ASSERT_THAT(spec, NotNull()); |
369 | + |
370 | + auto menu = mir_surface_create_sync(spec); |
371 | + mir_surface_spec_release(spec); |
372 | + |
373 | + ASSERT_THAT(menu, IsValid()); |
374 | + EXPECT_EQ(mir_surface_get_type(menu), mir_surface_type_menu); |
375 | + |
376 | + mir_surface_release_sync(parent); |
377 | + mir_surface_release_sync(menu); |
378 | +} |
379 | + |
380 | |
381 | === modified file 'tests/integration-tests/client/test_mirsurface.cpp' |
382 | --- tests/integration-tests/client/test_mirsurface.cpp 2014-12-19 04:42:39 +0000 |
383 | +++ tests/integration-tests/client/test_mirsurface.cpp 2015-01-13 16:38:19 +0000 |
384 | @@ -19,6 +19,7 @@ |
385 | #include "mir_test_framework/connected_client_headless_server.h" |
386 | #include "mir_test_doubles/stub_scene_surface.h" |
387 | #include "mir_test/fake_shared.h" |
388 | +#include "mir_test/validity_matchers.h" |
389 | |
390 | #include "mir/scene/surface.h" |
391 | #include "mir/scene/surface_creation_parameters.h" |
392 | @@ -73,6 +74,31 @@ |
393 | parent_field_matches(arg, spec->parent); |
394 | } |
395 | |
396 | +MATCHER(IsAMenu, "") |
397 | +{ |
398 | + return arg.type == mir_surface_type_menu; |
399 | +} |
400 | + |
401 | +MATCHER_P(HasParent, parent, "") |
402 | +{ |
403 | + return arg.parent_id.is_set() && arg.parent_id.value().as_value() == parent->id(); |
404 | +} |
405 | + |
406 | +MATCHER_P(MatchesAttachment, rect, "") |
407 | +{ |
408 | + return arg.attachment_rect.is_set() && |
409 | + arg.attachment_rect.value().top_left.x.as_int() == rect.left && |
410 | + arg.attachment_rect.value().top_left.y.as_int() == rect.top && |
411 | + arg.attachment_rect.value().size.width.as_uint32_t() == rect.width && |
412 | + arg.attachment_rect.value().size.height.as_uint32_t() == rect.height; |
413 | +} |
414 | + |
415 | +MATCHER_P(MatchesEdge, edge, "") |
416 | +{ |
417 | + return arg.edge_attachment.is_set() && |
418 | + arg.edge_attachment.value() == edge; |
419 | +} |
420 | + |
421 | } |
422 | |
423 | struct ClientMirSurface : mtf::ConnectedClientHeadlessServer |
424 | @@ -103,9 +129,9 @@ |
425 | spec.pref_orientation = mir_orientation_mode_landscape; |
426 | } |
427 | |
428 | - std::unique_ptr<MirSurface, std::function<void(MirSurface*)>> create_surface() |
429 | + std::unique_ptr<MirSurface, std::function<void(MirSurface*)>> create_surface(MirSurfaceSpec* spec) |
430 | { |
431 | - return {mir_surface_create_sync(&spec), [](MirSurface *surf){mir_surface_release_sync(surf);}}; |
432 | + return {mir_surface_create_sync(spec), [](MirSurface *surf){mir_surface_release_sync(surf);}}; |
433 | } |
434 | |
435 | MirSurfaceSpec spec{nullptr, 640, 480, mir_pixel_format_abgr_8888}; |
436 | @@ -118,10 +144,10 @@ |
437 | { |
438 | EXPECT_CALL(*mock_surface_coordinator, add_surface(AllOf(MatchesRequired(&spec), MatchesOptional(&spec)),_)); |
439 | |
440 | - auto surf = create_surface(); |
441 | + auto surf = create_surface(&spec); |
442 | |
443 | // A valid surface is needed to be specified as a parent |
444 | - ASSERT_TRUE(mir_surface_is_valid(surf.get())); |
445 | + ASSERT_THAT(surf.get(), IsValid()); |
446 | spec.parent = surf.get(); |
447 | |
448 | // The second time around we don't care if the surface gets created, |
449 | @@ -130,5 +156,29 @@ |
450 | spec.output_id = arbitrary_output_id; |
451 | |
452 | EXPECT_CALL(*mock_surface_coordinator, add_surface(MatchesOptional(&spec),_)); |
453 | - create_surface(); |
454 | + create_surface(&spec); |
455 | +} |
456 | + |
457 | +TEST_F(ClientMirSurface, as_menu_sends_correct_params) |
458 | +{ |
459 | + EXPECT_CALL(*mock_surface_coordinator, add_surface(_,_)); |
460 | + auto parent = create_surface(&spec); |
461 | + ASSERT_THAT(parent.get(), IsValid()); |
462 | + |
463 | + MirRectangle attachment_rect{100, 200, 300, 400}; |
464 | + MirEdgeAttachment edge{mir_edge_attachment_horizontal}; |
465 | + |
466 | + auto spec_deleter = [](MirSurfaceSpec* spec) {mir_surface_spec_release(spec);}; |
467 | + std::unique_ptr<MirSurfaceSpec, decltype(spec_deleter)> menu_spec{ |
468 | + mir_connection_create_spec_for_menu_surface(connection, 640, 480, |
469 | + mir_pixel_format_abgr_8888, parent.get(), &attachment_rect, |
470 | + edge), |
471 | + spec_deleter |
472 | + }; |
473 | + |
474 | + ASSERT_THAT(menu_spec, NotNull()); |
475 | + |
476 | + EXPECT_CALL(*mock_surface_coordinator, add_surface(AllOf(IsAMenu(), |
477 | + HasParent(parent.get()), MatchesAttachment(attachment_rect), MatchesEdge(edge)),_)); |
478 | + create_surface(menu_spec.get()); |
479 | } |
As already discussed, I don't think it's a good idea to make make creation functions type-specific. The result is we'll have exponentially more API functions this way, and higher coupling to type-specific semantics. But I lost that argument and so will abstain... although there is a cosmetic issue that needs fixing:
(1) Headers containing documentation that will be read by users as such should be wrapped within 80 columns so they display in users' terminals correctly: client/ mir_toolkit/ mir_surface. h
include/
We don't impose this standard on our own internal code, but the public stuff needs to be more consumable.