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. > > > + </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 >
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel