[PATCH weston 06/17] xdg-shell: Require a buffer and a wl_surface.commit for mapping a window

[Pekka Paalanen]

On Tue, 7 Apr 2015 23:12:31 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Wed, Apr 08, 2015 at 11:32:20AM +0800, Jonas Ådahl wrote:
> > On Tue, Apr 07, 2015 at 06:36:56PM -0700, Jasper St. Pierre wrote:
> > > On Tue, Apr 7, 2015 at 6:03 PM, Bryce Harrington <bryce at osg.samsung.com> wrote:
> > > > On Tue, Apr 07, 2015 at 05:01:21PM +0800, Jonas Ådahl wrote:
> > > >> Require the client to have attached (either previously committed, or
> > > >> newly) a buffer to the corresponding wl_surface, and that the window
> > > >> will not be potentially mapped until calling wl_surface.commit after
> > > >> having created the window. This is required to make valid double
> > > >> buffered xdg_surface state possible when creating a window.
> > > >>
> > > >> Currently there is no double buffered state in xdg_popup, but it should
> > > >> behave the same as xdg_surface, and for making it future proof in case
> > > >> we want to add double buffered state to xdg_popup.
> > > >>
> > > >> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > > >> ---
> > > >>  protocol/xdg-shell.xml | 8 ++++++++
> > > >>  1 file changed, 8 insertions(+)
> > > >>
> > > >> diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml
> > > >> index 0b6c999..d316e06 100644
> > > >> --- a/protocol/xdg-shell.xml
> > > >> +++ b/protocol/xdg-shell.xml
> > > >> @@ -149,6 +149,10 @@
> > > >>        It provides requests to treat surfaces like windows, allowing to set
> > > >>        properties like maximized, fullscreen, minimized, and to move and resize
> > > >>        them, and associate metadata like title and app id.
> > > >> +
> > > >> +      For a xdg_surface to be mapped by the compositor, the wl_surface must
> > > >> +      have a buffer attached to it, and wl_surface.commit must have been called
> > > >> +      after having created the xdg_surface object.
> > > >
> > > > That reads a bit awkward to me; could it be improved with the following?
> > > >
> > > >     In order for the compositor to map a created xdg_surface object, a
> > > >     buffer must first be attached to the wl_surface and
> > > >     wl_surface.commit called.
> > > 
> > > It doesn't describe the full story. wl_surface.commit needs to be
> > > called *after* the xdg_surface is created.
> > > 
> > > I believe the wl_surface.attach and get_xdg_surface can happen in
> > > either order, though it might be nice to establish some form of
> > > standard order for that.
> > 
> > Not sure that is a good idea. It'd mean we have to disallow otherwise
> > valid non-role-assigned surfaces which seems a bit odd. We also need to
> > allow committing the surface without attaching a buffer so that the
> > compositor can configure the xdg_surface before mapping it (e.g. when
> > starting as maximized).
> > 
> > Anyway, how about:
> > 
> > "The client must call wl_surface.commit on the corresponding wl_surface
> > for the xdg_surface state to take effect. Prior to committing the new
> > state, it can set up initial configuration, such as maximizing or setting
> > a window geometry.
> > 
> > Even without attaching a buffer the compositor must respond to initial
> > committed configuration, for instance sending a configure event with
> > expected window geometry if the client maximized its surface during
> > initialization.
> > 
> > For a surface to be mapped by the compositor the client must have
> > committed both an xdg_surface state and a buffer."
> 
> Yes, that's a lot clearer.

Yup, I like this one. I mean, at least I get the intention from this
and the discussion.


> > > >>      </description>
> > > >>
> > > >>      <request name="destroy" type="destructor">
> > > >> @@ -473,6 +477,10 @@
> > > >>        The x and y arguments specify where the top left of the popup
> > > >>        should be placed, relative to the local surface coordinates of the
> > > >>        parent surface.
> > > >> +
> > > >> +      For a xdg_popup to be mapped by the compositor, the wl_surface must
> > > >> +      have a buffer attached to it, and wl_surface.commit must have been
> > > >> +      called after having created the xdg_popup object.
> > > > As above

This probably needs similar rewording too, right?

In any case, the idea is
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

and can land on my behalf.

However, this one is harder to judge if we need an experimental version
bump. Has Gtk+ already worked like this during the current
experimental version? If yes, I think maybe it doesn't need a version
bump.

Are there other major xdg-shell users who might care?

Btw. what about xdg-shell forbidding committing a NULL buffer? Doesn't
that conflict with this? Or do you need to revise that to say that only
attach(NULL)+commit is illegal, but commit while the pending/current
buffer are NULL is ok? Or when there is no pending buffer set
(newly_attached=false in weston)?


Thanks,
pq