Code review comment for lp:~alan-griffiths/mir/first-pass-of-surface-spec-modification

*Needs Discussion*

> This looks mostly sensible, but does mean that we need a separate API for
> introspecting the state of surfaces. IIRC the longer-term plan was to move all
> the surface introspection into MirSurfaceSpec accessors and deprecate things
> such as MirSurfaceParameters.

While I'm not strongly against it I'm not sure there is currently such a plan. The client trying to use the "existing state" with a surface specification leads to synchronization issues. Like this:

> That said, now that I think of it some more, I think being able to get a
> SurfaceSpec from an existing surface and apply it would probably a misfeature,
> as the possibility for clients to accidentally override user changes would be
> high.

~~~~

> Hm. If we need to support morphing from different surface types - dialog to
> normal, for example - we'll need to have a mir_surface_spec_set_type and this
> will be somewhat redundant with the mir_connection_surface_spec_for_* family
> of functions. A client would be able to mir_create_spec_for_changes(), fill in
> all the same details as mir_connection_surface_spec_for_normal(), and then
> pass it to mir_surface_create() - whereupon they'd somewhat unexpectedly get
> an abort(), because the MirConnection isn't set.
>
> I'm not sure what I think about this. On the one hand, it might be nice to not
> have the same type for both. On the other, that's obvious duplication. I guess
> I lean towards the former; one type with perhaps surprising behaviour.

Well, while MirSurfaceSpec isn't currently polymorphic the various mir_surface_spec_set_XXX() functions can fail (and return false). So different behavior from MirSurfaceSpec's created by different create functions is consistent with the API.

For example, mir_surface_spec_set_type() could fail if the MirSurfaceSpec was not created for change.

Alternatively, we could just constrain the state of MirSurfaceSpec in the functions that accept it and use the various mir_connection_create_spec_for_XXX functions as shortcuts.

> 17 +mir_connection_create_spec_for_changes(MirConnection* connection);
> Why does this take a MirConnection? It's not used for anything, and it doesn't
> seem likely that it'll be used for anything in the future.

Initially it was for consistency, but given the points you raise above we could use it to initialize MirSurfaceSpec::connection - that way the client could theoretically pass the MirSurfaceSpec to mir_surface_create() in the scenario you describe.

Although, if we decide on that approach this function should probably be renamed mir_connection_create_spec(MirConnection* connection) - and the existing functions are then "shortcuts" to the valid states for creation.