On March 3, 2018 12:14 PM, Quentin Glidic <sardemff7+wayl...@sardemff7.net> wrote: > On 3/2/18 4:33 PM, Simon Ser wrote: > > This adds a new protocol to negotiate server- and client-side rendering of > > window decorations for xdg-toplevels. This allows compositors that want > > to draw decorations themselves to send their preference to clients, and > > clients that prefer server-side decorations to request them. > > > > This is inspired by a protocol from KDE [1] which has been implemented in > > KDE and Sway and was submitted for consideration in 2017 [2]. This patch > > provides an updated protocol with those concerns taken into account. > > > > Signed-off-by: Simon Ser <cont...@emersion.fr> > > Reviewed-by: Drew DeVault <s...@cmpwn.com> > > Reviewed-by: David Edmundson <da...@davidedmundson.co.uk> > > Reviewed-by: Alan Griffiths <alan.griffi...@canonical.com> > > Reviewed-by: Tony Crisci <t...@dubstepdish.com> > > > > [1] > > https://github.com/KDE/kwayland/blob/master/src/client/protocols/server-decoration.xml > > [2] > > https://lists.freedesktop.org/archives/wayland-devel/2017-October/035564.html > > This is a nice protocol, and I would implement it (in clients) in its > current form, but I still have a few comments.
Thanks for your comments! > > --- > > > > This was iterated on privately between representatives of Sway and wlroots > > (Simon Ser, Drew DeVault and Tony Crisci), KDE and Qt (David Edmundson), > > and Mir (Alan Griffiths). > > > > A proof-of-concept of a client and server implementation is available at > > [1]. > > > > Changes from v1 to v2: moved the enum above in the protocol, added > > Signed-off-by and Reviewed-by tags, updated the commit message. > > > > [1] https://github.com/swaywm/wlroots/pull/638 > > > > Makefile.am | 1 + > > unstable/xdg-toplevel-decoration/README | 4 + > > .../xdg-toplevel-decoration-unstable-v1.xml | 127 > > +++++++++++++++++++++ > > 3 files changed, 132 insertions(+) > > create mode 100644 unstable/xdg-toplevel-decoration/README > > create mode 100644 > > unstable/xdg-toplevel-decoration/xdg-toplevel-decoration-unstable-v1.xml > > > > diff --git a/Makefile.am b/Makefile.am > > index 4b9a901..07744e9 100644 > > --- a/Makefile.am > > +++ b/Makefile.am > > @@ -17,6 +17,7 @@ unstable_protocols = > > \ > > > > unstable/keyboard-shortcuts-inhibit/keyboard-shortcuts-inhibit-unstable-v1.xml > > \ > > unstable/xdg-output/xdg-output-unstable-v1.xml > > \ > > unstable/input-timestamps/input-timestamps-unstable-v1.xml \ > > + > > unstable/xdg-toplevel-decoration/xdg-toplevel-decoration-unstable-v1.xml > > \ > > $(NULL) > > > > stable_protocols = > > \ > > diff --git a/unstable/xdg-toplevel-decoration/README > > b/unstable/xdg-toplevel-decoration/README > > new file mode 100644 > > index 0000000..e927110 > > --- /dev/null > > +++ b/unstable/xdg-toplevel-decoration/README > > @@ -0,0 +1,4 @@ > > +xdg_toplevel_decoration protocol > > + > > +Maintainers: > > +Simon Ser <cont...@emersion.fr> > > diff --git > > a/unstable/xdg-toplevel-decoration/xdg-toplevel-decoration-unstable-v1.xml > > b/unstable/xdg-toplevel-decoration/xdg-toplevel-decoration-unstable-v1.xml > > new file mode 100644 > > index 0000000..acc1ed2 > > --- /dev/null > > +++ > > b/unstable/xdg-toplevel-decoration/xdg-toplevel-decoration-unstable-v1.xml > > @@ -0,0 +1,127 @@ > > +<?xml version="1.0" encoding="UTF-8"?> > > +<protocol name="xdg_toplevel_decoration_unstable_v1"> > > + <copyright> > > + Copyright © 2018 Simon Ser > > + > > + Permission is hereby granted, free of charge, to any person obtaining a > > + copy of this software and associated documentation files (the > > "Software"), > > + to deal in the Software without restriction, including without > > limitation > > + the rights to use, copy, modify, merge, publish, distribute, > > sublicense, > > + and/or sell copies of the Software, and to permit persons to whom the > > + Software is furnished to do so, subject to the following conditions: > > + > > + The above copyright notice and this permission notice (including the > > next > > + paragraph) shall be included in all copies or substantial portions of > > the > > + Software. > > + > > + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > EXPRESS OR > > + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > MERCHANTABILITY, > > + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > > SHALL > > + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > > OTHER > > + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > > + DEALINGS IN THE SOFTWARE. > > + </copyright> > > + > > + <interface name="zxdg_toplevel_decoration_manager_v1" version="1"> > > + <description summary="window decoration manager"> > > + This interface permits choosing between client-side and server-side > > + window decorations for a toplevel surface. > > + > > + A window decoration is a user interface component used to move, > > resize and > > + change a window's state. It can be managed either by the client > > (part of > > + the surface) or by the server. > > You mention decorations only, but this (or some other text below maybe) > should mention shadows as well. You may consider them as part of the > decoration, but xdg_surface.set_window_geometry() is designed to include > decoration and exclude shadows. Yet, e.g. GTK+ allows to resize using > shadows IIRC. > > Nit: I think GTK+ allows to move from any dead zone in the surface too. :-) xdg-shell's wording includes drop-shadows as part of decorations: > Client-side decorations often have invisible portions like drop-shadows which > should be ignored for the purposes of aligning, placing and constraining > windows. So "decorations" here stand for both visible parts such as the titlebar and invisible parts such as drop-shadows. Do you think the protocol needs to disambiguate these concepts? > > + By advertizing this interface the server anounces support for > > server-side > > + window decorations. > > Typos: advertising and announces. Right. > > + > > + Warning! The protocol described in this file is experimental and > > + backward incompatible changes may be made. Backward compatible > > changes > > + may be added together with the corresponding interface version bump. > > + Backward incompatible changes are done by bumping the version number > > in > > + the protocol and interface names and resetting the interface version. > > + Once the protocol is to be declared stable, the 'z' prefix and the > > + version number in the protocol and interface names are removed and > > the > > + interface version number is reset. > > + </description> > > + > > + <enum name="error"> > > + <entry name="unconfigured_buffer" value="1"/> > > + <entry name="already_constructed" value="2"/> > > + </enum> > > + > > + <request name="destroy" type="destructor"> > > + <description summary="destroy the decoration manager object"> > > + Destroy the decoration manager. > > + </description> > > + </request> > > + > > + <request name="get_decoration"> > > + <description summary="create a new decoration object"> > > + Create a new decoration object associated with the given toplevel. > > + > > + Creating a zxdg_toplevel_decoration_v1 from a xdg_toplevel which > > has a > > Nit: always use the final names in the descriptions, that it is still > clear enough and it avoids noise in future patches. Will do. > > + buffer attached or committed is a client error, and any attempts > > by a > > + client to attach or manipulate a buffer prior to the first > > + zxdg_toplevel_decoration_v1.configure call must also be treated as > > + errors. > > + </description> > > + <arg name="id" type="new_id" > > interface="zxdg_toplevel_decoration_v1"/> > > + <arg name="toplevel" type="object" interface="xdg_toplevel"/> > > + </request> > > + </interface> > > + > > + <interface name="zxdg_toplevel_decoration_v1" version="1"> > > + <description summary="decoration object for a toplevel surface"> > > + The decoration object allows the client to switch between a > > client-side > > + and server-side window decoration for a toplevel surface. > > + </description> > > + > > + <request name="destroy" type="destructor"> > > + <description summary="destroy the decoration object"> > > + Switch back to client-side-only window decoration mode. > > + </description> > > + </request> > > + > > + <request name="set_mode"> > > + <description summary="set the decoration mode"> > > + Set the toplevel surface decoration mode. > > + > > + After requesting a decoration mode, the compositor will respond by > > + emitting a xdg_surface.configure event. The client should then > > update > > + its content, drawing it with or without decorations depending on > > the > > + received mode. The client must also acknowledge the configure when > > + committing the new content (see xdg_surface.ack_configure). > > + > > + The compositor can ignore this request. > > + </description> > > + <arg name="mode" type="uint" enum="mode" summary="the decoration > > mode"/> > > + </request> > > This request I’m still not sure about it. > Why would an SSD-capable client would want to switch from CSD > back-and-forth? What is the use case here? (Preferably a user use case.) One reason is consistency with xdg-shell. But there are also real use-cases: - A compositor might prefer SSDs or CSDs depending of the window container. For instance, a tiled compositor might prefer SSDs for tiled windows and CSDs for floating windows. Windows can be moved between a tiling and a floating container at run-time. - Clients might expose a user setting that allows toggling SSDs. For instance, Chrome/Chromium already has such a feature. Requiring the user to restart the app or to quickly close and reopen the main window offers poor UX. > If there is a serious use case, then it is probably the nicest way. > Though I can think of another one: > 1a. destroying the object gets back to CSD *at the next commit*. (Your > destructor description is not clear on when it happens, and yet it says > it will happen ;-)) > 1b. destroying the xdg_surface altogether gets back to CSD. > 2. recreating an xdg_surface gets back to SSD You're right wrt the destructor description, the change should happen at the next commit. > If there is none, destroying the object can just be an error. > > IMO, the configure event is enough to carry the compositor decision, and > a client wanting SSD has little chance to want CSD back. > > If we drop the request, it can always be added back as an addition to > unstable-v1 (since adding is not a breaking change). > > If we keep the request we should change the wording, as I commented on > IRC, to “the compositor may respond”. (There is a pending patch to > xdg-shell for that too.) The change hasn't been accepted yet, so I'll wait a little. As for other comments, I believe I covered them in my answer above. > > + > > + <event name="preferred_mode"> > > + <description summary="advertise the server's preferred mode"> > > + The preferred_mode event describes the server's preferred > > decoration > > + mode for this toplevel surface. The event is sent when binding to > > the > > + decoration object and whenever the preferred mode changes. > > + </description> > > + <arg name="mode" type="uint" enum="mode" summary="the preferred > > mode"/> > > + </event> > > + > > + <event name="configure"> > > + <description summary="suggest a surface change"> > > + The configure event asks the client to change its decoration mode. > > The > > + configured state should not be applied immediately. See > > + xdg_surface.configure for details. > > + </description> > > + <arg name="mode" type="uint" enum="mode" summary="the decoration > > mode"/> > > + </event> > > + > > + <enum name="mode"> > > + <description summary="window decoration modes"> > > + These values describe window decoration modes. > > + </description> > > + <entry name="client" value="1" summary="client-side window > > decoration"/> > > + <entry name="server" value="2" summary="server-side window > > decoration"/> > > + </enum> > > + </interface> > > +</protocol> > > > > Last but not least: it should be much much clearer that the compositor > is in charge here. This is not about magic SSD, clients must support CSD > in all cases and should not error out if this global is not here. And > even then, it may want CSD in some cases. I've added this in the protocol description: > Note that even if the server supports server-side window decorations, clients > must still support client-side decorations. > Thanks, > > -- > > Quentin “Sardem FF7” Glidic > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel