<html> <head> <base href="https://bugs.freedesktop.org/"> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - Palm detection with TrackPoint buttons" href="https://bugs.freedesktop.org/show_bug.cgi?id=101574#c1">Comment # 1</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - Palm detection with TrackPoint buttons" href="https://bugs.freedesktop.org/show_bug.cgi?id=101574">bug 101574</a> from <a class="email" href="mailto:peter.hutterer@who-t.net" title="Peter Hutterer <peter.hutterer@who-t.net>"> Peter Hutterer</a> <pre>Comment on <a href="attachment.cgi?id=132213" name="attach_132213" title="patch to add upper of touchpad into exclusion zones">attachment 132213</a> <a href="attachment.cgi?id=132213&action=edit" title="patch to add upper of touchpad into exclusion zones">[details]</a> <a href='page.cgi?id=splinter.html&bug=101574&attachment=132213'>[review]</a> patch to add upper of touchpad into exclusion zones Review of <a href="attachment.cgi?id=132213" name="attach_132213" title="patch to add upper of touchpad into exclusion zones">attachment 132213</a> <a href="attachment.cgi?id=132213&action=edit" title="patch to add upper of touchpad into exclusion zones">[details]</a> <a href='page.cgi?id=splinter.html&bug=101574&attachment=132213'>[review]</a>: ----------------------------------------------------------------- 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 <a class="bz_bug_link bz_status_ASSIGNED " title="ASSIGNED - Palm detection: ignore moves over a pressure threshold" href="show_bug.cgi?id=94236">bug 94236</a> 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 <a href="http://wayland.freedesktop.org/libinput/doc/latest/palm_detection.html">http://wayland.freedesktop.org/libinput/doc/latest/palm_detection.html</a> (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?</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>