[PATCH wayland v2] protocol: Clarify the behaviour of wl_surface.attach

[Pekka Paalanen]

On Wed, 15 Feb 2017 12:17:10 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:

> On Tue, Feb 14, 2017 at 10:20:06AM -0600, Derek Foreman wrote:
> > Attaching a NULL wl_buffer to a surface is not always valid, but
> > the previous text indicated it was.
> > 
> > Instead, let's define what NULL attachment does for all the surface
> > roles defined in wayland.xml, stop giving a blanket definition of
> > its behavior in wl_surface.attach, and warn developers that they
> > should refer to other protocol documentation for a full understanding
> > of wl_surface.attach.  
> 
> Good to see these things cleared up. Have some comments on the wording
> below, and the "cursor" role behaviour seems still undefined when
> attaching a NULL buffer.
> 
> > 
> > Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> > ---
> > 
> > Changes from v1:
> >  pretty much everything - define NULL attach for wl_shell specifically
> >  and remove the generic statement from wl_surface.attach
> >  refer readers to "other documentation"
> > 
> >  protocol/wayland.xml | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index 29b63be..1a76e60 100644
> > --- a/protocol/wayland.xml
> > +++ b/protocol/wayland.xml
> > @@ -1002,6 +1002,10 @@
> >        the related wl_surface is destroyed. On the client side,
> >        wl_shell_surface_destroy() must be called before destroying
> >        the wl_surface object.
> > +
> > +      Attaching a NULL wl_buffer to a surface assigned a role by
> > +      wl_shell will remove the content from the surface after the
> > +      next commit.  
> 
> How is "removes the content" compared to unmaps? Does this mean a shell
> implementation need to keep track of the position when the shell surface
> is later remapped? That would be a new requirement that was previously
> undefined behaviour, and is not what mutter does at the moment.

Maintaining position has been how I have always read it, but then I
shouldn't be allowed near desktop shell protocols. I even thought that
unmap also maintains the position.

> >      </description>
> >  
> >      <request name="pong">
> > @@ -1324,6 +1328,9 @@
> >        <description summary="set the surface contents">
> >  	Set a buffer as the content of this surface.
> >  
> > +	The role of the surface influences the behaviour of attach,
> > +	so this documentation is incomplete without further reading.
> > +  
> 
> Possible alternative wording, possibly in the end of the <description/>,
> as it talks about things that is specified below:
> 
> 	The effect committing an attached buffer to a surface depends on
> 	what role the surface has been or is going to be assigned to.
> 	See the corresponding role specification for further details.

Is it missing the case for "is currently"? I forget our rules about
changing an assigned role, but I think the wording you want here is for
current or the next assigned role, to cover the case where a
wl_surface.commit makes the wl_surface illegal for a following role
assignment, not just for doing an illegal thing while the role is
already assigned. What role it may have had in the past should not
matter.

> 
> This makes read more as the behaviour of *attach* does not change, but
> the effect of having attached and committed a buffer.

Yes, it is important to make that distiction. Attach alone does
nothing, since a following attach may overwrite the pending buffer
again before it is committed. Not that clients would be sane to do
that, but for completeness.

With the fixed wording:
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Not R-b because I have not read all the affected role specs in
wayland.xml.


Thanks,
pq

> 
> 
> Jonas
> 
> >  	The new size of the surface is calculated based on the buffer
> >  	size transformed by the inverse buffer_transform and the
> >  	inverse buffer_scale. This means that the supplied buffer
> > @@ -1358,9 +1365,6 @@
> >  	the surface contents. However, if the client destroys the
> >  	wl_buffer before receiving the wl_buffer.release event, the surface
> >  	contents become undefined immediately.
> > -
> > -	If wl_surface.attach is sent with a NULL wl_buffer, the
> > -	following wl_surface.commit will remove the surface content.
> >        </description>
> >        <arg name="buffer" type="object" interface="wl_buffer" allow-null="true"
> >  	   summary="buffer of surface contents"/>
> > -- 
> > 2.11.0
> >   

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170215/88087295/attachment.sig>