<div dir="ltr">I've inlined some replies below.<br><br><div class="gmail_quote"><div dir="ltr">On Fri, May 27, 2016 at 1:13 PM Yong Bakos <<a href="mailto:junk@humanoriented.com">junk@humanoriented.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On May 27, 2016, at 10:29 AM, Mike Blumenkrantz <<a href="mailto:zmike@osg.samsung.com" target="_blank">zmike@osg.samsung.com</a>> wrote:<br>
><br>
> this adds a method for compositors to change various draw attributes<br>
> for a surface<br>
><br>
> Signed-off-by: Mike Blumenkrantz <<a href="mailto:zmike@osg.samsung.com" target="_blank">zmike@osg.samsung.com</a>><br>
> Signed-off-by: Jonas Ådahl <<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>><br>
<br>
Hi Mike & Jonas,<br>
A question about communicating default state, and some<br>
minor nits you can certainly ignore, inline below.<br>
<br>
<br>
> ---<br>
> unstable/xdg-shell/xdg-shell-unstable-v6.xml | 69 ++++++++++++++++++++++++++++<br>
> 1 file changed, 69 insertions(+)<br>
><br>
> diff --git a/unstable/xdg-shell/xdg-shell-unstable-v6.xml b/unstable/xdg-shell/xdg-shell-unstable-v6.xml<br>
> index dfd7e84..0fa76d4 100644<br>
> --- a/unstable/xdg-shell/xdg-shell-unstable-v6.xml<br>
> +++ b/unstable/xdg-shell/xdg-shell-unstable-v6.xml<br>
> @@ -843,6 +843,75 @@<br>
>       </description><br>
>     </request><br>
><br>
> +    <enum name="draw_state"><br>
> +      <description summary="draw states for the surface"><br>
> +        The draw state enum defines optional states which describe how<br>
<br>
nit: that describe<br></blockquote><div><br></div><div>"which" is correct and intended in this case.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +        a client should draw a surface. A client must at least support the<br>
> +        default state, and support for optional draw states is explicitly<br>
> +        advertised using xdg_toplevel.set_available_draw_states.<br>
> +<br>
> +        The default draw state implies that the client draws the surface<br>
> +        with complete window decorations.<br>
<br>
nit: Same paragraph, so remove line break<br></blockquote><div><br></div><div>Line break was intentional: documentation will not generate a line break, but anyone reading the protocol file will see it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +        This may include, e.g., window frame and drop shadow.<br>
> +<br>
> +        Each draw state defines an alteration to the default. Some draw<br>
> +        states may be combined, while some are mutually exclusive. See<br>
> +        each draw state for details.<br>
> +<br>
> +        Desktop environments may extend this enum by taking up a range of<br>
> +        values and documenting the range they chose in this description.<br>
> +        They are not required to document the values for the range that they<br>
> +        chose. Ideally, any good extensions from a desktop environment should<br>
<br>
nit: extension (singular)<span style="line-height:1.5"> </span></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +        make its way into standardization into this enum.<br>
> +<br>
> +        The current reserved ranges are:<br>
> +<br>
> +        0x0000 - 0x0FFF: xdg-shell core values, documented below.<br>
> +        0x1000 - 0x1FFF: EFL<br>
> +      </description><br>
<br>
should there be a 0 entry in the enum for default?<br>
(see my comment re: empty array below)<br></blockquote><div><br></div><div>Other state enums begin at 1.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +      <entry name="no_shadow" value="1" summary="no dropshadow"><br>
> +        <description summary="the surface without a dropshadow"><br>
> +          The "no_shadow" draw state implies that the client must not draw<br>
<br>
nit: not draw a<br></blockquote><div><br></div><div>Using "a" would imply that the drop shadow is a singular unit instead of a combination of multiple shadow regions--a possible case.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +          drop shadow around the surface. This may have side effects<br>
> +          on usability, e.g., the inability to activate client-initiated<br>
> +          interactive resize.<br>
> +        </description><br>
> +      </entry><br>
> +    </enum><br>
> +<br>
> +    <event name="configure_draw_states"><br>
> +      <description summary="set the draw state(s) of the surface"><br>
> +        Set the draw state(s) which the client should use to draw a given<br>
<br>
nit: that the client should<br></blockquote><div><br></div><div>"which" is correct and intended in this case.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +        surface. The absence of this event prior to an xdg_surface.configure<br>
> +        event indicates that no change has occurred in the draw state since the<br>
> +        previous xdg_surface.configure.<br>
> +<br>
> +        Sending an empty array of states with this method resets a surface to the<br>
> +        default draw state.<br>
<br>
Would it not be more explicit for compositors to pass a "default" enum value rather<br>
than an empty array? (Assuming there is a default in the draw_state enum, per my<br>
comment above.)<br>
But, you will definitely know better than I!<br></blockquote><div><br></div><div>This was discussed, and the resulting decision was that implementations would be simplified if the default value never got passed here.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> +        This event is not sent by itself but as a latched state sent prior to<br>
> +        the xdg_surface.configure event. When received, a client should adapt<br>
> +        the drawing of the surface according to the state and respond to the<br>
> +        configure event accordingly. See xdg_surface.ack_configure for<br>
> +        details.<br>
> +<br>
> +        A compositor will only configure a client to draw with optional states on a<br>
> +        given surface using the states which were advertised by that surface using<br>
<br>
nit: that were advertised<br></blockquote><div><br></div><div>"which" is correct and intended in this case.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +        xdg_toplevel.set_available_draw_states.<br>
> +      </description><br>
> +      <arg name="states" type="array" enum="draw_state"/><br>
> +    </event><br>
> +<br>
> +    <request name="set_available_draw_states"><br>
> +      <description summary="advertise optional draw states for the window"><br>
<br>
nit: advertise available draw states<br>
Seems clearer, as there's no separation between "available" and "optional," since<br>
all draw states are optional. (Not being consistent here makes the reader second-<br>
guess, "are there available ones and optional ones?")<br></blockquote><div><br></div><div>It's possible explicit required states may be added in the future; your proposed change here would go against the main idea of this patch, i.e., not passing required states.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +        Inform the compositor of optional draw states which are available<br>
> +        for the xdg_toplevel.<br>
<br>
nit: of available draw states for the xdg_toplevel.<br>
(same as previous reason)<br></blockquote><div><br></div><div>Same as above.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> +        Calling this after an xdg_toplevel's first commit will raise a client error.<br>
> +      </description><br>
> +      <arg name="states" type="array" enum="draw_state"/><br>
<br>
Just a sanity check, since I haven't seen it in a protocol spec yet. Does scanner handle<br>
this combination of array and enum correctly?<br></blockquote><div><br></div><div>Good catch. This also affects the event above it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +    </request><br>
> +<br>
>     <event name="configure"><br>
>       <description summary="suggest a surface change"><br>
>       This configure event asks the client to resize its toplevel surface or<br>
> --<br>
> 2.5.5<br>
<br>
yong<br>
<br>
<br>
_______________________________________________<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><br>
</blockquote></div></div>