Feedback on xdg-shell v5

[Martin Graesslin]

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.

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

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

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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160429/a6b88e58/attachment-0001.sig>