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

[Peter Hutterer]

On Fri, Mar 06, 2015 at 11:49:37AM +0100, Hans de Goede wrote:
> 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.

Tested this, and 10mm is IMO way too high. I increased the timeout to test
how much 10mm actually are and it's a lot, well past the point where I'd
expect it to kick in. 5mm feels about right (and is close to what it was
before), so I left it in. 10mm is almost impossible to hit within the
timeout.
 
> >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)

fixed, thanks.

> >
> >  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.

fixed

Cheers,
   Peter

> >+			} 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