Mir

Code review comment for lp:~vanvugt/mir/managed-surface

Revision history for this message
Robert Carr (robertcarr) wrote :

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

« Back to merge proposal