Feedback on xdg-shell v5

Fri Apr 29 15:06:08 UTC 2016

On Fri, Apr 29, 2016 at 4:38 AM Martin Graesslin <mgraesslin at kde.org> 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.
>

I disagree that ping should be outside xdg-shell. The spec for this is
perfectly functional and has been successfully implemented by a number of
compositors/toolkits; claiming that you doubt its practicality based on
your choice to add complexity by using threads (and the bugs that you've
described in your implementation) is not a strong argument towards proving
that ping is not able to be successfully implemented.

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

Compositors can continue to send sizes for all configure events. There is
nothing in the spec which prohibits this behavior, it only says that sizes
may be omitted.

>
> >
> > > 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_______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160429/e8b1250b/attachment-0001.html>