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

Fri May 27 17:30:46 UTC 2016

I've inlined some replies below.

On Fri, May 27, 2016 at 1:13 PM Yong Bakos <junk at humanoriented.com> wrote:

> 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
>

"which" is correct and intended in this case.

>
> > +        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
>

Line break was intentional: documentation will not generate a line break,
but anyone reading the protocol file will see it.

>
> > +        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)
>

Other state enums begin at 1.

>
> > +      <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
>

Using "a" would imply that the drop shadow is a singular unit instead of a
combination of multiple shadow regions--a possible case.

>
> > +          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
>

"which" is correct and intended in this case.

>
> > +        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 was discussed, and the resulting decision was that implementations
would be simplified if the default value never got passed here.

>
> > +
> > +        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
>

"which" is correct and intended in this case.

>
> > +        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?")
>

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.

>
> > +        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)
>

Same as above.

>
> > +
> > +        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?
>

Good catch. This also affects the event above it.

>
> > +    </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
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160527/c4b5aaa8/attachment.html>