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

[Jonas Ådahl]

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.

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


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
>