[PATCH] xdg-shell: Align surface life-time rules with that of wl_shell
Sjoerd Simons
sjoerd.simons at collabora.co.uk
Mon Jul 21 11:14:05 PDT 2014
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
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6170 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140721/92f6d641/attachment.bin>
More information about the wayland-devel
mailing list