On Tue, Jun 20, 2017 at 06:55:41PM +0100, David Edmundson wrote:
>
> From 093ed1a17a483792e316f932e15a566ab2653838 Mon Sep 17 00:00:00 2001
> From: David Edmundson <k...@davidedmundson.co.uk>
> Date: Tue, 20 Jun 2017 18:51:45 +0100
> Subject: [PATCH 2/2] xdg-shell/positioner: Replace edge bitfield with extended
> enum
>
> Bitfields allowed for impossible combinations of anchor edges, such as
> being on the left and right edge. Use of explicit enumerations means we
> don't need to handle that case.
Although it is not a requirement, we usually add Signed-off-by here. Do
you want to add it?
> ---
> stable/xdg-shell/xdg-shell.xml | 71
> ++++++++++++++++++------------------------
> 1 file changed, 30 insertions(+), 41 deletions(-)
>
> diff --git a/stable/xdg-shell/xdg-shell.xml b/stable/xdg-shell/xdg-shell.xml
> index ffba86d..020af9e 100644
> --- a/stable/xdg-shell/xdg-shell.xml
> +++ b/stable/xdg-shell/xdg-shell.xml
> @@ -179,63 +179,52 @@
> <arg name="height" type="int" summary="height of anchor rectangle"/>
> </request>
>
> - <enum name="anchor" bitfield="true">
> - <entry name="none" value="0"
> - summary="the center of the anchor rectangle"/>
> - <entry name="top" value="1"
> - summary="the top edge of the anchor rectangle"/>
> - <entry name="bottom" value="2"
> - summary="the bottom edge of the anchor rectangle"/>
> - <entry name="left" value="4"
> - summary="the left edge of the anchor rectangle"/>
> - <entry name="right" value="8"
> - summary="the right edge of the anchor rectangle"/>
> +
nit: extra newline
> + <enum name="anchor">
> + <entry name="none" value="0"/>
> + <entry name="top" value="1"/>
> + <entry name="bottom" value="2"/>
> + <entry name="left" value="4"/>
> + <entry name="top_left" value="5"/>
> + <entry name="bottom_left" value="6"/>
> + <entry name="right" value="8"/>
> + <entry name="top_right" value="9"/>
> + <entry name="bottom_right" value="10"/>
> </enum>
>
> <request name="set_anchor">
> <description summary="set anchor rectangle anchor edges">
> Defines a set of edges for the anchor rectangle. These are used to
> derive an anchor point that the child surface will be positioned
> - relative to. If two orthogonal edges are specified (e.g. 'top' and
> - 'left'), then the anchor point will be the intersection of the edges
> - (e.g. the top left position of the rectangle); otherwise, the derived
> - anchor point will be centered on the specified edge, or in the center of
> - the anchor rectangle if no edge is specified.
> -
> - If two parallel anchor edges are specified (e.g. 'left' and 'right'),
> - the invalid_input error is raised.
> + relative to. The anchor point will be in the specified corner, in the
> center
> + of a specified edge, or in the center of the anchor rectangle if no edge
> + is specified.
We don't define a set of "edges" anymore, so I suggest changing that
too. We still derive the resulting point however, so we should still
include that.
I suggest this wording, which tries to minimize the change:
Defines the anchor point for the anchor rectangle. The are used
to derive an anchor point that the child surface will be
positioned relative to. If a corner anchor is set (e.g.
'top_left' or 'bottom_right'), the anchor point will be at the
specified corner; otherwise, the derived anchor point will be
centered on the specified edge, or in the center of the anchor
rectangle if no edge is specified.
> </description>
> <arg name="anchor" type="uint" enum="anchor"
> - summary="bit mask of anchor edges"/>
> + summary="anchor edge"/>
Not really an 'edge' anymore either, so maybe just "anchor" is better
here.
> </request>
>
> - <enum name="gravity" bitfield="true">
> - <entry name="none" value="0"
> - summary="center over the anchor edge"/>
> - <entry name="top" value="1"
> - summary="position above the anchor edge"/>
> - <entry name="bottom" value="2"
> - summary="position below the anchor edge"/>
> - <entry name="left" value="4"
> - summary="position to the left of the anchor edge"/>
> - <entry name="right" value="8"
> - summary="position to the right of the anchor edge"/>
> + <enum name="gravity">
> + <entry name="none" value="0"/>
> + <entry name="top" value="1"/>
> + <entry name="bottom" value="2"/>
> + <entry name="left" value="4"/>
> + <entry name="top_left" value="5"/>
> + <entry name="bottom_left" value="6"/>
> + <entry name="right" value="8"/>
> + <entry name="top_right" value="9"/>
> + <entry name="bottom_right" value="10"/>
> </enum>
>
> <request name="set_gravity">
> <description summary="set child surface gravity">
> - Defines in what direction a surface should be positioned, relative to
> - the anchor point of the parent surface. If two orthogonal gravities are
> - specified (e.g. 'bottom' and 'right'), then the child surface will be
> - placed in the specified direction; otherwise, the child surface will be
> - centered over the anchor point on any axis that had no gravity
> - specified.
> -
> - If two parallel gravities are specified (e.g. 'left' and 'right'), the
> - invalid_input error is raised.
> + Defines which edge of the surface should be positioned relative to
> + the anchor of the parent surface. The anchor point will be in the
> specified corner,
> + in the center of a specified edge, or in the center of the anchor
> rectangle if no edge
> + is specified.
I suggest continuing talking about "direction" here. An alternative to
the above could be, changing the original text just a bit:
Defines in what direction a surface should be positioned,
relative to the anchor point of the parent surface. If a corner
gravity is specified (e.g. 'bottom_right' or 'top_left'), then
the child surface will be placed in the specified direction;
otherwise, the child surface will be centered over the anchor
point on any axis that had no gravity specified.
The semantical change itself I have no objection to. It seems reasonable
to use the same concept as the interactive resize enum. Thanks a lot for
working on this.
Jonas
> </description>
> <arg name="gravity" type="uint" enum="gravity"
> - summary="bit mask of gravity directions"/>
> + summary="gravity direction"/>
> </request>
>
> <enum name="constraint_adjustment" bitfield="true">
> --
> 2.12.0
>
> _______________________________________________
> 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
