<div dir="ltr">Hi,<div><br></div><div>Sorry I'm late to the review on this, I've been on an extended vacation.</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Nov 15, 2017 at 8:01 AM David Edmundson <<a href="mailto:david@davidedmundson.co.uk">david@davidedmundson.co.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 30, 2017 at 5:03 AM, Jonas Ådahl <span dir="ltr"><<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_3069416305206079433gmail-">On Fri, Oct 27, 2017 at 01:13:15PM +0100, David Edmundson wrote:<br>
> The server decoration protocol negotiates between the client and server<br>
> whether the client should default to drawing window decorations, and<br>
> informs the compositor what the client is doing.<br>
><br>
> This is useful not just for a compostior that is doing decorations<br>
> itself, but much more importantly for a toolkit, such as Qt which<br>
> primarily targets embedded and IVI applications, not to have to modify<br>
> clients<br>
> to add a header bar which makes them usable on a desktop compositor.<br>
><br>
> This file is currently copied in multiple places across GTK, Sway as well<br>
> as being needed in both Qt and KDE. We should have this in a shared<br>
> place.<br>
<br>
</span>I think that this functionality is in scope for wayland-protocols, but<br>
wayland-protocols was AFAIK never meant as a distribution of arbitrary<br>
external protocols. In the beginning, a major reason why it was created<br>
at all was because wayland.xml was getting too large and it was too<br>
inconvenient to wait for wayland releases for adding a new protocol or<br>
protocol version would we instead add new XML files to the wayland repo.<br>
<br>
I'd expect protocols that aim to be included here to be willing to go<br>
through protocol review and adhere to the conventions we have set up.<br>
I have read through the protocol and will provide feedback as if the<br>
protocol aims to follow the conventions we have in place.<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Sounds reasonable. <br></div><div>I understand you don't want to have a generic dumping ground.</div><div><br></div><div>Thanks for the very thorough review.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="m_3069416305206079433gmail-"><br>
> ---<br>
> unstable/server-decoration/server-decoration.xml | 94<br>
> ++++++++++++++++++++++++<br>
> 1 file changed, 94 insertions(+)<br>
> create mode 100644 unstable/server-decoration/server-decoration.xml<br>
><br>
> diff --git a/unstable/server-decoration/server-decoration.xml<br>
> b/unstable/server-decoration/server-decoration.xml<br>
> new file mode 100644<br>
> index 0000000..8bc106c<br>
> --- /dev/null<br>
> +++ b/unstable/server-decoration/server-decoration.xml<br>
> @@ -0,0 +1,94 @@<br>
> +<?xml version="1.0" encoding="UTF-8"?><br>
> +<protocol name="server_decoration"><br>
> + <copyright><![CDATA[<br>
</span></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> + Copyright (C) 2015 Martin Grنكlin</blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<span class="m_3069416305206079433gmail-">> +<br>
> + This program is free software: you can redistribute it and/or modify<br>
> + it under the terms of the GNU Lesser General Public License as<br>
> published by<br>
> + the Free Software Foundation, either version 2.1 of the License, or<br>
> + (at your option) any later version.<br>
> +<br>
> + This program is distributed in the hope that it will be useful,<br>
> + but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
> + GNU Lesser General Public License for more details.<br>
> +<br>
> + You should have received a copy of the GNU Lesser General Public<br>
> License<br>
> + along with this program. If not, see <<a href="http://www.gnu.org/licenses/" rel="noreferrer" target="_blank">http://www.gnu.org/licenses/</a>>.<br>
<br>
</span>As mentioned elsewhere, it mixing different licenses could be somewhat<br>
inconvenient.<br>
<span class="m_3069416305206079433gmail-"><br>
> + ]]></copyright><br>
> + <interface name="org_kde_kwin_server_decoration_manager" version="1"><br>
<br>
</span>I believe a protocol related to window decorations belong in the "xdg"<br>
family of protocols, extending 'xdg_wm_base'. For example<br>
'xdg_wm_window_decorations' could be the name of a window decoration<br>
extension to xdg_wm_base (that is the main interface of xdg_shell).<br></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"></blockquote><br></div><div class="gmail_quote">Sure, my initial intention was to share the spec that's currently being used, rather than <br></div><div class="gmail_quote">define something "xdg official". <br></div><div class="gmail_quote"><br></div><div class="gmail_quote">Long term the latter is probably better even if it is going to <br></div><div class="gmail_quote">be more work for all involved.</div><div class="gmail_quote"><br></div><div class="gmail_quote"></div><div class="gmail_quote"></div><div class="gmail_quote"></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="m_3069416305206079433gmail-h5"><br>
> + <description summary="Server side window decoration manager"><br>
> + This interface allows to coordinate whether the server should<br>
> create<br>
> + a server-side window decoration around a wl_surface representing a<br>
> + shell surface (wl_shell_surface or similar). By announcing support<br>
> + for this interface the server indicates that it supports server<br>
> + side decorations.<br>
> + </description><br>
> + <request name="create"><br>
> + <description summary="Create a server-side decoration object for a<br>
> given surface"><br>
> + When a client creates a server-side decoration object it<br>
> indicates<br>
> + that it supports the protocol. The client is supposed to tell<br>
> the<br>
> + server whether it wants server-side decorations or will provide<br>
> + client-side decorations.<br>
> +<br>
> + If the client does not create a server-side decoration object<br>
> for<br>
> + a surface the server interprets this as lack of support for<br>
> this<br>
> + protocol and considers it as client-side decorated.<br>
> Nevertheless a<br>
> + client-side decorated surface should use this protocol to<br>
> indicate<br>
> + to the server that it does not want a server-side deco.<br>
<br>
</div></div>What is the purpose of a client not supporting server side decorations<br>
to create this object anyway? I assume functionality wise it shouldn't<br>
make any difference right?<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">It shouldn't. <br></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_3069416305206079433gmail-"><br>
> + </description><br>
> + <arg name="id" type="new_id"<br>
> interface="org_kde_kwin_server_decoration"/><br>
> + <arg name="surface" type="object" interface="wl_surface"/><br>
> + </request><br>
> + <enum name="mode"><br>
> + <description summary="Possible values to use in request_mode<br>
> and the event mode."/><br>
> + <entry name="None" value="0" summary="Undecorated: The surface<br>
> is not decorated at all, neither server nor client-side. An example is a<br>
> popup surface which should not be decorated."/><br>
<br>
</span>Why would a popup surface create a window decoration object at all? It<br>
seems pointless. I might be missing something but it seems to only make<br>
any sense to only deal with toplevels. A request creating the window<br>
decoration object could for example require that a surface passed is 'an<br>
xdg_toplevel or equivalent'.<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>You're right that a popup wouldn't need to.</div><div>There are top levels that need it setting to 0. Like splash screens.<br></div><div><br></div><div>The other issue we have is this doesn't attach to an xdg_toplevel/popup but to the <br></div><div>wl_surface. A surface has no concept of toplevels/popups and that becomes messy to define cross specs.<br></div><div><br></div><div>This was primarily due to the timeframe when it was made as we were actively supporting wl_shell_surface and xdg_shell is still changing.<br></div><div><br></div><div>If we have to do the rename, and thus an API break, then I think it would make sense to attach to the stable toplevel and then that goes away.<br></div></div></div></div></blockquote><div><br></div><div>This has now been stabilized, so the protocol should be updated to use xdg_toplevel as suggested.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="m_3069416305206079433gmail-"><br>
> + <entry name="Client" value="1" summary="Client-side<br>
> decoration: The decoration is part of the surface and the client."/><br>
> + <entry name="Server" value="2" summary="Server-side<br>
> decoration: The server embeds the surface into a decoration frame."/><br>
<br>
</span>style nit: For consistency, use lower case for all entry names. They'll<br>
become upper case anyway in the C file.<br>
<span class="m_3069416305206079433gmail-"><br>
> + </enum><br>
> + <event name="default_mode"><br>
> + <description summary="The default mode used on the server"><br>
> + This event is emitted directly after binding the interface.<br>
> It contains<br>
> + the default mode for the decoration. When a new server<br>
> decoration object<br>
> + is created this new object will be in the default mode until<br>
> the first<br>
> + request_mode is requested.<br>
> +<br>
> + The server may change the default mode at any time.<br>
> + </description><br>
> + <arg name="mode" type="uint" summary="The default decoration<br>
> mode applied to newly created server decorations."/><br>
> + </event><br>
<br>
</span>Using a potentially changing 'default_mode' seems racy to me, especially<br>
if the protocol allows a client to rely on what is default to choose.<br>
Exposing a 'preferred_mode' that doesn't actually affect the state seems<br>
less prone to races, as a client will then actively take the<br>
'preferred_mode' into consideration when setting up its state.<br>
<span class="m_3069416305206079433gmail-"><br></span></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I think that's pretty much the intention. Seems sensible.<br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div></div></div></div></blockquote><div><br></div><div>This should probably use the mode enum in the arg?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_3069416305206079433gmail-">
> + </interface><br>
> + <interface name="org_kde_kwin_server_decoration" version="1"><br>
<br>
</span>Correct me if I'm wrong, but this is an object about decorating a<br>
toplevel (i.e. non-popup/non-tooltip) window right? A suggestion of what<br>
to call it is "xdg_toplevel_window_decorator", to indicate that it<br>
extends the xdg_toplevel (or an equivalent).<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="m_3069416305206079433gmail-"><br>
> + <request name="release" type="destructor"><br>
<br>
</span>This should be "destroy", as otherwise wayland-scanner will create a<br>
_destroy() function that doesn't actually destroy the server side<br>
object.<br>
<span class="m_3069416305206079433gmail-"><br>
> + <description summary="release the server decoration object"/><br>
> + </request><br>
> + <enum name="mode"><br>
> + <description summary="Possible values to use in request_mode<br>
> and the event mode."/><br>
> + <entry name="None" value="0" summary="Undecorated: The surface<br>
> is not decorated at all, neither server nor client-side. An example is a<br>
> popup surface which should not be decorated."/><br>
> + <entry name="Client" value="1" summary="Client-side<br>
> decoration: The decoration is part of the surface and the client."/><br>
> + <entry name="Server" value="2" summary="Server-side<br>
> decoration: The server embeds the surface into a decoration frame."/><br>
> + </enum><br>
<br>
</span>No need to redefine the same mode enum here. Just use the one in the<br>
parent interface.<br>
<span class="m_3069416305206079433gmail-"><br>
> + <request name="request_mode"><br>
> + <description summary="The decoration mode the surface wants to<br>
> use."/><br>
> + <arg name="mode" type="uint" summary="The mode this surface<br>
> wants to use."/><br>
> + </request><br>
<br>
</span>For consistency with other xdg_* protocols, I'd suggest calling this<br>
"set_mode" instead.<br>
<span class="m_3069416305206079433gmail-"><br>
> + <event name="mode"><br>
> + <description summary="The new decoration mode applied by the<br>
> server"><br>
> + This event is emitted directly after the decoration is<br>
> created and<br>
> + represents the base decoration policy by the server. E.g. a<br>
> server<br>
> + which wants all surfaces to be client-side decorated will<br>
> send Client,<br>
> + a server which wants server-side decoration will send Server.<br>
> +<br>
> + The client can request a different mode through the<br>
> decoration request.<br>
> + The server will acknowledge this by another event with the<br>
> same mode. So<br>
> + even if a server prefers server-side decoration it's<br>
> possible to force a<br>
> + client-side decoration.<br>
> +<br>
> + The server may emit this event at any time. In this case the<br>
> client can<br>
> + again request a different mode. It's the responsibility of<br>
> the server to<br>
> + prevent a feedback loop.<br>
<br>
</span>The way this seem to work seems racy, as it doesn't contain any<br>
information about how to guarantee that a window is displayed properly<br>
every frame. I think it should instead make use of the mechanism we now<br>
have in place that is meant to fix exactly this type of problems:<br>
xdg_surface.configure & xdg_surface.ack_configure. By using extending<br>
that functionality, a client and compositor can negotiate a state while<br>
guaranteeing that when a client sends a surface state, the compositor<br>
will know exactly what its configuration is. xdg_toplevel and xdg_popup<br>
already extends this functionality for negotiating state, and a<br>
decoration protocol should do the same.<br></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"></div><div class="gmail_quote"><br></div><div class="gmail_quote">You're right the spec doesn't specify this very well at all, which needs fixing.</div></div><div class="gmail_extra"></div><div class="gmail_extra"><div class="gmail_quote">We definitely want to enforce the client's mode to be in sync with the buffer.<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">Personally I'd just change the docs to force servers and surfaces to be static set on bind and before the first commit.<br></div></div><div class="gmail_extra"><br><br></div></div>
_______________________________________________<br>
wayland-devel mailing list<br>
<a href="mailto:wayland-devel@lists.freedesktop.org" target="_blank">wayland-devel@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/wayland-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/wayland-devel</a></blockquote><div><br></div><div>My take on the protocol is that it's a good start, and I could see Enlightenment supporting a standardized version of this, but I think it has a few issues which could be improved on:<br></div><div><br></div><div>* Too complex. A compositor which advertises this protocol should be a compositor which uses SSD; CSD is the default and is a requirement, so having a mode for it makes no sense. Once an application has bound the global, there should be a simple "request" method for an xdg_surface (which must be called prior to the first commit on that surface). This would inform the server that it will use SSD for the lifetime of the surface, or until a null buffer is attached to reset the state of the surface.</div><div><br></div><div>* This should use xdg_surface, not toplevel or wl_surface. Popups will indeed not use SSD borders, but a compositor may want to provide dropshadows or some other effect which can be considered SSD, so the application must guarantee that this is viable by enabling SSD on those surfaces as well. Additionally, the protocol should likely specify that attempting to enable SSD on a surface which has called set_geometry (or vice versa) is a client error.</div><div><br></div><div>* Lacks features: primarily the ability to disable CSD while also not providing SSD. An example of this would be terminal emulators which want to be able to use SSD but have a gui option to "toggle border" in order to copy a commandline option. A request to toggle the border would not necessarily need to be tied to a specific surface commit since it would not impact the client's drawing in any way and would only be available to clients which have already enabled SSD by using the initial activation request.</div><div><br></div><div>Making the above changes would eliminate the existing race conditions, simplify the protocol and implementations, and provide features likely to be desired by applications.</div></div></div>