Feedback on xdg-shell v5

Fri Apr 29 08:55:00 UTC 2016

On Fri, Apr 29, 2016 at 10:37:49AM +0200, Martin Graesslin wrote:
> On Thursday, April 28, 2016 5:36:58 PM CEST Jonas Ådahl wrote:
> > On Thu, Apr 28, 2016 at 10:36:14AM +0200, Martin Graesslin wrote:
> > > Hi Wayland and Plasma,
> > > 
> > > I finished the implementation of xdg-shell in KWayland [1] and KWin [2].
> > > Overall it was more straight forward than I would have expected.
> > > Especially
> > > the changes to KWin were surprisingly minor (with one huge exception).
> > > 
> > > Now some questions/remarks:
> > > * What's a server supposed to do in use_unstable_version?
> > 
> > If a client requests the wrong version, the compositor should send an
> > error, effectively disconnecting the client.
> > 
> > Note that this request is gone in v6 and is replaced by the same
> > unstable-protocol-version semantics used in wayland-protocols.
> 
> Ok, that's what I though it is. I was just very unsure as killing the client 
> is nothing I would consider "negotiate version" :-P
> 
> > 
> > > * Why is the ping on the shell and not on the surface? I would have
> > > expected to ping the surface. At least that's how I would want to do it
> > > from KWin.
> > Because you don't ping a surface, you ping the client. It's the client
> > that may be inresponsive, and if the client is in responsive, it's safe
> > to assume all its surfaces are as well.
> 
> Sorry, but I doubt this will work in practice. I have experienced many freezes 
> with QtWayland applications and in all cases it would respond to the ping. 
> Consider:
> * Thread 1: dead-locked waiting for a frame callback
> * Thread 2: Wayland connection getting ping, thread alive, will send pong
> 
> I fear that the concept of pinging clients doesn't work in a world where 
> connections are in a thread.
> 
> Anyway if it's about whether the client is responsive, I suggest to make it a 
> dedicated protocol. It's not really something which belongs to xdg-shell, it's 
> a more general concept.

Indeed. It hasen't really got anything to do with any windows. The user
facing feature it enables is as i mentioned the "Force quit" thing.

> 
> > 
> > > * Would it be possible to split the states and geometry? I find it weird
> > > that I have to send a configure request with a size of 0/0 when informing
> > > a surface that it's active.
> > 
> > Possible - yes, worth it? I don't know. It's clearly specified that
> > 0 means "let the client come up with it". If you think there is a clear
> > benefit, you're welcome to send a patch targeting v6.
> 
> Yes, I think it might be worth it. I don't want that the client starts to 
> think it can pick a different size when it becomes active.

Semantically it would be no difference at all I imagine.

> 
> > 
> > > The biggest problem for me is the request set_window_geometry. I think I
> > > mentioned it already before on the wayland list. Basically I have no idea
> > > how to implement that in KWin. We have only one geometry for a window and
> > > that's mapped to the size of the surface/x11 pixmap. This geometry is
> > > used all over the place, for positioning, for resizing and for rendering.
> > > I cannot add support for this without either breaking code or having to
> > > introduce a new internal API. That's lots of work with high potential for
> > > breakage :-(
> > > 
> > > From the description it seems to be only relevant for shadows. Could we
> > > make shadows an optional part in xdg-shell? That the server can announce
> > > that it supports shadows in the surface?
> > 
> > It's not only about shadow. Let me explain a scenario where a window
> > geometry is needed, even when there is a mode where no shadow is drawn
> > by the client.
> > 
> > Consider we have the following window:
> > 
> > 
> >     +-------------------------------------------+
> > 
> >     |                   A window              X |
> > 
> >     +-------------------------------------------+
> > 
> >     |                     /\                    |
> >     |                    
> >     |                    /  \                   |
> >     |                   
> >     |                   /    \                  |
> >     |                  
> >     |                  /      \                 |
> >     |                 
> >     |                 /________\                |
> > 
> >     +-------------------------------------------+
> > 
> > It can be split into two logical rectangles/areas: the window title, and
> > the main content. This window may be consisting of two separate
> > non-overlapping surfaces, for example one GLES surface, and one SHM
> > surface. Only one of these surfaces will be the "window", while the
> > other will be a subsurface.
> > 
> > xdg_surface.set_window_geometry would here be used to describe, in
> > relation to whatever surface was gets to act as "window", what the
> > window geometry would be.
> 
> sorry, I didn't get the example at all.

The "window" consists of two non-overlapping wl_surfaces. One is the
title, on is the part with the rectangle.

Lets say the wl_surface with the rectangle, is the one we use to create
the xdg_surface.

The wl_surface with the title is a wl_subsurface wl_surface that is
positioned above (north of) the wl_surface with the triangle.

In rectangles, we might for example have this:

   Title: 400x40+0-40 (its 40 high, positioned at (0, -40) relative to
                       its parent.)
   Triangle: 400x100 (its a xdg_surface, the parent of the wl_surface,
                      so it has no position)

The window geometry would be: 400x140+0-40.

> > 
> > > Also I'm not quite sure about that, but it looks to me like QtWayland
> > > thinks that the set_window_geometry is the geometry of the
> > > client-side-decoration, while on GTK it looked to me like being the size
> > > of the shadow. Either I did not understand that correctly, or our
> > > toolkits are not compatible.
> > Not sure what you mean here. The window geometry is the geometry of the
> > window (main content, window frame, window title etc) excluding the
> > shadow, in relation to the surface used to create the xdg_surface.
> 
> Ah I mixed that around: On GTK it looks like being the geometry of the window 
> without the shadow (but with decoration), on Qt it seems to be the geometry of 
> the window without the decoration.

Ok, I see. It seems QT got it wrong then. Think of the window geometry
as the region that would "stick" to other windows when you move it
around. When you drag it upwards, it might momentarily stick to the
bottom of some window that is above it.

> 
> > 
> > So if you have a surface which is 800x600, and shadow is 10 wide on all
> > edges, then you'd have a window geometry which is x: 10, y: 10, width:
> > 780, height: 580.
> > 
> > > At the moment I haven't implemented this request yet in KWayland as I
> > > would
> > > not be able to use it in KWin anyway.
> > > 
> > > As a note: if it's just about shadow for us in Plasma that's a rather
> > > useless request. We already have a Wayland shadow implementation which
> > > allows us to have the shadow outside the surface.
> > 
> > So, what you are saying is that you want to change the xdg_shell to only
> > guarantee support for hybrid the CSD-SSD case. I don't agree that that
> > change would be for the best. There are a few reasons for this:
> 
> I'm not saying anything about CSD-SSD here. I'm only talking about the shadow.

I don't think we can talk about shadows without talking about CSD/SSD,
as the shadow, functionality wise, is part of the decorations.

Jonas

> 
> For us the geoemtry is difficult to implement and if it's only about shadows 
> it's difficult to justify that I spend weeks on implementing that or risk the 
> stability of our overall product due to the heavy changes this will need.
> 
> Cheers
> Martin

> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel