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

[Derek Foreman]

On 15/02/17 06:50 AM, Pekka Paalanen wrote:
> 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.

We can't re-assign a role - the wl_surface text is clear that a 
wl_surface, once given a role, retains it for its lifetime... but we 
could attach to a surface that does not yet have an assigned role.

The commit can change the role, though, so I guess what we're ultimately 
interested in is the role the surface will have after the commit completes.

I guess things get sticky when we attach a buffer to a surface, commit, 
then attempt to create an xdg v6 shell surface from it.

Or even attach a buffer to a surface (no commit!), then attempt to 
create an xdg v6 surface from it.

In both of these cases the error is not generated by the attach, or by 
the commit, but by the attempt to generate an xdg v6 surface later, 
which does not actually try set a role at that point at all :).

This leaves me banging my head on my desk because that puts me very 
close to where I started:

The text for attach needs to explain what happens when there is no 
surface role present.  ie)  I can attach, commit, attach NULL, commit. 
This will remove the surface content, and I should then be able to 
create an xdg v6 surface from the surface safely, because no buffer is 
attached.  (This seems like it could be interpreted as an error based on 
one way of reading xdgv6, but weston does not appear to post an error 
for this)

What I was originally interested in seems to be more along the lines of:
Some roles do not allow surfaces to have their content removed.

xdg v6 for example, expects no content at creation time, but after you 
commit content in response to the first configure, you may never remove 
the content again?

Thanks,
Derek

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