[PATCH libinput] touchpad: fix clickfinger behavior with a thumb being present

[Peter Hutterer]

On Fri, May 19, 2017 at 07:32:28PM +0200, Friedrich Schöller wrote:
> With a thumb on the touchpad, a two-finger click was always treated as
> a middle-click. This patch takes the thumb into account and treats the
> click as a right-click, if the touchpad supports tracking of at least
> three fingers.

Looks good, I just have a few nitpicks, because why not ;)

also, please make sure the test-suite succeeds (I haven't checked yet) and
I'd love to have tests to specifically test this new behaviour to make sure
we don't accidentally break it in the future.

> ---
>  src/evdev-mt-touchpad-buttons.c | 90 +++++++++++++++++++++++++++++------------
>  1 file changed, 65 insertions(+), 25 deletions(-)
> 
> diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c
> index 895cf0e..30c2948 100644
> --- a/src/evdev-mt-touchpad-buttons.c
> +++ b/src/evdev-mt-touchpad-buttons.c
> @@ -978,54 +978,94 @@ out:
>  	return within_distance;
>  }
>  
> +static inline bool
> +tp_clickfinger_within_distance_3(struct tp_dispatch *tp,
> +				 struct tp_touch *t1,
> +				 struct tp_touch *t2,
> +				 struct tp_touch *t3)
> +{
> +	if (!t1 || !t2 || !t3)
> +		return false;
> +
> +	double d12 =
> +		(t1->point.x - t2->point.x) * (t1->point.x - t2->point.x) +
> +		(t1->point.y - t2->point.y) * (t1->point.y - t2->point.y);
> +	double d23 =
> +		(t2->point.x - t3->point.x) * (t2->point.x - t3->point.x) +
> +		(t2->point.y - t3->point.y) * (t2->point.y - t3->point.y);
> +	double d31 =
> +		(t3->point.x - t1->point.x) * (t3->point.x - t1->point.x) +
> +		(t3->point.y - t1->point.y) * (t3->point.y - t1->point.y);

this is a classic source of errors for copy/paste and x/y mixups, please
#define a macro that you can call three times to avoid that (or a static
inline that takes two struct points).

> +
> +	if (d12 > d23 && d12 > d31) {
> +		return tp_clickfinger_within_distance(tp, t2, t3) &&
> +		       tp_clickfinger_within_distance(tp, t3, t1);
> +	}
> +
> +	if (d23 > d31 && d23 > d12) {
> +		return tp_clickfinger_within_distance(tp, t3, t1) &&
> +		       tp_clickfinger_within_distance(tp, t1, t2);
> +	}
> +
> +	return tp_clickfinger_within_distance(tp, t1, t2) &&
> +	       tp_clickfinger_within_distance(tp, t2, t3);

this is a bit confusing. I get that you're always measuring the distance
between the two closest two, but have a look at the close_touches bits
removed in  316f30d2f28b62f90efc2bdd9e6f3d83116070e7. It always calculates
all three, but I think it's easier to read.

Cheers,
   Peter

> +}
> +
>  static uint32_t
>  tp_clickfinger_set_button(struct tp_dispatch *tp)
>  {
> -	uint32_t button;
> +	bool thumb_detected = false;
>  	unsigned int nfingers = tp->nfingers_down;
>  	struct tp_touch *t;
>  	struct tp_touch *first = NULL,
> -			*second = NULL;
> +	                *second = NULL,
> +	                *third = NULL;
>  
> -	if (nfingers != 2)
> -		goto out;
> +	if (nfingers < 2)
> +		return BTN_LEFT;
> +	if (nfingers > 3)
> +		return BTN_MIDDLE;
> +
> +	/* Thumb detection is unreliable if the number of fingers on the
> +	 * touchpad is greater than the number of touches that can be
> +	 * tracked. */
> +	if (nfingers == 3 && tp->num_slots < 3)
> +		return BTN_MIDDLE;
>  
> -	/* two fingers down on the touchpad. Check for distance
> -	 * between the fingers. */
>  	tp_for_each_touch(tp, t) {
>  		if (t->state != TOUCH_BEGIN && t->state != TOUCH_UPDATE)
>  			continue;
>  
> -		if (t->thumb.state == THUMB_STATE_YES)
> +		if (t->thumb.state == THUMB_STATE_YES) {
> +			thumb_detected = true;
>  			continue;
> +		}
>  
>  		if (!first)
>  			first = t;
>  		else if (!second)
>  			second = t;
> +		else if (!third)
> +			third = t;
>  	}
>  
> -	if (!first || !second) {
> -		nfingers = 1;
> -		goto out;
> -	}
> +	if (nfingers == 2) {
> +		if (thumb_detected)
> +			return BTN_LEFT;
>  
> -	if (tp_clickfinger_within_distance(tp, first, second))
> -		nfingers = 2;
> -	else
> -		nfingers = 1;
> +		if (tp_clickfinger_within_distance(tp, first, second))
> +			return BTN_RIGHT;
> +		else
> +			return BTN_LEFT;
> +	} else {
> +		if (thumb_detected)
> +			return BTN_RIGHT;
>  
> -out:
> -	switch (nfingers) {
> -	case 0:
> -	case 1: button = BTN_LEFT; break;
> -	case 2: button = BTN_RIGHT; break;
> -	default:
> -		button = BTN_MIDDLE; break;
> -		break;
> +		if (tp_clickfinger_within_distance_3(tp, first, second, third))
> +			return BTN_MIDDLE;
> +		else
> +			return BTN_RIGHT;
>  	}
> -
> -	return button;
>  }
>  
>  static int
> -- 
> 2.9.4
>