<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. </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. </div>--Jason <div><div><div><div class="gmail_extra"> <div class="gmail_quote"> On Mon, Jul 21, 2014 at 12:07 PM, Jasper St. Pierre <<a href="mailto:jstpierre@mecheye.net" target="_blank">jstpierre@mecheye.net</a>> wrote: <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. </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. </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. </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. </div></div><div class="gmail_extra"><div><div class="h5"> <div class="gmail_quote">On Mon, Jul 21, 2014 at 2:14 PM, Sjoerd Simons <<a href="mailto:sjoerd.simons@collabora.co.uk" target="_blank">sjoerd.simons@collabora.co.uk</a>> wrote: <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: > On Mon, Jul 21, 2014 at 1:47 AM, Sjoerd Simons < > <a href="mailto:sjoerd.simons@collabora.co.uk" target="_blank">sjoerd.simons@collabora.co.uk</a>> 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. </div>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 :). <div> > 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. </div>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 :) <div> > 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. </div>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. <div> > 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. </div>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 :) <div><div> > --Jason Ekstrand > > > > > > Signed-off-by: Sjoerd Simons <<a href="mailto:sjoerd.simons@collabora.co.uk" target="_blank">sjoerd.simons@collabora.co.uk</a>> > > --- > > 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 > > <a href="mailto:wayland-devel@lists.freedesktop.org" target="_blank">wayland-devel@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> > > </div></div> _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org" target="_blank">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div> </div></div>-- Jasper </div> _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div> </div></div></div></div></div>