[PATCH libinput v3 02/17] touchpad: after a click, lock the finger to its current position

[Peter Hutterer]

On Tue, May 20, 2014 at 04:34:50PM +0200, Hans de Goede wrote:
> From: Peter Hutterer <peter.hutterer at who-t.net>
> 
> On clickpads, clicking the pad usually causes some motion events. To avoid
> erroneous movements, lock the finger into position on the click and don't
> allow for motion events until we move past a given threshold (currently 2% of
> the touchpad diagonal).
> 
> HdG:
> Instead of pinning the lowest touch assuming that that is the one holding
> the physical button, simply pin all touches, and unpin as soon as a touch
> is moved more then the threshold.

Can you merge the two commit messages together into one? if we don't have
the original patch in the tree we don't need the specific history of this
patch. if you are concerned about preserving authorship feel free to change
to yourself as author or add a Co-authored-by: tag, either you or me, I
don't mind either way. That applies to all other patches too please.


> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Reviewed-by: Jonas Ådahl <jadahl at gmail.com>
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
> Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>  src/evdev-mt-touchpad.c | 80 ++++++++++++++++++++++++-------------------------
>  src/evdev-mt-touchpad.h | 12 +++++++-
>  2 files changed, 50 insertions(+), 42 deletions(-)
> 
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index b4a89e3..381bb90 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -32,6 +32,7 @@
>  #define DEFAULT_MIN_ACCEL_FACTOR 0.16
>  #define DEFAULT_MAX_ACCEL_FACTOR 1.0
>  #define DEFAULT_HYSTERESIS_MARGIN_DENOMINATOR 700.0
> +#define DEFAULT_BUTTON_MOTION_THRESHOLD 0.02 /* in percent of size */

Didn't spot this but the comment should probably be changed to
        /* 2% of size */ 
to avoid confusion about it being 2% or 0.02%.

other than that - series looks good, rev-by where missing if you want. 
tested on my x220 and on the T440s and it works as expected.

Send me a pull request for the lot and I'll merge it, no need to re-send all
these patches.

Cheers,
   Peter


>  
>  static inline int
>  tp_hysteresis(int in, int center, int margin)
> @@ -158,6 +159,7 @@ tp_begin_touch(struct tp_dispatch *tp, struct tp_touch *t)
>  		tp_motion_history_reset(t);
>  		t->dirty = true;
>  		t->state = TOUCH_BEGIN;
> +		t->pinned.is_pinned = false;
>  		tp->nfingers_down++;
>  		assert(tp->nfingers_down >= 1);
>  		tp->queued |= TOUCHPAD_EVENT_MOTION;
> @@ -182,6 +184,7 @@ tp_end_touch(struct tp_dispatch *tp, struct tp_touch *t)
>  	t->dirty = true;
>  	t->is_pointer = false;
>  	t->state = TOUCH_END;
> +	t->pinned.is_pinned = false;
>  	assert(tp->nfingers_down >= 1);
>  	tp->nfingers_down--;
>  	tp->queued |= TOUCHPAD_EVENT_MOTION;
> @@ -346,50 +349,43 @@ tp_process_key(struct tp_dispatch *tp,
>  }
>  
>  static void
> -tp_unpin_finger(struct tp_dispatch *tp)
> +tp_unpin_finger(struct tp_dispatch *tp, struct tp_touch *t)
>  {
> -	struct tp_touch *t;
> -	tp_for_each_touch(tp, t) {
> -		if (t->is_pinned) {
> -			t->is_pinned = false;
> +	unsigned int xdist, ydist;
> +	struct tp_touch *tmp = NULL;
> +
> +	if (!t->pinned.is_pinned)
> +		return;
> +
> +	xdist = abs(t->x - t->pinned.center_x);
> +	ydist = abs(t->y - t->pinned.center_y);
>  
> -			if (t->state != TOUCH_END &&
> -			    tp->nfingers_down == 1)
> -				t->is_pointer = true;
> +	if (xdist * xdist + ydist * ydist <
> +			tp->buttons.motion_dist * tp->buttons.motion_dist)
> +		return;
> +
> +	t->pinned.is_pinned = false;
> +
> +	tp_for_each_touch(tp, tmp) {
> +		if (tmp->is_pointer)
>  			break;
> -		}
>  	}
> +
> +	if (t->state != TOUCH_END && !tmp->is_pointer)
> +		t->is_pointer = true;
>  }
>  
>  static void
> -tp_pin_finger(struct tp_dispatch *tp)
> +tp_pin_fingers(struct tp_dispatch *tp)
>  {
> -	struct tp_touch *t,
> -			*pinned = NULL;
> +	struct tp_touch *t;
>  
>  	tp_for_each_touch(tp, t) {
> -		if (t->is_pinned) {
> -			pinned = t;
> -			break;
> -		}
> -	}
> -
> -	assert(!pinned);
> -
> -	pinned = tp_current_touch(tp);
> -
> -	if (tp->nfingers_down != 1) {
> -		tp_for_each_touch(tp, t) {
> -			if (t == pinned)
> -				continue;
> -
> -			if (t->y > pinned->y)
> -				pinned = t;
> -		}
> +		t->is_pointer = false;
> +		t->pinned.is_pinned = true;
> +		t->pinned.center_x = t->x;
> +		t->pinned.center_y = t->y;
>  	}
> -
> -	pinned->is_pinned = true;
> -	pinned->is_pointer = false;
>  }
>  
>  static void
> @@ -409,16 +405,19 @@ tp_process_state(struct tp_dispatch *tp, uint32_t time)
>  
>  		tp_motion_hysteresis(tp, t);
>  		tp_motion_history_push(t);
> +
> +		tp_unpin_finger(tp, t);
>  	}
>  
> -	/* We have a physical button down event on a clickpad. For drag and
> -	   drop, this means we try to identify which finger pressed the
> -	   physical button and "pin" it, i.e. remove pointer-moving
> -	   capabilities from it.
> +	/*
> +	 * We have a physical button down event on a clickpad. To avoid
> +	 * spurious pointer moves by the clicking finger we pin all fingers.
> +	 * We unpin fingers when they move more then a certain threshold to
> +	 * to allow drag and drop.
>  	 */
>  	if ((tp->queued & TOUCHPAD_EVENT_BUTTON_PRESS) &&
>  	    !tp->buttons.has_buttons)
> -		tp_pin_finger(tp);
> +		tp_pin_fingers(tp);
>  }
>  
>  static void
> @@ -441,9 +440,6 @@ tp_post_process_state(struct tp_dispatch *tp, uint32_t time)
>  
>  	tp->buttons.old_state = tp->buttons.state;
>  
> -	if (tp->queued & TOUCHPAD_EVENT_BUTTON_RELEASE)
> -		tp_unpin_finger(tp);
> -
>  	tp->queued = TOUCHPAD_EVENT_NONE;
>  }
>  
> @@ -798,6 +794,8 @@ tp_init(struct tp_dispatch *tp,
>  	tp->hysteresis.margin_y =
>  		diagonal / DEFAULT_HYSTERESIS_MARGIN_DENOMINATOR;
>  
> +	tp->buttons.motion_dist = diagonal * DEFAULT_BUTTON_MOTION_THRESHOLD;
> +
>  	if (libevdev_has_event_code(device->evdev, EV_KEY, BTN_MIDDLE) ||
>  	    libevdev_has_event_code(device->evdev, EV_KEY, BTN_RIGHT))
>  		tp->buttons.has_buttons = true;
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 2bdb329..e5fbd7a 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -77,7 +77,6 @@ struct tp_touch {
>  	bool dirty;
>  	bool fake;				/* a fake touch */
>  	bool is_pointer;			/* the pointer-controlling touch */
> -	bool is_pinned;				/* holds the phys. button */
>  	int32_t x;
>  	int32_t y;
>  	uint32_t millis;
> @@ -92,6 +91,16 @@ struct tp_touch {
>  		int32_t center_x;
>  		int32_t center_y;
>  	} hysteresis;
> +
> +	/* A pinned touchpoint is the one that pressed the physical button
> +	 * on a clickpad. After the release, it won't move until the center
> +	 * moves more than a threshold away from the original coordinates
> +	 */
> +	struct {
> +		bool is_pinned;
> +		int32_t center_x;
> +		int32_t center_y;
> +	} pinned;
>  };
>  
>  struct tp_dispatch {
> @@ -122,6 +131,7 @@ struct tp_dispatch {
>  		bool has_buttons;		/* true for physical LMR buttons */
>  		uint32_t state;
>  		uint32_t old_state;
> +		uint32_t motion_dist;		/* for pinned touches */
>  	} buttons;				/* physical buttons */
>  
>  	struct {
> -- 
> 1.9.0
>