[Wayland-bugs] [Bug 101574] Palm detection with TrackPoint buttons

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=101574

--- Comment #1 from Peter Hutterer <peter.hutterer at who-t.net> ---
Comment on attachment 132213
  --> https://bugs.freedesktop.org/attachment.cgi?id=132213
patch to add upper of touchpad into exclusion zones

Review of attachment 132213:
-----------------------------------------------------------------

Thanks for the patch. I'm not opposed to the idea itself, but I'm a bit worried
about introducing this and ending up with a lot of palm misdetections. So we
need to have quite high confidence in the area that is ignored now, the
proposed 25% seem a bit high for this.


fwiw, for merging I'd need a patch in the form of git-format-patch. And the
documentation updates, and a few test cases to make sure the behaviour is
correct :)

::: src/evdev-mt-touchpad.c
@@ +553,5 @@
>  	if (t->state != TOUCH_BEGIN)
>  		return false;
>  
> +	if (t->pressure < 150)
> +		return false;

that seems part of a different patch. And we definitely can't hard-code the
150, every device has different thresholds. This is bug 94236 btw.

@@ +562,1 @@
>  		return false;

please make sure you keep the indentation correct, i.e. lined-up. This applies
to all hunks where appropriate

@@ +666,3 @@
>  		delta = device_delta(t->point, t->palm.first);
>  		dirs = phys_get_direction(tp_phys_delta(tp, delta));
>  		if ((dirs & DIRECTIONS) && !(dirs & ~DIRECTIONS))

this one won't work correctly. Have a look at
http://wayland.freedesktop.org/libinput/doc/latest/palm_detection.html (which
btw would need updates for this patch) and the A and B arrows in the graphic
there. we only check for left/right directions here, if you started a touch in
the top palm zone and moved vertically down, this won't detect it and the touch
remains a palm. That interaction is relatively common though so we'd have
misdetection here.

@@ +729,5 @@
>  	/* palm must start in exclusion zone, it's ok to move into
>  	   the zone without being a palm */
>  	if (t->state != TOUCH_BEGIN ||
> +		(t->point.x > tp->palm.left_edge && t->point.x < tp->palm.right_edge &&
> +		t->point.y > tp->palm.upper_edge))

We have this condition 3 times now (possibly more, outside the patch context).
I think at this point it's best to have a static inline tp_palm_in_edge()
helper that returns true/false. Makes the code more readable and less likely to
introduce typos/copy-paste errors.

@@ +2313,5 @@
>  	tp->palm.right_edge = edges.x;
> +
> +	mm.y = height * 0.25;
> +	edges = evdev_device_mm_to_units(device, &mm);
> +	tp->palm.upper_edge = edges.y;

tbh, I think this is way too big. On my touchpad here this would be 2.5cm from
the top which is well within the normal finger interaction area. That's a T440
without separate trackpoint buttons, but still. Did you measure the locations
of your palm touches to figure out where the affected zone is?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-bugs/attachments/20170626/7528b8db/attachment-0001.html>