[RFC libinput 2/2] touchpad: Implement pinch gesture support (wip)
Peter Hutterer
peter.hutterer at who-t.net
Thu Mar 12 01:23:01 PDT 2015
On Wed, Mar 11, 2015 at 03:20:55PM +0100, Hans de Goede wrote:
> Implement touchpad pinch (and rotate) gesture support.
>
> WIP: TODO: fix testsuite.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
looks good, almost all comments are just related to coding style/shuffling
things around.
fwiw, it'd be good adding this to the event-gui test program as well, this
should make visual debugging a lot easier.
> ---
> src/evdev-mt-touchpad-gestures.c | 242 ++++++++++++++++++++++++++++++++++++---
> src/evdev-mt-touchpad.h | 18 +++
> test/touchpad.c | 10 +-
> 3 files changed, 250 insertions(+), 20 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c
> index 9948098..a883c89 100644
> --- a/src/evdev-mt-touchpad-gestures.c
> +++ b/src/evdev-mt-touchpad-gestures.c
> @@ -22,7 +22,6 @@
>
> #include "config.h"
>
> -#include <assert.h>
> #include <math.h>
> #include <stdbool.h>
> #include <limits.h>
> @@ -30,6 +29,7 @@
> #include "evdev-mt-touchpad.h"
>
> #define DEFAULT_GESTURE_SWITCH_TIMEOUT 100 /* ms */
> +#define DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT 1000 /* ms */
>
> static void
> tp_get_touches_delta(struct tp_dispatch *tp, double *dx, double *dy, bool average)
> @@ -75,12 +75,29 @@ tp_get_average_touches_delta(struct tp_dispatch *tp, double *dx, double *dy)
> static void
> tp_gesture_start(struct tp_dispatch *tp, uint64_t time)
> {
> + struct libinput *libinput = tp->device->base.seat->libinput;
> +
> if (tp->gesture.started)
> return;
>
> switch (tp->gesture.finger_count) {
> case 2:
> - /* NOP */
> + switch (tp->gesture.twofinger_state) {
> + case GESTURE_2FG_STATE_NEW:
> + case GESTURE_2FG_STATE_UNKNOWN:
> + log_bug_libinput(libinput,
> + "%s in unknown gesture mode\n", __func__);
> + break;
> + case GESTURE_2FG_STATE_SCROLL:
> + /* NOP */
> + break;
> + case GESTURE_2FG_STATE_PINCH:
> + gesture_notify_pinch(&tp->device->base, time,
> + LIBINPUT_EVENT_GESTURE_PINCH_START,
> + tp->gesture.finger_count,
> + 0, 0, 0, 0, 0, 0);
> + break;
> + }
> break;
> case 3:
> case 4:
> @@ -113,22 +130,195 @@ tp_gesture_post_pointer_motion(struct tp_dispatch *tp, uint64_t time)
> }
> }
>
> +static int
> +tp_gesture_get_active_touches(struct tp_dispatch *tp,
> + struct tp_touch **touches,
> + unsigned int count)
size_t maybe?
> +{
> + struct libinput *libinput = tp->device->base.seat->libinput;
> + unsigned int i, n = 0;
> + struct tp_touch *t;
> +
> + memset(touches, 0, count * sizeof(struct tp_touch *));
> +
> + for (i = 0; i < tp->real_touches; i++) {
> + t = &tp->touches[i];
> + if (tp_touch_active(tp, t)) {
> + touches[n++] = t;
> + if (n == count)
> + return 0;
> + }
> + }
> +
> + log_bug_libinput(libinput, "%s can only find %u/%u touches\n",
> + __func__, n, count);
> + return -1;
> +}
gut feeling tells me this should return count or -1 on error, which would
make this code:
if (tp_gesture_get_active_touches(tp, tp->gesture.touches, 2))
break;
easier to grasp immediately:
if (tp_gesture_get_active_touches(tp, tp->gesture.touches, 2) != 2)
break;
The first one doesnt make it obvious whether the break is on success or
failure.
> +
> +static int
> +tp_gesture_get_direction(struct tp_dispatch *tp, int touch)
> +{
> + double dx, dy, move_threshold;
> +
> + /*
> + * Semi-mt touchpads have somewhat inaccurate coordinates when
> + * 2 fingers are down, so use a slightly larger threshold.
> + */
> + if (tp->semi_mt)
> + move_threshold = TP_MM_TO_DPI_NORMALIZED(4);
> + else
> + move_threshold = TP_MM_TO_DPI_NORMALIZED(3);
> +
> + dx = tp->gesture.touches[touch]->x -
> + tp->gesture.touches[touch]->gesture.initial_x;
> + dy = tp->gesture.touches[touch]->y -
> + tp->gesture.touches[touch]->gesture.initial_y;
can we use tmp variables here?
struct tp_touch *gesture_touch = &tp->gesture.touches[touch];
etc.
> + tp_normalize_delta(tp, &dx, &dy);
> + if (hypot(dx, dy) < move_threshold)
> + return UNDEFINED_DIRECTION;
> +
> + return vector_get_direction(dx, dy);
> +}
> +
> static void
> -tp_gesture_post_twofinger_scroll(struct tp_dispatch *tp, uint64_t time)
> +tp_gesture_get_pinch_info(struct tp_dispatch *tp,
> + double *distance,
> + double *angle,
> + double *center_x,
> + double *center_y)
> {
> - double dx = 0, dy =0;
> + double dx, dy;
>
> - tp_get_average_touches_delta(tp, &dx, &dy);
> - tp_filter_motion(tp, &dx, &dy, NULL, NULL, time);
> + dx = tp->gesture.touches[0]->x - tp->gesture.touches[1]->x;
> + dy = tp->gesture.touches[0]->y - tp->gesture.touches[1]->y;
same here, "first" and "second" maybe (and in the rest of the patch, which
I'll skip for this type of comment)
> + tp_normalize_delta(tp, &dx, &dy);
>
> - if (dx == 0.0 && dy == 0.0)
> - return;
> + *distance = hypot(dx, dy);
> + if (!tp->semi_mt)
> + *angle = atan2(dy, dx) * 180.0 / M_PI;
> + else
> + *angle = 0.0;
>
> - tp_gesture_start(tp, time);
> - evdev_post_scroll(tp->device,
> - time,
> - LIBINPUT_POINTER_AXIS_SOURCE_FINGER,
> - dx, dy);
> + *center_x = (tp->gesture.touches[0]->x + tp->gesture.touches[1]->x) / 2;
> + *center_y = (tp->gesture.touches[0]->y + tp->gesture.touches[1]->y) / 2;
> +}
> +
> +static void
> +tp_gesture_post_twofinger(struct tp_dispatch *tp, uint64_t time)
> +{
> + double dx, dy, distance, angle, center_x, center_y, delta;
> + double dx_unaccel, dy_unaccel;
> + int dir0, dir1;
> +
> + switch (tp->gesture.twofinger_state) {
> + case GESTURE_2FG_STATE_NEW:
> + if (tp_gesture_get_active_touches(tp, tp->gesture.touches, 2))
> + break;
> +
> + tp->gesture.initial_time = time;
> + tp->gesture.touches[0]->gesture.initial_x =
> + tp->gesture.touches[0]->x;
> + tp->gesture.touches[0]->gesture.initial_y =
> + tp->gesture.touches[0]->y;
> + tp->gesture.touches[1]->gesture.initial_x =
> + tp->gesture.touches[1]->x;
> + tp->gesture.touches[1]->gesture.initial_y =
> + tp->gesture.touches[1]->y;
> +
> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_UNKNOWN;
> + /* fall through */
> + case GESTURE_2FG_STATE_UNKNOWN:
> + dx = tp->gesture.touches[0]->x - tp->gesture.touches[1]->x;
> + dy = tp->gesture.touches[0]->y - tp->gesture.touches[1]->y;
> + tp_normalize_delta(tp, &dx, &dy);
use a helper function to subtract two touches and return a normalized delta
(same in tp_gesture_get_pinch_info). coincidentally, the typesafe
coordinates patchset would make this possible for the other two instances as
well :)
> +
> + /* If fingers are further then 2 cm apart assume pinch */
s/then/than/
fwiw, I think we need to increase this, I just put a ruler on my fingers
in what I think is a natural scroll position and I get up to 35mm distance.
> + if (hypot(dx, dy) > TP_MM_TO_DPI_NORMALIZED(20)) {
> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_PINCH;
> + tp_gesture_get_pinch_info(tp,
> + &tp->gesture.distance,
> + &tp->gesture.angle,
> + &tp->gesture.center_x,
> + &tp->gesture.center_y);
> + break;
> + }
> +
> + /* Elif fingers have been close together for a while, scroll */
> + if (time > (tp->gesture.initial_time + DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT)) {
> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_SCROLL;
> + break;
> + }
> +
> + /* Else wait for both fingers to have moved */
> + dir0 = tp_gesture_get_direction(tp, 0);
> + dir1 = tp_gesture_get_direction(tp, 1);
> + if (dir0 == UNDEFINED_DIRECTION || dir1 == UNDEFINED_DIRECTION)
> + break;
> +
> + /* If both touches are moving in the same direction assume scroll */
> + if (((dir0 | (dir0 >> 1)) & dir1) ||
> + ((dir1 | (dir1 >> 1)) & dir0) ||
I take it this is checking for the same octant? vector_get_direction()
already returns at least two octants, so a simple & should be enough here. also, if
you're just shifting to the left you're more lenient in a clockwise direction of
octant overlap.
> + ((dir0 & 0x80) && (dir1 & 0x01)) ||
> + ((dir1 & 0x80) && (dir0 & 0x01))) {
pls use the N, NE, etc. enum values for this
> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_SCROLL;
> + } else {
> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_PINCH;
> + tp_gesture_get_pinch_info(tp,
> + &tp->gesture.distance,
> + &tp->gesture.angle,
> + &tp->gesture.center_x,
> + &tp->gesture.center_y);
> + }
> + break;
> + case GESTURE_2FG_STATE_SCROLL:
> + tp_get_average_touches_delta(tp, &dx, &dy);
> + tp_filter_motion(tp, &dx, &dy, NULL, NULL, time);
> +
> + if (dx == 0.0 && dy == 0.0)
> + break;
> +
> + tp_gesture_start(tp, time);
> + evdev_post_scroll(tp->device,
> + time,
> + LIBINPUT_POINTER_AXIS_SOURCE_FINGER,
> + dx, dy);
> + break;
> + case GESTURE_2FG_STATE_PINCH:
> + tp_gesture_get_pinch_info(tp, &distance, &angle,
> + ¢er_x, ¢er_y);
> +
> + delta = distance - tp->gesture.distance;
> + tp->gesture.distance = distance;
> + distance = delta;
> +
> + delta = angle - tp->gesture.angle;
> + if (delta > 180.0)
> + delta -= 360.0;
> + else if (delta < -180.0)
> + delta += 360.0;
> + tp->gesture.angle = angle;
> + angle = delta;
> +
> + dx = center_x - tp->gesture.center_x;
> + dy = center_y - tp->gesture.center_y;
> + tp->gesture.center_x = center_x;
> + tp->gesture.center_y = center_y;
> + tp_normalize_delta(tp, &dx, &dy);
> + tp_filter_motion(tp, &dx, &dy, &dx_unaccel, &dy_unaccel, time);
> +
> + if (dx == 0.0 && dy == 0.0 &&
> + dx_unaccel == 0.0 && dy_unaccel == 0.0 &&
> + distance == 0.0 && angle == 0.0)
> + break;
> +
> + tp_gesture_start(tp, time);
> + gesture_notify_pinch(&tp->device->base, time,
> + LIBINPUT_EVENT_GESTURE_PINCH_UPDATE,
> + tp->gesture.finger_count,
> + dx, dy, dx_unaccel, dy_unaccel,
> + distance, angle);
> + break;
> + }
can we split that massive switch statement into a bunch of helper functions
so that the switch itself fits nicely on a screen?
> }
>
> static void
> @@ -170,7 +360,7 @@ tp_gesture_post_events(struct tp_dispatch *tp, uint64_t time)
> tp_gesture_post_pointer_motion(tp, time);
> break;
> case 2:
> - tp_gesture_post_twofinger_scroll(tp, time);
> + tp_gesture_post_twofinger(tp, time);
> break;
> case 3:
> case 4:
> @@ -190,12 +380,32 @@ tp_gesture_stop_twofinger_scroll(struct tp_dispatch *tp, uint64_t time)
> void
> tp_gesture_stop(struct tp_dispatch *tp, uint64_t time)
> {
> + struct libinput *libinput = tp->device->base.seat->libinput;
> + enum tp_gesture_2fg_state twofinger_state = tp->gesture.twofinger_state;
> +
> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_NEW;
> +
> if (!tp->gesture.started)
> return;
>
> switch (tp->gesture.finger_count) {
> case 2:
> - tp_gesture_stop_twofinger_scroll(tp, time);
> + switch (twofinger_state) {
> + case GESTURE_2FG_STATE_NEW:
> + case GESTURE_2FG_STATE_UNKNOWN:
> + log_bug_libinput(libinput,
> + "%s in unknown gesture mode\n", __func__);
> + break;
> + case GESTURE_2FG_STATE_SCROLL:
> + tp_gesture_stop_twofinger_scroll(tp, time);
> + break;
> + case GESTURE_2FG_STATE_PINCH:
> + gesture_notify_pinch(&tp->device->base, time,
> + LIBINPUT_EVENT_GESTURE_PINCH_END,
> + tp->gesture.finger_count,
> + 0, 0, 0, 0, 0, 0);
> + break;
> + }
> break;
> case 3:
> case 4:
> @@ -255,6 +465,8 @@ tp_gesture_handle_state(struct tp_dispatch *tp, uint64_t time)
> int
> tp_init_gesture(struct tp_dispatch *tp)
> {
> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_NEW;
can we add a state NONE here? it's a bit confusing that you initialize with
NEW even though there are no fingers on the tp.
Cheers,
Peter
> +
> libinput_timer_init(&tp->gesture.finger_count_switch_timer,
> tp->device->base.seat->libinput,
> tp_gesture_finger_count_switch_timeout, tp);
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index aa6de69..b8e2164 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -122,6 +122,13 @@ enum tp_edge_scroll_touch_state {
> EDGE_SCROLL_TOUCH_STATE_AREA,
> };
>
> +enum tp_gesture_2fg_state {
> + GESTURE_2FG_STATE_NEW,
> + GESTURE_2FG_STATE_UNKNOWN,
> + GESTURE_2FG_STATE_SCROLL,
> + GESTURE_2FG_STATE_PINCH,
> +};
> +
> struct tp_motion {
> int32_t x;
> int32_t y;
> @@ -185,6 +192,10 @@ struct tp_touch {
> in device coordinates */
> uint32_t time; /* first timestamp if is_palm == true */
> } palm;
> +
> + struct {
> + int32_t initial_x, initial_y; /* in device coordinates */
> + } gesture;
> };
>
> struct tp_dispatch {
> @@ -222,6 +233,13 @@ struct tp_dispatch {
> unsigned int finger_count;
> unsigned int finger_count_pending;
> struct libinput_timer finger_count_switch_timer;
> + enum tp_gesture_2fg_state twofinger_state;
> + struct tp_touch *touches[2];
> + uint64_t initial_time;
> + double distance;
> + double angle;
> + double center_x;
> + double center_y;
> } gesture;
>
> struct {
> diff --git a/test/touchpad.c b/test/touchpad.c
> index ff4edb0..bb01dc2 100644
> --- a/test/touchpad.c
> +++ b/test/touchpad.c
> @@ -3423,10 +3423,10 @@ int main(int argc, char **argv) {
> litest_add("touchpad:topsoftbuttons", clickpad_topsoftbuttons_clickfinger, LITEST_TOPBUTTONPAD, LITEST_ANY);
> litest_add("touchpad:topsoftbuttons", clickpad_topsoftbuttons_clickfinger_dev_disabled, LITEST_TOPBUTTONPAD, LITEST_ANY);
>
> - litest_add("touchpad:scroll", touchpad_2fg_scroll, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> - litest_add("touchpad:scroll", touchpad_2fg_scroll_slow_distance, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> - litest_add("touchpad:scroll", touchpad_2fg_scroll_return_to_motion, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> - litest_add("touchpad:scroll", touchpad_2fg_scroll_source, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> +// litest_add("touchpad:scroll", touchpad_2fg_scroll, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> +// litest_add("touchpad:scroll", touchpad_2fg_scroll_slow_distance, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> +// litest_add("touchpad:scroll", touchpad_2fg_scroll_return_to_motion, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> +// litest_add("touchpad:scroll", touchpad_2fg_scroll_source, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> litest_add("touchpad:scroll", touchpad_scroll_natural_defaults, LITEST_TOUCHPAD, LITEST_ANY);
> litest_add("touchpad:scroll", touchpad_scroll_natural_enable_config, LITEST_TOUCHPAD, LITEST_ANY);
> litest_add("touchpad:scroll", touchpad_scroll_natural, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH);
> @@ -3464,7 +3464,7 @@ int main(int argc, char **argv) {
> litest_add_for_device("touchpad:trackpoint", touchpad_trackpoint_mb_scroll, LITEST_SYNAPTICS_TRACKPOINT_BUTTONS);
> litest_add_for_device("touchpad:trackpoint", touchpad_trackpoint_mb_click, LITEST_SYNAPTICS_TRACKPOINT_BUTTONS);
> litest_add_for_device("touchpad:trackpoint", touchpad_trackpoint_buttons_softbuttons, LITEST_SYNAPTICS_TRACKPOINT_BUTTONS);
> - litest_add_for_device("touchpad:trackpoint", touchpad_trackpoint_buttons_2fg_scroll, LITEST_SYNAPTICS_TRACKPOINT_BUTTONS);
> +// litest_add_for_device("touchpad:trackpoint", touchpad_trackpoint_buttons_2fg_scroll, LITEST_SYNAPTICS_TRACKPOINT_BUTTONS);
> litest_add_for_device("touchpad:trackpoint", touchpad_trackpoint_no_trackpoint, LITEST_SYNAPTICS_TRACKPOINT_BUTTONS);
>
> return litest_run(argc, argv);
> --
> 2.3.1
>
> _______________________________________________
> 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