Feedback on xdg-shell v5

Thu Apr 28 09:36:58 UTC 2016

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.

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

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

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

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

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:

Wayland has always been "what you see is rendered by clients, then
composited by the compositor". Of course there are cases where this is
not true, for example there may be effects that are not possible to have
a client do, but it is the direction we should be working against.
Making the default CSD-SSD goes against that.

Weston (and mutter for that matter) I assume will never implement the
default "properly" (i.e. rendering server side shadows). We'd always
rely on clients implementing optional features to function properly.

It'd effectively make the default case broken and partly unusable in
any compositor not supporting full the CSD+SSD hybrid solution (i.e.
server side shadows). In the case where the client doesn't draw any
shadow, and the server doesn't draw any shadow, it'll also make it hard
to support interactive resize, since the shadow region is usually used
as a trigger area for resizing. For example, if you have a border-less
window, without shadow you'll suddenly have no area where you can let
your user click-drag to resize the window. The alternative is to have
logical invisible regions outside of the window implemented by the
compositor, but that make this even more a hybrid solution, but now also
with completely invisible special reagions.

I'd much rather see a solid default which does things "the Wayland way"
(as described above) that will work reasonably well in a minimalistic
default weston reference shell, and doesn't imply that the compositor
should go into effect drawing territory, and I don't think the hybrid
solution is this.

Jonas

> 
> Cheers
> Martin
> 
> [1] https://phabricator.kde.org/D1506
> [2] https://phabricator.kde.org/D1507

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