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

[Daniel van Vugt]

Introduce a client API for surface parenting.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2042
http://jenkins.qa.ubuntu.com/job/mir-ci/2024/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/13
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/13/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

===
28 +MirWaitHandle* mir_surface_set_parent(MirSurface* surface, MirSurface* parent);
===

So in the previous MP there was some points of discussion:

1. mir_surface_set_parent needing to accept a callback+context pointer - my take is since we already receive surface events it's not needed in the api.

2. The bigger issue - No semantic meaning in setting a parent (at least not yet).
So essentially the discussion is do we favor for example for specifying dialogs/popups

A -

set_parent(...)
set_surface_type(...)
set_relative_position(...)

or B as Chris suggests:

set_dialog(surface, parent)
set_popup(surface, parent, relative_position)

I favor B as it has the semantic information we need to implement for our immediate goals and it limits error checking the state combinations of A.

Not dissaprove as parts of this are still useful for approach B above.

review: Needs Fixing

[Robert Carr (robertcarr)]

I didn't look for programming or style errors.

At a high level goal though im told our target is to enable menus next week and so this branch wont be enough as it lacks child surface positioning.

Of course it would be possible to implement menus with this branch and a little more:
mir_surface_set_parent(me, parent)
mir_surface_set_relative_position(me, parent, 17, 34)
mir_surface_set_type(menu)
mir_surface_realise(me)

It raises some debate: Can parents be changed? What happens if I set a relative position but am parented? Should mir_surface_set_parent and set_relative_position be combined? Should you be able to set a parent without a relative position! What should these functions be called? Is setting a menu type without a parent an error or does the shell simply fail to change type for you? Does the creation of these sort of attribute dances effectively mean creating an implied Window Management protocol which starts to look very Xey.

Ultimately I think all these questions do have answers but they are not very interesting. Its much more interesting to have menus next week and I think chriss proposal of mir_surface_create_menu, etc family of functions lets us move quickest. mir_surface_set_parent, etc, could be added later.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alberto:
I strongly disagree. Your proposed API is not only more complex but also less capable, more redundant and more sensitive to ABI breaks. Redundant because set_dialog is the same as set_type(dialog). And redundant because relative positioning is useful to things other than just popups (e.g. the proposed decoration design where the titlebar is also a relatively positioned subsurface). More sensitive because the type "dialog" is just the current design open to change. When things do change like "dialog" you don't want to be deprecating entire functions with the type their names.

The API design proposed here is simpler, more flexible, has zero redundancy and less sensitive to ABI breaks. Remember we're not designing a toolkit with fixed semantics, but an API to support _any_ toolkit.

Robert:
Can parents be changed? Yes, see GTK. And even still, it's not sensible to impose arbitrary limitations without good reason.
What happens if I set a relative position but am (not?) parented? Am: You get positioned; Not: You don't get positioned. But also that question is not relevant to this proposal.
Should you be able to set a parent without a relative position!? Yes, two immediate use cases at least: floating toolboxes, indicators (auxiliary surfaces design doc).
What should these functions be called? Intentionally not a question to be answered in this small proposal.
Is setting a menu type without a parent an error or does the shell simply fail to change type for you? Semantics are not fully defined in Mir itself. We're open to letting the shell enfoce that (as all shells/toolkits are different which is a good thing).
Does the creation of these sort of attribute dances effectively mean creating an implied Window Management protocol which starts to look very Xey? No, we are designing an API to support toolkits.
"mir_surface_create_menu" is inappropriate for reasons mentioned at the start of this comment.

[Daniel van Vugt (vanvugt)]

A more visual explanation:

sometoolkit_create_menu(conn, window, x, y):
    surf = mir_connection_create_surface_sync(conn, parms);
    mir_surface_set_type(surf, mir_surface_type_popover);
    mir_surface_set_parent(surf, window);
    mir_surface_place_relative(surf, x, y);
    return surf;

So you see there are two important benefits:
  1. The Mir API is minimal in size (in fact intentionally normalized [1])
  2. The Mir API is maximally flexible; Mir doesn't need to know what a "menu" is. The toolkit is free to define whatever it likes.
[1] http://en.wikipedia.org/wiki/Database_normalization

It's also important to remember that our design documents detail the design of the Unity shell. Mir needs to support more than just Unity.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2044
http://jenkins.qa.ubuntu.com/job/mir-ci/2033/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/29
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/29
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/8/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/22/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/8
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/8/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3304/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15540

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Failure is lp:1390388

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2045
http://jenkins.qa.ubuntu.com/job/mir-ci/2050/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/47
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/47
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/27
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/39
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/39/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/27
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/27/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3320
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15608

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

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

> A more visual explanation:
>
> sometoolkit_create_menu(conn, window, x, y):
> surf = mir_connection_create_surface_sync(conn, parms);
> mir_surface_set_type(surf, mir_surface_type_popover);
> mir_surface_set_parent(surf, window);
> mir_surface_place_relative(surf, x, y);
> return surf;
>
> So you see there are two important benefits:
> 1. The Mir API is minimal in size (in fact intentionally normalized [1])
> 2. The Mir API is maximally flexible; Mir doesn't need to know what a "menu"
> is. The toolkit is free to define whatever it likes.
> [1] http://en.wikipedia.org/wiki/Database_normalization

The above also means that menus do not exist. Instead there will only be a combination of flags that might yield a related outcome. But the client/toolkit never states that intent. If a shell plans to implement a specific behavior or look for menues it will end up searching for those combinations of flags. Additionally menues and other popovers might not even be distinguishable based on the flags provided by the client/toolkit.

On the other hand of course I do like the idea of defining a menu as a set of behavioral flags that will be independently interpreted by different components in the shell. But thats an implementation detail on the server.

Still in a distant future we might decide to expose such things - a soon as they are or can be relatively well defined - to the client api, but I do think that we then still need a client API to declare the semantics or intent of a surface. Both things may coexist but I think the latter has a higher priority and is already in a well defined state (modulo permanent changes).

review: Needs Fixing

[Cemil Azizoglu (cemil-azizoglu)]

+1 for a semantics-based API as opposed to a state-based one. I wouldn't want to have to use an API where there are hundreds of states to choose from and a small subset of each has some special meaning (a menu, a tooltip, a dialog, etc) and other combinations are not even valid - giving too much flexibility to the user to shoot themselves in the foot.

I'd like to be able to just call one function to create, say, a menu. Simple enough for the user to get started using our API, simple enough for us to manage. Of course, as the need arises, we can later break down the API and provide finer level of control as is being proposed here. We can do this on a case-by-case basis as we gain some insight into what users want to achieve and where it makes sense to have more flexibility. It's easy, and backwards-compatible, to do that. However, it's not easy to take it back cf. server ABI.

So IMO the API that is being proposed here is rather the implementation and could/should stay as an internal API, which could then be made public, with some extra state validation, if needed.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> +1 for a semantics-based API as opposed to a state-based one. I wouldn't want
> to have to use an API where there are hundreds of states to choose from and a
> small subset of each has some special meaning (a menu, a tooltip, a dialog,
> etc) and other combinations are not even valid - giving too much flexibility
> to the user to shoot themselves in the foot.
>
> I'd like to be able to just call one function to create, say, a menu. Simple
> enough for the user to get started using our API, simple enough for us to
> manage. Of course, as the need arises, we can later break down the API and
> provide finer level of control as is being proposed here. We can do this on a
> case-by-case basis as we gain some insight into what users want to achieve and
> where it makes sense to have more flexibility. It's easy, and backwards-
> compatible, to do that. However, it's not easy to take it back cf. server ABI.
>
> So IMO the API that is being proposed here is rather the implementation and
> could/should stay as an internal API, which could then be made public, with
> some extra state validation, if needed.

If we have such an "internal API" in the client code I think we lose any benefit of having a "semantics-based API". The value I can see from in the latter approach would come from passing the semantic information "this is a menu" to the server so that the shell can implement policies to control any fine grained state.

[Cemil Azizoglu (cemil-azizoglu)]

> If we have such an "internal API" in the client code I think we lose any
> benefit of having a "semantics-based API". The value I can see from in the
> latter approach would come from passing the semantic information "this is a
> menu" to the server so that the shell can implement policies to control any
> fine grained state.

No I didn't mean it as a client-side API. The entry points of the client API defined by this MP would have corresponding functions on the server-side, which collectively implement the, say 'menu', functionality. I realize that it's confusing.. So feel free to scratch that comment altogether :-).

[Daniel van Vugt (vanvugt)]

More details and much more elaborate explanations are now duplicated on the mailing list: mir-devel. So I'll resist repeating myself here.

[Daniel van Vugt (vanvugt)]

I've just reviewed the Unity design doc with the misleading title "MIR: Surface Management v0.2" (an issue I've raised before because it's actually the design of Unity8, not Mir). It outlines 8 surface types/roles and 7 of them can have parents. Some required and some optional parenting.

So despite the fact I said I was going to look for a compromise, the design doc actually supports this current proposal rather strongly. Certainly we don't want to write 7 different set_parent client functions. Best case then is you've made the API 7 times more complex and still strictly support only one shell. More client functions might then be required for other shells or other surface types. It doesn't scale.

The design docs also detail complex state transitions and all the various combinations allowed. It's very complex for both surface_state and surface_type combinations, in concert with surface position and size etc. Some of these attributes whose consistency needs to be maintained for valid states exist only on the server side. So the client API can't enforce everything by itself obviously. You always have to "ask" the shell if any setting is allowed or has been accepted.

State validity and transitions will indeed be implemented on the server side because they're defined by the shell. There might be some sane defaults (e.g. Set state_fullscreen results in a resize), however the shell will have the power to wrap any state changes to guarantee its own desired semantics. And semantics may change at runtime; e.g. Unity8 in the phone form-factor only supports the "maximized" state for apps.

Back on the "menu" topic, I've proposed that as a real surface type today, separately. However since "menu" is only one of the seven Unity surface types that need parenting, and only some of those use parenting for relative positioning, it's clear we need a generic mir_surface_set_parent as a minimum.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2047
http://jenkins.qa.ubuntu.com/job/mir-ci/2065/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/68
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/68
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/48
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/54
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/54/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/48
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/48/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3340
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15669

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

We've rejected this approach to API design.

https://lists.ubuntu.com/archives/mir-devel/2014-November/000961.html

review: Disapprove

[Daniel van Vugt (vanvugt)]

I do make mistakes... And am happy to admit when I do.

However nobody has yet disproved (or even replied to) any of the logical arguments I have made thus far.

I am not always right. But no one has yet disproved anything I have said on this topic so far.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2048
http://jenkins.qa.ubuntu.com/job/mir-ci/2073/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/81
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/81
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/61
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/62
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/62/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/61
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/61/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3352
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15719

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2050
http://jenkins.qa.ubuntu.com/job/mir-ci/2104/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/124
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/124
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/104
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/93
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/93/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/104
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/104/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3392
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15826

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

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

I believe this horse has been beaten to death - discussed over email, in this MP, and on hangout with the architect. Rejecting.

review: Disapprove

[Daniel van Vugt (vanvugt)]

It's the only working solution right now for an important feature.

I'll keep the review up till someone proposes a better solution (or lands a worse one :)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2051
http://jenkins.qa.ubuntu.com/job/mir-ci/2113/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/118
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/102
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/102/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/118
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/118/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3406
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15860

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Alright, if someone decides later the alternate solutions are too painful then this branch is still here if you want it.

But I'll hide it again for now...

Unmerged revisions

2051. By Daniel van Vugt on 2014-11-17

Merge latest paren branch (and trunk) and fix a conflict.

2050. By Daniel van Vugt on 2014-11-14

Merge latest parent branch including trunk

2049. By Daniel van Vugt on 2014-11-13

Merge latest parent branch

2048. By Daniel van Vugt on 2014-11-12

Merge latest parent branch.

2047. By Daniel van Vugt on 2014-11-11

Merge latest parent (resolve criss-cross)

2046. By Daniel van Vugt on 2014-11-11

Merge latest trunk and fix a conflict

2045. By Daniel van Vugt on 2014-11-10

Merge latest parent branch (and trunk)

2044. By Daniel van Vugt on 2014-11-07

Merge latest parent branch

2043. By Daniel van Vugt on 2014-11-07

Merge latest parent branch (and trunk)

2042. By Daniel van Vugt on 2014-11-06

Merge latest parent branch (test enhancements only)

=== modified file 'client-ABI-sha1sums'
--- client-ABI-sha1sums	2014-11-14 03:03:37 +0000
+++ client-ABI-sha1sums	2014-11-14 03:03:37 +0000
@@ -5,7 +5,7 @@
 1ef8f51a3e3f8d1559266c5af58fbfde7cfabf0a  include/client/mir_toolkit/mir_cursor_configuration.h
 6ff12425fed19f2a5aa37390904c14ff6a647bcd  include/client/mir_toolkit/mir_prompt_session.h
 21d07e655e85eeec8a3523e1c6f9c2252176ec01  include/client/mir_toolkit/mir_screencast.h
-8609754db3be20e11e43858dd2c36b5bd480d5ec  include/client/mir_toolkit/mir_surface.h
+a77d1b2ae1c15944477512cee129d5746890239d  include/client/mir_toolkit/mir_surface.h
 b141c4d79802ad626d969249c0004744e5c2a525  include/client/mir_toolkit/mir_wait.h
 9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
 77a5955e53e1901766372b4fb29e34872ef62667  include/common/mir_toolkit/common.h

=== modified file 'include/client/mir_toolkit/mir_surface.h'
--- include/client/mir_toolkit/mir_surface.h	2014-10-01 06:25:56 +0000
+++ include/client/mir_toolkit/mir_surface.h	2014-11-14 03:03:37 +0000
@@ -287,6 +287,22 @@
  */
 MirOrientation mir_surface_get_orientation(MirSurface *surface);
 
+/**
+ * Set the parent of a surface.
+ *   \param [in] surface  The child surface that is being parented
+ *   \param [in] parent   The new parent surface (or NULL if none)
+ *   \return              A wait handle that is signaled when the operation
+ *                        has completed.
+ */
+MirWaitHandle* mir_surface_set_parent(MirSurface* surface, MirSurface* parent);
+
+/**
+ * Get the parent of a surface.
+ *   \param [in] surface  The surface being queried
+ *   \return              The parent surface (or NULL if none).
+ */
+MirSurface* mir_surface_get_parent(MirSurface* surface);
+
 #ifdef __cplusplus
 }
 /**@}*/

=== modified file 'src/client/connection_surface_map.h'
--- src/client/connection_surface_map.h	2014-10-01 06:25:56 +0000
+++ src/client/connection_surface_map.h	2014-11-14 03:03:37 +0000
@@ -36,6 +36,7 @@
     ~ConnectionSurfaceMap() noexcept;
 
     void with_surface_do(int surface_id, std::function<void(MirSurface*)> exec) const override;
+    MirSurface* find(int surface_id) const noexcept override;
     void insert(int surface_id, MirSurface* surface);
     void erase(int surface_id);
 

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2014-11-11 15:50:11 +0000
+++ src/client/mir_connection.cpp	2014-11-14 03:03:37 +0000
@@ -549,3 +549,8 @@
 {
     return logger;
 }
+
+MirSurface* MirConnection::get_surface(int id) const noexcept
+{
+    return surface_map->find(id);
+}

=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h	2014-10-28 00:29:39 +0000
+++ src/client/mir_connection.h	2014-11-14 03:03:37 +0000
@@ -136,6 +136,9 @@
     mir::protobuf::DisplayServer& display_server();
     std::shared_ptr<mir::logging::Logger> const& the_logger() const;
 
+    /// Find a surface by id, or return null.
+    MirSurface* get_surface(int id) const noexcept;
+
 private:
     // MUST be first data member so it is destroyed last.
     struct Deregisterer

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2014-11-11 15:50:11 +0000
+++ src/client/mir_surface.cpp	2014-11-14 03:03:37 +0000
@@ -171,6 +171,12 @@
     return surface.id().value();
 }
 
+MirSurface* MirSurface::parent() const noexcept
+{
+    std::lock_guard<decltype(mutex)> lock(mutex);
+    return connection->get_surface(attrib_cache[mir_surface_attrib_parent]);
+}
+
 bool MirSurface::is_valid(MirSurface* query)
 {
     std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
@@ -487,6 +493,7 @@
         case mir_surface_attrib_focus:
         case mir_surface_attrib_swapinterval:
         case mir_surface_attrib_dpi:
+        case mir_surface_attrib_parent:
             if (configure_result.has_ivalue())
                 attrib_cache[a] = configure_result.ivalue();
             else

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2014-11-11 15:50:11 +0000
+++ src/client/mir_surface.h	2014-11-14 03:03:37 +0000
@@ -87,6 +87,7 @@
     MirSurfaceParameters get_parameters() const;
     char const * get_error_message();
     int id() const;
+    MirSurface* parent() const noexcept;
     MirWaitHandle* next_buffer(mir_surface_callback callback, void * context);
     MirWaitHandle* get_create_wait_handle();
 

=== modified file 'src/client/mir_surface_api.cpp'
--- src/client/mir_surface_api.cpp	2014-11-11 15:50:11 +0000
+++ src/client/mir_surface_api.cpp	2014-11-14 03:03:37 +0000
@@ -332,3 +332,25 @@
 
     return result;
 }
+
+MirWaitHandle* mir_surface_set_parent(MirSurface* surf, MirSurface* parent)
+{
+    MirWaitHandle *result = nullptr;
+    try
+    {
+        if (surf && (!parent || MirSurface::is_valid(parent)))
+        {
+            int parent_id = parent ? parent->id() : mir_surface_parent_none;
+            result = surf->configure(mir_surface_attrib_parent, parent_id);
+        }
+    }
+    catch (std::exception const&)
+    {
+    }
+    return result;
+}
+
+MirSurface* mir_surface_get_parent(MirSurface* surf)
+{
+    return surf ? surf->parent() : nullptr;
+}

=== modified file 'src/client/surface_map.cpp'
--- src/client/surface_map.cpp	2014-10-01 06:25:56 +0000
+++ src/client/surface_map.cpp	2014-11-14 03:03:37 +0000
@@ -62,6 +62,13 @@
     }
 }
 
+MirSurface* mcl::ConnectionSurfaceMap::find(int surface_id) const noexcept
+{
+    std::lock_guard<std::mutex> lk(guard);
+    auto const it = surfaces.find(surface_id);
+    return (it != surfaces.end()) ? it->second : nullptr;
+}
+
 void mcl::ConnectionSurfaceMap::insert(int surface_id, MirSurface* surface)
 {
     std::lock_guard<std::mutex> lk(guard);

=== modified file 'src/client/surface_map.h'
--- src/client/surface_map.h	2013-12-17 17:55:59 +0000
+++ src/client/surface_map.h	2014-11-14 03:03:37 +0000
@@ -33,6 +33,7 @@
 public:
     virtual void with_surface_do(
         int surface_id, std::function<void(MirSurface*)> exec) const = 0;
+    virtual MirSurface* find(int surface_id) const noexcept = 0;
 
 protected:
     virtual ~SurfaceMap() = default;

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2014-11-11 17:46:49 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2014-11-14 03:03:37 +0000
@@ -329,6 +329,49 @@
     mir_connection_release(connection);
 }
 
+TEST_F(ClientLibrary, surface_parenting)
+{
+    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+    ASSERT_TRUE(mir_connection_is_valid(connection));
+
+    MirSurfaceParameters const parm =
+    {
+        __PRETTY_FUNCTION__,
+        640, 480,
+        mir_pixel_format_abgr_8888,
+        mir_buffer_usage_hardware,
+        mir_display_output_id_invalid
+    };
+
+    auto parent = mir_connection_create_surface_sync(connection, &parm);
+    ASSERT_TRUE(mir_surface_is_valid(parent));
+
+    auto child = mir_connection_create_surface_sync(connection, &parm);
+    ASSERT_TRUE(mir_surface_is_valid(child));
+
+    EXPECT_EQ(NULL, mir_surface_get_parent(child));
+
+    mir_wait_for(mir_surface_set_parent(child, parent));
+    EXPECT_EQ(parent, mir_surface_get_parent(child));
+
+    int foo;
+    auto invalid_surface = reinterpret_cast<MirSurface*>(&foo);
+    mir_wait_for(mir_surface_set_parent(child, invalid_surface));
+    EXPECT_EQ(parent, mir_surface_get_parent(child));
+
+    mir_wait_for(mir_surface_set_parent(child, NULL));
+    EXPECT_EQ(NULL, mir_surface_get_parent(child));
+
+    mir_wait_for(mir_surface_set_parent(child, parent));
+    EXPECT_EQ(parent, mir_surface_get_parent(child));
+
+    mir_surface_release_sync(parent);
+    EXPECT_EQ(NULL, mir_surface_get_parent(child));
+    mir_surface_release_sync(child);
+
+    mir_connection_release(connection);
+}
+
 #ifndef ANDROID
 TEST_F(ClientLibrary, surface_scanout_flag_toggles)
 {

=== modified file 'tests/acceptance-tests/test_client_surface_events.cpp'
--- tests/acceptance-tests/test_client_surface_events.cpp	2014-10-27 22:31:16 +0000
+++ tests/acceptance-tests/test_client_surface_events.cpp	2014-11-14 03:03:37 +0000
@@ -224,6 +224,39 @@
     }
 }
 
+TEST_F(ClientSurfaceEvents, client_receives_parenting_events)
+{
+    int surface_id = mir_debug_surface_id(surface);
+    int other_surface_id = mir_debug_surface_id(other_surface);
+
+    set_event_filter(mir_event_type_surface);
+
+    {
+        reset_last_event();
+        mir_wait_for(mir_surface_set_parent(surface, other_surface));
+
+        std::lock_guard<decltype(last_event_mutex)> last_event_lock{last_event_mutex};
+        EXPECT_THAT(last_event_surface, Eq(surface));
+        EXPECT_THAT(last_event.type, Eq(mir_event_type_surface));
+        EXPECT_THAT(last_event.surface.id, Eq(surface_id));
+        EXPECT_THAT(last_event.surface.attrib, Eq(mir_surface_attrib_parent));
+        EXPECT_THAT(last_event.surface.value, Eq(other_surface_id));
+    }
+
+    {
+        reset_last_event();
+        mir_wait_for(mir_surface_set_parent(surface, NULL));
+
+        std::lock_guard<decltype(last_event_mutex)> last_event_lock{last_event_mutex};
+        EXPECT_THAT(last_event_surface, Eq(surface));
+        EXPECT_THAT(last_event.type, Eq(mir_event_type_surface));
+        EXPECT_THAT(last_event.surface.id, Eq(surface_id));
+        EXPECT_THAT(last_event.surface.attrib, Eq(mir_surface_attrib_parent));
+        EXPECT_THAT(last_event.surface.value, Eq(mir_surface_parent_none));
+    }
+}
+
+
 struct OrientationEvents : ClientSurfaceEvents, ::testing::WithParamInterface<MirOrientation> {};
 
 TEST_P(OrientationEvents, surface_receives_orientation_events)

=== modified file 'tests/unit-tests/client/test_protobuf_rpc_channel.cpp'
--- tests/unit-tests/client/test_protobuf_rpc_channel.cpp	2014-10-28 13:59:51 +0000
+++ tests/unit-tests/client/test_protobuf_rpc_channel.cpp	2014-11-14 03:03:37 +0000
@@ -51,6 +51,11 @@
     void with_surface_do(int /*surface_id*/, std::function<void(MirSurface*)> /*exec*/) const
     {
     }
+
+    MirSurface* find(int) const noexcept
+    {
+        return nullptr;
+    }
 };
 
 class MockStreamTransport : public mclr::StreamTransport