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

[Peter Hutterer]

On Wed, Mar 25, 2015 at 09:14:08AM +0800, Jonas Ådahl wrote:
> On Wed, Mar 25, 2015 at 10:27:10AM +1000, Peter Hutterer wrote:
> > The axis_source event determines how an axis event was generated. This 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 v1:
> > - introduce axis_stop and axis_discrete as separate events instead of flags
> > - couple of documentation updates
> > - wl_seat/keyboard/touch/pointer bumped to version 5
> > 
> > This is for the API review only so far, I don't have the weston patches to
> > match yet.
> > 
> >  protocol/wayland.xml | 87 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 83 insertions(+), 4 deletions(-)
> > 
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index c5963e8..29e6f01 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,89 @@
> >        <description summary="release the pointer object"/>
> >      </request>
> >  
> > +    <!-- Version 4 additions: none -->
> 
> Usually don't tend to add these it seems. If we'd want to be consistent,
> we'd have to add comments like this in several other places.

ACK, removed locally.

> 
> > +
> > +    <!-- Version 5 additions -->
> > +
> > +    <enum name="axis_source">
> > +      <description summary="axis source types">
> > +	Describes the axis types of scroll events.
> > +      </description>
> > +      <entry name="unknown" value="0"/>
> > +      <entry name="wheel" value="1"/>
> > +      <entry name="finger" value="2"/>
> > +      <entry name="continuous" value="3"/>
> > +    </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 axis and timestamp values are identical to the one in the
> > +        subsequent wl_pointer.axis or wl_pointer.axis_stop event. For the
> > +        interpretation of the axis value, see the wl_pointer.axis event
> > +        documentation.
> > +
> > +        The source specifies how this event was generated. If the source is
> > +        finger, a wl_pointer.axis_stop event will be sent when the user
> > +        lifts the finger off the device.
> > +      </description>
> > +      <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
> 
> Why have a "time" parameter here, considering that this state does
> nothing on its own and which data should simply be just kept around until
> the committing event is received? Feels redundant.
> 
> > +      <arg name="axis" type="uint"/>
> 
> This one seems also redundant.

both removed

> > +      <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 type, 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.
> > +
> > +        The axis and timestamp values are to be interpreted identical to the
> > +        ones in the wl_pointer.axis event.
> > +      </description>
> > +      <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
> > +      <arg name="axis" type="uint"/>
> 
> Same as axis_source::time and axis_source.axis.

the axis_stop event stands on its own and is not followed by an axis event,
it needs those two values.

 
> > +    </event>
> > +
> > +    <event name="axis_discrete" since="5">
> > +      <description summary="axis click event">
> > +        Scroll and other axis discrete step information.
> > +
> > +        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.
> 
> Hmm. This means we wouldn't be able to convert the touch based scroll
> motions into the old scroll step clicks, since we wouldn't (reliably)
> know when a device would emit their own, since it doesn't specify
> exactly when to expect and not to expect. The client won't know whether
> there are "continuous scrolling devices" or not.
> 
> Wouldn't it be easier to just provide XI2 style smooth scroll vectors,
> i.e. steps in "discrete scale", where a mouse scroll click is -1.0 or
> 1.0 and continuous scrolling is a fraction of such. Clients implementing
> version 5 can simply use the accumulation method for emitting scroll
> clicks without having to rely on the old "divide by 10" hack.
> 
> This event is actually more like what I described in my previous mail
> that were more or less concluded to be not worth it, isn't it?

the wl_pointer.axis documentation defines the axis events to be in the same
coordinate space as motion data, there is nothing accommodating for mouse
wheel clicks. it's just magic knowledge with the value 10.
Adding this event set removes that magic knowledge, if you get an
axis_discrete event you can safely ignore the axis value and just scroll by
the discrete value. 

I don't think the two should be merged into the same coordinate space. It'd
be easier but also more error-prone. For example, on a touchpad you probably
want scrolling to move at the same speed as the cursor would otherwise so
the content follows the finger (this is a lot more obvious when natural
scrolling is enabled). But a click is a click and maps to say 3 lines, or a
page, or whatever in the context. They are physically two different
coordinate spaces, I don't think merging them will help.

So I think of this less as "add continuous sources" because that's what the
protocol already is. I think of it as adding wheel click information for
when a continuous space isn't right. Does that make sense or did I miss the
point?

> 
> > +
> > +        The axis and timestamp values are to be interpreted identical to the
> > +        ones in the wl_pointer.axis event.
> > +
> > +        The discrete value carries the directional information. e.g. a value
> > +        of -2 is two steps towards the negative direction of this axis.
> > +        <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
> > +        <arg name="axis" type="uint"/>
> 
> Same as axis_source::time and axis_source.axis.

removed, thanks.

Cheers,
   Peter
 
> 
> > +        <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 +1769,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.3
> >