[PATCH] xdg-shell: Align surface life-time rules with that of wl_shell

[Pekka Paalanen]

On Mon, 21 Jul 2014 13:23:23 -0700
Jason Ekstrand <jason at jlekstrand.net> wrote:

> 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.

Like I wrote in my recent blog post [1], not completely unrelated to
this discussion in irc, IIRC, I strongly suggest adding destructor
protocol unless you can prove that having destructor protocol is
actively harmful.

To decide what the destructor semantics are, if you already have
well-thought semantics for the no-destructor case, you can think
about what happens when the wl_surface gets destroyed, and make the
xdg_surface destructor work in a way that looks similar to the
client.

In irc, Jasper did raise a good question about what should happen
when the client destroys the xdg_shell object. The ping/pong
messages are(?) part of xdg_shell interface, not xdg_surface. In
that case my proposition was to make the behaviour the same as if
the client is not responding to pings at all. The client cannot
receive pings, so it cannot send pongs; therefore, assume the client
is unresponsive. Destroying xdg_shell while there are windows up is
something that a client should not do, so we just need a roughly
sane behaviour for a corner-case that should not normally be hit.

As for wl_surface re-use after destroying a role, I have found that
resetting a role to allow re-use is much easier and cleaner than
not to, in Weston code. That's why I've been silently advocating to
allow re-use, but at the same I don't really expect clients to use
that. Yes, conflicting... so I suppose I don't have a real strong
preference there. Removing a role loses all role-specific
information, so whether the wl_surface get re-used or re-created
does not make any difference.

But, we already do rely on re-use in some cases, I believe, like
for cursors. A wl_surface can be set as cursor, then lose the role,
and then be set again. We have to keep that working. However, that
is more of a Weston internal detail than something that is directly
apparent in a client.


Thanks,
pq

[1] http://ppaalanen.blogspot.fi/2014/07/wayland-protocol-design-object-lifespan.html


> 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 :)