On Tue, Jan 13, 2015 at 03:12:24PM +0800, Jonas Ådahl wrote:
> On Tue, Jan 13, 2015 at 03:21:24PM +1000, Peter Hutterer wrote:
> > The recent normalization of wheel events means we get the angle in degrees
> > but
> > we don't know how this corresponds to clicks. The M325 has a 20 degree click
> > angle, most other mice have 15 degrees. So an angle of 60 can be 3 or 4
> > click
> > events.
> >
> > Most clients care more about the click count than the angle on a mouse
> > wheel.
> > Provide that value when needed.
> >
> > Adding click_count to the axis event leaves the possibility of defining
> > discrete units for finger/continuous scroll sources in the future. Right
> > now,
> > these will always reuturn 0.
>
> Some comments/questions inline.
>
> >
> > Signed-off-by: Peter Hutterer <[email protected]>
> > ---
> > src/evdev-mt-touchpad-edge-scroll.c | 9 ++++++---
> > src/evdev.c | 26 +++++++++++++++++---------
> > src/libinput-private.h | 1 +
> > src/libinput.c | 11 ++++++++++-
> > src/libinput.h | 19 +++++++++++++++++++
> > src/libinput.sym | 1 +
> > test/pointer.c | 7 ++++++-
> > 7 files changed, 60 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/evdev-mt-touchpad-edge-scroll.c
> > b/src/evdev-mt-touchpad-edge-scroll.c
> > index a4dc093..8605034 100644
> > --- a/src/evdev-mt-touchpad-edge-scroll.c
> > +++ b/src/evdev-mt-touchpad-edge-scroll.c
> > @@ -327,7 +327,8 @@ tp_edge_scroll_post_events(struct tp_dispatch *tp,
> > uint64_t time)
> > pointer_notify_axis(device, time,
> > t->scroll.direction,
> >
> > LIBINPUT_POINTER_AXIS_SOURCE_FINGER,
> > - 0.0);
> > + 0.0,
> > + 0);
> > t->scroll.direction = -1;
> > }
> > continue;
> > @@ -351,7 +352,8 @@ tp_edge_scroll_post_events(struct tp_dispatch *tp,
> > uint64_t time)
> >
> > pointer_notify_axis(device, time, axis,
> > LIBINPUT_POINTER_AXIS_SOURCE_FINGER,
> > - *delta);
> > + *delta,
> > + 0);
> > t->scroll.direction = axis;
> >
> > tp_edge_scroll_handle_event(tp, t, SCROLL_EVENT_POSTED);
> > @@ -371,7 +373,8 @@ tp_edge_scroll_stop_events(struct tp_dispatch *tp,
> > uint64_t time)
> > pointer_notify_axis(device, time,
> > t->scroll.direction,
> > LIBINPUT_POINTER_AXIS_SOURCE_FINGER,
> > - 0.0);
> > + 0.0,
> > + 0);
> > t->scroll.direction = -1;
> > }
> > }
> > diff --git a/src/evdev.c b/src/evdev.c
> > index d80594d..10ee367 100644
> > --- a/src/evdev.c
> > +++ b/src/evdev.c
> > @@ -541,16 +541,20 @@ evdev_notify_axis(struct evdev_device *device,
> > uint64_t time,
> > enum libinput_pointer_axis axis,
> > enum libinput_pointer_axis_source source,
> > - double value)
> > + double value,
> > + double click_count)
> > {
> > - if (device->scroll.natural_scrolling_enabled)
> > + if (device->scroll.natural_scrolling_enabled) {
> > value *= -1;
> > + click_count *= -1;
>
> Since clicks is a count, not a vector, it should have no direction
> right? Meaning natural or not, a click count should always be > 0.
right, that's what I thought at first too but for the 99% use-case (counting
wheel clicks) that'd be an extra step in every client:
click = ...get_click_count()
value = ...get_value();
if (value < 0)
click = -click;
handle_wheel_events(click);
With the click_count using the direction, the value (and the sign of the
value) can be ignored. I think this is preferable, and less prone to buggy
clients.
[...]
> > diff --git a/src/libinput.h b/src/libinput.h
> > index f605e52..4009df3 100644
> > --- a/src/libinput.h
> > +++ b/src/libinput.h
> > @@ -726,6 +726,25 @@ enum libinput_pointer_axis_source
> > libinput_event_pointer_get_axis_source(struct libinput_event_pointer
> > *event);
> >
> > /**
> > + * @ingroup pointer
> > + *
> > + * Return the number of "clicks" for a given axis event. Clicks are a
> > + * discrete scrolling unit when the value returned by
> > + * libinput_event_pointer_get_axis_value() is not suitable.
>
> I assume the choice of using double to represent a click was deliberate,
> but considering that counting usually means integer numbers, it would be
> nice with some clarification what non-integer counts means.
two reasons:
consistency - the other pointer axis bits return doubles
option of "subpixel clicks" in the future. Not sure how that'll work
exactly, but if we have a click count for an otherwise continuous source, it
could be that a distance covered is equivalent to 2.5 clicks.
I don't have a use-case for this, and haven't thought much about how this
would work out exactly though.
Cheers,
Peter
> > + *
> > + * If the source is @ref LIBINPUT_POINTER_AXIS_SOURCE_WHEEL, the clicks
> > + * correspond to the number of physical mouse clicks.
> > + *
> > + * If the source is @ref LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS or @ref
> > + * LIBINPUT_POINTER_AXIS_SOURCE_FINGER, the click count is always 0.
> > + *
> > + * @return The click count for the given event. A negative number indicates
> > + * a click count along the axis' negative direction.
> > + */
> > +double
> > +libinput_event_pointer_get_axis_click_count(struct libinput_event_pointer
> > *event);
> > +
> > +/**
> > * @ingroup event_pointer
> > *
> > * @return The generic libinput_event of this event
> > diff --git a/src/libinput.sym b/src/libinput.sym
> > index 826bfde..8fccb8c 100644
> > --- a/src/libinput.sym
> > +++ b/src/libinput.sym
> > @@ -71,6 +71,7 @@ global:
> > libinput_event_pointer_get_absolute_y;
> > libinput_event_pointer_get_absolute_y_transformed;
> > libinput_event_pointer_get_axis;
> > + libinput_event_pointer_get_axis_click_count;
> > libinput_event_pointer_get_axis_source;
> > libinput_event_pointer_get_axis_value;
> > libinput_event_pointer_get_base_event;
> > diff --git a/test/pointer.c b/test/pointer.c
> > index d12c9f6..7581945 100644
> > --- a/test/pointer.c
> > +++ b/test/pointer.c
> > @@ -353,9 +353,12 @@ test_wheel_event(struct litest_device *dev, int which,
> > int amount)
> > up by a factor 15 */
> > const int scroll_step = 15;
> > int expected = amount * scroll_step;
> > + int click_count = amount;
> >
> > - if
> > (libinput_device_config_scroll_get_natural_scroll_enabled(dev->libinput_device))
> > + if
> > (libinput_device_config_scroll_get_natural_scroll_enabled(dev->libinput_device))
> > {
> > expected *= -1;
> > + click_count *= -1;
> > + }
> >
> > /* mouse scroll wheels are 'upside down' */
> > if (which == REL_WHEEL)
> > @@ -379,6 +382,8 @@ test_wheel_event(struct litest_device *dev, int which,
> > int amount)
> > ck_assert_int_eq(libinput_event_pointer_get_axis_value(ptrev),
> > expected);
> > ck_assert_int_eq(libinput_event_pointer_get_axis_source(ptrev),
> > LIBINPUT_POINTER_AXIS_SOURCE_WHEEL);
> > + ck_assert_int_eq(libinput_event_pointer_get_axis_click_count(ptrev),
> > + click_count);
> > libinput_event_destroy(event);
> > }
> >
> > --
> > 2.1.0
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
