On Wed, 15 Feb 2017 11:49:33 -0600 Derek Foreman <[email protected]> wrote:
> On 15/02/17 06:50 AM, Pekka Paalanen wrote: > > On Wed, 15 Feb 2017 12:17:10 +0800 > > Jonas Ådahl <[email protected]> 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 <[email protected]> > >>> --- > >>> > >>> 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 <[email protected]> > > > > 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 > >>> > > >
pgpGJpFpi2sUZ.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
