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

Sat May 28 14:56:16 UTC 2016

On May 28, 2016, at 9:51 AM, Mike Blumenkrantz <michael.blumenkrantz at gmail.com> wrote:
> 
> On Sat, May 28, 2016 at 9:40 AM Yong Bakos <junk at humanoriented.com <mailto:junk at humanoriented.com>> wrote:
> 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 <mailto: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?
> 
> 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.
>  
> 
> (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.)
> 
> 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.

Ah, that's a good point. I'll address the scanner change as planned this week.

Thank you,
yong

>  
> 
> 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/cac58c62/attachment-0001.html>