[RFC PATCH wayland] protocol: add wl_pointer.axis_source events
Jason Ekstrand
jason at jlekstrand.net
Mon Mar 23 00:55:45 PDT 2015
On Sun, Mar 22, 2015 at 8:57 PM, Jonas Ådahl <jadahl at gmail.com> wrote:
> 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.
Yes, we need to do that.
I have a few comments but I want to read through it again and think
about it more before making them. Please keep me in the CC.
--Jason
> 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
>>
>>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list