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

[Friedrich Schöller]

Hi Peter,

Thanks for the comments.

On 18.05.2017 03:17, Peter Hutterer wrote:
> Hi Friedrich,
> 
> On Tue, May 16, 2017 at 10:44:22PM +0200, Friedrich Schöller wrote:
>> With a thumb on the touchpad, a two-finger click was incorrectly
>> treated as a middle-click. This patch takes the thumb into account and
>> treats the click as a right-click.
> 
> fwiw, this was intentional behaviour, introduced in 316f30d2f28b62 with the
> comment:
>    It's reasonable to expect a thumb (or the other hand's index finger) to click
>    a button while a finger is down for movement. It's less reasonable to expect
>    this when two fingers are interacting with the touchpad, or when two fingers
>    click the touchpad (even on a large touchpad that's an awkward position).

This is interesting to hear. I personally never found this to be an awkward 
position.

> The problem is that while a index down + thumb is always [1] a one-finger
> click, it's not as well defined for two fingers down. But that doesn't
> matter, I'm happy to change that behaviour if we can detect the position
> correctly. The patch works, but it'll need a change please:
> on a touchpad with num_slots 2 the third finger position is based on the
> finger position of the first two fingers (see tp_position_fake_touches() or
> so).
> 
> with the patch applied, *when* the thumb is put down now matters:
> if the thumb is the first or second finger, a two-finger + thumb
> click is BTN_RIGHT, if the thumb is the third finger, a two-finger + thumb
> click is BTN_MIDDLE. That's inconsistent, so we need an extra for num_slots
> so this behaviour only gets enabled for devices that can reliably track 3 fingers.

Ah yes, I did not notice before, but you are right. Now I did a bit more testing 
and found that in order to get consistent behavior, we needed to check if all 
three fingers are within a certain distance. It seems to work quite well now. 
Let me know what you think. The patch follows.

Best,

Friedrich

> 
> Thanks
> 
> Cheers,
>     Peter
> 
> [1] fsvo 'always'
> 
>> ---
>>   src/evdev-mt-touchpad-buttons.c | 44 ++++++++++++++++++-----------------------
>>   1 file changed, 19 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c
>> index 895cf0e..ffb6a58 100644
>> --- a/src/evdev-mt-touchpad-buttons.c
>> +++ b/src/evdev-mt-touchpad-buttons.c
>> @@ -981,23 +981,24 @@ out:
>>   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;
>>   
>> -	if (nfingers != 2)
>> -		goto out;
>> +	if (nfingers < 2)
>> +		return BTN_LEFT;
>>   
>> -	/* 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;
>> +			nfingers--;
>>   			continue;
>> +		}
>>   
>>   		if (!first)
>>   			first = t;
>> @@ -1005,27 +1006,20 @@ tp_clickfinger_set_button(struct tp_dispatch *tp)
>>   			second = t;
>>   	}
>>   
>> -	if (!first || !second) {
>> -		nfingers = 1;
>> -		goto out;
>> -	}
>> +	if (nfingers < 2)
>> +		return BTN_LEFT;
>> +	else if (nfingers > 2)
>> +		return BTN_MIDDLE;
>>   
>> -	if (tp_clickfinger_within_distance(tp, first, second))
>> -		nfingers = 2;
>> +	/* Two fingers (not counting thumbs) are on the touchpad. If no
>> +	 * additional thumb was detected, we check the distance between the
>> +	 * touches. Some touchpads can not report more than two finger positions
>> +	 * so we do not check the distance if an additional thumb was detected.
>> +	 */
>> +	if (thumb_detected || tp_clickfinger_within_distance(tp, first, second))
>> +		return BTN_RIGHT;
>>   	else
>> -		nfingers = 1;
>> -
>> -out:
>> -	switch (nfingers) {
>> -	case 0:
>> -	case 1: button = BTN_LEFT; break;
>> -	case 2: button = BTN_RIGHT; break;
>> -	default:
>> -		button = BTN_MIDDLE; break;
>> -		break;
>> -	}
>> -
>> -	return button;
>> +		return BTN_LEFT;
>>   }
>>   
>>   static int
>> -- 
>> 2.9.4
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>