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.
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 <[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
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
