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

== include/shared/mir vs include/shared/mir_toolkit ==

The concept is that whatever is under */mir_toolkit (and only that) will be part of the toolkit interface. Since a toolkit/client needs this, mir_toolkit is the way to go. Keep in mind that this is still under shared/.

So, this is how I see things currently:

1. server/*, client/*, shared/* => which mir components need to access the definitions in the file
2. */mir, */mir_toolkit => whether the definitions are internal to mir, or (also) exposed to the toolkit API

== surfaces::Surface::set_type(),type() ==

I agree that this doesn't belong there. I don't have a problem putting it there *temporarily* to unblock work, if the related surface refactorings aren't expected to be done soon.

== configure() ==

At the Surface level, and for the limited number of attributes I am aware of, I would prefer to have explicit calls : app_session->set_surface_state(id, ...) or app_session->surface(id)->set_state(...) (since we are already exposing the surface) etc

At the RPC level: one approach is to use configure(attrib, value), another is to have <attrib>(value) calls. A third approach (not neccessarily preferable for this use case, just mentioning it for completeness) is to have, e.g.:

message SurfaceSettings {
    optional SurfaceId surfaceid = 1;
    optional int32 type = 2;
    optional int32 state = 3;
    optional bool visible = 4;
    ...
}

The frontend could then act depending on which values are actually included in the message. This method allows the client to set and retrieve all of the settings in one go.

For the number of attributes and the use cases I am aware of, my preference would be once again to be explicit, and have separate calls <attrib>().

Marking as "Needs information" => "Needs discussion"