[PATCH] xdg-shell: add draw states for xdg_surface

[Yong Bakos]

On May 27, 2016, at 10:29 AM, Mike Blumenkrantz <zmike at osg.samsung.com> wrote:
> 
> this adds a method for compositors to change various draw attributes
> for a surface
> 
> Signed-off-by: Mike Blumenkrantz <zmike at osg.samsung.com>
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>

Hi Mike & Jonas,
A question about communicating default state, and some
minor nits you can certainly ignore, inline below.


> ---
> unstable/xdg-shell/xdg-shell-unstable-v6.xml | 69 ++++++++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
> 
> diff --git a/unstable/xdg-shell/xdg-shell-unstable-v6.xml b/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> index dfd7e84..0fa76d4 100644
> --- a/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> +++ b/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> @@ -843,6 +843,75 @@
>       </description>
>     </request>
> 
> +    <enum name="draw_state">
> +      <description summary="draw states for the surface">
> +        The draw state enum defines optional states which describe how

nit: that describe

> +        a client should draw a surface. A client must at least support the
> +        default state, and support for optional draw states is explicitly
> +        advertised using xdg_toplevel.set_available_draw_states.
> +
> +        The default draw state implies that the client draws the surface
> +        with complete window decorations.

nit: Same paragraph, so remove line break

> +        This may include, e.g., window frame and drop shadow.
> +
> +        Each draw state defines an alteration to the default. Some draw
> +        states may be combined, while some are mutually exclusive. See
> +        each draw state for details.
> +
> +        Desktop environments may extend this enum by taking up a range of
> +        values and documenting the range they chose in this description.
> +        They are not required to document the values for the range that they
> +        chose. Ideally, any good extensions from a desktop environment should

nit: extension (singular)

> +        make its way into standardization into this enum.
> +
> +        The current reserved ranges are:
> +
> +        0x0000 - 0x0FFF: xdg-shell core values, documented below.
> +        0x1000 - 0x1FFF: EFL
> +      </description>

should there be a 0 entry in the enum for default?
(see my comment re: empty array below)

> +      <entry name="no_shadow" value="1" summary="no dropshadow">
> +        <description summary="the surface without a dropshadow">
> +          The "no_shadow" draw state implies that the client must not draw

nit: not draw a

> +          drop shadow around the surface. This may have side effects
> +          on usability, e.g., the inability to activate client-initiated
> +          interactive resize.
> +        </description>
> +      </entry>
> +    </enum>
> +
> +    <event name="configure_draw_states">
> +      <description summary="set the draw state(s) of the surface">
> +        Set the draw state(s) which the client should use to draw a given

nit: that the client should

> +        surface. The absence of this event prior to an xdg_surface.configure
> +        event indicates that no change has occurred in the draw state since the
> +        previous xdg_surface.configure.
> +
> +        Sending an empty array of states with this method resets a surface to the
> +        default draw state.

Would it not be more explicit for compositors to pass a "default" enum value rather
than an empty array? (Assuming there is a default in the draw_state enum, per my
comment above.)
But, you will definitely know better than I!

> +
> +        This event is not sent by itself but as a latched state sent prior to
> +        the xdg_surface.configure event. When received, a client should adapt
> +        the drawing of the surface according to the state and respond to the
> +        configure event accordingly. See xdg_surface.ack_configure for
> +        details.
> +
> +        A compositor will only configure a client to draw with optional states on a
> +        given surface using the states which were advertised by that surface using

nit: that were advertised

> +        xdg_toplevel.set_available_draw_states.
> +      </description>
> +      <arg name="states" type="array" enum="draw_state"/>
> +    </event>
> +
> +    <request name="set_available_draw_states">
> +      <description summary="advertise optional draw states for the window">

nit: advertise available draw states
Seems clearer, as there's no separation between "available" and "optional," since
all draw states are optional. (Not being consistent here makes the reader second-
guess, "are there available ones and optional ones?")

> +        Inform the compositor of optional draw states which are available
> +        for the xdg_toplevel.

nit: of available draw states for the xdg_toplevel.
(same as previous reason)

> +
> +        Calling this after an xdg_toplevel's first commit will raise a client error.
> +      </description>
> +      <arg name="states" type="array" enum="draw_state"/>

Just a sanity check, since I haven't seen it in a protocol spec yet. Does scanner handle
this combination of array and enum correctly?

> +    </request>
> +
>     <event name="configure">
>       <description summary="suggest a surface change">
> 	This configure event asks the client to resize its toplevel surface or
> -- 
> 2.5.5

yong