Merge into development-branch : first-pass-of-surface-spec-modification : Code : Mir

Reviewer	Review Type	Date Requested	Status
Robert Carr (community)			Approve on 2015-04-14
Kevin DuBois (community)			Approve on 2015-04-14
PS Jenkins bot (community)	continuous-integration		Approve on 2015-04-14
Thomas Voß (community)			Approve on 2015-04-14
Alexandros Frantzis (community)			Approve on 2015-04-14
Alberto Aguirre (community)			Approve on 2015-04-13
Alan Griffiths			Abstain on 2015-04-13
Chris Halse Rogers		2015-04-09	Approve on 2015-04-13
Review via email: mp+255707@code.launchpad.net

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-09:

#

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

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-04-10:

#

This looks mostly sensible, but does mean that we need a separate API for introspecting the state of surfaces. IIRC the longer-term plan was to move all the surface introspection into MirSurfaceSpec accessors and deprecate things such as MirSurfaceParameters.

That said, now that I think of it some more, I think being able to get a SurfaceSpec from an existing surface and apply it would probably a misfeature, as the possibility for clients to accidentally override user changes would be high.

Hm. If we need to support morphing from different surface types - dialog to normal, for example - we'll need to have a mir_surface_spec_set_type and this will be somewhat redundant with the mir_connection_surface_spec_for_* family of functions. A client would be able to mir_create_spec_for_changes(), fill in all the same details as mir_connection_surface_spec_for_normal(), and then pass it to mir_surface_create() - whereupon they'd somewhat unexpectedly get an abort(), because the MirConnection isn't set.

I'm not sure what I think about this. On the one hand, it might be nice to not have the same type for both. On the other, that's obvious duplication. I guess I lean towards the former; one type with perhaps surprising behaviour.

17 +mir_connection_create_spec_for_changes(MirConnection* connection);
Why does this take a MirConnection? It's not used for anything, and it doesn't seem likely that it'll be used for anything in the future.

review: Needs Information

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-04-10:

#

I guess that's “needs discussion” ☺

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-10:

#

*Needs Discussion*

> This looks mostly sensible, but does mean that we need a separate API for
> introspecting the state of surfaces. IIRC the longer-term plan was to move all
> the surface introspection into MirSurfaceSpec accessors and deprecate things
> such as MirSurfaceParameters.

While I'm not strongly against it I'm not sure there is currently such a plan. The client trying to use the "existing state" with a surface specification leads to synchronization issues. Like this:

> That said, now that I think of it some more, I think being able to get a
> SurfaceSpec from an existing surface and apply it would probably a misfeature,
> as the possibility for clients to accidentally override user changes would be
> high.

~~~~

> Hm. If we need to support morphing from different surface types - dialog to
> normal, for example - we'll need to have a mir_surface_spec_set_type and this
> will be somewhat redundant with the mir_connection_surface_spec_for_* family
> of functions. A client would be able to mir_create_spec_for_changes(), fill in
> all the same details as mir_connection_surface_spec_for_normal(), and then
> pass it to mir_surface_create() - whereupon they'd somewhat unexpectedly get
> an abort(), because the MirConnection isn't set.
>
> I'm not sure what I think about this. On the one hand, it might be nice to not
> have the same type for both. On the other, that's obvious duplication. I guess
> I lean towards the former; one type with perhaps surprising behaviour.

Well, while MirSurfaceSpec isn't currently polymorphic the various mir_surface_spec_set_XXX() functions can fail (and return false). So different behavior from MirSurfaceSpec's created by different create functions is consistent with the API.

For example, mir_surface_spec_set_type() could fail if the MirSurfaceSpec was not created for change.

Alternatively, we could just constrain the state of MirSurfaceSpec in the functions that accept it and use the various mir_connection_create_spec_for_XXX functions as shortcuts.

> 17 +mir_connection_create_spec_for_changes(MirConnection* connection);
> Why does this take a MirConnection? It's not used for anything, and it doesn't
> seem likely that it'll be used for anything in the future.

Initially it was for consistency, but given the points you raise above we could use it to initialize MirSurfaceSpec::connection - that way the client could theoretically pass the MirSurfaceSpec to mir_surface_create() in the scenario you describe.

Although, if we decide on that approach this function should probably be renamed mir_connection_create_spec(MirConnection* connection) - and the existing functions are then "shortcuts" to the valid states for creation.

*Needs Discussion*

> This looks mostly sensible, but does mean that we need a separate API for
> introspecting the state of surfaces. IIRC the longer-term plan was to move all
> the surface introspection into MirSurfaceSpec accessors and deprecate things
> such as MirSurfaceParameters.

While I'm not strongly against it I'm not sure there is currently such a plan. The client trying to use the "existing state" with a surface specification leads to synchronization issues. Like this:

> That said, now that I think of it some more, I think being able to get a
> SurfaceSpec from an existing surface and apply it would probably a misfeature,
> as the possibility for clients to accidentally override user changes would be
> high.

~~~~

> Hm. If we need to support morphing from different surface types - dialog to
> normal, for example - we'll need to have a mir_surface_spec_set_type and this
> will be somewhat redundant with the mir_connection_surface_spec_for_* family
> of functions. A client would be able to mir_create_spec_for_changes(), fill in
> all the same details as mir_connection_surface_spec_for_normal(), and then
> pass it to mir_surface_create() - whereupon they'd somewhat unexpectedly get
> an abort(), because the MirConnection isn't set.
> 
> I'm not sure what I think about this. On the one hand, it might be nice to not
> have the same type for both. On the other, that's obvious duplication. I guess
> I lean towards the former; one type with perhaps surprising behaviour.

Well, while MirSurfaceSpec isn't currently polymorphic the various mir_surface_spec_set_XXX() functions can fail (and return false). So different behavior from MirSurfaceSpec's created by different create functions is consistent with the API.

For example, mir_surface_spec_set_type() could fail if the MirSurfaceSpec was not created for change.

Alternatively, we could just constrain the state of MirSurfaceSpec in the functions that accept it and use the various mir_connection_create_spec_for_XXX functions as shortcuts.

> 17      +mir_connection_create_spec_for_changes(MirConnection* connection);
> Why does this take a MirConnection? It's not used for anything, and it doesn't
> seem likely that it'll be used for anything in the future.

Initially it was for consistency, but given the points you raise above we could use it to initialize MirSurfaceSpec::connection - that way the client could theoretically pass the MirSurfaceSpec to mir_surface_create() in the scenario you describe.

Although, if we decide on that approach this function should probably be renamed mir_connection_create_spec(MirConnection* connection) - and the existing functions are then "shortcuts" to the valid states for creation.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-10:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-04-10:

#

nits:
93 namespace { // Private for now. TODO: Finalize and publish later (LP: #1422522)
comment needs removal

46 +
47 +
extra whitespace

57 +MirSurfaceSpec::MirSurfaceSpec()
could we just remove this, and keep MirSurfaceSpec() = default?

review: Needs Fixing

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-04-10:

#

As for the API changes, I'm all for publishing some sort of surface-changing functions (with a slightly different way proposed in the rfc-branch https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/254982)

My points here:
After I call mir_surface_create(), the MirSurfaceSpec seems to be associated with the surface. It makes more sense (to me at least) to query the surface, where MirSurfaceSpec "lives", for the existing specification of the surface.

This also leads to the topic of guaranteeing that the surface is in a particular state while the client decides what to do with the surface next. Like, it seems sensible that we have to lock the surface state, and then apply all the changes at once, and then 'unlock' the surface for further server-started changes.

It also seems that just rejecting change-specs the server doesn't like will lead to a somewhat frustrating back-and-forth for the clients trying to apply the change they want and then being told for non-obvious reasons that the change that was requested was rejected.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-10:

#

> nits:
> 93 namespace { // Private for now. TODO: Finalize and publish later (LP:
> #1422522)
> comment needs removal

Done

>
> 46 +
> 47 +
> extra whitespace

Done

> 57 +MirSurfaceSpec::MirSurfaceSpec()
> could we just remove this, and keep MirSurfaceSpec() = default?

We could, but there's no advantage to having it inline (and it might change as result of discussion).

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-10:

#

> As for the API changes, I'm all for publishing some sort of surface-changing
> functions (with a slightly different way proposed in the rfc-branch
> https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/254982)

But isn't that now about arranging buffer streams within a surface, and not to do with modifying the surface.

> My points here:
> After I call mir_surface_create(), the MirSurfaceSpec seems to be associated
> with the surface. It makes more sense (to me at least) to query the surface,
> where MirSurfaceSpec "lives", for the existing specification of the surface.

I don't think the MirSurfaceSpec is associated with a surface. If I were to call mir_surface_create() three times and get three surfaces would you consider it associated with all of them?

> This also leads to the topic of guaranteeing that the surface is in a
> particular state while the client decides what to do with the surface next.
> Like, it seems sensible that we have to lock the surface state, and then apply
> all the changes at once, and then 'unlock' the surface for further server-
> started changes.

Clients cannot reasonably "lock" the state of the surface - the window manager needs to respond to user input, display changes etc.

> It also seems that just rejecting change-specs the server doesn't like will
> lead to a somewhat frustrating back-and-forth for the clients trying to apply
> the change they want and then being told for non-obvious reasons that the
> change that was requested was rejected.

There is no way for a client to get a meaningful response to a change spec request. It is "fire and forget". There will likely be corresponding events from the server - but even if the request is honored in full the surface may never be in the requested state because of concurrent changes by (e.g.) the user.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-10:

#

> There is no way for a client to get a meaningful response to a change spec
> request. It is "fire and forget". There will likely be corresponding events
> from the server - but even if the request is honored in full the surface may
> never be in the requested state because of concurrent changes by (e.g.) the
> user.

This was discussed to death in lp:~vanvugt/mir/wait-result/+merge/252403

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-10:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-10:

#

Another thought is that we don't necessarily need mir_surface_spec_set_type().

We could, for example, do this:

void morph_to_a_dialog(MirSurface* some_existing_surface)
{
    auto const spec = mir_connection_create_spec_for_modal_dialog(...);
    mir_surface_apply_spec(some_existing_surface, spec);
    mir_surface_spec_release(spec);
}

This way we avoid the complexity of managing the required parameters for the new type.

The "con" is that it is then difficult to "inherit" parameters from the existing surface.

But this is not a problem with the current MP - we can still chose several options building upon it.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-04-10:

#

I think this MP is reasonable.

I don't think a mir_surface_spec_set_type is necessary, as you can create a spec for a specific type using the mir_connection_create_spec_for_xxx set of APIs and just use it in mir_surface_apply_spec.

I know that would require passing width/height/pixel_format (which could be in theory be morphed too) but I wouldn't mind a user setting -1, -1, mir_pixel_format_invalid to signal mir_surface_apply_spec to ignore those settings. I would prefer that slight ugliness in order to keep the API consistent and avoid adding type/parent setters - which was something we discussed to death and decided against.

In any case this MP looks ok to me.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-10:

#

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-04-10:

#

nits fixed, unblocking while I think more about the observations in my above comment.

review: Abstain

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-04-13:

#

I'm conceptually happy with this.

I was thinking that we needed a surface-state-snapshot type, and that MirSurfaceSpec could be it, but on reflection I don't think we *do* need one.

For the specific case of Kevin's MirBufferStream bypass work, the introspection API is only necessary to ease testing - clients are perfectly capable of maintaining their own stacking and positioning state.

I'm in favour of doing the mir_connection_spec_for_* for surface type morphing - I'd maintain the requirement to set mir_pixel_format to the correct value, because that's entirely under the control of the client, but having a guard mir_dimension_last = -1 for width/height seems least ugly.

With that, I don't mind the useless MirConnection parameter to spec_for_changes :).

Why does mir_surface_apply_spec return a MirWaitHandle? I don't particularly mind it doing so, but as (extensively!) discussed, there's nothing useful a client can do with it.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-13:

#

*Needs Discussion*

> Why does mir_surface_apply_spec return a MirWaitHandle? I don't particularly
> mind it doing so, but as (extensively!) discussed, there's nothing useful a
> client can do with it.

I agree it is useless. But it is consistent with the other APIs we discussed. Is it better to be inconsistent and return void now? Or to plan on removing the wait handle in all similar APIs when we next break ABI compatibility?

review: Needs Information

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-04-13:

#

I would vote for just returning void, but it's not a blocking issue.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-13:

#

> I would vote for just returning void, but it's not a blocking issue.

Let's do it then

review: Abstain

Revision history for this message

Alberto Aguirre (albaguirre) on 2015-04-13:

#

review: Approve

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-04-13:

#

146 and 153
+void mir_surface_apply_spec(MirSurface* surface, MirSurfaceSpec* spec)
+...set_name...

Should catch exceptions right?

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-13:

#

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-04-13:

#

So, I'm okay with this approach, as opposed to some sort of "lock" guarantee, as that seems to raise all sorts of questions about what a 'locked' surface might mean.
The wait handle seemed useless too, so also okay with its removal.

Chris's point that the nested stuff (or other clients) can maintain some of their own info is a good one. I'm not sure though that we explain well what changing a surface spec means... It makes sense to me to say that "when the title name is set to "X" on the MirSurfaceSpec, it will be presented as "X" when the server chooses to show a title", as opposed to saying "when the title name is set to "X" on the MirSurfaceSpec, it can be silently rejected"

needs-info:
1) Shouldn't this take a MirSurfaceSpec* instead of of a MirSurface*?
31 +void mir_surface_set_title(MirSurface* surface, char const* name);

2) Is it (or will it be) possible to construct a MirSurfaceSpec that's invalid? For instance, if we have a mir_surface_spec_set_size(..., int x, int y), and someone tries to morph a tooltip to a tooltip that spans multiple monitors that seems to violate the meaning of the types in the spec document. It might make sense to indicate to the client that its request was "flat-out rejected forever", instead of "accepted as something that the server will try to apply when appropriate".

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-14:

#

> needs-info:
> 1) Shouldn't this take a MirSurfaceSpec* instead of of a MirSurface*?
> 31 +void mir_surface_set_title(MirSurface* surface, char const* name);

No, this is a signature change (dropping the wait handle) to an existing method that's a "convenience wrapper" around the MirSurfaceSpec calls needed to change a surface title. (C.f. lp:~vanvugt/mir/rename-api/+merge/252864/comments/628924.)

> 2) Is it (or will it be) possible to construct a MirSurfaceSpec that's
> invalid? For instance, if we have a mir_surface_spec_set_size(..., int x, int
> y), and someone tries to morph a tooltip to a tooltip that spans multiple
> monitors that seems to violate the meaning of the types in the spec document.
> It might make sense to indicate to the client that its request was "flat-out
> rejected forever", instead of "accepted as something that the server will try
> to apply when appropriate".

That's a poor example of a MirSurfaceSpec that's invalid: How is that any different from the existing scenario of a "someone" requesting a tooltip that spans multiple monitors without a modification? The server knows what monitors are attached at the time it processes the request (and what "shell space" exists on the monitors) and sizes the surface accordingly.

A better example would be creating a MirSurfaceSpec for changes and then calling mir_surface_create_sync() - which will then abort because the surface has no type.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-14:

#

> 146 and 153
> +void mir_surface_apply_spec(MirSurface* surface, MirSurfaceSpec* spec)
> +...set_name...
>
> Should catch exceptions right?

Fixed

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-04-14:

#

Looks sensible.

31 +void mir_surface_set_title(MirSurface* surface, char const* name);

Now that we have a way to perform the same task with a more general mechanism, I am not convinced that we should keep mir_surface_set_title. If we keep it then we will be tempted to add mir_surface_set_ATTR for every new attribute that can be changed. I prefer offering a single way to do things, even if it's slightly more complex (which is not much of a problem considering that we don't expect most clients to be written with the client API itself, but rather through toolkits).

review: Approve

Revision history for this message

Thomas Voß (thomas-voss) wrote on 2015-04-14:

#

LGTM, two remarks inline, though.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-14:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) on 2015-04-14:

#

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-04-14:

#

@Alan, I see now that its just a convenience wrapper, so okay with the signature.
resolved the second point on IRC, my assumption that there were invalid change MirSurfaceSpecs isn't part of the intended model.

okay by me, and can port base the rfc branch I have up on this one

review: Approve

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-04-14:

#

LGTM now

review: Approve

Mir

Merge lp:~alan-griffiths/mir/first-pass-of-surface-spec-modification into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'include/client/mir_toolkit/mir_surface.h'
 --- include/client/mir_toolkit/mir_surface.h	2015-04-09 06:20:31 +0000
 +++ include/client/mir_toolkit/mir_surface.h	2015-04-14 08:39:35 +0000
@@ -168,6 +168,18 @@
                                        MirPixelFormat format);
  /**
++ * Create a surface specification for updating a surface.
++ *
++ * This can be applied to one or more target surfaces using
++ * mir_surface_apply_spec(...).
++ *
++ * \param [in] connection   a valid mir connection
++ *
++ */
++MirSurfaceSpec*
++mir_connection_create_spec_for_changes(MirConnection* connection);
++
++/**
   * Create a surface from a given specification
+  *
+  *
@@ -577,9 +589,17 @@
   * Change the title (name) of a surface.
   *   \param [in] surface  The surface to rename
   *   \param [in] name     The new name
-- *   \returns             When the change has completed
-- */
--MirWaitHandle* mir_surface_set_title(MirSurface* surf, char const* name);
++ */
++void mir_surface_set_title(MirSurface* surface, char const* name);
++
++/**
++ * Request changes to the specification of a surface. The server will decide
++ * whether and how the request can be honoured.
++ *
++ *   \param [in] surface  The surface to rename
++ *   \param [in] spec     Spec with the requested changes applied
++ */
++void mir_surface_apply_spec(MirSurface* surface, MirSurfaceSpec* spec);
  #ifdef __cplusplus
+ }
 === modified file 'src/client/mir_surface.cpp'
 --- src/client/mir_surface.cpp	2015-04-13 12:07:15 +0000
 +++ src/client/mir_surface.cpp	2015-04-14 08:39:35 +0000
@@ -74,9 +74,8 @@
+     }
+ }
--MirSurfaceSpec::MirSurfaceSpec(MirSurface* preexisting)
++MirSurfaceSpec::MirSurfaceSpec()
+ {
--    self = preexisting;
+ }
  mir::protobuf::SurfaceParameters MirSurfaceSpec::serialize() const
 === modified file 'src/client/mir_surface.h'
 --- src/client/mir_surface.h	2015-04-09 14:29:02 +0000
 +++ src/client/mir_surface.h	2015-04-14 08:39:35 +0000
@@ -61,10 +61,9 @@
  struct MirSurfaceSpec
+ {
--    MirSurfaceSpec() = default;
++    MirSurfaceSpec();
      MirSurfaceSpec(MirConnection* connection, int width, int height, MirPixelFormat format);
      MirSurfaceSpec(MirConnection* connection, MirSurfaceParameters const& params);
--    MirSurfaceSpec(MirSurface* preexisting);
      mir::protobuf::SurfaceParameters serialize() const;
@@ -84,7 +83,6 @@
      mir::optional_value<MirSurfaceState> state;
      mir::optional_value<MirOrientationMode> pref_orientation;
--    mir::optional_value<MirSurface*> self;
      mir::optional_value<MirSurface*> parent;
      mir::optional_value<MirRectangle> aux_rect;
      mir::optional_value<MirEdgeAttachment> edge_attachment;
 === modified file 'src/client/mir_surface_api.cpp'
 --- src/client/mir_surface_api.cpp	2015-04-10 16:50:12 +0000
 +++ src/client/mir_surface_api.cpp	2015-04-14 08:39:35 +0000
@@ -583,44 +583,45 @@
      return nullptr;
+ }
--namespace { // Private for now. TODO: Finalize and publish later (LP: #1422522)
--
--MirSurfaceSpec* mir_surface_begin_changes(MirSurface* surf)
--{
--    mir::require(mir_surface_is_valid(surf));
--
--    MirSurfaceSpec* spec = nullptr;
--    try
--    {
--        spec = new MirSurfaceSpec(surf);
--    }
--    catch (std::exception const& ex)
--    {
--        MIR_LOG_UNCAUGHT_EXCEPTION(ex);
--    }
--
--    return spec;
--}
--
--MirWaitHandle* mir_surface_spec_commit_changes(MirSurfaceSpec* spec)
--{
--    if (!spec->self.is_set())
--        return nullptr;
--
--    auto surface = spec->self.value();
--    return surface->modify(*spec);
--}
--
--} // Private namespace. TODO: finalize morphing API and publish.
--
--MirWaitHandle* mir_surface_set_title(MirSurface* surf, char const* name)
--{
--    MirWaitHandle* result = nullptr;
--    if (auto spec = mir_surface_begin_changes(surf))
--    {
--        mir_surface_spec_set_name(spec, name);
--        result = mir_surface_spec_commit_changes(spec);
--        mir_surface_spec_release(spec);
--    }
--    return result;
++namespace
++{
++MirSurfaceSpec* create_spec_for_changes()
++try
++{
++    return new MirSurfaceSpec{};
++}
++catch (std::exception const& ex)
++{
++    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
++    std::abort();  // If we just failed to allocate a MirSurfaceSpec returning isn't safe
++}
++}
++
++MirSurfaceSpec* mir_connection_create_spec_for_changes(MirConnection* connection)
++{
++    mir::require(mir_connection_is_valid(connection));
++
++    return create_spec_for_changes();
++}
++
++void mir_surface_apply_spec(MirSurface* surface, MirSurfaceSpec* spec)
++try
++{
++    mir::require(mir_surface_is_valid(surface));
++    mir::require(spec);
++
++    surface->modify(*spec);
++}
++catch (std::exception const& ex)
++{
++    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
++    // Keep calm and carry on
++}
++
++void mir_surface_set_title(MirSurface* surface, char const* name)
++{
++    auto const spec = create_spec_for_changes();
++    mir_surface_spec_set_name(spec, name);
++    mir_surface_apply_spec(surface, spec);
++    mir_surface_spec_release(spec);
+ }
 === modified file 'src/client/symbols.map'
 --- src/client/symbols.map	2015-04-09 06:20:31 +0000
 +++ src/client/symbols.map	2015-04-14 08:39:35 +0000
@@ -113,6 +113,8 @@
  MIR_CLIENT_8.4 {   # New in Mir 0.13
   global:
++    mir_connection_create_spec_for_changes;
++    mir_surface_apply_spec;
      mir_surface_set_event_handler;
      mir_surface_set_title;
      mir_default_cursor_name;
 === modified file 'src/server/shell/canonical_window_manager.cpp'
 --- src/server/shell/canonical_window_manager.cpp	2015-04-09 06:20:31 +0000
 +++ src/server/shell/canonical_window_manager.cpp	2015-04-14 08:39:35 +0000
@@ -248,6 +248,26 @@
+ {
      if (modifications.name.is_set())
          surface->rename(modifications.name.value());
++
++    if (modifications.width.is_set() || modifications.height.is_set())
++    {
++        // TODO similar logic is needed in example window management policies
++        auto new_size = surface->size();
++
++        if (modifications.width.is_set())
++            new_size.width = modifications.width.value();
++
++        if (modifications.height.is_set())
++            new_size.height = modifications.height.value();
++
++        constrained_resize(
++            surface,
++            surface->top_left(),
++            new_size,
++            false,
++            false,
++            display_area);
++    }
+ }
  void msh::CanonicalWindowManagerPolicy::handle_delete_surface(std::shared_ptr<ms::Session> const& session, std::weak_ptr<ms::Surface> const& surface)
@@ -577,6 +597,17 @@
      Point new_pos = top_left + left_resize*delta.dx + top_resize*delta.dy;
++    return constrained_resize(surface, new_pos, new_size, left_resize, top_resize, bounds);
++}
++
++bool msh::CanonicalWindowManagerPolicy::constrained_resize(
++    std::shared_ptr<ms::Surface> const& surface,
++    Point new_pos,
++    Size new_size,
++    bool const left_resize,
++    bool const top_resize,
++    Rectangle const& bounds)
++{
      if (left_resize)
+     {
          if (new_pos.x < bounds.top_left.x)
@@ -615,15 +646,15 @@
      // "A vertically maximised surface is anchored to the top and bottom of
      // the available workspace and can have any width."
      case mir_surface_state_vertmaximized:
--        new_pos.y = old_pos.top_left.y;
--        new_size.height = old_pos.size.height;
++        new_pos.y = surface->top_left().y;
++        new_size.height = surface->size().height;
          break;
      // "A horizontally maximised surface is anchored to the left and right of
      // the available workspace and can have any height"
      case mir_surface_state_horizmaximized:
--        new_pos.x = old_pos.top_left.x;
--        new_size.width = old_pos.size.width;
++        new_pos.x = surface->top_left().x;
++        new_size.width = surface->size().width;
          break;
      // "A maximised surface is anchored to the top, bottom, left and right of the
@@ -639,7 +670,7 @@
      // TODO It is rather simplistic to move a tree WRT the top_left of the root
      // TODO when resizing. But for more sophistication we would need to encode
      // TODO some sensible layout rules.
--    move_tree(surface, new_pos-top_left);
++    move_tree(surface, new_pos-surface->top_left());
      return true;
+ }
 === modified file 'src/server/shell/canonical_window_manager.h'
 --- src/server/shell/canonical_window_manager.h	2015-04-09 06:20:31 +0000
 +++ src/server/shell/canonical_window_manager.h	2015-04-14 08:39:35 +0000
@@ -119,6 +119,13 @@
      bool drag(std::shared_ptr<scene::Surface> surface, geometry::Point to, geometry::Point from, geometry::Rectangle bounds);
      void move_tree(std::shared_ptr<scene::Surface> const& root, geometry::Displacement movement) const;
      void raise_tree(std::shared_ptr<scene::Surface> const& root) const;
++    bool constrained_resize(
++        std::shared_ptr<scene::Surface> const& surface,
++        geometry::Point new_pos,
++        geometry::Size new_size,
++        const bool left_resize,
++        const bool top_resize,
++        geometry::Rectangle const& bounds);
      Tools* const tools;
      std::shared_ptr<DisplayLayout> const display_layout;
 === modified file 'tests/acceptance-tests/test_client_surfaces.cpp'
 --- tests/acceptance-tests/test_client_surfaces.cpp	2015-04-09 06:20:31 +0000
 +++ tests/acceptance-tests/test_client_surfaces.cpp	2015-04-14 08:39:35 +0000
@@ -314,9 +314,9 @@
+      *
       * At least verify the rename completes without blocking...
       */
--    mir_wait_for_one(mir_surface_set_title(surf, "New Name"));
--    mir_wait_for_one(mir_surface_set_title(surf, ""));
--    mir_wait_for_one(mir_surface_set_title(surf, "Alice"));
++    mir_surface_set_title(surf, "New Name");
++    mir_surface_set_title(surf, "");
++    mir_surface_set_title(surf, "Alice");
      mir_surface_release_sync(surf);
+ }
 === modified file 'tests/acceptance-tests/test_surface_modifications.cpp'
 --- tests/acceptance-tests/test_surface_modifications.cpp	2015-04-02 12:57:34 +0000
 +++ tests/acceptance-tests/test_surface_modifications.cpp	2015-04-14 08:39:35 +0000
@@ -31,6 +31,7 @@
  namespace msh = mir::shell;
  namespace mt = mir::test;
++using namespace mir::geometry;
  using namespace testing;
  namespace
@@ -39,6 +40,7 @@
+ {
  public:
      MOCK_METHOD1(renamed, void(char const*));
++    MOCK_METHOD1(resized_to, void(Size const& size));
  };
  struct StubShell : msh::ShellWrapper
@@ -80,6 +82,17 @@
      MockSurfaceObserver surface_observer;
  };
++
++MATCHER_P(WidthEq, value, "")
++{
++    return Width(value) == arg.width;
++}
++
++MATCHER_P(HeightEq, value, "")
++{
++    return Height(value) == arg.height;
++}
++
+ }
  TEST_F(SurfaceModifications, rename_is_notified)
@@ -90,3 +103,60 @@
      mir_surface_set_title(surface, new_title);
+ }
++
++TEST_F(SurfaceModifications, surface_spec_name_is_notified)
++{
++    auto const new_title = __PRETTY_FUNCTION__;
++
++    EXPECT_CALL(surface_observer, renamed(StrEq(new_title)));
++
++    auto const spec = mir_connection_create_spec_for_changes(connection);
++
++    mir_surface_spec_set_name(spec, new_title);
++    mir_surface_apply_spec(surface, spec);
++    mir_surface_spec_release(spec);
++}
++
++TEST_F(SurfaceModifications, surface_spec_resize_is_notified)
++{
++    auto const new_width = 5;
++    auto const new_height = 7;
++
++    EXPECT_CALL(surface_observer, resized_to(Size{new_width, new_height}));
++
++    auto const spec = mir_connection_create_spec_for_changes(connection);
++
++    mir_surface_spec_set_width(spec, new_width);
++    mir_surface_spec_set_height(spec, new_height);
++
++    mir_surface_apply_spec(surface, spec);
++    mir_surface_spec_release(spec);
++}
++
++TEST_F(SurfaceModifications, surface_spec_change_width_is_notified)
++{
++    auto const new_width = 11;
++
++    EXPECT_CALL(surface_observer, resized_to(WidthEq(new_width)));
++
++    auto const spec = mir_connection_create_spec_for_changes(connection);
++
++    mir_surface_spec_set_width(spec, new_width);
++
++    mir_surface_apply_spec(surface, spec);
++    mir_surface_spec_release(spec);
++}
++
++TEST_F(SurfaceModifications, surface_spec_change_height_is_notified)
++{
++    auto const new_height = 13;
++
++    EXPECT_CALL(surface_observer, resized_to(HeightEq(new_height)));
++
++    auto const spec = mir_connection_create_spec_for_changes(connection);
++
++    mir_surface_spec_set_height(spec, new_height);
++
++    mir_surface_apply_spec(surface, spec);
++    mir_surface_spec_release(spec);
++}