[PATCH] xdg-shell: Align surface life-time rules with that of wl_shell
Jasper St. Pierre
jstpierre at mecheye.net
Mon Jul 21 12:07:05 PDT 2014
Note that I fixed GTK+ to always create a new wl_surface and never reuse
it. I don't think it makes any sense to keep the wl_surface around in the
background. I certainly don't think it ever makes sense to destroy the
xdg_surface and then make it into an xdg_popup or similar.
The semantics of "once a role of a wl_surface is established, it's locked
in" is something I'm comfortable supporting if it makes compositor
implementations easier, since I don't think switching at runtime is a valid
use case.
We should probably keep around the destructor to prevent leaks, but I'm not
really sure what destroying the role would mean, though.
And whatever we choose, we should try to make it consistent with the
subsurface roles, too, so we don't have different semantics for those.
On Mon, Jul 21, 2014 at 2:14 PM, Sjoerd Simons <
sjoerd.simons at collabora.co.uk> wrote:
> On Mon, 2014-07-21 at 10:03 -0700, Jason Ekstrand wrote:
> > On Mon, Jul 21, 2014 at 1:47 AM, Sjoerd Simons <
> > sjoerd.simons at collabora.co.uk> wrote:
> >
> > > Remove the explicit destroy method from xdg_surface and xdg_popup as
> > > neither of them can be re-created after being destroyed. As a results a
> > > wl_surface gets into an odd sort of odd limbo state after a
> > > xdg_{surface,popup}.destroy call where it not only no longer has an
> > > xdg_{surface,popup} associated with it but where it's also not possible
> > > to re-created one.
> > >
> > > This also happens to align the life-time rules of xdg_{surface,popup}
> to
> > > match those of wl_shell_surface, which is probably good for
> consistency.
> > >
> >
> > Sjoerd,
> > First of all, making xdg_shell consistent with wl_shell is a non-reason
> for
> > doing anything. The objective to xdg_shell is to *replace* wl_shell so
> > they are going to have different symantics.
>
> Ofcourse, but there is no reason to have different for the sake of being
> different. And for clients that either are moving from wl_shell to
> xdg_shell or what to implement both having similar semantics in places
> where it makes sense shoudl make things easier :).
>
> > Second, regarding your original patch. I'm sorry that it never got
> > reviewed, but implementation details of desktop-shell are, again, not a
> > good driving force for xdg_shell. If weston's desktop-shell has
> > wl_shell_surface symantics too closely tied to xdg_shell, that's a bug in
> > weston's desktop-shell, not a bug in xdg_shell.
>
> This was never about the semantics implemented by the desktop shell..
> However anyone that has a client implemention of xdg shell that *works
> with weston*, must already be using the weston semantics, so that seems
> a pretty strong pointer that that works for clients. Or at least apart
> from myself nobody seems to have bothered trying to fix it :)
>
> > Right now, weston's
> > desktop-shell is kind of a mess and, yes, it makes a lot of assumptions
> > about the lifecycles of objects both internal and external. When I did
> the
> > surface/view split, it was a pain because it made the tacit assumption
> that
> > a shell surface was a surface was a place on-screen. Separating these
> > assumptions will make desktop-shell much cleaner.
>
> Oh no disagreement there, if you look back in the list history at my
> initial patch (which was broken because of this mess) actually was an
> attempt to implement the explicit destroy methods in a way which would
> not cause the surface to go into limbo.
>
> > Third is the question of whether we want destructors or not. One of the
> > things we have learned is that destructors are very hard to properly add
> > later. If there is any chance that we will need them, we have to add
> them
> > before we stabilize the protocol. Also, I don't think there's any real
> > harm except for the possible case where the xdg_surface is hanging around
> > after the wl_surface is destroyed. I'd be 100% ok with defining any
> > requests on the xdg_surface to result in a protocol error if the
> wl_surface
> > is destroyed. It's possible that we could remove the destructor and tie
> > the two lifecycles together. However, we need to be *really* careful
> about
> > doing so.
>
> I don't really have strong feelings either way. When initially talked to
> Jasper on IRC when i ran into this problem area, he mentioned that it
> was never expected for clients to destroy an xdg surface and get a new
> one for the same wl_surface. Hence the odd semantics and this approach.
>
> In my opinion there are two sane semantics:
> * the xdg surfaces don't have an explicit destroy and their life-time
> gets strictly bound to the underlying wl_surface (which my patch
> does)
> * The xdg surfaces *do* have an explicit destroy, destroying the xdg
> surface "resets" the wl_surfaces. In the sense that it should be
> possible to associate a new xdg surface with the wl_surface.
>
> As long as it's one of those two i'm entirely happy :)
>
> > --Jason Ekstrand
> >
> >
> > >
> > > Signed-off-by: Sjoerd Simons <sjoerd.simons at collabora.co.uk>
> > > ---
> > > desktop-shell/shell.c | 25 ++-----------------------
> > > protocol/xdg-shell.xml | 24 ++++--------------------
> > > 2 files changed, 6 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > > index 593c7f3..efec100 100644
> > > --- a/desktop-shell/shell.c
> > > +++ b/desktop-shell/shell.c
> > > @@ -3370,13 +3370,6 @@ static const struct wl_shell_interface
> > > shell_implementation = {
> > > * xdg-shell implementation */
> > >
> > > static void
> > > -xdg_surface_destroy(struct wl_client *client,
> > > - struct wl_resource *resource)
> > > -{
> > > - wl_resource_destroy(resource);
> > > -}
> > > -
> > > -static void
> > > xdg_surface_set_parent(struct wl_client *client,
> > > struct wl_resource *resource,
> > > struct wl_resource *parent_resource)
> > > @@ -3534,7 +3527,6 @@ xdg_surface_set_minimized(struct wl_client
> *client,
> > > }
> > >
> > > static const struct xdg_surface_interface xdg_surface_implementation
> = {
> > > - xdg_surface_destroy,
> > > xdg_surface_set_parent,
> > > xdg_surface_set_title,
> > > xdg_surface_set_app_id,
> > > @@ -3662,18 +3654,6 @@ shell_surface_is_xdg_surface(struct
> shell_surface
> > > *shsurf)
> > > }
> > >
> > > /* xdg-popup implementation */
> > > -
> > > -static void
> > > -xdg_popup_destroy(struct wl_client *client,
> > > - struct wl_resource *resource)
> > > -{
> > > - wl_resource_destroy(resource);
> > > -}
> > > -
> > > -static const struct xdg_popup_interface xdg_popup_implementation = {
> > > - xdg_popup_destroy,
> > > -};
> > > -
> > > static void
> > > xdg_popup_send_configure(struct weston_surface *surface,
> > > int32_t width, int32_t height)
> > > @@ -3753,8 +3733,7 @@ xdg_get_xdg_popup(struct wl_client *client,
> > > wl_resource_create(client,
> > > &xdg_popup_interface, 1, id);
> > > wl_resource_set_implementation(shsurf->resource,
> > > - &xdg_popup_implementation,
> > > - shsurf,
> > > shell_destroy_shell_surface);
> > > + NULL, shsurf,
> > > shell_destroy_shell_surface);
> > > }
> > >
> > > static void
> > > @@ -3771,7 +3750,7 @@ shell_surface_is_xdg_popup(struct shell_surface
> > > *shsurf)
> > > {
> > > return wl_resource_instance_of(shsurf->resource,
> > > &xdg_popup_interface,
> > > - &xdg_popup_implementation);
> > > + NULL);
> > > }
> > >
> > > static const struct xdg_shell_interface xdg_implementation = {
> > > diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml
> > > index bd36231..ea005bc 100644
> > > --- a/protocol/xdg-shell.xml
> > > +++ b/protocol/xdg-shell.xml
> > > @@ -127,16 +127,6 @@
> > > the wl_surface object.
> > > </description>
> > >
> > > - <request name="destroy" type="destructor">
> > > - <description summary="remove xdg_surface interface">
> > > - The xdg_surface interface is removed from the wl_surface object
> > > - that was turned into a xdg_surface with
> > > - xdg_shell.get_xdg_surface request. The xdg_surface properties,
> > > - like maximized and fullscreen, are lost. The wl_surface loses
> > > - its role as a xdg_surface. The wl_surface is unmapped.
> > > - </description>
> > > - </request>
> > > -
> > > <request name="set_parent">
> > > <description summary="surface is a child of another surface">
> > > Child surfaces are stacked above their parents, and will be
> > > @@ -388,17 +378,11 @@
> > > parent surface, in surface local coordinates.
> > >
> > > xdg_popup surfaces are always transient for another surface.
> > > - </description>
> > >
> > > - <request name="destroy" type="destructor">
> > > - <description summary="remove xdg_surface interface">
> > > - The xdg_surface interface is removed from the wl_surface object
> > > - that was turned into a xdg_surface with
> > > - xdg_shell.get_xdg_surface request. The xdg_surface properties,
> > > - like maximized and fullscreen, are lost. The wl_surface loses
> > > - its role as a xdg_surface. The wl_surface is unmapped.
> > > - </description>
> > > - </request>
> > > + On the server side the object is automatically destroyed when
> > > + the related wl_surface is destroyed. On client side,
> > > xdg_popup.destroy()
> > > + must be called before destroying the wl_surface object.
> > > + </description>
> > >
> > > <event name="popup_done">
> > > <description summary="popup interaction is done">
> > > --
> > > 2.0.1
> > >
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > >
>
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
--
Jasper
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140721/9d1c5fe9/attachment.html>
More information about the wayland-devel
mailing list