[PATCH] xdg-shell: Align surface life-time rules with that of wl_shell
Jason Ekstrand
jason at jlekstrand.net
Mon Jul 21 13:23:23 PDT 2014
I think the way it's handled in subsurfaces allows you to create a
subsurface from a surface, delete the surface, and then re-use the surface
as something else. If I recall correctly, weston allows a fair amount of
surface re-use right now.
That said, I'm not opposed to not allowing it for xdg surfaces, but I do
think they should still have an explicit destructor.
--Jason
On Mon, Jul 21, 2014 at 12:07 PM, Jasper St. Pierre <jstpierre at mecheye.net>
wrote:
> 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
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140721/35f2893c/attachment-0001.html>
More information about the wayland-devel
mailing list