<div dir="ltr"><div><div>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.<br>
<br></div>That said, I'm not opposed to not allowing it for xdg surfaces, but I do think they should still have an explicit destructor.<br></div>--Jason<br><div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Jul 21, 2014 at 12:07 PM, Jasper St. Pierre <span dir="ltr"><<a href="mailto:jstpierre@mecheye.net" target="_blank">jstpierre@mecheye.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div>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.<br>


<br></div><div>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.<br>

<br></div><div>We should probably keep around the destructor to prevent leaks, but I'm not really sure what destroying the role would mean, though.<br><br></div><div>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.<br>

</div></div><div class="gmail_extra"><div><div class="h5"><br><br><div class="gmail_quote">On Mon, Jul 21, 2014 at 2:14 PM, Sjoerd Simons <span dir="ltr"><<a href="mailto:sjoerd.simons@collabora.co.uk" target="_blank">sjoerd.simons@collabora.co.uk</a>></span> wrote:<br>

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