[RFC PATCH v3 wayland] protocol: add wl_pointer.axis_source, axis_stop and axis_discrete events

[Peter Hutterer]

On Tue, Jun 02, 2015 at 04:06:23PM +0800, Jonas Ådahl wrote:
> Hi,
> 
> 
> I think this looks pretty decent now. Currently I have no more issues
> with the protocol, only minor nits and comments on wording etc. that can
> be found inline. Assuming those are addressed, this patch has my
> Reviewed-by.
> 
> On Thu, May 07, 2015 at 03:54:08PM +1000, Peter Hutterer wrote:
> > The axis_source event determines how an axis event was generated. That enables
> > clients to judge when to use kinetic scrolling.
> > 
> > The axis_stop event notifies a client about the termination of a scroll
> > sequence, likewise needed to calculate kinetic scrolling parameters.
> > 
> > The axis_discrete event provides the wheel click count. Previously the axis
> > value was some hardcoded number (10), with the discrete steps this enables a
> > client to differ between line-based scrolling on a mouse wheel and smooth
> > scrolling with a touchpad.
> > 
> > We can't extend the existing wl_pointer.axis events so we introduce a new
> > concept: latching events. These events (axis_source and axis_discrete)
> > are prefixed before a wl_pointer.axis or axis_stop event, i.e. the sequence
> > becomes:
> > 
> > wl_pointer.axis_source
> > wl_pointer.axis
> > wl_pointer.axis_source
> > wl_pointer.axis
> > wl_pointer.axis_source
> > wl_pointer.axis
> > wl_pointer.axis_source
> > wl_pointer.axis_stop
> > 
> > or:
> > 
> > wl_pointer.axis_source
> > wl_pointer.axis_discrete
> > wl_pointer.axis_axis
> > 
> > Clients need to combine the state of all events where needed.
> > ---
> > Changes to v2:
> > - more documentation, should be self-explanatory now
> > - drop time and axis values from the events where they are redundant
> > - axis_stop now carries an array so we can end more axes in one go
> > 
> > wl_pointer.axis_discrete.discrete stayed an int after some IRC chat with
> > Jonas. I've expanded the documentation a bit to hopefully make it clear this
> > event won't be generated for some devices. Let's leave converting from
> > pixel-space to discrete events up to the clients, they have more semantic
> > knowledge of what exactly is required.
> > 
> >  protocol/wayland.xml | 98 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 94 insertions(+), 4 deletions(-)
> > 
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index c5963e8..15f8d72 100644
> > --- a/protocol/wayland.xml
> > +++ b/protocol/wayland.xml
> > @@ -1337,7 +1337,7 @@
> >         wl_seat.get_keyboard or wl_seat.get_touch, the returned object is
> >         always of the same version as the wl_seat.
> >     -->
> > -  <interface name="wl_seat" version="4">
> > +  <interface name="wl_seat" version="5">
> >      <description summary="group of input devices">
> >        A seat is a group of keyboards, pointer and touch devices. This
> >        object is published as a global during start up, or when such a
> > @@ -1411,7 +1411,7 @@
> >    </interface>
> >  
> >    <!-- for the version see the comment in wl_seat -->
> > -  <interface name="wl_pointer" version="3">
> > +  <interface name="wl_pointer" version="5">
> >      <description summary="pointer input device">
> >        The wl_pointer interface represents one or more input devices,
> >        such as mice, which control the pointer location and pointer_focus
> > @@ -1572,10 +1572,100 @@
> >        <description summary="release the pointer object"/>
> >      </request>
> >  
> > +    <!-- Version 5 additions -->
> > +
> > +    <enum name="axis_source">
> > +      <description summary="axis source types">
> > +	Describes the source types for axis events. This indicates to the
> 
> nit: Seems you have a tab here while not in the other paragraphs.

I've replaced all of these with tabs now, which is the majority of the rest
of the file. a global search/replace is needed here at some point.

> 
> > +        client how an axis event was physically generated, a client may
> 
> nit: I'm not a native speaker, but shouldn't this be either a full stop
> (.) or a semi colon (;), instead of a comma (,)?
> 
> > +        adjust the user interface accordingly. For example, scroll events
> > +        from a "finger" source may be in a smooth coordinate space with
> > +        kinetic scrolling whereas a "wheel" source may be in discrete steps
> > +        of a number of lines.
> > +
> > +        The "continous" axis source is a device generating events in a
> > +        continuous coordinate space, but using something other than a
> > +        finger. One example for this source is button-based scrolling where
> > +        the vertical motion of a device is converted to scroll events while
> > +        a button is held down.
> > +      </description>
> > +      <entry name="wheel" value="0" summary="A physical wheel" />
> > +      <entry name="finger" value="1" summary="Finger on a touch surface" />
> 
> Just thought about the ambiguity of using "finger" as one usually uses a
> finger to scroll on a wheel. But whatever. Using "touch" would be rather
> ambiguous too I suppose.

fwiw, I specifically chose finger because it leaves the potential open for
thumb or other touch-like interactions (that we'll probably never see, but
it's nice to leave that avenue open).

> > +      <entry name="continuous" value="2" summary="Continuous coordinate space"/>
> > +    </enum>
> > +
> > +    <event name="axis_source" since="5">
> > +      <description summary="axis source event">
> > +        Scroll and other axis source notification.
> > +
> > +        This event does not occur on its own. It is sent before a
> > +        wl_pointer.axis or wl_pointer.axis_stop event and carries the source
> > +        information for that event. A client is expected to accumulate the
> > +        data in both events before proceeding.
> > +
> > +        The source specifies how this event was generated. If the source is
> > +        wl_pointer.axis_source.finger, a wl_pointer.axis_stop event will be
> > +        sent when the user lifts the finger off the device.
> > +
> > +        If the source is wl_pointer.axis_source.wheel or
> > +        wl_pointer.axis_source.continuous, a wl_pointer.axis_stop event may
> > +        be sent but is not guaranteed to be sent.
> > +
> > +        This event is optional. If the source is unknown for a particular
> > +        axis event sequence, no event is sent.
> > +
> > +        The order of wl_pointer.axis_discrete and wl_pointer.axis_source is
> > +        not guaranteed.
> > +      </description>
> > +      <arg name="axis_source" type="uint"/>
> > +    </event>
> > +
> > +    <event name="axis_stop" since="5">
> > +      <description summary="axis stop event">
> > +        Scroll and other axis stop notification.
> > +
> > +        For some wl_pointer.axis_source types, a wl_pointer.axis_stop event
> > +        is sent to notify a client that the axis sequence has terminated.
> > +        This enables the client to implement kinetic scrolling.
> > +        See the wl_pointer.axis_source documentation for information on when
> > +        this event may be generated.
> > +
> > +        Any wl_pointer.axis events after this event should be considered as
> > +        the start of a new axis motion.
> 
> Maybe should clarify thats its a stat of a new motion from the same
> source type as was just stopped. A stopped finger touch, does not affect
> the state of a ongoing continuous scroll.

fixed, "Any wl_pointer.axis events with the same axis_source after this
event should be considered as the start of a new axis motion."

> > +
> > +        The timestamp is to be interpreted identical to the timestamp in the
> > +        wl_pointer.axis event.
> 
> It is unclear what axis event you are referring to here, as this is not
> a latched state event. Maybe we should just "warn" that it may be identical
> to the previous axis event of the same source type?

how about: "For documentation on the timestamp value see the wl_pointer.axis
event"?

> 
> > +
> > +        The axes array lists all axes stopped with this event.
> > +      </description>
> > +      <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
> > +      <arg name="axes" type="array" summary="the axes stopped with this event"/>
> > +    </event>
> > +
> > +    <event name="axis_discrete" since="5">
> > +      <description summary="axis click event">
> > +        Scroll and other axis discrete step information.
> 
> This sentence sounds odd. Maybe "Scroll and other discrete step information"?

I'll rephrase all of them then: "Discrete step information for scroll or other axes".
and likewise for source and stop.

thanks for the review, much appreciated.

Cheers,
   Peter

> > +
> > +        This event does not occur on its own. It is sent before a
> > +        wl_pointer.axis event and carries the axis value of the
> > +        wl_pointer.axis event in discrete steps (e.g. mouse wheel clicks).
> > +
> > +        This event is optional, continuous scrolling devices
> > +        like two-finger scrolling on touchpads do not have discrete
> > +        steps and do not generate this event.
> > +
> > +        The discrete value carries the directional information. e.g. a value
> > +        of -2 is two steps towards the negative direction of this axis.
> > +
> > +        The order of wl_pointer.axis_discrete and wl_pointer.axis_source is
> > +        not guaranteed.
> > +        <arg name="discrete" type="int"/>
> > +      </description>
> > +    </event>
> >    </interface>
> >  
> >    <!-- for the version see the comment in wl_seat -->
> > -  <interface name="wl_keyboard" version="4">
> > +  <interface name="wl_keyboard" version="5">
> >      <description summary="keyboard input device">
> >        The wl_keyboard interface represents one or more keyboards
> >        associated with a seat.
> > @@ -1690,7 +1780,7 @@
> >    </interface>
> >  
> >    <!-- for the version see the comment in wl_seat -->
> > -  <interface name="wl_touch" version="3">
> > +  <interface name="wl_touch" version="5">
> >      <description summary="touchscreen input device">
> >        The wl_touch interface represents a touchscreen
> >        associated with a seat.
> > -- 
> > 2.3.5
> >