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

[Derek Foreman]

On 14/02/17 10:17 PM, Jonas Ådahl 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.

Oops, yes, setting a NULL surface is defined, I missed that attaching a 
NULL buffer was not defined there.  Thanks.

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

My understanding of unmap would be that it loses position, but removing 
the content would not.

The original text in the specification for attach was "removes the 
content", and I kept that.  As the spec is currently in git, that text 
should apply to both cursor surfaces and wl_shell surfaces.

The intent of this patch is not to mandate any new behaviour.

Do we also need to be more clear on whether "removes the content" 
retains the surface position or not?  It would seem that cursor surfaces 
should retain their position if a NULL buffer is attached.

I don't know what existing wl_shell users expect for a NULL buffer 
attachment.

>>      </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.
>
> This makes read more as the behaviour of *attach* does not change, but
> the effect of having attached and committed a buffer.

This is a good distinction to make, I'll use similar text for v3.
(But I'll wait until we sort the unmap vs remove content distinction 
before I post anything)

Thanks,
Derek
>
> 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
>>