[PATCH wayland-protocols 7/7] xdg-shell: Introduce xdg_positioner

Jonas Ådahl jadahl at gmail.com
Wed Apr 20 03:28:41 UTC 2016


On Tue, Apr 19, 2016 at 04:50:31PM +0000, Mike Blumenkrantz wrote:
> I've emerged from my bike shed in order to join this expert-level
> shedcraftsmanship discussion.

Excellent!

> 
> On Thu, Apr 14, 2016 at 4:28 AM Jonas Ådahl <jadahl at gmail.com> wrote:
> 
> > xdg_positioner is a method for declarative positioning of child surfaces
> > (currently only xdg_popup surfaces). A client creates a description of a
> > positioning logic using the xdg_positioner interface. The xdg_positioner
> > object is then used when creating a xdg_popup for describing how the
> > child surface should be positioned in relation to the parent surface.
> >
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > Signed-off-by: Mike Blumenkrantz <zmike at samsung.com>
> > ---
> >  unstable/xdg-shell/xdg-shell-unstable-v6.xml | 206
> > ++++++++++++++++++++++++++-
> >  1 file changed, 204 insertions(+), 2 deletions(-)
> >
> > diff --git a/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> > b/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> > index a2a6f12..2b51802 100644
> > --- a/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> > +++ b/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> > @@ -57,6 +57,15 @@
> >        </description>
> >      </request>
> >
> > +    <request name="create_positioner">
> > +      <description summary="create a positioner object">
> > +       Create a positioner object. A positioner object is used to position
> > +       surfaces relative to some parent surface. See the interface
> > description
> >
> 
> If this is going to operate based on a surface's parent (ie. set_parent)
> then it should read "A position object is used to position its surface(s)
> relative to their parent surface (see set_parent)" in order to avoid any
> confusion.

It's not meant to be used in set_parent() but in get_popup(). That
being said, maybe it'll be used by some other similar request later.

I'll add "see xdg_surface.get_popup", to point out where it is used
right now though.

> 
> Moreover, I'm thinking this object needs to somehow be tethered to a single
> toplevel hierarchy; the current text has no restrictions about the
> positioner's use, meaning that it could be used for eg.
> 
> Surface A
> [Positioner P]
> Popup A
> 
> Surface B
> [Positioner P]
> Popup B
> [Positioner P]
> Popup B2
> 
> which would be confusing, and could also lead to annoying calculation
> issues if Surface A was huge and Surface B was tiny.

The idea is that it's just like wl_region, i.e. its just stupid a
description. Whenever used, the compositor should copy the state of the
description at the time it was used.

I don't see any point in tying them to any surface. It'll just make
state tracking more complicated server side.

> 
> 
> > +       for details.
> > +      </description>
> > +      <arg name="id" type="new_id" interface="zxdg_positioner_v6"/>
> > +    </request>
> > +
> >      <request name="get_xdg_surface">
> >        <description summary="create a shell surface from a surface">
> >         This creates an xdg_surface for the given surface. While
> > xdg_surface
> > @@ -96,6 +105,186 @@
> >      </event>
> >    </interface>
> >
> > +  <interface name="zxdg_positioner_v6" version="1">
> > +    <description summary="child surface positioner">
> > +      The xdg_positioner provides an interface for constructing
> > positioning
> > +      rules used for positioning a child surface relative to another
> > surface
> >
> 
> Same as above.
> 
> 
> > +      in a certain way. It allows methods for defining a rule that will
> > make
> > +      the child surface stay within the border of the visible area of the
> >
> 
> I'd rather have this specifically reference "the positioner's surface(s)"
> or similar instead of "the child surface" in order to keep context within
> the positioner interface.

As mentioned above, I think adding persistent positioner-surface
relationships adds unnecessary complexity and state tracking. If we let
the positioner be a stupid box of values, I think life will be easier.

> 
> 
> > +      screen, with different ways in which the child surface should change
> > +      its position, including sliding along an axis, or flipping around a
> > +      rectangle.
> > +
> > +      See the various requests for details about possible rules.
> > +
> > +      An xdg_positioner object may be re-used when positioning different
> >
> 
> Saying "re-used" here makes it sound like the positioner can only have one
> surface attached to it at a time. I think something like "...may continue
> to be applied to any xdg_popup surfaces until destroyed..." would be more
> clear.

Yes, that is better.

> 
> 
> > +      surfaces until destroyed using xdg_positioner.destroy.
> > +    </description>
> > +
> > +    <enum name="error">
> > +      <entry name="invalid_input" value="0" summary="invalid input
> > provided"/>
> > +    </enum>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroy the xdg_positioner object">
> > +       Notify the compositor that the xdg_positioner will no longer be
> > used.
> >
> 
> Should this include an explicit note about what to do with surfaces which
> were using the positioner, eg. "No changes should be made to users of the
> positioner upon its destruction" ?

If we make it clear that the state of the positioner doesn't affect the
surface position after it has been used, I think we could leave this as
is.

> 
> 
> > +      </description>
> > +    </request>
> > +
> > +    <request name="set_anchor_rect">
> > +      <description summary="set the anchor rectangle of the parent
> > surface">
> >
> 
> "set the anchor rectangle within the parent surface"

Yes, that sounds better.

> 
> 
> > +       Specify the anchor rectangle of the parent surface that the child
> > +       surface will be placed relative to. The rectangle is relative to
> > the
> > +       window geometry as defined by xdg_surface.set_window_geometry of
> > the
> > +       parent surface. The rectangle must be at least be 1x1 large.
> > +
> > +       When used to position a child surface, the attach rectangle may not
> > +       extend outside of the window geometry of the parent surface.
> >
> 
> I think this also needs to be clarified to something like "The positioner's
> attach rectangle may not extend outside the window geometry of the user
> surface's parent" in order to handle the case where the positioner is used
> for multiple surfaces with different parents in the same hierarchy chain
> (assuming this is going to be allowed).

Trying to think of a case where we'd want to position things outside of
the window geometry, but can't so its sure, lets make that an error.

> 
> 
> > +      </description>
> > +      <arg name="x" type="int" summary="x position of anchor rectangle"/>
> > +      <arg name="y" type="int" summary="y position of anchor rectangle"/>
> > +      <arg name="width" type="int" summary="width of anchor rectangle"/>
> > +      <arg name="height" type="int" summary="height of anchor rectangle"/>
> > +    </request>
> > +
> > +    <enum name="anchor">
> > +      <entry name="none" value="0"
> > +            summary="the center of the anchor rectangle"/>
> > +      <entry name="left" value="1"
> > +            summary="the left edge of the anchor rectangle"/>
> > +      <entry name="right" value="2"
> > +            summary="the right edge of the anchor rectangle"/>
> > +      <entry name="top" value="4"
> > +            summary="the top edge of the anchor rectangle"/>
> > +      <entry name="bottom" value="8"
> > +            summary="the bottom edge of the anchor rectangle"/>
> > +    </enum>
> > +
> > +    <request name="set_anchor">
> > +      <description summary="set anchor rect anchor edges">
> > +       Set the anchor edges of the anchor rectangle. The anchor edges
> > +       defines where on the anchor rectangle the child surface should
> > +       positioned relative to. An anchor is a bit mask of zero to two
> > values of
> >
> 
> I understand what you mean by "zero to two", but saying "zero values of the
> anchor enum" is a bit weird since 0/none is an enum value.

True. Hmm. Will try to reword this some how.

> 
> 
> > +       the anchor enum. Two values on the same axis (for example left and
> > +       right) may not be combined.
> >
> 
> Are we defining an error for this case or ?

At first I had an error for each error case, but in the end I just added
one "invalid input" ala EINVAL. I'll try to reword the paragraph to
contain something about an error being raised.

> 
> 
> > +
> > +       If no anchor is set on any axis, the anchor position will be
> > positioned
> > +       at the center of the anchor rectangle on the unset axis. The
> > default
> > +       value is "none".
> > +      </description>
> > +      <arg name="anchor" type="uint" bitfield="true"
> > +          summary="bit mask of anchor edges"/>
> > +    </request>
> > +
> > +    <enum name="gravity">
> > +      <entry name="none" value="0"
> > +            summary="center above the anchor position"/>
> >
> 
> I think maybe you meant "center over" ?

Yes, sounds better.

> 
> 
> > +      <entry name="left" value="1"
> > +            summary="position to the left of the anchor position"/>
> > +      <entry name="right" value="2"
> > +            summary="position to the right of the anchor position"/>
> > +      <entry name="top" value="4"
> > +            summary="position above the anchor position"/>
> > +      <entry name="bottom" value="8"
> > +            summary="position below the anchor position"/>
> > +    </enum>
> > +
> > +    <request name="set_gravity">
> > +      <description summary="set child surface gravity">
> > +       The gravity defines in what direction a surface would be
> > positioned,
> > +       relative to the anchor position on the parent surface. A gravity
> > +       is a bit mask of zero to two values of the gravity enum. Two values
> > +       on the same axis (for example left and right) may not be combined.
> > +
> > +       If no gravity is set on an axis, the gravity for that axis will be
> > +       equivalent to setting "none" for that axis, resulting in the child
> > being
> > +       centered over the anchor on that axis.
> >
> 
> I think for all cases of "anchor" and "anchor position" in this request and
> the preceding enum we should probably be strict in using the term "anchor
> rectangle" in order to maintain consistency.

Will fix.

> 
> 
> > +      </description>
> > +      <arg name="gravity" type="uint" bitfield="true"
> > +          summary="bit mask of gravity directions"/>
> > +    </request>
> > +
> > +    <enum name="constrain_action">
> >
> 
> I don't like the use of "action" here as, to me, it seems to imply user
> action rather than compositor calculations. Maybe something more like
> "fallback_position" or "position_adjustment"?

How about "constraint_adjustment"?

> 
> 
> > +      <entry name="none" value="0">
> > +       <description summary="don't move the child surface when
> > constrained">
> > +         Don't alter the surface position even if it is constrained on
> > some
> > +         axis, for example partially outside the edge of monitor.
> > +       </description>
> > +      </entry>
> > +      <entry name="slide_x" value="1">
> > +       <description summary="move along the X-axis until unconstrained">
> > +         Slide the surface along the X-axis until it is no longer
> > constrained.
> > +         If the left side of the surface is constrained, slide towards the
> > +         right; if the right side of the surface is constrained, slide
> > towards
> > +         the left. If both sides are ends up being constrained, the
> > behaviour is
> >
> 
> I think the directionality here should be based on the gravity, eg. if LEFT
> gravity is used then it tries to move left if possible, moving right
> afterward only if a leftward movement is impossible or does not solve the
> positioning. This handles the case of both sides being constrained since
> the compositor is then unable to perform any positioning corrections.

Aha, I see. You want to make it predictable what happens if initially
one side is constrained, but it's not possible to make it completely
unconstrained. I'll amend according to your suggestion.

> 
> 
> > +         undefined.
> > +       </description>
> > +      </entry>
> > +      <entry name="slide_y" value="2">
> > +       <description summary="move along the Y-axis until unconstrained">
> > +         Slide the surface along the Y-axis until it is no longer
> > constrained.
> > +         If the top side of the surface is constrained, slide towards the
> > +         bottom; if the bottom side of the surface is constrained, slide
> > towards
> > +         the top. If both sides are ends up being constrained, the
> > behaviour is
> > +         undefined.
> > +       </description>
> > +      </entry>
> > +      <entry name="flip_x" value="4">
> > +       <description summary="invert the anchor and gravity on the X-axis">
> > +         Invert the anchor and gravity on the X-axis if the surface is
> > +         constrained on the X-axis. For example, if the left edge of the
> > +         surface is constrained, the gravity is 'left' and the anchor is
> > +         'left', change the gravity to 'right' and the anchor to 'right'.
> > If
> > +         the resulting position after inverting ends up also being
> > constrained,
> > +         the behaviour is undefined.
> > +       </description>
> > +      </entry>
> > +      <entry name="flip_y" value="8">
> > +       <description summary="invert the anchor and gravity on the Y-axis">
> > +         Invert the anchor and gravity on the Y-axis if the surface is
> > +         constrained on the Y-axis. For example, if the bottom edge of the
> > +         surface is constrained, the gravity is 'bottom' and the anchor is
> > +         'bottom', change the gravity to 'top' and the anchor to 'top'. If
> > +         the resulting position after inverting ends up also being
> > constrained,
> > +         the behaviour is undefined.
> > +       </description>
> > +      </entry>
> > +    </enum>
> > +
> > +    <request name="set_constrain_action">
> > +      <description summary="set the action to be done when constrained">
> >
> 
> Same as my above comment re:action.
> 
> 
> > +       Specify how the window should be positioned if the originally
> > intended
> > +       position caused the surface to be constrained, meaning partially
> > outside
> >
> 
> Probably "meaning at least partially" to cover the case where the surface
> is completely outside the screen.

I suppose so.

> 
> 
> > +       positioning boundaries set by the compositor. The action is set by
> > +       constructing a bitmask with one bit per axis set describing the
> > action
> > +       to be taken when the surface is constrained on that axis. If no
> > bit for
> > +       one axis is set, the compositor will assume that the child surface
> > +       should not change its position on that axis when constrained. The
> > +       default action is none.
> > +      </description>
> > +      <arg name="constrain_action" type="uint" bitfield="true"
> > +          summary="bit mask of constrain actions"/>
> > +    </request>
> > +
> > +    <request name="set_offset">
> > +      <description summary="set surface position offset">
> > +       Specify the surface position offset relative to the position of the
> > +       anchor on the anchor rectangle and the anchor on the surface. For
> > +       example if the anchor of the anchor rectangle is at (x, y), the
> > surface
> > +       has the gravity bottom|right, and the offset is (ox, oy), the
> > calculated
> > +       surface position will be (x + ox, y + oy). The offset position of
> > the
> > +       surface is the one used for constraint testing. See
> > +       set_constraint_actions.
> > +
> > +       An example use case is placing a popup menu on top of a user
> > interface
> > +       element, while aligning the user interface element of the parent
> > surface
> > +       with some user interface element placed somewhere in the popup
> > surface.
> > +      </description>
> > +      <arg name="x" type="int" summary="surface position x offset"/>
> >
> 
> These should probably be "anchor rectangle x/y offset"

Should it really? It is meant to be the offset the positioned surface
has to the anchor rectangle, not the offset of the anchor rectangle.


I.e. If I have this situation:


                 +-------+<--
                 |       |  |
popup surface -> |       |  | y  =  distance from popup edge to
                 |       |  |       anchor rect edge
                 +- - - -+<--
                 |       | <-- below is the button and the anchor rect
                 +- - - -+
                 |       |
                 +-------+


Then the offset is (0, -|y|).


Jonas

> 
> 
> > +      <arg name="y" type="int" summary="surface position y offset"/>
> > +    </request>
> > +  </interface>
> > +
> >    <interface name="zxdg_surface_v6" version="1">
> >      <description summary="desktop user interface surface base interface">
> >        An interface that may be implemented by a wl_surface, for
> > @@ -167,8 +356,7 @@
> >        </description>
> >        <arg name="id" type="new_id" interface="zxdg_popup_v6"/>
> >        <arg name="parent" type="object" interface="zxdg_surface_v6"/>
> > -      <arg name="x" type="int"/>
> > -      <arg name="y" type="int"/>
> > +      <arg name="positioner" type="object"
> > interface="zxdg_positioner_v6"/>
> >      </request>
> >
> >      <request name="set_window_geometry">
> > @@ -675,6 +863,20 @@
> >        <arg name="serial" type="uint" summary="the serial of the user
> > event"/>
> >      </request>
> >
> > +    <event name="configure">
> > +      <description summary="configure the popup surface">
> > +       This event asks the popup surface to configure itself given the
> > +       configuration. It is not sent by itself but is a latched state
> > finalized
> > +       by the xdg_surface.configure event.
> > +
> > +       The x and y arguments represents the position the popup was placed
> > at
> > +       given the xdg_positioner rule, relative to the upper left corner
> > of the
> > +       window geometry of the parent surface.
> > +      </description>
> > +      <arg name="x" type="int" summary="X position relative to parent
> > surface"/>
> > +      <arg name="y" type="int" summary="Y position relative to parent
> > surface"/>
> > +    </event>
> > +
> >
>      <event name="popup_done">
> >        <description summary="popup interaction is done">
> >         The popup_done event is sent out when a popup is dismissed by the
> > --
> > 2.4.3
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >


More information about the wayland-devel mailing list