On Sun, Mar 22, 2015 at 8:57 PM, Jonas Ådahl <jad...@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
