[PATCH wayland] protocol: define the concept of wl_surface role

[Pekka Paalanen]

On Mon, 18 Aug 2014 10:32:28 -0400
"Jasper St. Pierre" <jstpierre at mecheye.net> wrote:

> On Mon, Aug 18, 2014 at 10:14 AM, Pekka Paalanen <ppaalanen at gmail.com>
> wrote:
> 
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Define what a role is, and what restrictions there are.
> >
> > A change to existing behaviour is that a role cannot be changed at all
> > once set. However, this is unlikely to cause problems, as there is no
> > reason to re-use wl_surfaces in clients.
> >
> 
> Thanks for this. I was going to add it in the Publican docs, but those are
> significantly out of date and I spent more time laughing at them than
> writing things in them. We should fix that. :(

Yeah...

> Looks good overall. I'm going to be a terrible person and nitpick your
> grammar. Sorry about that in advance!

Ha! Good that someone cares about that once in a while. :-)

> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  protocol/wayland.xml | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index 2d57f69..39af61f 100644
> > --- a/protocol/wayland.xml
> > +++ b/protocol/wayland.xml
> > @@ -973,8 +973,24 @@
> >        local coordinates of the pixel content, in case a buffer_transform
> >        or a buffer_scale is used.
> >
> > -      Surfaces are also used for some special purposes, e.g. as
> > -      cursor images for pointers, drag icons, etc.
> > +      A surface without a "role" is fairly useless, a compositor does not
> > +      know where, when or how to present it. The role is the purpose of a
> > +      wl_surface. Examples of roles are a cursor for a pointer, a drag
> > icon,
> > +      a sub-surface, and a window as defined by a shell protocol.
> >
> 
> I'd give more concrete examples. "Examples of surface roles include the
> pointer icon (as set by wl_pointer.set_cursor), a drag icon (as set by
> wl_data_device.start_drag), ..."

I actually though of that but didn't bother, will add.

> +      A surface can have only one role at a time. Initially a wl_surface
> > +      does not have a role. Once a role is set, it can never be set to a
> > +      different role again.
> >
> 
> There is an important clarification here. If I have two surfaces like
> "pointer cursor", "I-beam cursor", I can call wl_pointer.set_cursor on
> either of them in succession. I could imagine someone being confused that
> "the pointer icon" role is a singleton, and when the I-beam cursor gains
> it, the pointer cursor loses it, and she might think that you would have to
> create a new wl_surface since it's illegal to set the wl_pointer.set_cursor
> role on the pointer cursor surface again.

Hmm, I thought the "different" was enough to say that. Roles are not
singletons either.

> What we're trying to do is prevent surface reuse, so you can't transform
> something that was once a pointer cursor into a wl_subsurface or anything
> like that.

Exactly.

> I'm not really sure how to describe this, though. I'm not too happy with
> this, but it's the best I came up with.
> 
> "A surface can only have one role at a time. When you first make a request
> to set a surface's role, the surface can then only have that role forever,
> even if it loses the role. For instance, if somebody sets the 'subsurface'
> role on a wl_surface, then it can only ever be a subsurface, even if you
> destroy the associated wl_subsurface object"
> 
> We should probably describe the concept of "losing the role" from e.g.
> wl_subsurface.destroy, wl_pointer.set_cursor above.

I'll think about it.

> +      A role is set by a request in another interface than wl_surface
> > +      itself. The protocol specification of the other interface
> > +      defines, that the request gives a specific role to a wl_surface.
> > +      Often, this request also creates a new protocol object, that
> > +      represents the role and extends the interface to the wl_surface.
> > +      If such a new protocol object is created, clients are
> > +      recommended to destroy it before they destroy the wl_surface.
> > +      Whether destroying the wl_surface first is legal, depends on the
> > +      protocol interface related to the role.
> >
> 
> I know you're a non-native speaker, so I won't bash your grammar in-depth.
> I'll just offer a replacement paragraph with everything fixed up. I'm also
> going to strengthen up the wording here a bit.
> 
> "Surface roles are set by requests in other interfaces such as
> wl_pointer.set_cursor. The request should explicitly mention that this
> request gives a role to a wl_surface. Often, this request also creates a
> new protocol object that represents the role and adds additional
> functionality to wl_surface. When clients want to destroy a surface, they
> must destroy this 'role object' before the wl_surface, except when
> specified otherwise."

Okay, that is also compatible with both wl_subsurface and
wl_shell_surface.

> We should also change the description of wl_subsurface to say that
> destroying the wl_surface before the role object is an exception to this
> rule, and is considered legacy behavior, so clients should try not to do it.

Sounds like you want to eventually deprecate the arbitrary destruction
order. I would not go that far quite yet. And it would be another
patch, too.

IMO the sub-surface specification does not encourage either way, it
merely states what happens.

We might even just drop the ", except when specified otherwise",
because no existing spec actually specifies or even recommends
otherwise.

I'll make a v2.


Thanks,
pq