[PATCH wayland] protocol: Clarify the meaning of NULL buffer attachment

[Pekka Paalanen]

On Tue, 14 Feb 2017 10:15:23 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 14/02/17 06:05 AM, Pekka Paalanen wrote:
> > On Mon, 13 Feb 2017 12:06:24 -0600
> > Derek Foreman <derekf at osg.samsung.com> wrote:
> >  
> >> This documents what has apparently been the case for ages - attaching a NULL
> >> buffer does *not* always remove the surface content, rather it has behaviour
> >> determined by the surface role (which may be documented elsewhere).
> >>
> >> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> >> ---
> >>  protocol/wayland.xml | 13 +++++++++++--
> >>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> >> index 29b63be..d7f7690 100644
> >> --- a/protocol/wayland.xml
> >> +++ b/protocol/wayland.xml
> >> @@ -1359,8 +1359,17 @@
> >>  	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.
> >> +	If wl_surface.attach is sent with a NULL wl_buffer, the result is
> >> +	determined by the surface role. For the result of attaching a NULL
> >> +	wl_buffer to a surface with a cursor role, see the documentation for
> >> +	wl_pointer.set_cursor.  
> >
> > Hi,
> >
> > either list all the roles defined in wayland.xml or none. I'd lean on
> > none to reduce duplication.  
> 
> Sure.
> 
> >> +
> >> +	The result of attaching a NULL buffer to a shell surface should be
> >> +	defined by the shell protocol specification.  As the result may be
> >> +	a posted error and a client disconnect, developers should be careful
> >> +	to read the appropriate protocol specification.  Attaching a NULL
> >> +	buffer to a wl_shell surface removes the surface content, but this
> >> +	behavior is not specified for all shell protocols.
> >>        </description>
> >>        <arg name="buffer" type="object" interface="wl_buffer" allow-null="true"
> >>  	   summary="buffer of surface contents"/>  
> >
> > Not sure shells should be mentioned explicitly either. If it's
> > role-specific, it's role-specific. Period.
> >
> > We do need to check that all roles actually do specify the behaviour.  
> 
> Good point.  XDG shell specifically does not.
> 
> > My suggestion would have been to keep the existing wording and add
> > "Surface roles may specify additional behaviour or errors for attaching
> > or committing buffers."  
> 
> I see no value in defining what NULL attachment does here at all, other 
> than to confuse people.  pointers and subsurfaces already have clear 
> text on what a NULL attachment does - wl_shell relied on the existing 
> text, we can just make its text more clear.

Ok.

> > That one is not limited to NULL buffers even. E.g. we have the dmabuf
> > protocol going to have the create_immediate request which potentially
> > creates an invalid wl_buffer, and it specifies (or rather deliberately
> > says it's not specified by the protocol) what happens if you attach and
> > commit it.  
> 
> The text in wayland.xml doesn't currently say anything about attaching 
> invalid buffers.  Though, I suppose this discussion actually boils down 
> to whether a NULL buffer is a valid buffer or not.  I think that gets 
> needlessly complicated to explain in the doc though.

My point was to not even have to define whether NULL counts as invalid
or not, or mention invalid. Just leave all that for the role spec. It
would be just about attach whatever.

> Outside of NULL behaviour, the rest of the text for wl_surface.attach 
> seems generic and unlikely to be overridden in another protocol.
> 
> > The one thing that will always happen regardless of the role, IMO, is
> > that the surface contents will get removed. Whether that also leads to a
> > fatal error or something else is outside the wl_surface spec.  
> 
> I think that's a pretty disingenuous way to write a spec. :)
> 
> Yes, it's technically correct, with xdg shell v6 your surface content 
> will be removed because your application has been terminated.  It seems 
> like that's probably not what any developer will actually want ever, so 
> the existing text seems to offer little value.

Right.


Thanks,
pq
-------------- 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/81466499/attachment.sig>