[PATCH libinput 5/3] touchpad: accumulate the initial scroll edge delta

[Hans de Goede]

Hi,

On 06-03-15 06:44, Peter Hutterer wrote:
> The previous setting of 10 wasn't 10 mm, it was used against the deltas
> normalized to a 1000DPI mouse, i.e. closer to 4mm. It was also also per-event,
> so a slow movement or a high-frequency touchpad can struggle to meet the
> threshold.
>
> Change the trigger to be ~5 mm from the initial touch down, accumulated until
> we either meet the threshold or the timeout expires.

You're forgetting the MAGIC factor here (see that is why we need it inside the macro).

So you're actually setting it to 12.5 mm, I suggest we stay with 10 mm, using the
new macro and adding the code to accumulate the delta is a good idea.

> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>   src/evdev-mt-touchpad-edge-scroll.c | 36 +++++++++++++++++++++++++++++++++---
>   src/evdev-mt-touchpad.h             |  2 ++
>   test/touchpad.c                     |  6 +++---
>   3 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad-edge-scroll.c b/src/evdev-mt-touchpad-edge-scroll.c
> index 28f29c2..8e1997e 100644
> --- a/src/evdev-mt-touchpad-edge-scroll.c
> +++ b/src/evdev-mt-touchpad-edge-scroll.c
> @@ -34,8 +34,7 @@
>      avoid accidentally locking in scrolling mode when trying to use the entire
>      touchpad to move the pointer. The user can wait for the timeout to trigger
>      to do a small scroll. */
> -/* In mm for touchpads with valid resolution, see tp_init_accel() */
> -#define DEFAULT_SCROLL_THRESHOLD 10.0
> +#define DEFAULT_SCROLL_THRESHOLD MM_TO_DPI_NORMALIZED(5)

TP_MM_TO_DPI_NORMALIZED(10)

>
>   enum scroll_event {
>   	SCROLL_EVENT_TOUCH,
> @@ -78,6 +77,8 @@ tp_edge_scroll_set_state(struct tp_dispatch *tp,
>   		break;
>   	case EDGE_SCROLL_TOUCH_STATE_EDGE_NEW:
>   		t->scroll.edge = tp_touch_get_edge(tp, t);
> +		t->scroll.initial_x = t->x;
> +		t->scroll.initial_y = t->y;
>   		libinput_timer_set(&t->scroll.timer,
>   				   t->millis + DEFAULT_SCROLL_LOCK_TIMEOUT);
>   		break;
> @@ -315,6 +316,7 @@ tp_edge_scroll_post_events(struct tp_dispatch *tp, uint64_t time)
>   	struct tp_touch *t;
>   	enum libinput_pointer_axis axis;
>   	double dx, dy, *delta;
> +	double initial_dx, initial_dy, *initial_delta;
>
>   	if (tp->scroll.method != LIBINPUT_CONFIG_SCROLL_EDGE)
>   		return 0;
> @@ -338,10 +340,12 @@ tp_edge_scroll_post_events(struct tp_dispatch *tp, uint64_t time)
>   			case EDGE_RIGHT:
>   				axis = LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL;
>   				delta = &dy;
> +				initial_delta = &initial_dy;
>   				break;
>   			case EDGE_BOTTOM:
>   				axis = LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL;
>   				delta = &dx;
> +				initial_delta = &initial_dx;
>   				break;
>   			default: /* EDGE_RIGHT | EDGE_BOTTOM */
>   				continue; /* Don't know direction yet, skip */
> @@ -350,7 +354,33 @@ tp_edge_scroll_post_events(struct tp_dispatch *tp, uint64_t time)
>   		tp_get_delta(t, &dx, &dy);
>   		tp_filter_motion(tp, &dx, &dy, NULL, NULL, time);
>
> -		if (fabs(*delta) < t->scroll.threshold)
> +		switch (t->scroll.edge_state) {
> +		case EDGE_SCROLL_TOUCH_STATE_NONE:
> +		case EDGE_SCROLL_TOUCH_STATE_AREA:
> +			log_bug_libinput(device->seat->libinput,
> +					 "unexpected scroll state %d\n",
> +					 t->scroll.edge_state);
> +			break;
> +		case EDGE_SCROLL_TOUCH_STATE_EDGE_NEW:
> +			initial_dx = t->x - t->scroll.initial_x;
> +			initial_dy = t->y - t->scroll.initial_y;
> +			tp_normalize_delta(tp,
> +					   &initial_dx,
> +					   &initial_dy);
> +			if (fabs(*initial_delta) < t->scroll.threshold) {
> +				*delta = 0.0;

It would be more consistent to write:
				dx = 0.0;
				dy = 0.0;
here, but either way works for me, your choice.
> +			} else {
> +				dx = initial_dx;
> +				dy = initial_dy;
> +			}
> +			break;
> +		case EDGE_SCROLL_TOUCH_STATE_EDGE:
> +			if (fabs(*delta) < t->scroll.threshold)
> +				*delta = 0.0;
> +			break;
> +		}
> +
> +		if (*delta == 0.0)
>   			continue;
>
>   		pointer_notify_axis(device, time,
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 8ea7d84..500eb59 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -171,6 +171,8 @@ struct tp_touch {
>   		int direction;
>   		double threshold;
>   		struct libinput_timer timer;
> +		int32_t initial_x;		/* in device coordinates */
> +		int32_t initial_y;		/* in device coordinates */
>   	} scroll;
>
>   	struct {
> diff --git a/test/touchpad.c b/test/touchpad.c
> index 5c8f579..d589d64 100644
> --- a/test/touchpad.c
> +++ b/test/touchpad.c
> @@ -2105,10 +2105,10 @@ START_TEST(touchpad_edge_scroll_no_motion)
>
>   	litest_drain_events(li);
>
> -	litest_touch_down(dev, 0, 99, 20);
> -	litest_touch_move_to(dev, 0, 99, 20, 99, 60, 10, 0);
> +	litest_touch_down(dev, 0, 99, 10);
> +	litest_touch_move_to(dev, 0, 99, 10, 99, 70, 10, 0);
>   	/* moving outside -> no motion event */
> -	litest_touch_move_to(dev, 0, 99, 60, 20, 80, 10, 0);
> +	litest_touch_move_to(dev, 0, 99, 70, 20, 80, 10, 0);
>   	/* moving down outside edge once scrolling had started -> scroll */
>   	litest_touch_move_to(dev, 0, 20, 80, 40, 99, 10, 0);
>   	litest_touch_up(dev, 0);


Other then keeping the threshold at 10 mm (with a version of the macro which applies
the magic) this looks good, so with that changed this is:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans