Mir

Merge lp:~albaguirre/mir/add-menu-api into lp:mir

Proposed by Alberto Aguirre
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
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

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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:
   include/client/mir_toolkit/mir_surface.h

We don't impose this standard on our own internal code, but the public stuff needs to be more consumable.

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

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/maximized/min), position (x,y relative to parent), decoration style (client or server decorations), min/max width/height, window title...

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.

Revision history for this message
Gerry Boland (gerboland) wrote :

https://lists.ubuntu.com/archives/mir-devel/2014-November/000961.html has been brought to my attention. I won't resurrect the argument, but it is pity the main voice of dissent was missing from that meeting.

The API design does makes sense, but I'm glad I'm not maintaining such an API. Objection withdrawn

Revision history for this message
Robert Carr (robertcarr) wrote :

lgtm

review: Approve
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

Looks good.

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

53 + if (!mir_surface_is_valid(parent)) abort();
54 + if (!rect) abort();

I think we need mir::require(mir_surface_is_valid(parent)) or MIR_REQUIRE(mir_surface_is_valid(parent)) (or similar)

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> 53 + if (!mir_surface_is_valid(parent)) abort();
> 54 + if (!rect) abort();
>
> I think we need mir::require(mir_surface_is_valid(parent)) or
> MIR_REQUIRE(mir_surface_is_valid(parent)) (or similar)

Done.

Revision history for this message
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/client/mir_toolkit/mir_surface.h
>
> 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.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
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_create_spec_for_menu_surface(connection, width, height, parent, adjacency_rect, format)?

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) :
review: Abstain
Revision history for this message
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 :(

Revision history for this message
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_create_spec_for_menu_surface(connection, width, height, parent,
> adjacency_rect, format)?

I don't understand what adjacency rect is supposed to represent, could you elaborate?

Revision history for this message
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 |
| |
+----------------+

---------------------- Screen edge ------------------

Revision history for this message
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/left-edge rather than top-edge/bottom-edge.

Revision history for this message
Robert Carr (robertcarr) wrote :

Seems like Chris is correct...

review: Needs Fixing
Revision history for this message
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_create_spec_for_menu_surface(connection, width, height, format, parent, attachment_rect, preferred_edge_direction)

Revision history for this message
Robert Carr (robertcarr) wrote :

This is strange but I would expect the c'tor arguments to be ordered otherwise:
create_spec_for_menu_surface(connection, parent, rect, direction, width, height, format)

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...on...in...due....or...other"

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.

Revision history for this message
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.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Chris Halse Rogers (raof) wrote :

Tests have some
+ ASSERT_TRUE(mir_surface_is_valid(menu));
this would be better with mir_test/validity_matchers.h and:
+ ASSERT_THAT(menu, IsValid())
(this gets you a helpful message when the assertion fails

What are the semantics of mir_edge_attachment_none? I don't think that value makes sense?

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_attachment_none mean?” Everything else is nice-to-have.

review: Needs Fixing
Revision history for this message
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.

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> This is strange but I would expect the c'tor arguments to be ordered
> otherwise:
> create_spec_for_menu_surface(connection, parent, rect, direction, width,
> 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_create_spec_for_normal_surface

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

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

358 + ASSERT_TRUE(spec != nullptr);

Could be ASSERT_THAT(spec, NotNull());

review: Approve
Revision history for this message
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->create_surface(params) instead of session mediator itself, which is now possible since optional_values are used in SurfaceCreationParameters

Revision history for this message
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->create_surface(params) instead of session mediator itself, which is
> now possible since optional_values are used in SurfaceCreationParameters

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.

Revision history for this message
Robert Carr (robertcarr) wrote :

>> I think that should be the responsibility of the implementation of session->create_surface(params) >> instead of session mediator itself, which is now possible since optional_values are used in
>> SurfaceCreationParameters

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

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> Tests have some
> + ASSERT_TRUE(mir_surface_is_valid(menu));
> this would be better with mir_test/validity_matchers.h and:
> + ASSERT_THAT(menu, IsValid())
> (this gets you a helpful message when the assertion fails
>
>
Done.

> What are the semantics of mir_edge_attachment_none? I don't think that value
> makes sense?
Changed the name to any - meaning both horizonal and vertical edge attachment is ok.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Chris Halse Rogers (raof) wrote :

Um, now mir_edge_attachment_any != mir_edge_attachment_horizontal | mir_edge_attachment_vertical, but means the same thing :(.

I think it should either be removed, or specified as equal to mir_edge_attachment_horizontal | mir_edge_attachment_vertical. I lean slightly towards the latter.

@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_attachment_any. It's possible we want to name the surface type differently. ☺

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.

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

> Um, now mir_edge_attachment_any != mir_edge_attachment_horizontal |
> mir_edge_attachment_vertical, but means the same thing :(.
>
> I think it should either be removed, or specified as equal to
> mir_edge_attachment_horizontal | mir_edge_attachment_vertical. I lean slightly
> 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.

Revision history for this message
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.

Revision history for this message
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 SurfaceCreationParameters into NormalSurfaceCreationParameters, MenuSurfaceCreationParameters, etc and explicitly asking the shell to create a normal, menu, etc surface.

Could you update the mir_connection_create_spec_for_menu_surface doc with a big INPUT SEMANTICS TBD warning so we don't forget to update it?

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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 }

Subscribers

People subscribed via source and target branches