[RFC PATCH wayland] protocol: add wl_pointer.axis_source events

[Peter Hutterer]

On Mon, Mar 23, 2015 at 10:23:18AM +0800, Jonas Ådahl wrote:
> On Mon, Mar 09, 2015 at 01:28:04PM +1000, Peter Hutterer wrote:
> > The axis source determines how an event was generated. That enables clients to
> > judge when to use kinetic scrolling.
> 
> Nice to see this happening!
> 
> I have not looked at the implementation so far, only the protocol. I have
> some comments below.
> 
> > 
> > We can't extend the existing wl_pointer.axis events so instead this new event
> > is prefixed before each wl_pointer.axis 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
> > 
> > Clients need to combine the state of the two events where needed.
> > 
> > Supported are a flag to notify the client when a sequence stops, which is the
> > only time when no axis events follows the axis_source event.
> > [Note: nothing in the protocol prevents us from sending 0/0 axis events, so
> >  we may not need this special case]
> > 
> > The step_distance indicates how the value in the following event should be
> > interpreted (i.e. divided) to get a discrete count of scroll events (i.e.
> > wheel clicks.
> > [Note: arguably it is more flexible to include the discrete count here rather
> >  than the step_distance. This would split the information across two events
> >  though]
> > ---
> >  protocol/wayland.xml | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 47 insertions(+), 1 deletion(-)
> > 
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index 88bbbc0..880f90a 100644
> > --- a/protocol/wayland.xml
> > +++ b/protocol/wayland.xml
> > @@ -1402,7 +1402,7 @@
> >  
> >    </interface>
> >  
> > -  <interface name="wl_pointer" version="3">
> > +  <interface name="wl_pointer" version="4">
> 
> Hmm. I think we should also increase the verison of wl_seat, wl_touch and
> wl_keyboard as part of this.

Is there a requirement to do this whenever one of them changes? I honestly
don't know, but I'd be surprised if we can't move them independently.

> 
> >      <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
> > @@ -1563,6 +1563,52 @@
> >        <description summary="release the pointer object"/>
> >      </request>
> >  
> > +    <!-- Version 4 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>
> > +
> > +    <enum name="axis_source_flags">
> > +      <description summary="axis flags">
> > +      </description>
> > +      <entry name="none" value="0x0" summary="emty flags"/>
> > +      <entry name="stop_scroll" value="0x1"
> > +        summary="indicates this is the final scroll event on this sequence"/>
> > +    </enum>
> > +
> > +    <event name="axis_source" since="4">
> > +      <description summary="axis source event">
> > +        Scroll and other axis source notification.
> > +
> > +        This event is sent before a wl_pointer.axis event and carries the
> > +        source information for the following axis event. Unless the
> > +        stop_scroll axis flag is set, this event is always followed by a
> > +        wl_pointer.axis event.
> > +
> > +        The axis and timestamp values are identical to the one in the
> > +        wl_pointer.axis event. For the interpretation of the axis value, see
> > +        the wl_pointer.axis event documentation.
> > +
> > +        The source specifies how this event was generated.
> > +
> > +        The step_distance denotes how to interpret the wl_pointer.axis value
> > +        in terms of discrete steps (i.e. mouse wheel clicks). If zero, the
> > +        value does not represent discrete steps.
> > +      </description>
> > +      <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
> > +      <arg name="axis" type="uint"/>
> > +      <arg name="axis_source" type="uint"/>
> > +      <arg name="axis_source_flags" type="uint"/>
> > +      <arg name="step_distance" type="uint"/>
> 
> Not a big fan of putting so many different things in the same event.
> 
> Regarding the flags, why can't such state also be made a latched state?
> If we want to add source-specific information to an axis event (like the
> stop_scroll flag), that might just as well require some additional data,
> and then a flag would not be flexible enough. For the stop_scroll flag,
> if just a 0 length axis event is not enough for that use case, we could
> add 'stop_scroll' event that is a dispatched event on a 0 length axis
> event (as you said, a 0 length axis event is not invalid).
> 
> Regarding the step_distance, I don't think it`s a bad thing to add
> information like that as a separate event working as a latched state
> adding information to wl_pointer.axis in the same way as the proposed
> 'axis_source' event already do; it is the way to extend events. I do
> think it'd be better to avoid the sometimes-latched-sometimes-not
> situation and make new events that extend another one always do that and
> nothing else.

ok, just to avoid any confusion: with 'latched' you mean add events that
release once the wl_pointer.axis event arrives. So instead of the above, the
event sequence for a stop event would be:
    wl_pointer.axis_source
    wl_pointer.scroll_stop
    wl_pointer.step_distance
    wl.pointer.axis

rather than the flags and whatnot. but you're still proposing to send these
for every event, rather than only when the value changes, right? So a
full touchpad scroll sequence would be:
    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.scroll_stop
    wl.pointer.axis

and _not_:
    wl_pointer.axis_source
    wl.pointer.axis
    wl.pointer.axis
    wl.pointer.axis
    wl.pointer.axis
    wl_pointer.scroll_stop
    wl.pointer.axis

correct? If so, I like it, it certainly feels cleaner than the various
flags.
 
> I'm thinking, should there be yet another event for doing actual step
> triggering? I assume clients need to more or less reset their step
> distance counter per source type and then trigger the step "clicks" when
> the distance reaches the threshold. While there will most likely ever
> only be one device per source type generating events, the protocol still
> makes it possible to not generate the "correct" number of discrete
> "steps" since the sources still clump together separate devices which
> should be processed indivdually. My gut feeling its a bit unnecessary,
> but just wanted to put the question out there.

hmm, scrolling on two devices simultaneously seems like a very niche use-case.
The event sounds like it should work but I wonder if it's worth the effort?
or even if it is whether we should wait for a couple of real-world use-cases
for this first before we end up with a solution that doesn't fix them.

For the case of scrolling with a mouse and then a touchpad, the axis sources
should cover that - unless it's a finger source, you can't rely on another
scroll event to come anyway, so you need to always process the one you
currently have. Which makes handling of less than a threshold iffy on those
devices anyway.

Cheers,
   Peter