[RFC libinput 2/2] touchpad: Implement pinch gesture support (wip)
Hans de Goede
hdegoede at redhat.com
Mon Mar 16 06:58:02 PDT 2015
Hi,
On 12-03-15 09:23, Peter Hutterer wrote:
> 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.
Hmm, I somehow missed this mail when sending the first non RFC version
of this patch, so not all review comments are addressed there.
>> ---
>> 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?
tp->real_touches is an unsigned int and I think we should be
consistent with which type we use to count touches.
>
>> +{
>> + 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.
I actually had that (return number of active touches found) in an earlier version
but ended up changing it during some shuffling, fixed in my personal tree
to return the count again.
>> +
>> +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.
Lets first wait for your typesafety patches to land, then I'll rebase
and rework this.
>> + 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)
Same here :)
>
>> + 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 :)
I would expect such a helper to be part of your typesafety work, so
lets see that get merged first.
>
>> +
>> + /* If fingers are further then 2 cm apart assume pinch */
>
> s/then/than/
Fixed.
> 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.
I bumped it to 30 mm in the patchset which I send this morning.
>
>> + 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.
In some cases (semi-mt touchpads) I've seen one finger move e.g. N/NE and the other
W/NW so this is deliberate.
> also, if
> you're just shifting to the left you're more lenient in a clockwise direction of
> octant overlap.
In the second statement dir1 and dir0 are swapped, so the above statement from
you is false unless I'm misunderstanding things.
>
>> + ((dir0 & 0x80) && (dir1 & 0x01)) ||
>> + ((dir1 & 0x80) && (dir0 & 0x01))) {
>
> pls use the N, NE, etc. enum values for this
This is to check for rolover the direction is not really relevant which is
relevant is that bit numbers 0 and 7 are checked which us much clearer to
see with raw hex.
>
>> + 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?
Already fixed in the patchset which I send out this morning.
>
>
>> }
>>
>> 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.
Fixed.
Regards,
Hans
More information about the wayland-devel
mailing list