Hi Mike, Regarding the combination of type="array" enum="foo"...
> On May 27, 2016, at 12:30 PM, Mike Blumenkrantz > <michael.blumenkra...@gmail.com> wrote: > > I've inlined some replies below. > > On Fri, May 27, 2016 at 1:13 PM Yong Bakos <j...@humanoriented.com > <mailto:j...@humanoriented.com>> wrote: > On May 27, 2016, at 10:29 AM, Mike Blumenkrantz <zm...@osg.samsung.com > <mailto:zm...@osg.samsung.com>> wrote: > > > > this adds a method for compositors to change various draw attributes > > for a surface > > > > Signed-off-by: Mike Blumenkrantz <zm...@osg.samsung.com > > <mailto:zm...@osg.samsung.com>> > > Signed-off-by: Jonas Ådahl <jad...@gmail.com <mailto:jad...@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
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel