<div dir="ltr"><br><div class="gmail_extra"><br><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="gmail-">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>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">
<span class="gmail-"><br>
> ---<br>
>  unstable/server-decoration/<wbr>server-decoration.xml | 94<br>
> ++++++++++++++++++++++++<br>
>  1 file changed, 94 insertions(+)<br>
>  create mode 100644 unstable/server-decoration/<wbr>server-decoration.xml<br>
><br>
> diff --git a/unstable/server-decoration/<wbr>server-decoration.xml<br>
> b/unstable/server-decoration/<wbr>server-decoration.xml<br>
> new file mode 100644<br>
> index 0000000..8bc106c<br>
> --- /dev/null<br>
> +++ b/unstable/server-decoration/<wbr>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>> +    Copyright (C) 2015 Martin Grنكlin<br>
<span class="gmail-">> +<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>><wbr>.<br>
<br>
</span>As mentioned elsewhere, it mixing different licenses could be somewhat<br>
inconvenient.<br>
<span class="gmail-"><br>
> +  ]]></copyright><br>
> +  <interface  name="org_kde_kwin_server_<wbr>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><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"><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="gmail-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>It shouldn't.  <br></div><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="gmail-"><br>
> +        </description><br>
> +        <arg name="id" type="new_id"<br>
> interface="org_kde_kwin_<wbr>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>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><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="gmail-"><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="gmail-"><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="gmail-"><br></span></blockquote><div>I think that's pretty much the intention. Seems sensible.<br></div><div><br></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="gmail-">
> +  </interface><br>
> +  <interface name="org_kde_kwin_server_<wbr>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_<wbr>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="gmail-"><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="gmail-"><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="gmail-"><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="gmail-"><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 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>