Merge lp:~vanvugt/mir/managed-surface into lp:mir

[Daniel van Vugt]

Introduce surface wrapping and ManagedSurface as a default wrapper to
implement common window management policy.

As a first example this branch implements all the surface states.
Now client calls to mir_surface_set_state actually work. And key combos
have been added to the demo shell for minimize, maximize, fullscreen,
vertmaximize and restoring.

For those who do manual testing, beware you will run into some pre-existing bugs:
https://bugs.launchpad.net/mir/+bugs?field.tag=fullscreen

[lp:~vanvugt/mir/managed-surface]

2181. By Daniel van Vugt on 2014-12-12

Fix clang SIGILL crash by avoiding int-enum pointer casting.

2182. By Daniel van Vugt on 2014-12-15

Merge latest trunk and fix a conflict.

2183. By Daniel van Vugt on 2014-12-15

Fix conflict properly.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2183
http://jenkins.qa.ubuntu.com/job/mir-ci/2393/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/504
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/504
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/478/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/390
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/390/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/478
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/478/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3662/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16553

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

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

I wonder if it would be easier (avoiding "SurfaceWrapper"), to use a controller pattern,

e.g. ms::SurfaceController::configure(ms::Surface const&, ...) ms::SurfaceController::resize...etc

Eventually implementation of these methods will require access to a lot of system state. At present, the display layout is pulled in, you can see obviously how it blossoms though, e.g. choosing placement requires looking at existing surfaces.

Likewise it seems as if these managed surface instances may have to share state? for example if you wanted to implement behavior like only one surface can be fullscreen at a time.

I have the thought that it may be easier to encode all this in a centralized controller object. This would make the responsibilities more clear, surface stores the data, the controller object acts as a way to access surface which injects policy as well. What do you think?

[Daniel van Vugt (vanvugt)]

Robert:
I spent over a week trying out the existing controller pattern and discussed it at several team meetings. The result was something that worked, but so ugly that I decided not to propose it (lp:~vanvugt/mir/states). So I spent another week on this one which actually solves a lot more problems, more elegantly.

This branch has added advantages over SurfaceController:
 * You can't call a_scene_surface->some_method() and accidentally bypass the management policy.
 * Any surface functionality can be modified by the shell in future this way.
 * Any surface metadata can be neatly attached to a surface instance by the shell, without touching the core BasicSurface. e.g. the restore location and soon a lot more like decoration state, minimize location, animation state etc.

So this approach solves a whole bunch of problems. And admittedly even during my first prototype where everything was in SurfaceController, I was aware that these other problems would necessitate a surface wrapping approach eventually anyway.

P.S. Any number of surfaces can be full screen on normal desktops right now. While a shell can choose to restrict that, it would be unusual.

[lp:~vanvugt/mir/managed-surface]

2184. By Daniel van Vugt on 2014-12-16

Merge latest trunk

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2184
http://jenkins.qa.ubuntu.com/job/mir-ci/2433/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/518
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/430
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/430/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/518
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/518/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3680
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16609

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

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

* You can't call a_scene_surface->some_method() and accidentally bypass the management policy.

Hmm true...

 * Any surface functionality can be modified by the shell in future this way.

Kind of a blessing and a curse on this one though right? in the sense that total flexibility means you provide nothing.

 * Any surface metadata can be neatly attached to a surface instance by the shell, without touching the core BasicSurface. e.g. the restore location and soon a lot more like decoration state, minimize location, animation state etc.

This could be solved by association...

I'll investigate your states branch though...still yet to form a full opinion.

[Daniel van Vugt (vanvugt)]

This branch will conflict with lp:mir right now. But that should be resolved by:
https://code.launchpad.net/~vanvugt/mir/revert-r2165/+merge/245063

[Alan Griffiths (alan-griffiths)]

I'm uneasy about this approach without yet having clear ideas of whether alternatives are any better.

I'm not convinced that the scene surface should be wrapped nor that the wrapper belongs in scene. Wrapping suggests that there is more behavior to Surface than I feel is appropriate. Maybe some of the multiple "Surface" roles need splitting out.

Anyway, I'm rambling. I really don't have an opinion yet.

[Robert Carr (robertcarr)]

I've looked through the states branch some.

I'm still inclined to lean towards the controller approach. The biggest wart I saw in the states branch was the drag_surface method on SurfaceController. I would have chosen an approach where the shell code implements what you've put in the surface controller (the shell would then just invoke surface_controller->move)...perhaps SurfaceObservers play some role too (reactive approach).

In lack of any clear differentiator for controller v. wrapping I put the following arguments in favor of controller:

1. Easy to start with a smaller interface, e.g. no wrapper of "client_input_fd". Of course, you could call this a blessing or a curse (e.g. is the wrapper interface flexible or vague?).
2. Provides a nice level of decoupling, if shell code is using this controller interface then we gain some freedom on most of the surface interface (much of which could probably be removed...especially if we switch to something like surface->input_target() rather than surface IS a input_target)
3. I speculate (with a fair amount of confidence) that these wrapper objects will end up having to share data with eachother...however in the controller approach the data can remain as private controller state as opposed to psuedo-public data exposed through either the surface interface or the wrapper implementation class (and I guess the shell would dynamic_cast from the interface to wrapper impl).

As for the arguments in favor of surface wrapper

 * You can't call a_scene_surface->some_method() and accidentally bypass the management policy.

I think this can be solved by API if we mandate use of a controller object by the shell.

 * Any surface functionality can be modified by the shell in future this way.

This is the one which is discussed above as a "blessing/curse".

 * Any surface metadata can be neatly attached to a surface instance by the shell, without touching the core BasicSurface. e.g. the restore location and soon a lot more like decoration state, minimize location, animation state etc.

I "prefer association and non member methods".

[Daniel van Vugt (vanvugt)]

This branch has additional benefits you missed, over the controller branch:

 * No inconsistent hacking of some state information (but not all) and policy into BasicSurface. The other branch required strange bespoke hacks in BasicSurface.

 * This branch solves significantly more future problems which require attaching extra variables to each surface. Oh, maybe I mentioned that.

 * This branch is dramatically cleaner in design than the other one. I've spent several weeks on both approaches and the other one was so ugly it made me depressed frankly. So I'm much more excited about this one and don't really want to think about the other branch.

[Alan Griffiths (alan-griffiths)]

I definitely don't want to suggest that lp:~vanvugt/mir/states is better than this. But I don't like this either.

At the moment I feel that if this really is the best approach then there's something fundamental wrong with the Mir design. I think I'm going to have to try writing some code to get a feel for why more "obvious" solutions don't actually work.

[Cemil Azizoglu (cemil-azizoglu)]

It may work well for WM policy affecting only a single surface (e.g. minimize, maximize, etc) but for things that involve state(s) to be applied across a number of surfaces, I imagine there will have to be an encompassing "coordinator" object to oversee the policy is enforced.

I can see the ManagedSurface class morphing into that in the future. Of course, the name would have to change and it should no longer inherit from ms::surface.

I guess my question is how would then a policy involving multiple windows/surfaces be enforced with this approach?

[Daniel van Vugt (vanvugt)]

Actually it's worked out happening backwards to how you describe. We already have a "coordinator" in SurfaceController. And I did start using just that in the previous "states" branch. However I found it to be inadequate for the reasons outlined above.

SurfaceController remains and is still used in this branch. But regardless of how much responsibility you want to give it, at some stage you do need to attach metadata to each surface, beyond what we call BasicSurface. And given we can't predict what every future shell will want to "attach" to each surface, it needs to be something extendible.

The absence of this feature has long been a problem for Unity8. They've had to resort to storing surface information in relatively convoluted ways. Having spoken to Gerry in the past he expressed a preference for this kind of solution. And even within Mir you can see a shell like mir_proving_server needs to attach some extra data to each surface if it wanted (for example) to implement animations (not something we want implemented in Mir but living in the shell code).

So you need to keep in mind two major goals here:
  1. libmirserver implements some sane default window management policy, so that shells don't need to reinvent the wheel each time.
  2. libmirserver needs to provide shells with a convenient mechanism of attaching their own member variables to each surface. Without the overhead of extra association containers to keep in sync.

The only reason Mir isn't yet superior to Compiz is because we haven't yet agreed on how to solve this problem (despite several solutions having been proposed). This being my third or fourth attempt at landing a solution to this problem, I believe this is by far the nicest solution yet. It addresses the main goals and is sufficiently elegant that I'm excited about building on it in future (instead of depressed as the "states" branch made me).

I'm always happy to accept a superior alternate solution. But it needs to be superior to what already exists.

[Cemil Azizoglu (cemil-azizoglu)]

It's not a matter of having a superior solution. I'm trying to convince myself (or be convinced) that this is a solution at all for complex use cases involving metadata affecting multiple surfaces.

I don't disagree with shells wanting to attach per-surface metadata. I am more worried about metadata that is NOT per-surface. With this approach, such data would need to be replicated in each ManagedSurface instance. Also, there would have to be a "coordinator" to get the job done, which seems to be missing in this approach (or at least I'm not seeing it).

Perhaps you can provide a concrete example (code would be nice) of how such a complex policy involving multiple surfaces would be enforced. Perhaps a carousel WM that you can slide your finger to turn it to browse various app windows you have open. The surfaces would be rendered on the sides of the carousel. As the carousel is turning, I imagine you'd need to keep track of how much it has turned so you can continue to render your windows in their respective positions around the carousel's perimeter. But how much it has turned is not a per-surface info, and it's awkward to treat it as such.

[Daniel van Vugt (vanvugt)]

> I don't disagree with shells wanting to attach per-surface metadata. I am more
> worried about metadata that is NOT per-surface. With this approach, such data
> would need to be replicated in each ManagedSurface instance. Also, there would
> have to be a "coordinator" to get the job done, which seems to be missing in
> this approach (or at least I'm not seeing it).

Absolutely that kind of thing will be required for some features. The example I like to think of is snapping ("magnetic") windows that like to align and attach to each other. But I don't intend to use ManagedSurface for everything, and don't intend on any "replication"/duplication.

We do have a "coordinator" already in the pre-existing class "SurfaceController". You don't see it much in this diff though because I didn't have to use it much for the surface states. You do see it used extensively in my previous adventure:
https://code.launchpad.net/~vanvugt/mir/states/+merge/243960

I think when the time comes that we need inter-surface behaviours, some of the wiring I prototyped in the above branch will be introduced.

[Daniel van Vugt (vanvugt)]

We do many effects like a "carousel" in Compiz and the Mir equivalent of that is already planned in the form of Surface/Renderable::transformation() for arbitrary 3D transformations of surfaces. But we don't really demonstrate it at all other than in mir_demo_standalone_render_surfaces.

When more people make use of Mir's 3D transformations I suspect the class design might change to make it easier to use. Although a shell might decide to ignore it completely and transform everything itself (like Unity8).

[Alan Griffiths (alan-griffiths)]

*Needs Discussion*

I think that my dislike of this and related branches comes from inconsistencies that have crept into the design of Mir.

We started Mir with the intent to have an MVC structure to our system with the model being the shared data representation of the system state, the views being subsystems like compositor, frontend and input and the controller being where the the shell plugs in.

But through a series of pragmatic decisions we've got to a point that is far away from there. There are items missing from the "model" we have in scene (like the output devices), there are things in the model that belong to the controller (like the surface state), there are interactions that bypass the controller and update the model (like the frontend creating surfaces on a session instead of forwarding the request to the shell).

It sort-of-works but it gets increasingly clunky and I think we can usefully rework towards the original intent.

This MP continues in this misguided direction by putting logic and data to handle surface state changes into the model (as ManagedSurface). It is like saying that because the walls of a building are not sound we'll fit a shelf by building a scaffold outside and mount the shelf on a pair of horizontals that come through the window.

I think the right approach is to fix the current architecture rather that trying to work around it. Clearly that implies we should first agree what needs fixing and how.

review: Needs Information

[Robert Carr (robertcarr)]

I've tried to outline in improved-tiling-window-management MR some of the ways I'd like to see the architecture improve and how we can really get the window manager as an elegant controller. In light of being convinced it's fatally flawed, I am definitely inclined to lean towards continue working on the controller based approach than add this surface wrapper.

I hate to feel like I am repeating myself a fourth or fifth time on the same topic but I will try and summarize my thoughts again for the benefit of EU/AUS time discussion:

1. Model-View-Controller is a well known and understood pattern which has several additional benefits for us: Decoupling of model object (e.g.) surface from interface used to control it, and an easier way to store intra-surface state.
2. As I've outlined in the tiling-window-management MR I think untangling the controller may be able to provide a significant simplification to our current factory interfaces and the path from session mediator to scene.

[Robert Carr (robertcarr)]

3. I believe all the arguments in favor of this branch are bunk. I'll outline them trying to skip repetitions of the same argument:
3a. "You can't call a_scene_surface->some_method() and accidentally bypass the management policy."

This can be solved by API.

3b * Any surface functionality can be modified by the shell in future this way.

This is a tautology as an absolutism (define a C++ interface which prevents someone from doing something), and bad interface design as an ideal (maximal flexibility = doing nothing).

3c * Any surface metadata can be neatly attached to a surface instance by the shell, without touching the core BasicSurface. e.g. the restore location and soon a lot more like decoration state, minimize location, animation state etc.

This seems to be inventing a problem to justify a design. We all know this can be trivially solved by association.

3d * So I'm much more excited about this one and don't really want to think about the other branch.

I'm very excited about the improved tiling wm branch and don't really want to think about this branch.

[Daniel van Vugt (vanvugt)]

Alan:
I share your concerns and agree with your statements. If there's a way for us to get to a cleaner design, then I don't mind implementing surface management logic a 4th/5th/6th time. I would like us to be able to land some small first steps, somehow.

Robert:
I think your arguments lack objective justification:

3. OK, demonstrate it in a way that does not make things more complex, again.

3b. Flexibility is a good thing. I aim for code reuse to make things easier in the future. And this has been proven successful in the code I have landed in Mir over the past 2 years.

3c. I am not "inventing a problem". The problem is real as firstly demonstrated below (ManagedSurface contains restore_rect, and in future more such data like minimize_rect). Secondly, it's been clear for a long time that one cannot implement animations for example without storing animation state somewhere. And you agree "somewhere" is required. Your suggested "somewhere" of association is a hack that is inelegant and difficult to maintain. Because a separate associative container not only needs to be kept in sync with the real list of surfaces, but also notified of all surface changes (ManagedSurface solves both these problems easily).

3d. The tiling branch shouldn't come into the discussion yet. It doesn't solve the same problems as this branch does. I remind you of the requirements I seek to fulfil:

  1. libmirserver implements some sane default window management policy, so that shells don't need to reinvent the wheel each time.
  2. libmirserver needs to provide shells with a convenient mechanism of attaching their own member variables to each surface. Without the overhead of extra association containers to keep in sync.

This branch solves both while the tiling branches solve neither (yet).

But a single branch doesn't need to solve both. If it makes people happier, I can divide this one up into smaller pieces.

[Daniel van Vugt (vanvugt)]

It's worth noting that the window management logic is almost trivial in the grand scheme of things. It's 100 lines long (see 347-450), so 10% of the proposal. Everything else we're talking about here is architectural improvement and making Mir a better codebase for the future...

One can trivially implement those 100 lines of WM logic almost anywhere. The challenge is to make it elegant and reusable to solve other problems too. That's what I'm trying to do here.

[lp:~vanvugt/mir/managed-surface]

2185. By Daniel van Vugt on 2014-12-16

Merge latest trunk

2186. By Daniel van Vugt on 2014-12-17

Merge latest trunk

2187. By Daniel van Vugt on 2014-12-17

Merge latest trunk

2188. By Daniel van Vugt on 2014-12-17

Merge latest trunk

2189. By Daniel van Vugt on 2014-12-19

Merge latest trunk

2190. By Daniel van Vugt on 2014-12-29

Merge latest trunk

2191. By Daniel van Vugt on 2015-01-05

Merge latest trunk

2192. By Daniel van Vugt on 2015-01-06

Merge latest trunk

2193. By Daniel van Vugt on 2015-01-07

Merge latest trunk

2194. By Daniel van Vugt on 2015-01-09

Merge latest trunk

2195. By Daniel van Vugt on 2015-01-12

Merge latest trunk

2196. By Daniel van Vugt on 2015-01-14

Merge latest trunk

2197. By Daniel van Vugt on 2015-01-15

Merge latest trunk and fix a conflict.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2197
http://jenkins.qa.ubuntu.com/job/mir-ci/2656/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/863
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/863
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/825
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/653
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/653/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/825
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/825/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3942
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17097

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

review: Approve (continuous-integration)

[lp:~vanvugt/mir/managed-surface]

2198. By Daniel van Vugt on 2015-01-15

Merge latest trunk

2199. By Daniel van Vugt on 2015-01-15

Merge latest trunk

2200. By Daniel van Vugt on 2015-01-15

Merge latest trunk and fix conflicts.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2200
http://jenkins.qa.ubuntu.com/job/mir-ci/2658/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/868
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/868
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/830
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/655
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/655/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/830
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/830/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3947
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17107

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

Surface wrapper looks like a convenient class to have for mir server clients (though a client could of course roll their own as it's a straightforward wrapper), though that implies the client is willing to use dynamic_cast for downcasting, instead of using other means like associative containers.

As for managed surface, I would prefer a cleaner delineation between model and window management code (which should mostly be in the controller side of things) instead of combining both controller+model data into one object.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I didn't choose the name SurfaceWrapper. I forget what I had before but Alan insisted on that name :)

As for pure MVC vs conventional object orientation, yeah I partially agree for some things, but not others. You can see elsewhere I'm trying to make the controller approach available too:
  https://code.launchpad.net/~vanvugt/mir/wire-up-SurfaceController/+merge/246281
which works, and pre-dates this branch. Although I don't think everything would use that.

For example, we still need SurfaceWrapper (or something like it) to attach state to each surface from a bespoke shell, without needing external containers.

I did mention above three days ago that I'm willing to split this into smaller pieces that might better satisfy people in part. The only risk with smaller branches is that people (do) complain about not seeing the bigger picture to understand the intent.

[lp:~vanvugt/mir/managed-surface]

2201. By Daniel van Vugt on 2015-01-16

Merge latest trunk

2202. By Daniel van Vugt on 2015-01-16

Add support for the newly introduced mir_surface_state_horizmaximized

2203. By Daniel van Vugt on 2015-01-16

More tests to cover the new horizmaximized semantics

2204. By Daniel van Vugt on 2015-01-16

Update docs

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2204
http://jenkins.qa.ubuntu.com/job/mir-ci/2674/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/887
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/887
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/849
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/671
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/671/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/849
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/849/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3962
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17143

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

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

>> For example, we still need SurfaceWrapper (or something like it) to attach state to each surface
>> from a bespoke shell, without needing external containers.

I prefer this sentence backwards.

"For example, we still need a hash table (or something like it) to attach state to each surface from
 a bespoke shell, without needing surface wrappers"

review: Disapprove

[Daniel van Vugt (vanvugt)]

I think all this metadata can still be pointed to by a single instance of each surface object instead. We don't need to add new containers for that.

A hash table would obviously work, but it's neither the most efficient nor elegant solution. If you delete a surface for example, you'd also need to add all the logic to ensure the hash tables/maps you're maintaining in parallel get updated. Not to mention other methods during runtime. For example, if someone does set_state(foo) and your bespoke shell has an animation for foo, you need to have some wiring to initiate that. These are the future problems the approach proposed here solves.

While this branch is one example of achieving the goal without adding any new containers, there must be other ways too...

[lp:~vanvugt/mir/managed-surface]

2205. By Daniel van Vugt on 2015-01-19

Merge latest trunk

2206. By Daniel van Vugt on 2015-01-19

Fix logical conflict between the new "hidden" state and default
minimize behaviour.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2206
http://jenkins.qa.ubuntu.com/job/mir-ci/2691/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/906
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/906
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/868
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/688
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/688/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/868
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/868/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3979
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17178

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

BTW, if you want a pure controller approach please help me to make progress on that front too:
https://code.launchpad.net/~vanvugt/mir/wire-up-SurfaceController/+merge/246281

[lp:~vanvugt/mir/managed-surface]

2207. By Daniel van Vugt on 2015-01-20

Merge latest trunk

2208. By Daniel van Vugt on 2015-01-21

Merge latest trunk

2209. By Daniel van Vugt on 2015-01-21

Merge latest trunk

2210. By Daniel van Vugt on 2015-01-22

Merge latest trunk

2211. By Daniel van Vugt on 2015-01-22

Fix a conflict from upstream changes.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2211
http://jenkins.qa.ubuntu.com/job/mir-ci/2761/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/992
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/991/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/953
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/758
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/758/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/953
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/953/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4044
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17332

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

review: Needs Fixing (continuous-integration)

[lp:~vanvugt/mir/managed-surface]

2212. By Daniel van Vugt on 2015-01-22

Ping Jenkins

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2212
http://jenkins.qa.ubuntu.com/job/mir-ci/2769/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1001
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1000/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/962
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/766
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/766/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/962
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/962/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4050
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17339

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

review: Needs Fixing (continuous-integration)

[lp:~vanvugt/mir/managed-surface]

2213. By Daniel van Vugt on 2015-01-23

Merge latest trunk

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2213
http://jenkins.qa.ubuntu.com/job/mir-ci/2790/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1035
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1034
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/996/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/787
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/787/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/996/console

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Bug 1413748 again. Unrelated to the proposal -- it's happening on all branches.

[Alan Griffiths (alan-griffiths)]

> *Needs Discussion*
>
> I think that my dislike of this and related branches comes from
> inconsistencies that have crept into the design of Mir.
>
> We started Mir with the intent to have an MVC structure to our system with the
> model being the shared data representation of the system state, the views
> being subsystems like compositor, frontend and input and the controller being
> where the the shell plugs in.
>
> But through a series of pragmatic decisions we've got to a point that is far
> away from there. There are items missing from the "model" we have in scene
> (like the output devices), there are things in the model that belong to the
> controller (like the surface state), there are interactions that bypass the
> controller and update the model (like the frontend creating surfaces on a
> session instead of forwarding the request to the shell).
>
> It sort-of-works but it gets increasingly clunky and I think we can usefully
> rework towards the original intent.
>
> This MP continues in this misguided direction by putting logic and data to
> handle surface state changes into the model (as ManagedSurface). It is like
> saying that because the walls of a building are not sound we'll fit a shelf by
> building a scaffold outside and mount the shelf on a pair of horizontals that
> come through the window.
>
> I think the right approach is to fix the current architecture rather that
> trying to work around it. Clearly that implies we should first agree what
> needs fixing and how.

Discussions have been had and we're working ong fixing the architecture

review: Disapprove

[lp:~vanvugt/mir/managed-surface]

2214. By Daniel van Vugt on 2015-01-27

Merge latest trunk.

[Daniel van Vugt (vanvugt)]

OK, well more of the team might prefer MVC (although that's not yet certain). I'm a bit disappointed this branch isn't going anywhere after being up for review for 7 weeks.

Still, parts of this branch can be easily salvaged for future proposals.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2214
http://jenkins.qa.ubuntu.com/job/mir-ci/2818/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1074
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1073
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1035
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/815
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/815/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1035
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1035/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4093
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17492

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

review: Approve (continuous-integration)

Unmerged revisions

2214. By Daniel van Vugt on 2015-01-27

Merge latest trunk.

2213. By Daniel van Vugt on 2015-01-23

Merge latest trunk

2212. By Daniel van Vugt on 2015-01-22

Ping Jenkins

2211. By Daniel van Vugt on 2015-01-22

Fix a conflict from upstream changes.

2210. By Daniel van Vugt on 2015-01-22

Merge latest trunk

2209. By Daniel van Vugt on 2015-01-21

Merge latest trunk

2208. By Daniel van Vugt on 2015-01-21

Merge latest trunk

2207. By Daniel van Vugt on 2015-01-20

Merge latest trunk

2206. By Daniel van Vugt on 2015-01-19

Fix logical conflict between the new "hidden" state and default
minimize behaviour.

2205. By Daniel van Vugt on 2015-01-19

Merge latest trunk

=== modified file 'doc/demo_shell_controls.md'
--- doc/demo_shell_controls.md	2015-01-16 02:57:31 +0000
+++ doc/demo_shell_controls.md	2015-01-27 03:20:51 +0000
@@ -46,5 +46,10 @@
  - Reset display mode to default, on focussed monitor: *Ctrl-Alt-0*
  - Adjust window opacity/alpha: *Alt-mousewheel*
  - Zoom in/out: *Super-mousewheel*
+ - Minimize/restore the focussed window: *Alt-F9*
+ - Maximize/restore the focussed window: *Alt-F10*
+ - Fullscreen/restore the focussed window: *Alt-F11*
+ - Vertically maximize/restore the focussed window: *Alt-F12*
+ - Horizontally maximize/restore the focussed window: *Shift-Alt-F12*
 
 Want more? Log your requests at: https://bugs.launchpad.net/mir/+filebug
 
=== modified file 'examples/eglapp.c'
--- examples/eglapp.c	2015-01-21 00:49:28 +0000
+++ examples/eglapp.c	2015-01-27 03:20:51 +0000
@@ -155,29 +155,6 @@
     }
 }
 
-static const MirDisplayOutput *find_active_output(
-    const MirDisplayConfiguration *conf)
-{
-    const MirDisplayOutput *output = NULL;
-    int d;
-
-    for (d = 0; d < (int)conf->num_outputs; d++)
-    {
-        const MirDisplayOutput *out = conf->outputs + d;
-
-        if (out->used &&
-            out->connected &&
-            out->num_modes &&
-            out->current_mode < out->num_modes)
-        {
-            output = out;
-            break;
-        }
-    }
-
-    return output;
-}
-
 mir_eglapp_bool mir_eglapp_init(int argc, char *argv[],
                                 unsigned int *width, unsigned int *height)
 {
@@ -199,6 +176,7 @@
     unsigned int output_id = mir_display_output_id_invalid;
     char *mir_socket = NULL;
     char const* cursor_name = mir_default_cursor_name;
+    MirSurfaceState initial_state = mir_surface_state_restored;
 
     if (argc > 1)
     {
@@ -256,8 +234,7 @@
                     }
                     break;
                 case 'f':
-                    *width = 0;
-                    *height = 0;
+                    initial_state = mir_surface_state_fullscreen;
                     break;
                 case 's':
                     {
@@ -329,16 +306,6 @@
     MirDisplayConfiguration* display_config =
         mir_connection_create_display_config(connection);
 
-    const MirDisplayOutput *output = find_active_output(display_config);
-
-    if (output == NULL)
-    {
-        printf("No active outputs found.\n");
-        return 0;
-    }
-
-    const MirDisplayMode *mode = &output->modes[output->current_mode];
-
     unsigned int format[mir_pixel_formats];
     unsigned int nformats;
 
@@ -360,14 +327,15 @@
         }
     }
 
-    printf("Current active output is %dx%d %+d%+d\n",
-           mode->horizontal_resolution, mode->vertical_resolution,
-           output->position_x, output->position_y);
+    if (*width == 0 && *height == 0)
+    {
+        initial_state = mir_surface_state_fullscreen;
 
-    if (*width == 0)
-        *width = mode->horizontal_resolution;
-    if (*height == 0)
-        *height = mode->vertical_resolution;
+        // These should be ignored due to the initial_state, but it's
+        // unfortunately required that they're never zero:
+        *width = 1;
+        *height = 1;
+    }
 
     mir_display_config_destroy(display_config);
 
@@ -397,6 +365,7 @@
 
     CHECK(mir_surface_is_valid(surface), "Can't create a surface");
 
+    mir_surface_set_state(surface, initial_state);
     mir_surface_set_event_handler(surface, &delegate);
     
     MirCursorConfiguration *conf = mir_cursor_configuration_from_name(cursor_name);
 
=== modified file 'playground/demo-shell/window_manager.cpp'
--- playground/demo-shell/window_manager.cpp	2015-01-21 21:08:50 +0000
+++ playground/demo-shell/window_manager.cpp	2015-01-27 03:20:51 +0000
@@ -130,6 +130,23 @@
 
 } // namespace
 
+void me::WindowManager::toggle(MirSurfaceState state)
+{
+    if (auto const app = focus_controller->focussed_application().lock())
+    {
+        if (auto const surf = app->default_surface())
+            toggle(*surf, state);
+    }
+}
+
+void me::WindowManager::toggle(scene::Surface& surface, MirSurfaceState state)
+{
+    auto new_state = state;
+    if (new_state == surface.state())
+        new_state = mir_surface_state_restored;
+
+    surface.configure(mir_surface_attrib_state, new_state);
+}
 
 void me::WindowManager::toggle(ColourEffect which)
 {
@@ -217,6 +234,30 @@
             return true;
         }
         else if (event.key.modifiers & mir_key_modifier_alt &&
+                 event.key.scan_code == KEY_F9)
+        {
+            toggle(mir_surface_state_minimized);
+        }
+        else if (event.key.modifiers & mir_key_modifier_alt &&
+                 event.key.scan_code == KEY_F10)
+        {
+            toggle(mir_surface_state_maximized);
+        }
+        else if (event.key.modifiers & mir_key_modifier_alt &&
+                 event.key.scan_code == KEY_F11)
+        {
+            toggle(mir_surface_state_fullscreen);
+        }
+        else if (event.key.modifiers & mir_key_modifier_alt &&
+                 event.key.scan_code == KEY_F12)
+        {
+            MirSurfaceState state =
+                event.key.modifiers & mir_key_modifier_shift ?
+                mir_surface_state_horizmaximized :
+                mir_surface_state_vertmaximized;
+            toggle(state);
+        }
+        else if (event.key.modifiers & mir_key_modifier_alt &&
                  event.key.scan_code == KEY_F4)
         {
             auto const app = focus_controller->focussed_application().lock();
 
=== modified file 'playground/demo-shell/window_manager.h'
--- playground/demo-shell/window_manager.h	2015-01-20 10:07:15 +0000
+++ playground/demo-shell/window_manager.h	2015-01-27 03:20:51 +0000
@@ -23,6 +23,7 @@
 #include "mir/input/scene.h"
 #include "mir/geometry/displacement.h"
 #include "mir/geometry/size.h"
+#include "mir/scene/surface.h"
 #include "demo_renderer.h"
 
 #include <memory>
@@ -66,6 +67,9 @@
     WindowManager& operator=(const WindowManager&) = delete;
 
 private:
+    void toggle(MirSurfaceState state);
+    void toggle(scene::Surface& surface, MirSurfaceState state);
+
     std::shared_ptr<shell::FocusController> focus_controller;
     std::shared_ptr<graphics::Display> display;
     std::shared_ptr<compositor::Compositor> compositor;
 
=== added file 'src/include/server/mir/scene/surface_wrapper.h'
--- src/include/server/mir/scene/surface_wrapper.h	1970-01-01 00:00:00 +0000
+++ src/include/server/mir/scene/surface_wrapper.h	2015-01-27 03:20:51 +0000
@@ -0,0 +1,83 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#ifndef MIR_SCENE_SURFACE_WRAPPER_H_
+#define MIR_SCENE_SURFACE_WRAPPER_H_
+
+#include "mir/scene/surface.h"
+#include <memory>
+
+namespace mir { namespace scene {
+
+class SurfaceWrapper : public scene::Surface
+{
+public:
+    SurfaceWrapper(std::shared_ptr<Surface> const& impl);
+    virtual ~SurfaceWrapper();
+
+    // This might look like a long list, but the beauty of this is that once
+    // you inherit from SurfaceWrapper, you will never have to remember
+    // or maintain all these. Just override the function you need...
+
+    virtual std::shared_ptr<mir::input::InputChannel> input_channel() const override;
+    virtual mir::input::InputReceptionMode reception_mode() const override;
+    virtual std::string name() const override;
+    virtual geometry::Point top_left() const override;
+    virtual geometry::Size client_size() const override;
+    virtual geometry::Size size() const override;
+    virtual geometry::Rectangle input_bounds() const override;
+    virtual bool input_area_contains(mir::geometry::Point const&) const override;
+    virtual std::unique_ptr<graphics::Renderable> compositor_snapshot(void const*) const override;
+    virtual float alpha() const override;
+    virtual MirSurfaceType type() const override;
+    virtual MirSurfaceState state() const override;
+    virtual void hide() override;
+    virtual void show() override;
+    virtual bool visible() const override;
+    virtual void move_to(geometry::Point const&) override;
+    virtual void take_input_focus(std::shared_ptr<shell::InputTargeter> const&) override;
+    virtual void set_input_region(std::vector<geometry::Rectangle> const&) override;
+    virtual void allow_framedropping(bool) override;
+    virtual void resize(geometry::Size const&) override;
+    virtual void set_transformation(glm::mat4 const&) override;
+    virtual void set_alpha(float) override;
+    virtual void set_orientation(MirOrientation) override;
+    virtual void force_requests_to_complete() override;
+    virtual void add_observer(std::shared_ptr<SurfaceObserver> const&) override;
+    virtual void remove_observer(std::weak_ptr<SurfaceObserver> const&) override;
+    virtual void set_reception_mode(input::InputReceptionMode mode) override;
+    virtual void consume(MirEvent const&) override;
+    virtual void set_cursor_image(std::shared_ptr<graphics::CursorImage> const&) override;
+    virtual std::shared_ptr<graphics::CursorImage> cursor_image() const override;
+    virtual void request_client_surface_close() override;
+    virtual MirPixelFormat pixel_format() const override;
+    virtual void swap_buffers(graphics::Buffer*, std::function<void(graphics::Buffer*)>) override;
+    virtual bool supports_input() const override;
+    virtual int client_input_fd() const override;
+    virtual int configure(MirSurfaceAttrib, int) override;
+    virtual int query(MirSurfaceAttrib) override;
+    virtual void with_most_recent_buffer_do(std::function<void(graphics::Buffer&)> const& ) override;
+    virtual std::shared_ptr<Surface> parent() const override;
+
+protected:
+    std::shared_ptr<Surface> const raw_surface;
+};
+
+}} // namespace mir::scene
+
+#endif // MIR_SCENE_SURFACE_WRAPPER_H_
 
=== modified file 'src/server/scene/CMakeLists.txt'
--- src/server/scene/CMakeLists.txt	2015-01-14 06:39:13 +0000
+++ src/server/scene/CMakeLists.txt	2015-01-27 03:20:51 +0000
@@ -25,4 +25,6 @@
   prompt_session_manager_impl.cpp
   rendering_tracker.cpp
   default_coordinate_translator.cpp
+  surface_wrapper.cpp
+  managed_surface.cpp
 )
 
=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2015-01-22 03:10:13 +0000
+++ src/server/scene/basic_surface.cpp	2015-01-27 03:20:51 +0000
@@ -367,7 +367,9 @@
 
 bool ms::BasicSurface::visible(std::unique_lock<std::mutex>&) const
 {
-    return !hidden && first_frame_posted;
+    return !hidden
+        && first_frame_posted
+        && state_ != mir_surface_state_hidden;  // not overridable (?)
 }
 
 mi::InputReceptionMode ms::BasicSurface::reception_mode() const
@@ -435,7 +437,6 @@
     {
         state_ = s;
         lg.unlock();
-        set_hidden(s == mir_surface_state_hidden);
         
         observers.attrib_changed(mir_surface_attrib_state, s);
     }
 
=== modified file 'src/server/scene/default_configuration.cpp'
--- src/server/scene/default_configuration.cpp	2015-01-19 17:20:10 +0000
+++ src/server/scene/default_configuration.cpp	2015-01-27 03:20:51 +0000
@@ -90,6 +90,7 @@
                 std::make_shared<ms::SurfaceController>(
                     the_surface_factory(),
                     the_placement_strategy(),
+                    the_shell_display_layout(),
                     the_surface_stack_model()));
         });
 }
 
=== added file 'src/server/scene/managed_surface.cpp'
--- src/server/scene/managed_surface.cpp	1970-01-01 00:00:00 +0000
+++ src/server/scene/managed_surface.cpp	2015-01-27 03:20:51 +0000
@@ -0,0 +1,161 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#include "managed_surface.h"
+
+namespace mir { namespace scene {
+
+ManagedSurface::ManagedSurface(std::shared_ptr<Surface> const& raw,
+                       std::shared_ptr<shell::DisplayLayout> const& layout)
+    : SurfaceWrapper(raw)
+    , display_layout(layout)
+    , restore_rect{raw->top_left(), raw->size()}
+{
+}
+
+ManagedSurface::~ManagedSurface()
+{
+}
+
+int ManagedSurface::configure(MirSurfaceAttrib attrib, int value)
+{
+    int new_value = value;
+
+    if (attrib == mir_surface_attrib_state)
+        new_value = set_state(static_cast<MirSurfaceState>(value));
+
+    return SurfaceWrapper::configure(attrib, new_value);
+}
+
+MirSurfaceState ManagedSurface::set_state(MirSurfaceState desired)
+{
+    // TODO: Eventually this whole function should be atomic (LP: #1395957)
+    geometry::Rectangle old_win{top_left(), size()}, new_win = old_win;
+
+    auto fullscreen = old_win;
+    display_layout->size_to_output(fullscreen);
+
+    // TODO: Shell should define workarea to exclude panels/launchers/docks
+    auto workarea = fullscreen;
+
+    auto old_state = state();
+    if (old_state != desired)
+    {
+        switch (old_state)
+        {
+        case mir_surface_state_minimized:
+            show();
+            break;
+        case mir_surface_state_restored:
+            restore_rect = old_win;
+            break;
+        default:
+            break;
+        }
+    }
+
+    switch (desired)
+    {
+    case mir_surface_state_fullscreen:
+        new_win = fullscreen;
+        break;
+    case mir_surface_state_maximized:
+        new_win = workarea;
+        break;
+    case mir_surface_state_vertmaximized:
+        new_win.top_left.y = workarea.top_left.y;
+        new_win.size.height = workarea.size.height;
+        break;
+    case mir_surface_state_horizmaximized:
+        new_win.top_left.x = workarea.top_left.x;
+        new_win.size.width = workarea.size.width;
+        break;
+    case mir_surface_state_restored:
+        new_win = restore_rect;
+        break;
+    case mir_surface_state_minimized:
+        hide();
+        break;
+    default:
+        break;
+    }
+
+    if (new_win != old_win)
+    {
+        /*
+         * Important: Call to parent class move_to/resize functions.
+         * We don't want policy enforcement of this class getting in the way
+         * here because our own versions will switch based on current state(),
+         * which is incorrect behaviour for the 'desired' new state.
+         */
+        SurfaceWrapper::move_to(new_win.top_left);
+        SurfaceWrapper::resize(new_win.size);
+    }
+
+    // TODO: In future the desired state may be rejected based on other
+    //       factors such as surface type.
+    return desired;
+}
+
+void ManagedSurface::move_to(geometry::Point const& desired)
+{
+    // TODO: Eventually this whole function should be atomic (LP: #1395957)
+    auto new_pos = desired;
+
+    switch (state())
+    {
+        case mir_surface_state_fullscreen:
+        case mir_surface_state_maximized:
+            return;
+        case mir_surface_state_vertmaximized:
+            new_pos.y = top_left().y;
+            break;
+        case mir_surface_state_horizmaximized:
+            new_pos.x = top_left().x;
+            break;
+        default:
+            break;
+    }
+
+    SurfaceWrapper::move_to(new_pos);
+}
+
+void ManagedSurface::resize(geometry::Size const& desired)
+{
+    // TODO: Eventually this whole function should be atomic (LP: #1395957)
+    auto new_size = desired;
+
+    switch (state())
+    {
+        case mir_surface_state_fullscreen:
+        case mir_surface_state_maximized:
+            return;
+        case mir_surface_state_vertmaximized:
+            new_size.height = size().height;
+            break;
+        case mir_surface_state_horizmaximized:
+            new_size.width = size().width;
+            break;
+        default:
+            break;
+    }
+
+    SurfaceWrapper::resize(new_size);
+}
+
+}} // namespace mir::scene
 
=== added file 'src/server/scene/managed_surface.h'
--- src/server/scene/managed_surface.h	1970-01-01 00:00:00 +0000
+++ src/server/scene/managed_surface.h	2015-01-27 03:20:51 +0000
@@ -0,0 +1,50 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#ifndef MIR_SCENE_MANAGED_SURFACE_H_
+#define MIR_SCENE_MANAGED_SURFACE_H_
+
+#include "mir/scene/surface_wrapper.h"
+#include "mir/shell/display_layout.h"
+#include "mir/geometry/rectangle.h"
+#include "mir_toolkit/common.h"
+#include <memory>
+
+namespace mir { namespace scene {
+
+class ManagedSurface : public SurfaceWrapper
+{
+public:
+    ManagedSurface(std::shared_ptr<Surface> const&,
+                   std::shared_ptr<shell::DisplayLayout> const&);
+    virtual ~ManagedSurface();
+
+    int configure(MirSurfaceAttrib attrib, int value) override;
+    void move_to(geometry::Point const&) override;
+    void resize(geometry::Size const&) override;
+
+private:
+    std::shared_ptr<shell::DisplayLayout> const display_layout;
+    geometry::Rectangle restore_rect;
+
+    MirSurfaceState set_state(MirSurfaceState state);
+};
+
+}} // namespace mir::scene
+
+#endif // MIR_SCENE_MANAGED_SURFACE_H_
 
=== modified file 'src/server/scene/surface_controller.cpp'
--- src/server/scene/surface_controller.cpp	2015-01-14 06:39:13 +0000
+++ src/server/scene/surface_controller.cpp	2015-01-27 03:20:51 +0000
@@ -21,26 +21,36 @@
 #include "mir/scene/surface_factory.h"
 #include "mir/scene/surface.h"
 #include "mir/scene/placement_strategy.h"
+#include "mir/shell/display_layout.h"
+#include "managed_surface.h"
 
 namespace ms = mir::scene;
 
 ms::SurfaceController::SurfaceController(
     std::shared_ptr<SurfaceFactory> const& surface_factory,
     std::shared_ptr<ms::PlacementStrategy> const& placement_strategy,
+    std::shared_ptr<shell::DisplayLayout> const& display_layout,
     std::shared_ptr<SurfaceStackModel> const& surface_stack) :
     surface_factory(surface_factory),
     placement_strategy(placement_strategy),
+    display_layout(display_layout),
     surface_stack(surface_stack)
 {
 }
 
+std::shared_ptr<ms::Surface> ms::SurfaceController::wrap_surface(
+    std::shared_ptr<ms::Surface> const& raw)
+{
+    return std::make_shared<ManagedSurface>(raw, display_layout);
+}
+
 std::shared_ptr<ms::Surface> ms::SurfaceController::add_surface(
     SurfaceCreationParameters const& params,
     Session* session)
 {
     auto placed_params = placement_strategy->place(*session, params);
 
-    auto const surface = surface_factory->create_surface(placed_params);
+    auto const surface = wrap_surface(surface_factory->create_surface(placed_params));
     surface_stack->add_surface(surface, placed_params.depth, placed_params.input_mode);
     return surface;
 }
 
=== modified file 'src/server/scene/surface_controller.h'
--- src/server/scene/surface_controller.h	2015-01-14 06:39:13 +0000
+++ src/server/scene/surface_controller.h	2015-01-27 03:20:51 +0000
@@ -24,6 +24,9 @@
 
 namespace mir
 {
+
+namespace shell { class DisplayLayout; }
+
 namespace scene
 {
 class PlacementStrategy;
@@ -37,6 +40,7 @@
     SurfaceController(
         std::shared_ptr<SurfaceFactory> const& surface_factory,
         std::shared_ptr<PlacementStrategy> const& placement_strategy,
+        std::shared_ptr<shell::DisplayLayout> const& display_layout,
         std::shared_ptr<SurfaceStackModel> const& surface_stack);
 
     std::shared_ptr<Surface> add_surface(
@@ -47,9 +51,24 @@
 
     void raise(std::weak_ptr<Surface> const& surface) override;
 
+    /**
+     * wrap_surface allows you wrap ("decorate") a surface in your own
+     * window management policy-enforcing class. It's important to keep
+     * the wrapping stage out of SurfaceFactory, so that a shell may choose
+     * to both partly reuse the default wrapper (ManagedSurface) for
+     * window management logic while also retaining access to the underlying
+     * BasicSurface if it wants to do anything that would be disallowed by
+     * the default policy of ManagedSurface.
+     * TODO: Move this up to SurfaceCoordinator (or some bespoke factory)
+     *       in the public API later.
+     */
+    virtual std::shared_ptr<Surface>
+        wrap_surface(std::shared_ptr<Surface> const& raw);
+
 private:
     std::shared_ptr<SurfaceFactory> const surface_factory;
     std::shared_ptr<PlacementStrategy> const placement_strategy;
+    std::shared_ptr<shell::DisplayLayout> const display_layout;
     std::shared_ptr<SurfaceStackModel> const surface_stack;
 };
 
 
=== added file 'src/server/scene/surface_wrapper.cpp'
--- src/server/scene/surface_wrapper.cpp	1970-01-01 00:00:00 +0000
+++ src/server/scene/surface_wrapper.cpp	2015-01-27 03:20:51 +0000
@@ -0,0 +1,229 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#include "mir/scene/surface_wrapper.h"
+
+namespace mir { namespace scene {
+
+SurfaceWrapper::SurfaceWrapper(std::shared_ptr<Surface> const& impl)
+    : raw_surface(impl)
+{
+}
+
+SurfaceWrapper::~SurfaceWrapper()
+{
+}
+
+std::shared_ptr<mir::input::InputChannel> SurfaceWrapper::input_channel() const
+{
+    return raw_surface->input_channel();
+}
+
+mir::input::InputReceptionMode SurfaceWrapper::reception_mode() const
+{
+    return raw_surface->reception_mode();
+}
+
+std::string SurfaceWrapper::name() const
+{
+    return raw_surface->name();
+}
+
+geometry::Point SurfaceWrapper::top_left() const
+{
+    return raw_surface->top_left();
+}
+
+geometry::Size SurfaceWrapper::client_size() const
+{
+    return raw_surface->client_size();
+}
+
+geometry::Size SurfaceWrapper::size() const
+{
+    return raw_surface->size();
+}
+
+geometry::Rectangle SurfaceWrapper::input_bounds() const
+{
+    return raw_surface->input_bounds();
+}
+
+bool SurfaceWrapper::input_area_contains(mir::geometry::Point const& p) const
+{
+    return raw_surface->input_area_contains(p);
+}
+
+std::unique_ptr<graphics::Renderable> SurfaceWrapper::compositor_snapshot(void const* id) const
+{
+    return raw_surface->compositor_snapshot(id);
+}
+
+float SurfaceWrapper::alpha() const
+{
+    return raw_surface->alpha();
+}
+
+MirSurfaceType SurfaceWrapper::type() const
+{
+    return raw_surface->type();
+}
+
+MirSurfaceState SurfaceWrapper::state() const
+{
+    return raw_surface->state();
+}
+
+void SurfaceWrapper::hide()
+{
+    raw_surface->hide();
+}
+
+void SurfaceWrapper::show()
+{
+    raw_surface->show();
+}
+
+bool SurfaceWrapper::visible() const
+{
+    return raw_surface->visible();
+}
+
+void SurfaceWrapper::move_to(geometry::Point const& p)
+{
+    raw_surface->move_to(p);
+}
+
+void SurfaceWrapper::take_input_focus(std::shared_ptr<shell::InputTargeter> const& t)
+{
+    raw_surface->take_input_focus(t);
+}
+
+void SurfaceWrapper::set_input_region(std::vector<geometry::Rectangle> const& r)
+{
+    raw_surface->set_input_region(r);
+}
+
+void SurfaceWrapper::allow_framedropping(bool b)
+{
+    raw_surface->allow_framedropping(b);
+}
+
+void SurfaceWrapper::resize(geometry::Size const& s)
+{
+    raw_surface->resize(s);
+}
+
+void SurfaceWrapper::set_transformation(glm::mat4 const& t)
+{
+    raw_surface->set_transformation(t);
+}
+
+void SurfaceWrapper::set_alpha(float a)
+{
+    raw_surface->set_alpha(a);
+}
+
+void SurfaceWrapper::set_orientation(MirOrientation orientation)
+{
+    raw_surface->set_orientation(orientation);
+}
+
+void SurfaceWrapper::force_requests_to_complete()
+{
+    raw_surface->force_requests_to_complete();
+}
+
+void SurfaceWrapper::add_observer(std::shared_ptr<SurfaceObserver> const& ob)
+{
+    raw_surface->add_observer(ob);
+}
+
+void SurfaceWrapper::remove_observer(std::weak_ptr<SurfaceObserver> const& ob)
+{
+    raw_surface->remove_observer(ob);
+}
+
+void SurfaceWrapper::set_reception_mode(input::InputReceptionMode mode)
+{
+    raw_surface->set_reception_mode(mode);
+}
+
+void SurfaceWrapper::consume(MirEvent const& e)
+{
+    raw_surface->consume(e);
+}
+
+void SurfaceWrapper::set_cursor_image(std::shared_ptr<graphics::CursorImage> const& i)
+{
+    raw_surface->set_cursor_image(i);
+}
+
+std::shared_ptr<graphics::CursorImage> SurfaceWrapper::cursor_image() const
+{
+    return raw_surface->cursor_image();
+}
+
+void SurfaceWrapper::request_client_surface_close()
+{
+    raw_surface->request_client_surface_close();
+}
+
+MirPixelFormat SurfaceWrapper::pixel_format() const
+{
+    return raw_surface->pixel_format();
+}
+
+void SurfaceWrapper::swap_buffers(graphics::Buffer* old_buffer,
+                           std::function<void(graphics::Buffer*)> callback)
+{
+    raw_surface->swap_buffers(old_buffer, callback);
+}
+
+bool SurfaceWrapper::supports_input() const
+{
+    return raw_surface->supports_input();
+}
+
+int SurfaceWrapper::client_input_fd() const
+{
+    return raw_surface->client_input_fd();
+}
+
+int SurfaceWrapper::configure(MirSurfaceAttrib a, int v)
+{
+    return raw_surface->configure(a, v);
+}
+
+int SurfaceWrapper::query(MirSurfaceAttrib a)
+{
+    return raw_surface->query(a);
+}
+
+void SurfaceWrapper::with_most_recent_buffer_do(
+    std::function<void(graphics::Buffer&)> const& callback)
+{
+    raw_surface->with_most_recent_buffer_do(callback);
+}
+
+std::shared_ptr<Surface> SurfaceWrapper::parent() const
+{
+    return raw_surface->parent();
+}
+
+}} // namespace mir::scene
 
=== modified file 'tests/include/mir_test_doubles/mock_surface.h'
--- tests/include/mir_test_doubles/mock_surface.h	2015-01-14 06:39:13 +0000
+++ tests/include/mir_test_doubles/mock_surface.h	2015-01-27 03:20:51 +0000
@@ -57,6 +57,11 @@
     MOCK_METHOD0(force_requests_to_complete, void());
     MOCK_METHOD0(advance_client_buffer, std::shared_ptr<graphics::Buffer>());
 
+    MOCK_CONST_METHOD0(state, MirSurfaceState());
+
+    MOCK_METHOD1(move_to, void(geometry::Point const&));
+    MOCK_CONST_METHOD0(top_left, geometry::Point());
+    MOCK_METHOD1(resize, void(geometry::Size const&));
     MOCK_CONST_METHOD0(size, geometry::Size());
     MOCK_CONST_METHOD0(pixel_format, MirPixelFormat());
 
 
=== modified file 'tests/unit-tests/scene/CMakeLists.txt'
--- tests/unit-tests/scene/CMakeLists.txt	2015-01-14 06:39:13 +0000
+++ tests/unit-tests/scene/CMakeLists.txt	2015-01-27 03:20:51 +0000
@@ -15,6 +15,7 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_surface.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_surface_impl.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_basic_surface.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_managed_surface.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_surface_stack.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_surface_controller.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_legacy_scene_change_notification.cpp
 
=== added file 'tests/unit-tests/scene/test_managed_surface.cpp'
--- tests/unit-tests/scene/test_managed_surface.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/scene/test_managed_surface.cpp	2015-01-27 03:20:51 +0000
@@ -0,0 +1,249 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#include "src/server/scene/managed_surface.h"
+#include "mir_test_doubles/mock_surface.h"
+#include "mir_test_doubles/mock_display_layout.h"
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+using namespace testing;
+using namespace mir::scene;
+using namespace mir::geometry;
+using namespace mir::test::doubles;
+
+struct ManagedSurfaceTest : public Test
+{
+    std::shared_ptr<MockDisplayLayout>
+        mock_display_layout{std::make_shared<NiceMock<MockDisplayLayout>>()};
+
+    std::shared_ptr<MockSurface>
+        mock_basic_surface{std::make_shared<NiceMock<MockSurface>>()};
+    MirSurfaceState state_ = mir_surface_state_restored;
+    Rectangle position{{12,34}, {56,78}};
+
+    Rectangle const random_place{{31,41}, {59,26}};
+    Rectangle const fullscreen{{0,0}, {1366,768}};
+    Rectangle const maximized{fullscreen}; // will be different in future
+
+    void SetUp()
+    {
+        ON_CALL(*mock_display_layout, size_to_output(_))
+            .WillByDefault(SetArgReferee<0>(fullscreen));
+
+        // This could be served better by a FakeSurface later, but this
+        // requires less code for now.
+        ON_CALL(*mock_basic_surface, size())
+            .WillByDefault(ReturnPointee(&position.size));
+        ON_CALL(*mock_basic_surface, resize(_))
+            .WillByDefault(SaveArg<0>(&position.size));
+
+        ON_CALL(*mock_basic_surface, top_left())
+            .WillByDefault(ReturnPointee(&position.top_left));
+        ON_CALL(*mock_basic_surface, move_to(_))
+            .WillByDefault(SaveArg<0>(&position.top_left));
+
+        ON_CALL(*mock_basic_surface, state())
+            .WillByDefault(ReturnPointee(&state_));
+        ON_CALL(*mock_basic_surface, configure(mir_surface_attrib_state, _))
+            .WillByDefault(Invoke(
+                [this](MirSurfaceAttrib, int v) -> int
+                {
+                    state_ = (MirSurfaceState)v;
+                    return v;
+                }
+            ));
+    }
+};
+
+TEST_F(ManagedSurfaceTest, goes_fullscreen_and_restores)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    Rectangle const restored{surf.top_left(), surf.size()};
+    ASSERT_NE(fullscreen, restored);
+
+    InSequence seq;
+    EXPECT_CALL(*mock_basic_surface, move_to(fullscreen.top_left));
+    EXPECT_CALL(*mock_basic_surface, resize(fullscreen.size));
+    EXPECT_CALL(*mock_basic_surface, move_to(restored.top_left));
+    EXPECT_CALL(*mock_basic_surface, resize(restored.size));
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_fullscreen);
+    surf.configure(mir_surface_attrib_state, mir_surface_state_restored);
+}
+
+TEST_F(ManagedSurfaceTest, goes_maximized_and_restores)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    Rectangle const restored{surf.top_left(), surf.size()};
+    ASSERT_NE(maximized, restored);
+
+    InSequence seq;
+    EXPECT_CALL(*mock_basic_surface, move_to(maximized.top_left));
+    EXPECT_CALL(*mock_basic_surface, resize(maximized.size));
+    EXPECT_CALL(*mock_basic_surface, move_to(restored.top_left));
+    EXPECT_CALL(*mock_basic_surface, resize(restored.size));
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_maximized);
+    surf.configure(mir_surface_attrib_state, mir_surface_state_restored);
+}
+
+TEST_F(ManagedSurfaceTest, goes_vertmaximized_and_restores)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    Rectangle const restored{surf.top_left(), surf.size()};
+    ASSERT_NE(maximized, restored);
+
+    Rectangle const vertmaximized{
+        {restored.top_left.x, maximized.top_left.y},
+        {restored.size.width, maximized.size.height} };
+
+    InSequence seq;
+    EXPECT_CALL(*mock_basic_surface, move_to(vertmaximized.top_left));
+    EXPECT_CALL(*mock_basic_surface, resize(vertmaximized.size));
+    EXPECT_CALL(*mock_basic_surface, move_to(restored.top_left));
+    EXPECT_CALL(*mock_basic_surface, resize(restored.size));
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_vertmaximized);
+    surf.configure(mir_surface_attrib_state, mir_surface_state_restored);
+}
+
+TEST_F(ManagedSurfaceTest, goes_horizmaximized_and_restores)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    Rectangle const restored{surf.top_left(), surf.size()};
+    ASSERT_NE(maximized, restored);
+
+    Rectangle const horizmaximized{
+        {maximized.top_left.x, restored.top_left.y},
+        {maximized.size.width, restored.size.height} };
+
+    InSequence seq;
+    EXPECT_CALL(*mock_basic_surface, move_to(horizmaximized.top_left));
+    EXPECT_CALL(*mock_basic_surface, resize(horizmaximized.size));
+    EXPECT_CALL(*mock_basic_surface, move_to(restored.top_left));
+    EXPECT_CALL(*mock_basic_surface, resize(restored.size));
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_horizmaximized);
+    surf.configure(mir_surface_attrib_state, mir_surface_state_restored);
+}
+
+TEST_F(ManagedSurfaceTest, goes_minimized_and_restores)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    InSequence seq;
+    EXPECT_CALL(*mock_basic_surface, hide());
+    EXPECT_CALL(*mock_basic_surface, show());
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_minimized);
+    surf.configure(mir_surface_attrib_state, mir_surface_state_restored);
+}
+
+TEST_F(ManagedSurfaceTest, multistate_restores_to_original)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+    Rectangle const restored{surf.top_left(), surf.size()};
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_vertmaximized);
+    surf.move_to({surf.top_left().x.as_int() + 5, surf.top_left().y});
+    EXPECT_NE(restored.top_left, surf.top_left());
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_fullscreen);
+    EXPECT_EQ(fullscreen.top_left, surf.top_left());
+    EXPECT_EQ(fullscreen.size, surf.size());
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_restored);
+    EXPECT_EQ(restored.top_left, surf.top_left());
+    EXPECT_EQ(restored.size, surf.size());
+}
+
+TEST_F(ManagedSurfaceTest, fullscreen_cant_move)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    Rectangle const restored{surf.top_left(), surf.size()};
+    ASSERT_NE(fullscreen, restored);
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_fullscreen);
+    surf.move_to(random_place.top_left);
+    surf.resize(random_place.size);
+    ASSERT_NE(random_place, fullscreen);
+    EXPECT_EQ(fullscreen.top_left, surf.top_left());
+    EXPECT_EQ(fullscreen.size, surf.size());
+}
+
+TEST_F(ManagedSurfaceTest, maximized_cant_move)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    Rectangle const restored{surf.top_left(), surf.size()};
+    ASSERT_NE(maximized, restored);
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_maximized);
+    surf.move_to(random_place.top_left);
+    surf.resize(random_place.size);
+    ASSERT_NE(random_place, maximized);
+    EXPECT_EQ(maximized.top_left, surf.top_left());
+    EXPECT_EQ(maximized.size, surf.size());
+}
+
+TEST_F(ManagedSurfaceTest, vertmaximized_can_move_only_horizontally)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_vertmaximized);
+    surf.move_to(random_place.top_left);
+    surf.resize(random_place.size);
+    EXPECT_EQ(random_place.top_left.x, surf.top_left().x);
+    EXPECT_EQ(maximized.top_left.y, surf.top_left().y);
+    EXPECT_EQ(random_place.size.width, surf.size().width);
+    EXPECT_EQ(maximized.size.height, surf.size().height);
+}
+
+TEST_F(ManagedSurfaceTest, horizmaximized_can_move_only_vertically)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_horizmaximized);
+    surf.move_to(random_place.top_left);
+    surf.resize(random_place.size);
+    EXPECT_EQ(maximized.top_left.x, surf.top_left().x);
+    EXPECT_EQ(random_place.top_left.y, surf.top_left().y);
+    EXPECT_EQ(maximized.size.width, surf.size().width);
+    EXPECT_EQ(random_place.size.height, surf.size().height);
+}
+
+TEST_F(ManagedSurfaceTest, restored_can_move_freely)
+{
+    ManagedSurface surf(mock_basic_surface, mock_display_layout);
+
+    Rectangle const restored{surf.top_left(), surf.size()};
+    EXPECT_NE(restored, random_place);
+
+    surf.configure(mir_surface_attrib_state, mir_surface_state_restored);
+    surf.move_to(random_place.top_left);
+    surf.resize(random_place.size);
+
+    EXPECT_EQ(random_place.top_left, surf.top_left());
+    EXPECT_EQ(random_place.size, surf.size());
+}
 
=== modified file 'tests/unit-tests/scene/test_surface_controller.cpp'
--- tests/unit-tests/scene/test_surface_controller.cpp	2015-01-14 06:39:13 +0000
+++ tests/unit-tests/scene/test_surface_controller.cpp	2015-01-27 03:20:51 +0000
@@ -24,6 +24,7 @@
 #include "mir_test_doubles/stub_scene_session.h"
 
 #include "mir_test_doubles/mock_surface.h"
+#include "mir_test_doubles/mock_display_layout.h"
 #include "mir_test/fake_shared.h"
 
 #include <gtest/gtest.h>
@@ -62,7 +63,8 @@
 struct SurfaceController : testing::Test
 {
     MockPlacementStrategy placement_strategy;
-    mtd::MockSurface mock_surface;
+    testing::NiceMock<mtd::MockDisplayLayout> mock_display_layout;
+    testing::NiceMock<mtd::MockSurface> mock_surface;
     std::shared_ptr<ms::Surface> const expect_surface = mt::fake_shared(mock_surface);
     testing::NiceMock<MockSurfaceAllocator> mock_surface_allocator;
     MockSurfaceStackModel model;
@@ -73,6 +75,10 @@
         using namespace ::testing;
         ON_CALL(mock_surface_allocator, create_surface(_)).WillByDefault(Return(expect_surface));
         ON_CALL(placement_strategy, place(_, _)).WillByDefault(ReturnArg<1>());
+        ON_CALL(mock_surface, size())
+            .WillByDefault(Return(geom::Size{12,34}));
+        ON_CALL(mock_surface, top_left())
+            .WillByDefault(Return(geom::Point{56,78}));
     }
 };
 }
@@ -81,11 +87,34 @@
 {
     using namespace ::testing;
 
-    ms::SurfaceController controller(
+    class MockSurfaceController : public ms::SurfaceController
+    {
+    public:
+        MockSurfaceController(
+            std::shared_ptr<ms::SurfaceFactory> const& surface_factory,
+            std::shared_ptr<ms::PlacementStrategy> const& placement_strategy,
+            std::shared_ptr<msh::DisplayLayout> const& display_layout,
+            std::shared_ptr<ms::SurfaceStackModel> const& surface_stack)
+            : ms::SurfaceController(surface_factory,
+                                    placement_strategy,
+                                    display_layout,
+                                    surface_stack)
+        {
+        }
+
+        MOCK_METHOD1(wrap_surface, std::shared_ptr<ms::Surface>(
+                                     std::shared_ptr<ms::Surface> const&));
+    };
+
+    MockSurfaceController controller(
         mt::fake_shared(mock_surface_allocator),
         mt::fake_shared(placement_strategy),
+        mt::fake_shared(mock_display_layout),
         mt::fake_shared(model));
 
+    EXPECT_CALL(controller, wrap_surface(expect_surface))
+        .WillOnce(Return(expect_surface));
+
     InSequence seq;
     EXPECT_CALL(placement_strategy, place(_, _)).Times(1);
     EXPECT_CALL(mock_surface_allocator, create_surface(_)).Times(1).WillOnce(Return(expect_surface));
@@ -105,6 +134,7 @@
     ms::SurfaceController controller(
         mt::fake_shared(mock_surface_allocator),
         mt::fake_shared(placement_strategy),
+        mt::fake_shared(mock_display_layout),
         mt::fake_shared(model));
 
     EXPECT_CALL(model, raise(_)).Times(1);
@@ -121,6 +151,7 @@
     ms::SurfaceController controller(
         mt::fake_shared(mock_surface_allocator),
         mt::fake_shared(placement_strategy),
+        mt::fake_shared(mock_display_layout),
         mt::fake_shared(model));
 
     auto params = ms::a_surface();
@@ -139,6 +170,7 @@
     ms::SurfaceController controller(
         mt::fake_shared(mock_surface_allocator),
         mt::fake_shared(placement_strategy),
+        mt::fake_shared(mock_display_layout),
         mt::fake_shared(model));
 
     auto params = ms::a_surface();