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

[Jonas Ådahl]

On Mon, Mar 23, 2015 at 01:21:38PM +1000, Peter Hutterer wrote:
> 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.

wl_touch and wl_keyboard might not be necessary, but you need to at
least bump the wl_seat version as well. Any later additions to
wl_keyboard and wl_touch would then need to bump to wl_seat.version + 1
(i.e. increase by > 1), which is why I thought it could be reasonable to
bump all at once to avoid those future confusion, but it doesn't seem
what has been done before. Since wl_seat is already at version 4 from a
previous addition to wl_kebyoard, we actually need to bump both
wl_pointer and wl_seat to 5.

The reason for the need to also bump the wl_seat version is that clients
does not specify a version when creating the wl_pointer object, meaning
the wl_pointer/wl_touch/wl_keyboard will always be the same version as
the wl_seat they were created from.

> 
> > 
> > >      <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.

Exactly. Meaning a "latched state" is purely an addition to a
"committing" event, and not a persistent state. Might be a good idea to
describe this as a good way to extend events.

Shouldn't just as well just go with wl_pointer.axis_discrete instead of
wl_pointer.step_distance? It's less confusing since we don't need to
define how a client should deal with step_distance changing.

>  
> > 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.

The point would be to just get rid of the threshold stuff from the
clients completely. If a client would want to do the classic click
scroll events, then it'd ignore everything except a wl_pointer.axis_step
which would just be converted to whatever internal format for click
scrolls the client would have. But yea, not sure its worth the effort.


Jonas

> 
> Cheers,
>    Peter
> 
>