Merge into development-branch : managed-surface : Code : Mir

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-01-27
Alan Griffiths		2014-12-12	Disapprove on 2015-01-23
Robert Carr (community)			Disapprove on 2015-01-16
Alberto Aguirre (community)			Needs Fixing on 2015-01-16
Review via email: mp+244542@code.launchpad.net

lp:~vanvugt/mir/managed-surface updated on 2014-12-15

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-12-15:

#

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

review: Needs Fixing (continuous-integration)

Reply

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-12-15:

#

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?

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-12-16:

#

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.

Reply

lp:~vanvugt/mir/managed-surface updated on 2014-12-16

2184. By Daniel van Vugt on 2014-12-16: Merge latest trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-12-16:

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-12-18:

#

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

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-12-18:

#

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

Reply

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-01-05:

#

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.

Reply

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-01-05:

#

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

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

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-06:

#

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.

Reply

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-01-06:

#

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.

Reply

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-01-06:

#

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?

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-07:

#

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.

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.

Reply

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-01-07:

#

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.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-08:

#

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

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-08:

#

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

Reply

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-01-12:

#

*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

Reply

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-01-12:

#

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.

Reply

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-01-12:

#

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.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-13:

#

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.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-13:

#

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.

Reply

lp:~vanvugt/mir/managed-surface updated on 2015-01-15

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-15:

#

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

review: Approve (continuous-integration)

Reply

lp:~vanvugt/mir/managed-surface updated on 2015-01-15

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-15:

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-01-16:

#

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

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-16:

#

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.

Reply

lp:~vanvugt/mir/managed-surface updated on 2015-01-16

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

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-16:

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-01-16:

#

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

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-19:

#

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

Reply

lp:~vanvugt/mir/managed-surface updated on 2015-01-19

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-19:

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-20:

#

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

Reply

lp:~vanvugt/mir/managed-surface updated on 2015-01-22

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-22:

#

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

review: Needs Fixing (continuous-integration)

Reply

lp:~vanvugt/mir/managed-surface updated on 2015-01-22

2212. By Daniel van Vugt on 2015-01-22: Ping Jenkins

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-22:

#

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

review: Needs Fixing (continuous-integration)

Reply

lp:~vanvugt/mir/managed-surface updated on 2015-01-23

2213. By Daniel van Vugt on 2015-01-23: Merge latest trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-23:

#

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)

Reply

Revision history for this message

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

#

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

Reply

Revision history for this message

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

#

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

Reply

lp:~vanvugt/mir/managed-surface updated on 2015-01-27

2214. By Daniel van Vugt on 2015-01-27: Merge latest trunk.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-27:

#

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.

Reply

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Reply

Mir

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers

 === 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 <daniel.van.vugt@canonical.com>
++ */
++
++#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 <daniel.van.vugt@canonical.com>
++ */
++
++#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 <daniel.van.vugt@canonical.com>
++ */
++
++#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 <daniel.van.vugt@canonical.com>
++ */
++
++#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 <daniel.van.vugt@canonical.com>
++ */
++
++#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();