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

Sat May 28 13:39:59 UTC 2016

Hi Mike,
Regarding the combination of type="array" enum="foo"...

> On May 27, 2016, at 12:30 PM, Mike Blumenkrantz <michael.blumenkrantz at gmail.com> wrote:
> 
> I've inlined some replies below.
> 
> On Fri, May 27, 2016 at 1:13 PM Yong Bakos <junk at humanoriented.com <mailto:junk at humanoriented.com>> wrote:
> On May 27, 2016, at 10:29 AM, Mike Blumenkrantz <zmike at osg.samsung.com <mailto: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 <mailto:zmike at osg.samsung.com>>
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com <mailto: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.

As we discussed via IRC (27 May), the scanner will choke on this. While we talked about
making a change to the scanner to allow this, perhaps such a change doesn't make sense.

Given a type="array", scanner will generate a parameter of type wl_array.

Perhaps the short story here is to just remove the enum from this arg, and the similar
arg in the configure_draw_states event above. What do you think?

(I wonder if it's the DTD that can change, so the scanner's validation step
will catch the unsupported combination of type="array" enum="foo". My gut tells me that
DTDs don't support this logic, but I'll dig into this.)

yong

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160528/12c40899/attachment-0001.html>