[PATCH wayland-protocols 00/11] Declaring xdg-shell stable

Wed Jun 21 01:27:46 UTC 2017

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 <kde at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel