On Sat, May 28, 2016 at 9:40 AM Yong Bakos <j...@humanoriented.com> wrote:
> 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> wrote: > >> On May 27, 2016, at 10:29 AM, Mike Blumenkrantz <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> >> > Signed-off-by: Jonas Ådahl <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? > 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 >> >
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel