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

Submitted by Derek Foreman on Feb. 14, 2017, 4:20 p.m.

Details

Message ID 20170214162006.26431-1-derekf@osg.samsung.com
State New
Headers show
Series "protocol: Clarify the meaning of NULL buffer attachment" ( rev: 2 ) in Wayland

Not browsing as part of any series.

Commit Message

Derek Foreman Feb. 14, 2017, 4:20 p.m.
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.

Signed-off-by: Derek Foreman <derekf@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(-)

Patch hide | download patch | download mbox

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

Comments

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@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
>
On Wed, 15 Feb 2017 12:17:10 +0800
Jonas Ådahl <jadahl@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@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@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
> >
On Tue, Feb 14, 2017 at 11:17 PM Jonas Ådahl <jadahl@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@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.
>

Enlightenment does this.


>
> >      </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
> >
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
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@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
>>
On 15/02/17 06:50 AM, Pekka Paalanen wrote:
> On Wed, 15 Feb 2017 12:17:10 +0800
> Jonas Ådahl <jadahl@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@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@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
>>>
>
On Wed, 15 Feb 2017 11:49:33 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> On 15/02/17 06:50 AM, Pekka Paalanen wrote:
> > On Wed, 15 Feb 2017 12:17:10 +0800
> > Jonas Ådahl <jadahl@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@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.

Ok, right, so that's what the spec needs to cover then.

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

IIRC not all roles require a commit to become set. But that should be
irrelevant for wl_surface spec.

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

I would expect the xdg_shell spec to explicitly forbid that.

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

And this too, since the idea IIRC was to not draw anything until the
client knows what size it should be.

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

I'd say those details need to be explained in the xdg_shell spec.

> 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 

Yes.

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

To me it would be silly to assume or require that compositors need to
track the whole history of a wl_surface to generate errors. What
actually matters, IMO, is the current and pending wl_surface state at
the time of any related request (not just commit).

To me, these are completely the same from the wl_surface state point of
view in the scope of this discussion:
- create wl_surface
- create wl_surface, attach NULL buffer
- create wl_surface, attach and commit NULL buffer
- create wl_surface, (attach a real buffer){0..n}, attach and commit
  NULL buffer
- create wl_surface, (attach and commit a real buffer){0..n}, attach
  and commit NULL buffer

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

Sounds like you've come a full circle in the above. :-)
I wouldn't write that in the wl_surface spec, though. It's both too
specific and unhelpfully vague.

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

I'll let xdg_shell devs answer that. I have happily forgot all about
why they didn't want to allow committing a NULL buffer.


Thanks,
pq

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