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

Sat May 28 14:51:16 UTC 2016

On Sat, May 28, 2016 at 9:40 AM Yong Bakos <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> wrote:
>
> 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.
>
>
> 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.

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