Mir

Merge lp:~alan-griffiths/mir/more-surface-resize into lp:mir

more-surface-resize
Merge into development-branch

Proposed by Alan Griffiths on 2015-04-21

Status:

Merged

Approved by:

Alan Griffiths on 2015-04-29

Approved revision:

2526

Merged at revision:

2523

Proposed branch:

lp:~alan-griffiths/mir/more-surface-resize

Merge into:

lp:mir

Diff against target:

1181 lines (+634/-172)

14 files modified

examples/server_example_canonical_window_manager.cpp (+115/-58)
examples/server_example_canonical_window_manager.h (+6/-2)
include/client/mir_toolkit/mir_surface.h (+53/-0)
include/server/mir/scene/surface_creation_parameters.h (+5/-0)
include/server/mir/shell/surface_specification.h (+6/-0)
src/client/mir_surface.cpp (+34/-0)
src/client/mir_surface.h (+6/-0)
src/client/mir_surface_api.cpp (+48/-0)
src/client/symbols.map (+6/-2)
src/protobuf/mir_protobuf.proto (+14/-0)
src/server/frontend/session_mediator.cpp (+26/-0)
src/server/shell/canonical_window_manager.cpp (+115/-55)
src/server/shell/canonical_window_manager.h (+6/-2)
tests/acceptance-tests/test_surface_modifications.cpp (+194/-53)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/more-surface-resize

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2015-04-28
Alexandros Frantzis (community)			Approve on 2015-04-28
Kevin DuBois (community)			Approve on 2015-04-28
Alan Griffiths			Abstain on 2015-04-28
Alberto Aguirre		2015-04-21	Approve on 2015-04-27
Andreas Pokorny (community)			Approve on 2015-04-27
Daniel van Vugt			Needs Fixing on 2015-04-24
Review via email: mp+256912@code.launchpad.net

Commit Message

shell: handle size increments and aspect ratios

Description of the Change

shell: handle size increments and aspect ratios

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

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

review: Approve (continuous-integration)

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

OK.

review: Approve

Daniel van Vugt (vanvugt) wrote on 2015-04-23:

This seems a bit odd and smells fishy:
examples/server_example_canonical_window_manager.cpp

Why any change to examples? The policy and logic for resize constraints should be entirely in src/server/. Nothing should need to be implemented by consumers of CanonicalWindowManager. The goal is to implement WM policy once (src/server/), and not in each shell.

The default policy should just use best fit and modify any requested dimensions so that they satisfy the constraints. For example:
* Policy is 16:9 aspect ratio and shell tries to set 80x80 -> new size is 80x45
* Policy is increments of 8x16 (a terminal) and shell tries to set 812x323 -> new size is 808x320 or 816x336 (depending on the previous dimensions and direction of resize).

So the goal is policy enforcement with the simplest interface possible, which is no code change required to any shell.

review: Needs Information

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

The changes to examples are because the team decided that me::CanonicalWindowManagerPolicyCopy should be a copy of msh::CanonicalWindowManagerPolicy. (Give or take experiments like titlebars)

Daniel van Vugt (vanvugt) wrote on 2015-04-24:

(surprised but question answered :)

review: Abstain

Daniel van Vugt (vanvugt) wrote on 2015-04-24:

(1) Setting aspect ratio the user usually just wants it fixed (video players) or completely free (say zeros). So it would ease usability if these were a single function with five parameters:

458 +bool mir_surface_spec_set_min_aspect_ratio(MirSurfaceSpec* spec, unsigned x, unsigned y)
470 +bool mir_surface_spec_set_max_aspect_ratio(MirSurfaceSpec* spec, unsigned x, unsigned y)

Similarly, when you set the resize granularity, it should be a single function instead of two:

236 +bool mir_surface_spec_set_width_inc(MirSurfaceSpec* spec, unsigned width_inc);
249 +bool mir_surface_spec_set_height_inc(MirSurfaceSpec* spec, unsigned height_inc);

Arguably all the resize constraints should be bundled into a single structure, but I think that would contradict the new design too much.

review: Needs Fixing

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

*Needs Discussion*

> (1) Setting aspect ratio the user usually just wants it fixed (video players)
> or completely free (say zeros). So it would ease usability if these were a
> single function with five parameters:
>
> 458 +bool mir_surface_spec_set_min_aspect_ratio(MirSurfaceSpec* spec,
> unsigned x, unsigned y)
> 470 +bool mir_surface_spec_set_max_aspect_ratio(MirSurfaceSpec* spec,
> unsigned x, unsigned y)
>
> Similarly, when you set the resize granularity, it should be a single function
> instead of two:
>
> 236 +bool mir_surface_spec_set_width_inc(MirSurfaceSpec* spec, unsigned
> width_inc);
> 249 +bool mir_surface_spec_set_height_inc(MirSurfaceSpec* spec, unsigned
> height_inc);
>
> Arguably all the resize constraints should be bundled into a single structure,
> but I think that would contradict the new design too much.

I'm not a fan of interfaces that use "magic values" (say zeros) but will go with what the majority prefers.

review: Needs Information

lp:~alan-griffiths/mir/more-surface-resize updated on 2015-04-27

2514. By Alan Griffiths on 2015-04-27: merge lp:mir

Andreas Pokorny (andreas-pokorny) wrote on 2015-04-27:

Nit: + 240 s/hight/height

Maintaining ABI compatibility of caller stored structures seems harder than having separate functions. Given the use cases I can think of right now, most clients might only use the width/height increment functions or the aspect ration functions. Hence those could be combined to single functions. Then again most of the existing setter 'methods' are unary.

review: Approve

lp:~alan-griffiths/mir/more-surface-resize updated on 2015-04-27

2515. By Alan Griffiths on 2015-04-27: Nitfix

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

I have a slight preference for more a well-defined functions in the api as opposed to one function that rolls a lot of tasks in.
I'd be okay with the api as-is, or reducing to 3 new functions seems good to me as well:
mir_surface_spec_set_max_aspect_ratio
mir_surface_spec_set_min_aspect_ratio
mir_surface_spec_set_size_inc

needs-info:
Do we need a way to reset the width_inc/height_inc?

review: Needs Information

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

> I have a slight preference for more a well-defined functions in the api as
> opposed to one function that rolls a lot of tasks in.
> I'd be okay with the api as-is, or reducing to 3 new functions seems good to
> me as well:
> mir_surface_spec_set_max_aspect_ratio
> mir_surface_spec_set_min_aspect_ratio
> mir_surface_spec_set_size_inc

I think this needs to be consistent. If we take this approach we should also merge:

mir_surface_spec_set_min_width + mir_surface_spec_set_min_height => mir_surface_spec_set_min_size

mir_surface_spec_set_max_width + mir_surface_spec_set_max_height => mir_surface_spec_set_max_size

> needs-info:
> Do we need a way to reset the width_inc/height_inc?

Good question!

One that should also apply to [min|max]_[aspect|width|height] (or size)

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

I think the 4 apis are just fine. It's just a surface spec, so the value are not in flight until it's commited.

Having them separate is consistent with other existing API's ( set_width and set_height, etc.)

review: Approve

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

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

review: Approve (continuous-integration)

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

I think the majority view is that this API is fine.

~~~~

But we may need another MP to add a bunch of "reset" functions for surface properties that can be set.

Or, alternatively, have some values that give default behavior:

  mir_surface_default_min_width = 0,
  mir_surface_default_width_inc = 1,
  mir_surface_default_aspect_x = 0,
  etc.

review: Abstain

Gerry Boland (gerboland) wrote on 2015-04-27:

As outside perspective, I like this api. I modify the MirSurfaceSpec one property at a time, and know if each modification was allowed or not. When ready, commit.

While on that subject, is there a possibility of a race between me calling mir_surface_spec_set_{width,height}_inc and committing that MirSurfaceSpec, and the client changing those values?

Nitpick:
Mir never shied away from long descriptive function names. Why abbreviate "increment" to "inc"? My first guess for the meaning of "inc" is not increment.

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

> As outside perspective, I like this api. I modify the MirSurfaceSpec one
> property at a time, and know if each modification was allowed or not. When
> ready, commit.
>
> While on that subject, is there a possibility of a race between me calling
> mir_surface_spec_set_{width,height}_inc and committing that MirSurfaceSpec,
> and the client changing those values?

Well, by definition, that is a race. What will happen is that the server will handle the requests in some order. The resulting surface size could depend upon the order.

> Nitpick:
> Mir never shied away from long descriptive function names. Why abbreviate
> "increment" to "inc"? My first guess for the meaning of "inc" is not
> increment.

Fair. I'll fix.

lp:~alan-griffiths/mir/more-surface-resize updated on 2015-04-27

2516. By Alan Griffiths on 2015-04-27: Don't abbreviate increment in function names

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

> But we may need another MP to add a bunch of "reset" functions for surface
> properties that can be set.

So, after the discussion today, I think that its something we might need, but could come once the need arises.

review: Abstain

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

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

review: Needs Fixing (continuous-integration)

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

101: [ RUN ] AndroidInputReceiverSetup.slow_raw_input_doesnt_cause_frameskipping
101: /mir/tests/unit-tests/client/input/test_android_input_receiver.cpp:277: Failure
101: Expected: (duration) > (1ms), actual: 8-byte object <C7-2B 0F-00 00-00 00-00> vs 8-byte object <01-00 00-00 00-00 00-00>
101: [ FAILED ] AndroidInputReceiverSetup.slow_raw_input_doesnt_cause_frameskipping (7 ms)

lp:1394369

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

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

review: Approve (continuous-integration)

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

needs-fixings:
122+ auto const error = new_size.width.as_int()*(long)ar.y - new_size.height.as_int()*(long)ar.x;
c-cast

264+ * \param [in] x numerator
265+ * \param [in] y denominator
Its obvious from context that the ratio is x/y, but might make it clearer that that the ratio is x/y (and also if the ratio gets affected by 90 degree rotation requests)

around l383, should we check that the min aspect ratio is less than the max?

227+ * Defines an arithmetic progression of sizes starting with min_width (if set, otherwise 0)
would a default of 1 be better? That way the server wouldn't be stepping over the clients default of 0 by default. (zero seems to be a request to fix the surface size to min_width, min_height)

review: Needs Fixing

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

267 + * \note The requested aspect ration is a hint only. The server is not guaranteed
279 + * \note The requested aspect ration is a hint only. The server is not guaranteed

408+ struct AspectRation { unsigned x; unsigned y; };

Typo 'ration'.

261+ * Set the minimum aspect ratio
262+ *
263+ * \param [in] spec Specification to mutate
264+ * \param [in] x numerator
265+ * \param [in] y denominator

I think it would be clearer to express the aspect ratio in terms of width and height, not x and y.

715+ auto const error = new_size.width.as_int()*(long)ar.y - new_size.height.as_int()*(long)ar.x;
716+
717+ if (error > 0)

Should this be the other way around?

error > 0 => W*y-H*x > 0 => W/H > x/y => the new aspect ratio is larger than the min aspect ratio so we shouldn't need to touch the W,H values, or am I misunderstanding what "min aspect ratio" means?

Needs fixing/info

review: Needs Fixing

lp:~alan-griffiths/mir/more-surface-resize updated on 2015-04-28

2517. By Alan Griffiths on 2015-04-28: Renames for typos and clarity
2518. By Alan Griffiths on 2015-04-28: Another pass of x => width, y => height
2519. By Alan Griffiths on 2015-04-28: Another pass of x => width, y => height
2520. By Alan Griffiths on 2015-04-28: Final pass of x => width, y => height
2521. By Alan Griffiths on 2015-04-28: No C-stype casts
2522. By Alan Griffiths on 2015-04-28: Fix interpretation of aspect ratio (with a clearer test expectation)
2523. By Alan Griffiths on 2015-04-28: merge lp:mir

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

> 267 + * \note The requested aspect ration is a hint only. The server is
> not guaranteed
> 279 + * \note The requested aspect ration is a hint only. The server is
> not guaranteed
>
> 408+ struct AspectRation { unsigned x; unsigned y; };
>
> Typo 'ration'.
>
> 261+ * Set the minimum aspect ratio
> 262+ *
> 263+ * \param [in] spec Specification to mutate
> 264+ * \param [in] x numerator
> 265+ * \param [in] y denominator
>
> I think it would be clearer to express the aspect ratio in terms of width and
> height, not x and y.
>
> 715+ auto const error = new_size.width.as_int()*(long)ar.y -
> new_size.height.as_int()*(long)ar.x;
> 716+
> 717+ if (error > 0)
>
> Should this be the other way around?
>
> error > 0 => W*y-H*x > 0 => W/H > x/y => the new aspect ratio is larger than
> the min aspect ratio so we shouldn't need to touch the W,H values, or am I
> misunderstanding what "min aspect ratio" means?
>
> Needs fixing/info

fixed

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

> needs-fixings:
> 122+ auto const error = new_size.width.as_int()*(long)ar.y -
> new_size.height.as_int()*(long)ar.x;
> c-cast

fixed

> 264+ * \param [in] x numerator
> 265+ * \param [in] y denominator
> Its obvious from context that the ratio is x/y, but might make it clearer that
> that the ratio is x/y (and also if the ratio gets affected by 90 degree
> rotation requests)

I'm not clear what this is asking for.

> around l383, should we check that the min aspect ratio is less than the max?

We could, but would we handle it any differently?

> 227+ * Defines an arithmetic progression of sizes starting with min_width (if
> set, otherwise 0)
> would a default of 1 be better?

I don't follow. If min_width isn't set the series is:

0, width_inc, 2*width_inc, ...

You're suggesting this:

1, 1+width_inc, 1+2*width_inc, ...

> That way the server wouldn't be stepping over
> the clients default of 0 by default. (zero seems to be a request to fix the
> surface size to min_width, min_height)

And I don't follow this.

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

122+ auto const error = new_size.height.as_int()*(long)ar.width - new_size.width.as_int()*(long)ar.height;
123+
124+ if (error > 0)
125+ {
126+ if (new_size.height.as_int() > new_size.width.as_int())
127+ {
128+ new_size.width = new_size.width + DeltaX((error+(ar.height-1))/ar.height);
129+ }
130+ else
131+ {
132+ new_size.height = new_size.height - DeltaY((error+(ar.width-1))/ar.width);
133+ }
134+ }

Some questions (Aw=aspect W, Ah=aspect H):

1. In this case, since W/H < Aw/Ah, we can either increase W or decrease H to increase the ratio, as is done in the two branches of the if statement. What's the rationale behind using the relation between W and H to decide whether to change W or H?

2. What's the logic behind the formula in DeltaX (and DeltaY respectively)? If we want W/H to reach Aw/Ah we could increase W by error/Ah, so it's not clear why we need the other terms (the proposed formula leads to an increase of error/Ah + 1 + 1/Ah).

It would be good to add a comment documenting how the proposed computations and decisions work.

review: Needs Information

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

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

review: Approve (continuous-integration)

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

> > 227+ * Defines an arithmetic progression of sizes starting with min_width
> (if
> > set, otherwise 0)
> > would a default of 1 be better?
>
> I don't follow. If min_width isn't set the series is:
>
> 0, width_inc, 2*width_inc, ...
>
> You're suggesting this:
>
> 1, 1+width_inc, 1+2*width_inc, ...
>
> > That way the server wouldn't be stepping over
> > the clients default of 0 by default. (zero seems to be a request to fix the
> > surface size to min_width, min_height)
>
> And I don't follow this.

So, I was suggesting that min_width is always set and gives the sequence:
min_width, min_width + 1*width_inc, min_width + 2*width_inc, etc
And, with that 1 would be a good default value, because then you get:
min_width, min_width+1, min_width+2, etc.
effectively, saying that the server can resize to any width and still fit within the client's setting.
If min_width is set to zero, then the client's sequence is:
min_width, min_width, min_width, etc...
and the client requests no server resizes.

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

> > 264+ * \param [in] x numerator
> > 265+ * \param [in] y denominator
> > Its obvious from context that the ratio is x/y, but might make it clearer
> that
> > that the ratio is x/y (and also if the ratio gets affected by 90 degree
> > rotation requests)
>
> I'm not clear what this is asking for.

Just some more comments in the vein of:

261+ * Set the minimum aspect ratio
to
* Set the minimum aspect ratio. Aspect ratio is defined as the ratio of the x direction over
* the y direction. Aspect ratio is not inverted/affected by mir_surface_spec_set_preferred_orientation.
(or how the ratio is treated if its rotated 90/270 degrees)

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

> Some questions (Aw=aspect W, Ah=aspect H):
>
> 1. In this case, since W/H < Aw/Ah, we can either increase W or decrease H to
> increase the ratio, as is done in the two branches of the if statement. What's
> the rationale behind using the relation between W and H to decide whether to
> change W or H?

I admit it is arbitrary. And now I examine it, probably wrong.

My (misguided) thought was that changing the smaller one leads to a bigger effect on the ratio for a smaller change. But suppose we want a ratio of 10:1, but have 10:20. Then my approach leads to 200:20 when 10:2 would probably be better.

> 2. What's the logic behind the formula in DeltaX (and DeltaY respectively)? If
> we want W/H to reach Aw/Ah we could increase W by error/Ah, so it's not clear
> why we need the other terms (the proposed formula leads to an increase of
> error/Ah + 1 + 1/Ah).

It's to ensure rounding up. (I didn't think common incantation in integer arithmetic needed explaining - I was wrong.)

> It would be good to add a comment documenting how the proposed computations
> and decisions work.

And even better to get it right. ;)

review: Needs Fixing

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

> It's to ensure rounding up. (I didn't think common incantation in integer arithmetic needed explaining - I was wrong.)

Yes, this will benefit from being more explicit (I missed it even though I have used/implemented it in the past [1]).

[1] src/platforms/mesa/client/client_platform.cpp , division_ceiling()

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

> > > 227+ * Defines an arithmetic progression of sizes starting with min_width
> > (if
> > > set, otherwise 0)
> > > would a default of 1 be better?
> >
> > I don't follow. If min_width isn't set the series is:
> >
> > 0, width_inc, 2*width_inc, ...
> >
> > You're suggesting this:
> >
> > 1, 1+width_inc, 1+2*width_inc, ...
> >
> > > That way the server wouldn't be stepping over
> > > the clients default of 0 by default. (zero seems to be a request to fix
> the
> > > surface size to min_width, min_height)
> >
> > And I don't follow this.
>
> So, I was suggesting that min_width is always set and gives the sequence:
> min_width, min_width + 1*width_inc, min_width + 2*width_inc, etc
> And, with that 1 would be a good default value, because then you get:
> min_width, min_width+1, min_width+2, etc.

OK, I think you're writing "*min_width* is always set" when you're suggesting a default for *width_inc*.

So, we're actually talking about the same behavior except, possibly, in cases where the client applies a spec to a surface that has previously had width_inc set:

What we have is a surface spec where width_inc is optional and only if that is set by the client will it update the width_inc on the server.

Are you suggesting that the surface spec should always have a width_inc (default 1) and that this should override the server setting if not set by the client? I don't think that is right.

lp:~alan-griffiths/mir/more-surface-resize updated on 2015-04-28

2524. By Alan Griffiths on 2015-04-28: Better clearer min/max size logic
2525. By Alan Griffiths on 2015-04-28: Better clearer min/max aspect ratio doc comments
2526. By Alan Griffiths on 2015-04-28: merge lp:mir

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

> And even better to get it right. ;)

fixed

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

review: Abstain

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

cleared up my confusion over irc (re the width_inc, height_inc), lgtm

review: Approve

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

Looks good.

review: Approve

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

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

review: Approve (continuous-integration)

Daniel van Vugt (vanvugt) wrote on 2015-04-29:

(1) Still not a fan of the extraneous functions and longer names. And "inc" is better than "increment" because the former is actually more common in programming circles and less likely to take you off the edge of the screen when typing.

(2) Does anyone know why having two aspect ratios is useful? I know X has it, but why? In my experience in building toolkits only two options (one ratio) are useful:
(a) Fixed aspect ratio (e.g. video player); or
(b) No fixed aspect ratio (although the toolkit will feed back and modify the min/max dimensions according to how well its widgets fit, that's not an aspect thing).

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

> (1) Still not a fan of the extraneous functions and longer names. And "inc" is
> better than "increment" because the former is actually more common in
> programming circles and less likely to take you off the edge of the screen
> when typing.

"Do not use abbreviations unless they are extremely well known outside your project"

http://unity.ubuntu.com/mir/cppguide/index.html?showone=General_Naming_Rules#General_Naming_Rules

It is a fine line - "min" and "max" have been accepted as common abbreviations whereas "inc" isn't common enough.

> (2) Does anyone know why having two aspect ratios is useful? I know X has it,
> but why? In my experience in building toolkits only two options (one ratio)
> are useful:
> (a) Fixed aspect ratio (e.g. video player); or
> (b) No fixed aspect ratio (although the toolkit will feed back and modify
> the min/max dimensions according to how well its widgets fit, that's not an
> aspect thing).

No, but following established practice in the domain seems prudent.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'examples/server_example_canonical_window_manager.cpp'
 --- examples/server_example_canonical_window_manager.cpp	2015-04-23 07:11:04 +0000
 +++ examples/server_example_canonical_window_manager.cpp	2015-04-28 16:01:14 +0000
@@ -63,11 +63,15 @@
      state{mir_surface_state_restored},
      restore_rect{surface->top_left(), surface->size()},
      session{session},
--    parent{surface->parent()},
++    parent{params.parent},
      min_width{params.min_width},
      min_height{params.min_height},
      max_width{params.max_width},
--    max_height{params.max_height}
++    max_height{params.max_height},
++    width_inc{params.width_inc},
++    height_inc{params.height_inc},
++    min_aspect{params.min_aspect},
++    max_aspect{params.max_aspect}
+ {
+ }
@@ -323,21 +327,25 @@
+ {
      auto& surface_info = tools->info_for(surface);
++    #define COPY_IF_SET(field)\
++        if (modifications.field.is_set())\
++        surface_info.field = modifications.field
++
++    COPY_IF_SET(min_width);
++    COPY_IF_SET(min_height);
++    COPY_IF_SET(max_width);
++    COPY_IF_SET(max_height);
++    COPY_IF_SET(min_width);
++    COPY_IF_SET(width_inc);
++    COPY_IF_SET(height_inc);
++    COPY_IF_SET(min_aspect);
++    COPY_IF_SET(max_aspect);
++
++    #undef COPY_IF_SET
++
      if (modifications.name.is_set())
          surface->rename(modifications.name.value());
--    if (modifications.min_width.is_set())
--        surface_info.min_width = modifications.min_width;
--
--    if (modifications.min_height.is_set())
--        surface_info.min_height = modifications.min_height;
--
--    if (modifications.max_width.is_set())
--        surface_info.max_width = modifications.max_width;
--
--    if (modifications.max_height.is_set())
--        surface_info.max_height = modifications.max_height;
--
      if (modifications.width.is_set() || modifications.height.is_set())
+     {
          auto new_size = surface->size();
@@ -710,53 +718,103 @@
  bool me::CanonicalWindowManagerPolicyCopy::constrained_resize(
      std::shared_ptr<ms::Surface> const& surface,
--    Point new_pos,
--    Size new_size,
++    Point const& requested_pos,
++    Size const& requested_size,
      bool const left_resize,
      bool const top_resize,
      Rectangle const& bounds)
+ {
      auto const& surface_info = tools->info_for(surface);
--    if (surface_info.min_width.is_set() && surface_info.min_width.value() > new_size.width)
--    {
--        if (left_resize)
--        {
--            new_pos.x += surface_info.min_width.value() - new_size.width;
--        }
--
--        new_size.width = surface_info.min_width.value();
--    }
--
--    if (surface_info.min_height.is_set() && surface_info.min_height.value() > new_size.height)
--    {
--        if (top_resize)
--        {
--            new_pos.y += surface_info.min_height.value() - new_size.height;
--        }
--
--        new_size.height = surface_info.min_height.value();
--    }
--
--    if (surface_info.max_width.is_set() && surface_info.max_width.value() < new_size.width)
--    {
--        if (left_resize)
--        {
--            new_pos.x += surface_info.max_width.value() - new_size.width;
--        }
--
--        new_size.width = surface_info.max_width.value();
--    }
--
--    if (surface_info.max_height.is_set() && surface_info.max_height.value() < new_size.height)
--    {
--        if (top_resize)
--        {
--            new_pos.y += surface_info.max_height.value() - new_size.height;
--        }
--
--        new_size.height = surface_info.max_height.value();
--    }
++    auto const min_width  = surface_info.min_width.is_set()  ? surface_info.min_width.value()  : Width{};
++    auto const min_height = surface_info.min_height.is_set() ? surface_info.min_height.value() : Height{};
++    auto const max_width  = surface_info.max_width.is_set()  ?
++        surface_info.max_width.value()  : Width{std::numeric_limits<int>::max()};
++    auto const max_height = surface_info.max_height.is_set() ?
++        surface_info.max_height.value() : Height{std::numeric_limits<int>::max()};
++
++    Point new_pos = requested_pos;
++    Size new_size = requested_size;
++
++    if (surface_info.min_aspect.is_set())
++    {
++        auto const ar = surface_info.min_aspect.value();
++
++        auto const error = new_size.height.as_int()*long(ar.width) - new_size.width.as_int()*long(ar.height);
++
++        if (error > 0)
++        {
++            // Add (denominator-1) to numerator to ensure rounding up
++            auto const width_correction  = (error+(ar.height-1))/ar.height;
++            auto const height_correction = (error+(ar.width-1))/ar.width;
++
++            if (width_correction < height_correction)
++            {
++                new_size.width = new_size.width + DeltaX(width_correction);
++            }
++            else
++            {
++                new_size.height = new_size.height - DeltaY(height_correction);
++            }
++        }
++    }
++
++    if (surface_info.max_aspect.is_set())
++    {
++        auto const ar = surface_info.max_aspect.value();
++
++        auto const error = new_size.width.as_int()*long(ar.height) - new_size.height.as_int()*long(ar.width);
++
++        if (error > 0)
++        {
++            // Add (denominator-1) to numerator to ensure rounding up
++            auto const height_correction = (error+(ar.width-1))/ar.width;
++            auto const width_correction  = (error+(ar.height-1))/ar.height;
++
++            if (width_correction < height_correction)
++            {
++                new_size.width = new_size.width - DeltaX(width_correction);
++            }
++            else
++            {
++                new_size.height = new_size.height + DeltaY(height_correction);
++            }
++        }
++    }
++
++    if (min_width > new_size.width)
++        new_size.width = min_width;
++
++    if (min_height > new_size.height)
++        new_size.height = min_height;
++
++    if (max_width < new_size.width)
++        new_size.width = max_width;
++
++    if (max_height < new_size.height)
++        new_size.height = max_height;
++
++    if (surface_info.width_inc.is_set())
++    {
++        auto const width = new_size.width.as_int() - min_width.as_int();
++        auto inc = surface_info.width_inc.value().as_int();
++        if (width % inc)
++            new_size.width = min_width + DeltaX{inc*(((2L*width + inc)/2)/inc)};
++    }
++
++    if (surface_info.height_inc.is_set())
++    {
++        auto const height = new_size.height.as_int() - min_height.as_int();
++        auto inc = surface_info.height_inc.value().as_int();
++        if (height % inc)
++            new_size.height = min_height + DeltaY{inc*(((2L*height + inc)/2)/inc)};
++    }
++
++    if (left_resize)
++        new_pos.x += new_size.width - requested_size.width;
++
++    if (top_resize)
++        new_pos.y += new_size.height - requested_size.height;
      if (left_resize)
+     {
@@ -788,8 +846,7 @@
              new_size.height = new_size.height + to_bottom_right.dy;
+     }
--    auto& info = tools->info_for(surface);
--    switch (info.state)
++    switch (surface_info.state)
+     {
      case mir_surface_state_restored:
          break;
@@ -816,7 +873,7 @@
          return true;
+     }
--    info.titlebar->resize({new_size.width, Height{title_bar_height}});
++    surface_info.titlebar->resize({new_size.width, Height{title_bar_height}});
      surface->resize(new_size);
      // TODO It is rather simplistic to move a tree WRT the top_left of the root
 === modified file 'examples/server_example_canonical_window_manager.h'
 --- examples/server_example_canonical_window_manager.h	2015-04-17 14:31:42 +0000
 +++ examples/server_example_canonical_window_manager.h	2015-04-28 16:01:14 +0000
@@ -54,6 +54,10 @@
      optional_value<geometry::Height> min_height;
      optional_value<geometry::Width> max_width;
      optional_value<geometry::Height> max_height;
++    mir::optional_value<geometry::DeltaX> width_inc;
++    mir::optional_value<geometry::DeltaY> height_inc;
++    mir::optional_value<shell::SurfaceAspectRatio> min_aspect;
++    mir::optional_value<shell::SurfaceAspectRatio> max_aspect;
  };
  // standard window management algorithm:
@@ -132,8 +136,8 @@
      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,
++        geometry::Point const& requested_pos,
++        geometry::Size const& requested_size,
          const bool left_resize,
          const bool top_resize,
          geometry::Rectangle const& bounds);
 === modified file 'include/client/mir_toolkit/mir_surface.h'
 --- include/client/mir_toolkit/mir_surface.h	2015-04-28 10:29:48 +0000
 +++ include/client/mir_toolkit/mir_surface.h	2015-04-28 16:01:14 +0000
@@ -239,6 +239,32 @@
  bool mir_surface_spec_set_height(MirSurfaceSpec* spec, unsigned height);
  /**
++ * Set the requested width increment, in pixels.
++ * Defines an arithmetic progression of sizes starting with min_width (if set, otherwise 0)
++ * into which the surface prefers to be resized.
++ *
++ * \param [in] spec       Specification to mutate
++ * \param [in] width_inc  Requested width increment.
++ * \return                False if width increment is invalid for a surface of this type
++ * \note    The requested dimensions are a hint only. The server is not guaranteed to
++ *          create a surface of any specific width or height.
++ */
++bool mir_surface_spec_set_width_increment(MirSurfaceSpec *spec, unsigned width_inc);
++
++/**
++ * Set the requested height increment, in pixels
++ * Defines an arithmetic progression of sizes starting with min_height (if set, otherwise 0)
++ * into which the surface prefers to be resized.
++ *
++ * \param [in] spec       Specification to mutate
++ * \param [in] height_inc Requested height increment.
++ * \return                False if height increment is invalid for a surface of this type
++ * \note    The requested dimensions are a hint only. The server is not guaranteed to
++ *          create a surface of any specific width or height.
++ */
++bool mir_surface_spec_set_height_increment(MirSurfaceSpec *spec, unsigned height_inc);
++
++/**
   * Set the minimum width, in pixels
+  *
   * \param [in] spec     Specification to mutate
@@ -280,6 +306,33 @@
   *          surface of any specific width or height.
   */
  bool mir_surface_spec_set_max_height(MirSurfaceSpec* spec, unsigned max_height);
++
++/**
++ * Set the minimum aspect ratio. This is the minimum ratio of surface width to height.
++ * It is independent of orientation changes and/or preferences.
++ *
++ * \param [in] spec     Specification to mutate
++ * \param [in] width    numerator
++ * \param [in] height   denominator
++ * \return              False if minimum aspect is invalid for a surface of this type
++ * \note    The requested aspect ratio is a hint only. The server is not guaranteed
++ *          to create a surface of any specific aspect.
++ */
++bool mir_surface_spec_set_min_aspect_ratio(MirSurfaceSpec* spec, unsigned width, unsigned height);
++
++/**
++ * Set the maximum aspect ratio. This is the maximum ratio of surface width to height.
++ * It is independent of orientation changes and/or preferences.
++ *
++ * \param [in] spec     Specification to mutate
++ * \param [in] width    numerator
++ * \param [in] height   denominator
++ * \return              False if maximum aspect is invalid for a surface of this type
++ * \note    The requested aspect ratio is a hint only. The server is not guaranteed
++ *          to create a surface of any specific aspect.
++ */
++bool mir_surface_spec_set_max_aspect_ratio(MirSurfaceSpec* spec, unsigned width, unsigned height);
++
  /**
   * Set the requested pixel format.
   * \param [in] spec     Specification to mutate
 === modified file 'include/server/mir/scene/surface_creation_parameters.h'
 --- include/server/mir/scene/surface_creation_parameters.h	2015-04-16 18:50:58 +0000
 +++ include/server/mir/scene/surface_creation_parameters.h	2015-04-28 16:01:14 +0000
@@ -28,6 +28,7 @@
  #include "mir/frontend/surface_id.h"
  #include "mir/input/input_reception_mode.h"
  #include "mir/optional_value.h"
++#include "mir/shell/surface_specification.h"
  #include <memory>
  #include <string>
@@ -94,6 +95,10 @@
      optional_value<geometry::Height> min_height;
      optional_value<geometry::Width> max_width;
      optional_value<geometry::Height> max_height;
++    mir::optional_value<geometry::DeltaX> width_inc;
++    mir::optional_value<geometry::DeltaY> height_inc;
++    mir::optional_value<shell::SurfaceAspectRatio> min_aspect;
++    mir::optional_value<shell::SurfaceAspectRatio> max_aspect;
  };
  bool operator==(const SurfaceCreationParameters& lhs, const SurfaceCreationParameters& rhs);
 === modified file 'include/server/mir/shell/surface_specification.h'
 --- include/server/mir/shell/surface_specification.h	2015-04-10 08:20:39 +0000
 +++ include/server/mir/shell/surface_specification.h	2015-04-28 16:01:14 +0000
@@ -33,6 +33,8 @@
+ {
  namespace shell
+ {
++struct SurfaceAspectRatio { unsigned width; unsigned height; };
++
  /// Specification of surface properties requested by client
  struct SurfaceSpecification
+ {
@@ -52,6 +54,10 @@
      optional_value<geometry::Height> min_height;
      optional_value<geometry::Width> max_width;
      optional_value<geometry::Height> max_height;
++    mir::optional_value<geometry::DeltaX> width_inc;
++    mir::optional_value<geometry::DeltaY> height_inc;
++    mir::optional_value<SurfaceAspectRatio> min_aspect;
++    mir::optional_value<SurfaceAspectRatio> max_aspect;
      // TODO scene::SurfaceCreationParameters overlaps this content but has additional fields:
      //    geometry::Point top_left;
 === modified file 'src/client/mir_surface.cpp'
 --- src/client/mir_surface.cpp	2015-04-23 07:11:04 +0000
 +++ src/client/mir_surface.cpp	2015-04-28 16:01:14 +0000
@@ -96,6 +96,10 @@
      SERIALIZE_OPTION_IF_SET(min_height, message);
      SERIALIZE_OPTION_IF_SET(max_width, message);
      SERIALIZE_OPTION_IF_SET(max_height, message);
++    SERIALIZE_OPTION_IF_SET(width_inc, message);
++    SERIALIZE_OPTION_IF_SET(height_inc, message);
++    // min_aspect is a special case (below)
++    // max_aspect is a special case (below)
      if (parent.is_set() && parent.value() != nullptr)
          message.set_parent_id(parent.value()->id());
@@ -108,6 +112,18 @@
          message.mutable_aux_rect()->set_height(aux_rect.value().height);
+     }
++    if (min_aspect.is_set())
++    {
++        message.mutable_min_aspect()->set_width(min_aspect.value().width);
++        message.mutable_min_aspect()->set_height(min_aspect.value().height);
++    }
++
++    if (max_aspect.is_set())
++    {
++        message.mutable_max_aspect()->set_width(max_aspect.value().width);
++        message.mutable_max_aspect()->set_height(max_aspect.value().height);
++    }
++
      return message;
+ }
@@ -563,6 +579,10 @@
      COPY_IF_SET(min_height);
      COPY_IF_SET(max_width);
      COPY_IF_SET(max_height);
++    COPY_IF_SET(width_inc);
++    COPY_IF_SET(height_inc);
++    // min_aspect is a special case (below)
++    // max_aspect is a special case (below)
      #undef COPY_IF_SET
      if (spec.surface_name.is_set())
@@ -584,6 +604,20 @@
          rect->set_height(value.height);
+     }
++    if (spec.min_aspect.is_set())
++    {
++        auto const aspect = surface_specification->mutable_min_aspect();
++        aspect->set_width(spec.min_aspect.value().width);
++        aspect->set_height(spec.min_aspect.value().height);
++    }
++
++    if (spec.max_aspect.is_set())
++    {
++        auto const aspect = surface_specification->mutable_max_aspect();
++        aspect->set_width(spec.max_aspect.value().width);
++        aspect->set_height(spec.max_aspect.value().height);
++    }
++
      modify_wait_handle.expect_result();
      server->modify_surface(0, &mods, &modify_result,
                google::protobuf::NewCallback(this, &MirSurface::on_modified));
 === modified file 'src/client/mir_surface.h'
 --- src/client/mir_surface.h	2015-04-09 15:59:52 +0000
 +++ src/client/mir_surface.h	2015-04-28 16:01:14 +0000
@@ -67,6 +67,8 @@
      mir::protobuf::SurfaceParameters serialize() const;
++    struct AspectRatio { unsigned width; unsigned height; };
++
      // Required parameters
      MirConnection* connection{nullptr};
@@ -91,6 +93,10 @@
      mir::optional_value<int> min_height;
      mir::optional_value<int> max_width;
      mir::optional_value<int> max_height;
++    mir::optional_value<int> width_inc;
++    mir::optional_value<int> height_inc;
++    mir::optional_value<AspectRatio> min_aspect;
++    mir::optional_value<AspectRatio> max_aspect;
  };
  struct MirSurface
 === modified file 'src/client/mir_surface_api.cpp'
 --- src/client/mir_surface_api.cpp	2015-04-17 14:33:09 +0000
 +++ src/client/mir_surface_api.cpp	2015-04-28 16:01:14 +0000
@@ -632,3 +632,51 @@
      MIR_LOG_UNCAUGHT_EXCEPTION(ex);
      // Keep calm and carry on
+ }
++
++bool mir_surface_spec_set_width_increment(MirSurfaceSpec *spec, unsigned width_inc)
++try
++{
++    spec->width_inc = width_inc;
++    return true;
++}
++catch (std::exception const& ex)
++{
++    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
++    return false;
++}
++
++bool mir_surface_spec_set_height_increment(MirSurfaceSpec *spec, unsigned height_inc)
++try
++{
++    spec->height_inc = height_inc;
++    return true;
++}
++catch (std::exception const& ex)
++{
++    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
++    return false;
++}
++
++bool mir_surface_spec_set_min_aspect_ratio(MirSurfaceSpec* spec, unsigned width, unsigned height)
++try
++{
++    spec->min_aspect = {width, height};
++    return true;
++}
++catch (std::exception const& ex)
++{
++    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
++    return false;
++}
++
++bool mir_surface_spec_set_max_aspect_ratio(MirSurfaceSpec* spec, unsigned width, unsigned height)
++try
++{
++    spec->max_aspect = {width, height};
++    return true;
++}
++catch (std::exception const& ex)
++{
++    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
++    return false;
++}
 === modified file 'src/client/symbols.map'
 --- src/client/symbols.map	2015-04-23 07:11:04 +0000
 +++ src/client/symbols.map	2015-04-28 16:01:14 +0000
@@ -163,10 +163,14 @@
      mir_surface_event_get_attribute;
      mir_surface_event_get_attribute_value;
      mir_surface_set_swapinterval;
++    mir_surface_spec_set_height_increment;
++    mir_surface_spec_set_max_aspect_ratio;
++    mir_surface_spec_set_max_width;
++    mir_surface_spec_set_max_height;
++    mir_surface_spec_set_min_aspect_ratio;
      mir_surface_spec_set_min_width;
      mir_surface_spec_set_min_height;
--    mir_surface_spec_set_max_width;
--    mir_surface_spec_set_max_height;
++    mir_surface_spec_set_width_increment;
      mir_surface_swap_buffers;
      mir_surface_swap_buffers_sync;
      mir_touch_event_modifiers;
 === modified file 'src/protobuf/mir_protobuf.proto'
 --- src/protobuf/mir_protobuf.proto	2015-04-23 07:11:04 +0000
 +++ src/protobuf/mir_protobuf.proto	2015-04-28 16:01:14 +0000
@@ -31,6 +31,16 @@
    optional int32 min_height = 14;
    optional int32 max_width = 15;
    optional int32 max_height = 16;
++  optional int32 width_inc = 17;
++  optional int32 height_inc = 18;
++  optional SurfaceAspectRatio min_aspect = 19;
++  optional SurfaceAspectRatio max_aspect = 20;
++}
++
++message SurfaceAspectRatio
++{
++    required uint32 width  = 1;
++    required uint32 height = 2;
+ }
  // If and when we break our protocol backward-compatibility, this could be
@@ -53,6 +63,10 @@
    optional int32 min_height = 14;
    optional int32 max_width = 15;
    optional int32 max_height = 16;
++  optional int32 width_inc = 17;
++  optional int32 height_inc = 18;
++  optional SurfaceAspectRatio min_aspect = 19;
++  optional SurfaceAspectRatio max_aspect = 20;
+ }
 === modified file 'src/server/frontend/session_mediator.cpp'
 --- src/server/frontend/session_mediator.cpp	2015-04-28 10:29:48 +0000
 +++ src/server/frontend/session_mediator.cpp	2015-04-28 16:01:14 +0000
@@ -238,9 +238,17 @@
      COPY_IF_SET(min_height);
      COPY_IF_SET(max_width);
      COPY_IF_SET(max_height);
++    COPY_IF_SET(width_inc);
++    COPY_IF_SET(height_inc);
      #undef COPY_IF_SET
++    if (request->has_min_aspect())
++        params.min_aspect = { request->min_aspect().width(), request->min_aspect().height()};
++
++    if (request->has_max_aspect())
++        params.max_aspect = { request->max_aspect().width(), request->max_aspect().height()};
++
      auto const surf_id = shell->create_surface(session, params);
      auto surface = session->get_surface(surf_id);
@@ -461,6 +469,10 @@
          COPY_IF_SET(min_height);
          COPY_IF_SET(max_width);
          COPY_IF_SET(max_height);
++        COPY_IF_SET(width_inc);
++        COPY_IF_SET(height_inc);
++        // min_aspect is a special case (below)
++        // max_aspect is a special case (below)
          #undef COPY_IF_SET
@@ -470,6 +482,20 @@
              mods.aux_rect = {{rect.left(), rect.top()}, {rect.width(), rect.height()}};
+         }
++        if (surface_specification.has_min_aspect())
++            mods.min_aspect =
++                {
++                    surface_specification.min_aspect().width(),
++                    surface_specification.min_aspect().height()
++                };
++
++        if (surface_specification.has_max_aspect())
++            mods.max_aspect =
++                {
++                    surface_specification.max_aspect().width(),
++                    surface_specification.max_aspect().height()
++                };
++
          auto const id = mf::SurfaceId(request->surface_id().value());
          shell->modify_surface(session, id, mods);
 === modified file 'src/server/shell/canonical_window_manager.cpp'
 --- src/server/shell/canonical_window_manager.cpp	2015-04-23 07:11:04 +0000
 +++ src/server/shell/canonical_window_manager.cpp	2015-04-28 16:01:14 +0000
@@ -27,6 +27,8 @@
  #include <linux/input.h>
  #include <csignal>
++#include <climits>
++
  namespace msh = mir::shell;
  namespace ms = mir::scene;
  using namespace mir::geometry;
@@ -43,11 +45,15 @@
      state{mir_surface_state_restored},
      restore_rect{surface->top_left(), surface->size()},
      session{session},
--    parent{surface->parent()},
++    parent{params.parent},
      min_width{params.min_width},
      min_height{params.min_height},
      max_width{params.max_width},
--    max_height{params.max_height}
++    max_height{params.max_height},
++    width_inc{params.width_inc},
++    height_inc{params.height_inc},
++    min_aspect{params.min_aspect},
++    max_aspect{params.max_aspect}
+ {
+ }
@@ -256,21 +262,25 @@
+ {
      auto& surface_info = tools->info_for(surface);
++    #define COPY_IF_SET(field)\
++        if (modifications.field.is_set())\
++        surface_info.field = modifications.field
++
++    COPY_IF_SET(min_width);
++    COPY_IF_SET(min_height);
++    COPY_IF_SET(max_width);
++    COPY_IF_SET(max_height);
++    COPY_IF_SET(min_width);
++    COPY_IF_SET(width_inc);
++    COPY_IF_SET(height_inc);
++    COPY_IF_SET(min_aspect);
++    COPY_IF_SET(max_aspect);
++
++    #undef COPY_IF_SET
++
      if (modifications.name.is_set())
          surface->rename(modifications.name.value());
--    if (modifications.min_width.is_set())
--        surface_info.min_width = modifications.min_width;
--
--    if (modifications.min_height.is_set())
--        surface_info.min_height = modifications.min_height;
--
--    if (modifications.max_width.is_set())
--        surface_info.max_width = modifications.max_width;
--
--    if (modifications.max_height.is_set())
--        surface_info.max_height = modifications.max_height;
--
      if (modifications.width.is_set() || modifications.height.is_set())
+     {
          auto new_size = surface->size();
@@ -623,53 +633,103 @@
  bool msh::CanonicalWindowManagerPolicy::constrained_resize(
      std::shared_ptr<ms::Surface> const& surface,
--    Point new_pos,
--    Size new_size,
++    Point const& requested_pos,
++    Size const& requested_size,
      bool const left_resize,
      bool const top_resize,
      Rectangle const& bounds)
+ {
      auto const& surface_info = tools->info_for(surface);
--    if (surface_info.min_width.is_set() && surface_info.min_width.value() > new_size.width)
--    {
--        if (left_resize)
--        {
--            new_pos.x += surface_info.min_width.value() - new_size.width;
--        }
--
--        new_size.width = surface_info.min_width.value();
--    }
--
--    if (surface_info.min_height.is_set() && surface_info.min_height.value() > new_size.height)
--    {
--        if (top_resize)
--        {
--            new_pos.y += surface_info.min_height.value() - new_size.height;
--        }
--
--        new_size.height = surface_info.min_height.value();
--    }
--
--    if (surface_info.max_width.is_set() && surface_info.max_width.value() < new_size.width)
--    {
--        if (left_resize)
--        {
--            new_pos.x += surface_info.max_width.value() - new_size.width;
--        }
--
--        new_size.width = surface_info.max_width.value();
--    }
--
--    if (surface_info.max_height.is_set() && surface_info.max_height.value() < new_size.height)
--    {
--        if (top_resize)
--        {
--            new_pos.y += surface_info.max_height.value() - new_size.height;
--        }
--
--        new_size.height = surface_info.max_height.value();
--    }
++    auto const min_width  = surface_info.min_width.is_set()  ? surface_info.min_width.value()  : Width{};
++    auto const min_height = surface_info.min_height.is_set() ? surface_info.min_height.value() : Height{};
++    auto const max_width  = surface_info.max_width.is_set()  ?
++        surface_info.max_width.value()  : Width{std::numeric_limits<int>::max()};
++    auto const max_height = surface_info.max_height.is_set() ?
++        surface_info.max_height.value() : Height{std::numeric_limits<int>::max()};
++
++    Point new_pos = requested_pos;
++    Size new_size = requested_size;
++
++    if (surface_info.min_aspect.is_set())
++    {
++        auto const ar = surface_info.min_aspect.value();
++
++        auto const error = new_size.height.as_int()*long(ar.width) - new_size.width.as_int()*long(ar.height);
++
++        if (error > 0)
++        {
++            // Add (denominator-1) to numerator to ensure rounding up
++            auto const width_correction  = (error+(ar.height-1))/ar.height;
++            auto const height_correction = (error+(ar.width-1))/ar.width;
++
++            if (width_correction < height_correction)
++            {
++                new_size.width = new_size.width + DeltaX(width_correction);
++            }
++            else
++            {
++                new_size.height = new_size.height - DeltaY(height_correction);
++            }
++        }
++    }
++
++    if (surface_info.max_aspect.is_set())
++    {
++        auto const ar = surface_info.max_aspect.value();
++
++        auto const error = new_size.width.as_int()*long(ar.height) - new_size.height.as_int()*long(ar.width);
++
++        if (error > 0)
++        {
++            // Add (denominator-1) to numerator to ensure rounding up
++            auto const height_correction = (error+(ar.width-1))/ar.width;
++            auto const width_correction  = (error+(ar.height-1))/ar.height;
++
++            if (width_correction < height_correction)
++            {
++                new_size.width = new_size.width - DeltaX(width_correction);
++            }
++            else
++            {
++                new_size.height = new_size.height + DeltaY(height_correction);
++            }
++        }
++    }
++
++    if (min_width > new_size.width)
++        new_size.width = min_width;
++
++    if (min_height > new_size.height)
++        new_size.height = min_height;
++
++    if (max_width < new_size.width)
++        new_size.width = max_width;
++
++    if (max_height < new_size.height)
++        new_size.height = max_height;
++
++    if (surface_info.width_inc.is_set())
++    {
++        auto const width = new_size.width.as_int() - min_width.as_int();
++        auto inc = surface_info.width_inc.value().as_int();
++        if (width % inc)
++            new_size.width = min_width + DeltaX{inc*(((2L*width + inc)/2)/inc)};
++    }
++
++    if (surface_info.height_inc.is_set())
++    {
++        auto const height = new_size.height.as_int() - min_height.as_int();
++        auto inc = surface_info.height_inc.value().as_int();
++        if (height % inc)
++            new_size.height = min_height + DeltaY{inc*(((2L*height + inc)/2)/inc)};
++    }
++
++    if (left_resize)
++        new_pos.x += new_size.width - requested_size.width;
++
++    if (top_resize)
++        new_pos.y += new_size.height - requested_size.height;
      if (left_resize)
+     {
 === modified file 'src/server/shell/canonical_window_manager.h'
 --- src/server/shell/canonical_window_manager.h	2015-04-16 18:50:58 +0000
 +++ src/server/shell/canonical_window_manager.h	2015-04-28 16:01:14 +0000
@@ -50,6 +50,10 @@
      optional_value<geometry::Height> min_height;
      optional_value<geometry::Width> max_width;
      optional_value<geometry::Height> max_height;
++    mir::optional_value<geometry::DeltaX> width_inc;
++    mir::optional_value<geometry::DeltaY> height_inc;
++    mir::optional_value<SurfaceAspectRatio> min_aspect;
++    mir::optional_value<SurfaceAspectRatio> max_aspect;
  };
  // standard window management algorithm:
@@ -126,8 +130,8 @@
      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,
++        geometry::Point const& requested_pos,
++        geometry::Size const& requested_size,
          const bool left_resize,
          const bool top_resize,
          geometry::Rectangle const& bounds);
 === modified file 'tests/acceptance-tests/test_surface_modifications.cpp'
 --- tests/acceptance-tests/test_surface_modifications.cpp	2015-04-23 07:11:04 +0000
 +++ tests/acceptance-tests/test_surface_modifications.cpp	2015-04-28 16:01:14 +0000
@@ -137,15 +137,25 @@
          EXPECT_CALL(surface_observer, renamed(StrEq(new_title))).
              WillOnce(InvokeWithoutArgs([&]{ server_ready.raise(); }));
--        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);
++        apply_changes([&](MirSurfaceSpec* spec)
++            {
++                mir_surface_spec_set_name(spec, new_title);
++            });
          server_ready.wait();
+     }
++    template<typename Specifier>
++    void apply_changes(Specifier const& specifier) const
++    {
++        auto const spec = mir_connection_create_spec_for_changes(connection);
++
++        specifier(spec);
++
++        mir_surface_apply_spec(surface, spec);
++        mir_surface_spec_release(spec);
++    }
++
      MockSurfaceObserver surface_observer;
      std::weak_ptr<ms::Surface> shell_surface;
  };
@@ -168,11 +178,10 @@
      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);
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_name(spec, new_title);
++        });
+ }
  TEST_F(SurfaceModifications, surface_spec_resize_is_notified)
@@ -182,13 +191,11 @@
      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);
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_width(spec, new_width);
++            mir_surface_spec_set_height(spec, new_height);
++        });
+ }
  TEST_F(SurfaceModifications, surface_spec_change_width_is_notified)
@@ -197,12 +204,10 @@
      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);
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_width(spec, new_width);
++        });
+ }
  TEST_F(SurfaceModifications, surface_spec_change_height_is_notified)
@@ -211,24 +216,20 @@
      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);
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_height(spec, new_height);
++        });
+ }
  TEST_F(SurfaceModifications, surface_spec_min_width_is_respected)
+ {
      auto const min_width = 17;
--    {
--        auto const spec = mir_connection_create_spec_for_changes(connection);
--        mir_surface_spec_set_min_width(spec, min_width);
--        mir_surface_apply_spec(surface, spec);
--        mir_surface_spec_release(spec);
--    }
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_min_width(spec, min_width);
++        });
      ensure_server_has_processed_setup();
@@ -245,12 +246,10 @@
+ {
      auto const min_height = 19;
--    {
--        auto const spec = mir_connection_create_spec_for_changes(connection);
--        mir_surface_spec_set_min_height(spec, min_height);
--        mir_surface_apply_spec(surface, spec);
--        mir_surface_spec_release(spec);
--    }
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_min_height(spec, min_height);
++        });
      ensure_server_has_processed_setup();
@@ -267,12 +266,10 @@
+ {
      auto const max_width = 23;
--    {
--        auto const spec = mir_connection_create_spec_for_changes(connection);
--        mir_surface_spec_set_max_width(spec, max_width);
--        mir_surface_apply_spec(surface, spec);
--        mir_surface_spec_release(spec);
--    }
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_max_width(spec, max_width);
++        });
      ensure_server_has_processed_setup();
@@ -289,12 +286,10 @@
+ {
      auto const max_height = 29;
--    {
--        auto const spec = mir_connection_create_spec_for_changes(connection);
--        mir_surface_spec_set_max_height(spec, max_height);
--        mir_surface_apply_spec(surface, spec);
--        mir_surface_spec_release(spec);
--    }
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_max_height(spec, max_height);
++        });
      ensure_server_has_processed_setup();
@@ -306,3 +301,149 @@
      generate_alt_click_at(bottom_right);
      generate_alt_move_to(bottom_right + DeltaY(max_height));
+ }
++
++TEST_F(SurfaceModifications, surface_spec_width_inc_is_respected)
++{
++    auto const width_inc = 13;
++
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_width_increment(spec, width_inc);
++        });
++
++    ensure_server_has_processed_setup();
++
++    auto const shell_surface = this->shell_surface.lock();
++    auto const bottom_right = shell_surface->input_bounds().bottom_right() - Displacement{1,1};
++
++    Size actual;
++    EXPECT_CALL(surface_observer, resized_to(_)).WillOnce(SaveArg<0>(&actual));
++
++    generate_alt_click_at(bottom_right);
++    generate_alt_move_to(bottom_right + DeltaX(16));
++
++    EXPECT_TRUE(actual.width.as_int() % width_inc == 0);
++}
++
++TEST_F(SurfaceModifications, surface_spec_with_min_width_and_width_inc_is_respected)
++{
++    auto const width_inc = 13;
++    auto const min_width = 7;
++
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_width_increment(spec, width_inc);
++            mir_surface_spec_set_min_width(spec, min_width);
++        });
++
++    ensure_server_has_processed_setup();
++
++    auto const shell_surface = this->shell_surface.lock();
++    auto const bottom_right = shell_surface->input_bounds().bottom_right() - Displacement{1,1};
++
++    Size actual;
++    EXPECT_CALL(surface_observer, resized_to(_)).WillOnce(SaveArg<0>(&actual));
++
++    generate_alt_click_at(bottom_right);
++    generate_alt_move_to(bottom_right + DeltaX(16));
++
++    EXPECT_TRUE((actual.width.as_int() - min_width) % width_inc == 0);
++}
++
++TEST_F(SurfaceModifications, surface_spec_height_inc_is_respected)
++{
++    auto const height_inc = 13;
++
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_height_increment(spec, height_inc);
++        });
++
++    ensure_server_has_processed_setup();
++
++    auto const shell_surface = this->shell_surface.lock();
++    auto const bottom_right = shell_surface->input_bounds().bottom_right() - Displacement{1,1};
++
++    Size actual;
++    EXPECT_CALL(surface_observer, resized_to(_)).WillOnce(SaveArg<0>(&actual));
++
++    generate_alt_click_at(bottom_right);
++    generate_alt_move_to(bottom_right + DeltaY(16));
++
++    EXPECT_TRUE(actual.height.as_int() % height_inc == 0);
++}
++
++TEST_F(SurfaceModifications, surface_spec_with_min_height_and_height_inc_is_respected)
++{
++    auto const height_inc = 13;
++    auto const min_height = 7;
++
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_height_increment(spec, height_inc);
++            mir_surface_spec_set_min_height(spec, min_height);
++        });
++
++    ensure_server_has_processed_setup();
++
++    auto const shell_surface = this->shell_surface.lock();
++    auto const bottom_right = shell_surface->input_bounds().bottom_right() - Displacement{1,1};
++
++    Size actual;
++    EXPECT_CALL(surface_observer, resized_to(_)).WillOnce(SaveArg<0>(&actual));
++
++    generate_alt_click_at(bottom_right);
++    generate_alt_move_to(bottom_right + DeltaY(16));
++
++    EXPECT_TRUE((actual.height.as_int() - min_height) % height_inc == 0);
++}
++
++TEST_F(SurfaceModifications, surface_spec_with_min_aspect_ratio_is_respected)
++{
++    auto const aspect_width = 11;
++    auto const aspect_height = 7;
++
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_min_aspect_ratio(spec, aspect_width, aspect_height);
++        });
++
++    ensure_server_has_processed_setup();
++
++    auto const shell_surface = this->shell_surface.lock();
++    auto const bottom_right = shell_surface->input_bounds().bottom_right() - Displacement{1,1};
++    auto const bottom_left = shell_surface->input_bounds().bottom_left() + Displacement{1,-1};
++
++    Size actual;
++    EXPECT_CALL(surface_observer, resized_to(_)).WillOnce(SaveArg<0>(&actual));
++
++    generate_alt_click_at(bottom_right);
++    generate_alt_move_to(bottom_left);
++
++    EXPECT_THAT(actual.width.as_float()/actual.height.as_float(), Ge(float(aspect_width)/aspect_height));
++}
++
++TEST_F(SurfaceModifications, surface_spec_with_max_aspect_ratio_is_respected)
++{
++    auto const aspect_width = 7;
++    auto const aspect_height = 11;
++
++    apply_changes([&](MirSurfaceSpec* spec)
++        {
++            mir_surface_spec_set_max_aspect_ratio(spec, aspect_width, aspect_height);
++        });
++
++    ensure_server_has_processed_setup();
++
++    auto const shell_surface = this->shell_surface.lock();
++    auto const bottom_right = shell_surface->input_bounds().bottom_right() - Displacement{1,1};
++    auto const top_right = shell_surface->input_bounds().top_right() - Displacement{1,-1};
++
++    Size actual;
++    EXPECT_CALL(surface_observer, resized_to(_)).WillOnce(SaveArg<0>(&actual));
++
++    generate_alt_click_at(bottom_right);
++    generate_alt_move_to(top_right);
++
++    EXPECT_THAT(actual.width.as_float()/actual.height.as_float(), Le(float(aspect_width)/aspect_height));
++}