<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sat, May 28, 2016 at 9:40 AM 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"><div style="word-wrap:break-word">Hi Mike,<div>Regarding the combination of type="array" enum="foo"...</div><div><br><div></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div>On May 27, 2016, at 12:30 PM, Mike Blumenkrantz <<a href="mailto:michael.blumenkrantz@gmail.com" target="_blank">michael.blumenkrantz@gmail.com</a>> wrote:</div><br><div><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" target="_blank">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></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div><div>As we discussed via IRC (27 May), the scanner will choke on this. While we talked about</div><div>making a change to the scanner to allow this, perhaps such a change doesn't make sense.</div><div><br></div><div>Given a type="array", scanner will generate a parameter of type wl_array.</div><div><br></div><div>Perhaps the short story here is to just remove the enum from this arg, and the similar</div><div>arg in the configure_draw_states event above. What do you think?</div></div></div></div></blockquote><div><br></div><div>Yes, I supposed that my previous reply would indicate my plan to do this, but it seems I should have been more explicit. This is a trivial change, so I am not going to create a v2 thread for it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div><br></div><div>(I wonder if it's the DTD that can change, so the scanner's validation step</div><div>will catch the unsupported combination of type="array" enum="foo". My gut tells me that</div><div>DTDs don't support this logic, but I'll dig into this.)</div></div></div></div></blockquote><div><br></div><div>I think it would be useful to allow the annotation, even if there was no difference in the scanner output. Anyone who reads the xml directly would find it useful.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div><br></div><div>yong</div></div></div></div><div style="word-wrap:break-word"><div><div><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><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></blockquote></div></div></div></blockquote></div></div></div></blockquote></div></div>