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

[Alberto Aguirre]

Add API to specify client menu surfaces (LP: #1324101)

Add API to specify client menu surfaces

[Daniel van Vugt (vanvugt)]

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

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2150
http://jenkins.qa.ubuntu.com/job/mir-ci/2389/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/500
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/500
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/474/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/386
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/386/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/474
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/474/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3661/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16552

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

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

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.

[Gerry Boland (gerboland)]

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

[Robert Carr (robertcarr)]

lgtm

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Looks good.

review: Approve

[Alan Griffiths (alan-griffiths)]

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)

[Alberto Aguirre (albaguirre)]

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

[Alberto Aguirre (albaguirre)]

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

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2153
http://jenkins.qa.ubuntu.com/job/mir-ci/2471/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/588
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/588
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/562
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/468
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/468/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/562
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/562/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3718
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16669

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

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

[Daniel van Vugt (vanvugt)]

review: Abstain

[Daniel van Vugt (vanvugt)]

Yes, this is the shell-specific semantics being baked into the client API I was warning about a while back :(

[Alberto Aguirre (albaguirre)]

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

[Chris Halse Rogers (raof)]

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

[Chris Halse Rogers (raof)]

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.

[Robert Carr (robertcarr)]

Seems like Chris is correct...

review: Needs Fixing

[Alberto Aguirre (albaguirre)]

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

[Robert Carr (robertcarr)]

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.

[Robert Carr (robertcarr)]

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

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2160
http://jenkins.qa.ubuntu.com/job/mir-ci/2608/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/786
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/786
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/748
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/605
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/605/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/748
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/748/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3877
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16974

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

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

[Daniel van Vugt (vanvugt)]

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

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

[Alan Griffiths (alan-griffiths)]

358 + ASSERT_TRUE(spec != nullptr);

Could be ASSERT_THAT(spec, NotNull());

review: Approve

[Alberto Aguirre (albaguirre)]

> 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

[Alan Griffiths (alan-griffiths)]

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

[Robert Carr (robertcarr)]

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

[Alberto Aguirre (albaguirre)]

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

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2165
http://jenkins.qa.ubuntu.com/job/mir-ci/2622/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/808
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/808/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/770/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/619/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/770/console

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

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

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2166
http://jenkins.qa.ubuntu.com/job/mir-ci/2625/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/813
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/813
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/775
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/622
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/622/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/775
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/775/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3900
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17018

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2167
http://jenkins.qa.ubuntu.com/job/mir-ci/2634/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/829
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/829
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/791
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/631
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/631/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/791
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/791/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3916
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17045

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

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

[Chris Halse Rogers (raof)]

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

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

=== modified file 'client-ABI-sha1sums'
--- client-ABI-sha1sums	2015-01-06 03:29:48 +0000
+++ client-ABI-sha1sums	2015-01-13 16:38:19 +0000
@@ -6,11 +6,11 @@
 d739af6e64db6b314a727b5fb00be662b98ccd57  include/client/mir_toolkit/mir_platform_message.h
 9d50df5a141ca03ee8a79f7e844ed4b8b3b7d5d3  include/client/mir_toolkit/mir_prompt_session.h
 21d07e655e85eeec8a3523e1c6f9c2252176ec01  include/client/mir_toolkit/mir_screencast.h
-82a2a3d219747a778284811ba0c01e0fdf167b30  include/client/mir_toolkit/mir_surface.h
+8ea06d0a56ef400ae7813312fd4edb8114a4632a  include/client/mir_toolkit/mir_surface.h
 b141c4d79802ad626d969249c0004744e5c2a525  include/client/mir_toolkit/mir_wait.h
 6f7b4ecc22afba923806ed2bd7d8244be90b0cfd  include/client/mir_toolkit/version.h
 3350dac884d6006753de2a955bc7a05663cd9449  include/common/mir_toolkit/client_types.h
-c7c708734715a6d1b6fb2652584adb912071a518  include/common/mir_toolkit/common.h
+20086d47f8747ba34f8d2c4aba75503adb347247  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h
 
=== modified file 'common-ABI-sha1sums'
--- common-ABI-sha1sums	2015-01-06 03:29:48 +0000
+++ common-ABI-sha1sums	2015-01-13 16:38:19 +0000
@@ -17,7 +17,7 @@
 31b9c24e2ce7194aeea6694e81c160354033d28a  include/common/mir/optional_value.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 3350dac884d6006753de2a955bc7a05663cd9449  include/common/mir_toolkit/client_types.h
-c7c708734715a6d1b6fb2652584adb912071a518  include/common/mir_toolkit/common.h
+20086d47f8747ba34f8d2c4aba75503adb347247  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h
 
=== modified file 'include/client/mir_toolkit/mir_surface.h'
--- include/client/mir_toolkit/mir_surface.h	2014-12-12 14:51:33 +0000
+++ include/client/mir_toolkit/mir_surface.h	2015-01-13 16:38:19 +0000
@@ -51,6 +51,40 @@
                                                               MirPixelFormat format);
 
 /**
+ * Create a surface specification for a menu surface.
+ *
+ * Positioning of the surface is specified with respect to the parent surface
+ * via an adjacency 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 or similar constraints. In addition, the server can use
+ * the edge affinity hint to consider only horizontal or only vertical adjacency
+ * edges in the given rectangle.
+ *
+ * \param [in] connection   Connection the surface will be created on
+ * \param [in] width        Requested width. The server is not guaranteed to
+ *                          return a surface of this width.
+ * \param [in] height       Requested height. The server is not guaranteed to
+ *                          return a surface of this height.
+ * \param [in] format       Pixel format for the surface.
+ * \param [in] parent       A valid parent surface for this menu.
+ * \param [in] rect         The adjacency rectangle. The server is not
+ *                          guaranteed to create a surface at the requested
+ *                          location.
+ * \param [in] edge         The preferred edge direction to attach to. Use
+ *                          mir_edge_attachment_any for no preference.
+ * \return                  A handle that can be passed to mir_surface_create()
+ *                          to complete construction.
+ */
+MirSurfaceSpec*
+mir_connection_create_spec_for_menu_surface(MirConnection* connection,
+                                            int width,
+                                            int height,
+                                            MirPixelFormat format,
+                                            MirSurface* parent,
+                                            MirRectangle* rect,
+                                            MirEdgeAttachment edge);
+
+/**
  * Create a surface from a given specification
  *
  *
 
=== modified file 'include/common/mir_toolkit/common.h'
--- include/common/mir_toolkit/common.h	2014-12-19 02:31:34 +0000
+++ include/common/mir_toolkit/common.h	2015-01-13 16:38:19 +0000
@@ -159,6 +159,13 @@
                                mir_orientation_mode_landscape_any
 } MirOrientationMode;
 
+typedef enum MirEdgeAttachment
+{
+    mir_edge_attachment_vertical = 1 << 0,
+    mir_edge_attachment_horizontal = 1 << 1,
+    mir_edge_attachment_any = mir_edge_attachment_vertical |
+                              mir_edge_attachment_horizontal
+} MirEdgeAttachment;
 /**@}*/
 
 #endif
 
=== modified file 'include/server/mir/scene/surface_creation_parameters.h'
--- include/server/mir/scene/surface_creation_parameters.h	2014-12-19 04:40:45 +0000
+++ include/server/mir/scene/surface_creation_parameters.h	2015-01-13 16:38:19 +0000
@@ -67,6 +67,10 @@
 
     SurfaceCreationParameters& with_parent_id(frontend::SurfaceId const& id);
 
+    SurfaceCreationParameters& with_attachment_rect(geometry::Rectangle const& rect);
+
+    SurfaceCreationParameters& with_edge_attachment(MirEdgeAttachment edge);
+
     std::string name;
     geometry::Size size;
     geometry::Point top_left;
@@ -80,6 +84,8 @@
     mir::optional_value<MirSurfaceType> type;
     mir::optional_value<MirOrientationMode> preferred_orientation;
     mir::optional_value<frontend::SurfaceId> parent_id;
+    mir::optional_value<geometry::Rectangle> attachment_rect;
+    mir::optional_value<MirEdgeAttachment> edge_attachment;
 };
 
 bool operator==(const SurfaceCreationParameters& lhs, const SurfaceCreationParameters& rhs);
 
=== modified file 'platform-ABI-sha1sums'
--- platform-ABI-sha1sums	2015-01-08 03:16:37 +0000
+++ platform-ABI-sha1sums	2015-01-13 16:38:19 +0000
@@ -17,7 +17,7 @@
 31b9c24e2ce7194aeea6694e81c160354033d28a  include/common/mir/optional_value.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 3350dac884d6006753de2a955bc7a05663cd9449  include/common/mir_toolkit/client_types.h
-c7c708734715a6d1b6fb2652584adb912071a518  include/common/mir_toolkit/common.h
+20086d47f8747ba34f8d2c4aba75503adb347247  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h
 
=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2015-01-08 03:16:37 +0000
+++ server-ABI-sha1sums	2015-01-13 16:38:19 +0000
@@ -17,7 +17,7 @@
 31b9c24e2ce7194aeea6694e81c160354033d28a  include/common/mir/optional_value.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 3350dac884d6006753de2a955bc7a05663cd9449  include/common/mir_toolkit/client_types.h
-c7c708734715a6d1b6fb2652584adb912071a518  include/common/mir_toolkit/common.h
+20086d47f8747ba34f8d2c4aba75503adb347247  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h
@@ -96,7 +96,7 @@
 5bab4dc0a6488b8b9d47e958c6bab94f1dcf2c57  include/server/mir/scene/surface_buffer_access.h
 bbc9e2e2bb71634cd6f1c5c0430093e10e74fa23  include/server/mir/scene/surface_configurator.h
 e5e4dd7bcaf186810043fa0f05be42d7e49b0843  include/server/mir/scene/surface_coordinator.h
-6d669f4235c2237fb35578671405db882b6aafba  include/server/mir/scene/surface_creation_parameters.h
+8e3636b736700d72520709c8a61f5e85d9231d4c  include/server/mir/scene/surface_creation_parameters.h
 7da48dbedb4430ce322a50ef8f4e93ce08bdf147  include/server/mir/scene/surface.h
 587e22d751656ce2d9536afdf5659276ff9bbc46  include/server/mir/scene/surface_observer.h
 7ef3e99901168cda296d74d05a979f47bf9c3ff1  include/server/mir/server_action_queue.h
 
=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2014-12-19 04:37:16 +0000
+++ src/client/mir_surface.cpp	2015-01-13 16:38:19 +0000
@@ -89,6 +89,14 @@
     SERIALIZE_OPTION_IF_SET(pref_orientation, message);
     if (parent.is_set() && parent.value() != nullptr)
         message.set_parent_id(parent.value()->id());
+    if (attachment_rect.is_set())
+    {
+        message.mutable_attachment_rect()->set_left(attachment_rect.value().left);
+        message.mutable_attachment_rect()->set_top(attachment_rect.value().top);
+        message.mutable_attachment_rect()->set_width(attachment_rect.value().width);
+        message.mutable_attachment_rect()->set_height(attachment_rect.value().height);
+    }
+    SERIALIZE_OPTION_IF_SET(edge_attachment, message);
 
     return message;
 }
 
=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2014-12-19 04:37:16 +0000
+++ src/client/mir_surface.h	2015-01-13 16:38:19 +0000
@@ -78,6 +78,8 @@
     mir::optional_value<MirOrientationMode> pref_orientation;
 
     mir::optional_value<MirSurface*> parent;
+    mir::optional_value<MirRectangle> attachment_rect;
+    mir::optional_value<MirEdgeAttachment> edge_attachment;
 };
 
 struct MirSurface : public mir::client::ClientSurface
 
=== modified file 'src/client/mir_surface_api.cpp'
--- src/client/mir_surface_api.cpp	2015-01-06 03:29:48 +0000
+++ src/client/mir_surface_api.cpp	2015-01-13 16:38:19 +0000
@@ -51,6 +51,25 @@
     return new MirSurfaceSpec{connection, width, height, format};
 }
 
+MirSurfaceSpec* mir_connection_create_spec_for_menu_surface(MirConnection* connection,
+                                                            int width,
+                                                            int height,
+                                                            MirPixelFormat format,
+                                                            MirSurface* parent,
+                                                            MirRectangle* rect,
+                                                            MirEdgeAttachment edge)
+{
+    mir::require(mir_surface_is_valid(parent));
+    mir::require(rect != nullptr);
+
+    auto spec = new MirSurfaceSpec{connection, width, height, format};
+    spec->type = mir_surface_type_menu;
+    spec->parent = parent;
+    spec->attachment_rect = *rect;
+    spec->edge_attachment = edge;
+    return spec;
+}
+
 MirSurface* mir_surface_create_sync(MirSurfaceSpec* requested_specification)
 {
     MirSurface* surface = nullptr;
 
=== modified file 'src/client/symbols.map'
--- src/client/symbols.map	2014-12-12 14:51:33 +0000
+++ src/client/symbols.map	2015-01-13 16:38:19 +0000
@@ -33,4 +33,8 @@
     mir_surface_set_preferred_orientation;
     mir_surface_get_preferred_orientation;
     mir_surface_spec_set_preferred_orientation;
-} MIR_CLIENT_8.1;
 \ No newline at end of file
+} MIR_CLIENT_8.1;
+
+MIR_CLIENT_8.3 {
+    mir_connection_create_spec_for_menu_surface;
+} MIR_CLIENT_8.2;
 \ No newline at end of file
 
=== removed file 'src/include/common/mir/require.h'
--- src/include/common/mir/require.h	2014-12-19 05:01:58 +0000
+++ src/include/common/mir/require.h	1970-01-01 00:00:00 +0000
@@ -1,32 +0,0 @@
-/*
- * Copyright © 2014 Canonical Ltd.
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Lesser General Public License version 3 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- * Authored by: Alberto Aguirre <[e-mail]>
- */
-
-#ifndef MIR_REQUIRE_H_
-#define MIR_REQUIRE_H_
-
-#include <cstdlib>
-
-namespace mir
-{
-inline void require(bool const expr)
-{
-    if (!expr) std::abort();
-}
-}
-
-#endif /* MIR_REQUIRE_H_ */
 
=== modified file 'src/protobuf/mir_protobuf.proto'
--- src/protobuf/mir_protobuf.proto	2014-12-19 04:38:08 +0000
+++ src/protobuf/mir_protobuf.proto	2015-01-13 16:38:19 +0000
@@ -24,6 +24,9 @@
   optional int32 state = 8;
   optional int32 pref_orientation = 9;
   optional int32 parent_id = 10;
+  
+  optional Rectangle attachment_rect = 11;
+  optional int32 edge_attachment = 12;
 }
 
 message SurfaceId {
 
=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2014-12-19 04:40:45 +0000
+++ src/server/frontend/session_mediator.cpp	2015-01-13 16:38:19 +0000
@@ -198,6 +198,17 @@
     if (request->has_parent_id())
         params.with_parent_id(SurfaceId{request->parent_id()});
 
+    if (request->has_attachment_rect())
+    {
+        params.with_attachment_rect(geom::Rectangle{
+            {request->attachment_rect().left(), request->attachment_rect().top()},
+            {request->attachment_rect().width(), request->attachment_rect().height()}
+        });
+    }
+
+    if (request->has_edge_attachment())
+        params.with_edge_attachment(static_cast<MirEdgeAttachment>(request->edge_attachment()));
+
     auto const surf_id = session->create_surface(params);
 
     auto surface = session->get_surface(surf_id);
 
=== modified file 'src/server/scene/surface_creation_parameters.cpp'
--- src/server/scene/surface_creation_parameters.cpp	2014-12-19 04:40:45 +0000
+++ src/server/scene/surface_creation_parameters.cpp	2015-01-13 16:38:19 +0000
@@ -124,6 +124,18 @@
     return *this;
 }
 
+ms::SurfaceCreationParameters& ms::SurfaceCreationParameters::with_attachment_rect(geometry::Rectangle const& rect)
+{
+    attachment_rect = rect;
+    return *this;
+}
+
+ms::SurfaceCreationParameters& ms::SurfaceCreationParameters::with_edge_attachment(MirEdgeAttachment edge)
+{
+    edge_attachment = edge;
+    return *this;
+}
+
 bool ms::operator==(
     const SurfaceCreationParameters& lhs,
     const ms::SurfaceCreationParameters& rhs)
 
=== modified file 'tests/acceptance-tests/test_client_surfaces.cpp'
--- tests/acceptance-tests/test_client_surfaces.cpp	2014-12-17 15:20:40 +0000
+++ tests/acceptance-tests/test_client_surfaces.cpp	2015-01-13 16:38:19 +0000
@@ -21,6 +21,7 @@
 
 #include "mir_test_framework/connected_client_headless_server.h"
 #include "mir_test_framework/any_surface.h"
+#include "mir_test/validity_matchers.h"
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -190,7 +191,7 @@
 TEST_P(WithOrientation, have_requested_preferred_orientation)
 {
     auto spec = mir_connection_create_spec_for_normal_surface(connection, 1, 1, mir_pixel_format_abgr_8888);
-    ASSERT_TRUE(spec != nullptr);
+    ASSERT_THAT(spec, NotNull());
 
     MirOrientationMode mode{GetParam()};
     mir_surface_spec_set_preferred_orientation(spec, mode);
@@ -198,7 +199,7 @@
     auto surface = mir_surface_create_sync(spec);
     mir_surface_spec_release(spec);
 
-    ASSERT_TRUE(mir_surface_is_valid(surface));
+    ASSERT_THAT(surface, IsValid());
     EXPECT_EQ(mir_surface_get_preferred_orientation(surface), mode);
 
     mir_surface_release_sync(surface);
@@ -211,3 +212,22 @@
         mir_orientation_mode_portrait_any, mir_orientation_mode_landscape_any,
         mir_orientation_mode_any));
 
+TEST_F(ClientSurfaces, can_be_menus)
+{
+    auto parent = mtf::make_any_surface(connection);
+    MirRectangle attachment_rect{100, 200, 100, 100};
+
+    auto spec = mir_connection_create_spec_for_menu_surface(connection, 640, 480,
+        mir_pixel_format_abgr_8888, parent, &attachment_rect, mir_edge_attachment_vertical);
+    ASSERT_THAT(spec, NotNull());
+
+    auto menu = mir_surface_create_sync(spec);
+    mir_surface_spec_release(spec);
+
+    ASSERT_THAT(menu, IsValid());
+    EXPECT_EQ(mir_surface_get_type(menu), mir_surface_type_menu);
+
+    mir_surface_release_sync(parent);
+    mir_surface_release_sync(menu);
+}
+
 
=== modified file 'tests/integration-tests/client/test_mirsurface.cpp'
--- tests/integration-tests/client/test_mirsurface.cpp	2014-12-19 04:42:39 +0000
+++ tests/integration-tests/client/test_mirsurface.cpp	2015-01-13 16:38:19 +0000
@@ -19,6 +19,7 @@
 #include "mir_test_framework/connected_client_headless_server.h"
 #include "mir_test_doubles/stub_scene_surface.h"
 #include "mir_test/fake_shared.h"
+#include "mir_test/validity_matchers.h"
 
 #include "mir/scene/surface.h"
 #include "mir/scene/surface_creation_parameters.h"
@@ -73,6 +74,31 @@
            parent_field_matches(arg, spec->parent);
 }
 
+MATCHER(IsAMenu, "")
+{
+    return arg.type == mir_surface_type_menu;
+}
+
+MATCHER_P(HasParent, parent, "")
+{
+    return arg.parent_id.is_set() && arg.parent_id.value().as_value() == parent->id();
+}
+
+MATCHER_P(MatchesAttachment, rect, "")
+{
+    return arg.attachment_rect.is_set() &&
+           arg.attachment_rect.value().top_left.x.as_int() == rect.left &&
+           arg.attachment_rect.value().top_left.y.as_int() == rect.top &&
+           arg.attachment_rect.value().size.width.as_uint32_t() == rect.width &&
+           arg.attachment_rect.value().size.height.as_uint32_t() == rect.height;
+}
+
+MATCHER_P(MatchesEdge, edge, "")
+{
+    return arg.edge_attachment.is_set() &&
+           arg.edge_attachment.value() == edge;
+}
+
 }
 
 struct ClientMirSurface : mtf::ConnectedClientHeadlessServer
@@ -103,9 +129,9 @@
         spec.pref_orientation = mir_orientation_mode_landscape;
     }
 
-    std::unique_ptr<MirSurface, std::function<void(MirSurface*)>> create_surface()
+    std::unique_ptr<MirSurface, std::function<void(MirSurface*)>> create_surface(MirSurfaceSpec* spec)
     {
-        return {mir_surface_create_sync(&spec), [](MirSurface *surf){mir_surface_release_sync(surf);}};
+        return {mir_surface_create_sync(spec), [](MirSurface *surf){mir_surface_release_sync(surf);}};
     }
 
     MirSurfaceSpec spec{nullptr, 640, 480, mir_pixel_format_abgr_8888};
@@ -118,10 +144,10 @@
 {
     EXPECT_CALL(*mock_surface_coordinator, add_surface(AllOf(MatchesRequired(&spec), MatchesOptional(&spec)),_));
 
-    auto surf = create_surface();
+    auto surf = create_surface(&spec);
 
     // A valid surface is needed to be specified as a parent
-    ASSERT_TRUE(mir_surface_is_valid(surf.get()));
+    ASSERT_THAT(surf.get(), IsValid());
     spec.parent = surf.get();
 
     // The second time around we don't care if the surface gets created,
@@ -130,5 +156,29 @@
     spec.output_id = arbitrary_output_id;
 
     EXPECT_CALL(*mock_surface_coordinator, add_surface(MatchesOptional(&spec),_));
-    create_surface();
+    create_surface(&spec);
+}
+
+TEST_F(ClientMirSurface, as_menu_sends_correct_params)
+{
+    EXPECT_CALL(*mock_surface_coordinator, add_surface(_,_));
+    auto parent = create_surface(&spec);
+    ASSERT_THAT(parent.get(), IsValid());
+
+    MirRectangle attachment_rect{100, 200, 300, 400};
+    MirEdgeAttachment edge{mir_edge_attachment_horizontal};
+
+    auto spec_deleter = [](MirSurfaceSpec* spec) {mir_surface_spec_release(spec);};
+    std::unique_ptr<MirSurfaceSpec, decltype(spec_deleter)> menu_spec{
+        mir_connection_create_spec_for_menu_surface(connection, 640, 480,
+            mir_pixel_format_abgr_8888, parent.get(), &attachment_rect,
+            edge),
+        spec_deleter
+    };
+
+    ASSERT_THAT(menu_spec, NotNull());
+
+    EXPECT_CALL(*mock_surface_coordinator, add_surface(AllOf(IsAMenu(),
+        HasParent(parent.get()), MatchesAttachment(attachment_rect), MatchesEdge(edge)),_));
+    create_surface(menu_spec.get());
 }