[PATCH xf86-input-synaptics 8/8] Wait for *new* coordinates on a clickpad click before reporting the click

[Peter Hutterer]

On Fri, Feb 21, 2014 at 10:31:44AM +0100, Hans de Goede wrote:
> It is possible for a click to get reported before any related touch events
> get reported, here is the relevant part of an evemu-record session on a T440s:
> 
> E: 3.985585 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
> E: 3.997419 0003 0039 -001	# EV_ABS / ABS_MT_TRACKING_ID   -1
> E: 3.997419 0001 014a 0000	# EV_KEY / BTN_TOUCH            0
> E: 3.997419 0003 0018 0000	# EV_ABS / ABS_PRESSURE         0
> E: 3.997419 0001 0145 0000	# EV_KEY / BTN_TOOL_FINGER      0
> E: 3.997419 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
> E: 5.117881 0001 0110 0001	# EV_KEY / BTN_LEFT             1
> E: 5.117881 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
> E: 5.133422 0003 0039 0187	# EV_ABS / ABS_MT_TRACKING_ID   187
> E: 5.133422 0003 0035 3098	# EV_ABS / ABS_MT_POSITION_X    3098
> E: 5.133422 0003 0036 3282	# EV_ABS / ABS_MT_POSITION_Y    3282
> E: 5.133422 0003 003a 0046	# EV_ABS / ABS_MT_PRESSURE      46
> E: 5.133422 0001 014a 0001	# EV_KEY / BTN_TOUCH            1
> E: 5.133422 0003 0000 3102	# EV_ABS / ABS_X                3102
> E: 5.133422 0003 0001 3282	# EV_ABS / ABS_Y                3282
> E: 5.133422 0003 0018 0046	# EV_ABS / ABS_PRESSURE         46
> E: 5.133422 0001 0145 0001	# EV_KEY / BTN_TOOL_FINGER      1
> E: 5.133422 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
> 
> Notice the BTN_LEFT event all by itself!
> 
> If this happens, it may lead to the following problem scenario:
> -touch the touchpad in its right click area
> -let go of the touchpad
> -rapidly click in the middle area, so that BTN_LEFT gets reported before the
>  new coordinates (such as seen in the trace above, this may require some
>  practicing with evemu-record to reproduce)
> -the driver registers the click as a right click because it uses the
>  old coordinates from the cumulative coordinates to determine the
>  click location
> 
> This commit fixes this by:
> 1) Resetting the cumulative coordinates not only when no button is pressed,
>    but also when there is no finger touching the touchpad, so that when
>    we do get a touch the cumulative coordinates start at the right place
> 2) Delaying processing the BTN_LEFT down transition if there is no finger
>    touching the touchpad
> 
> This approach has one downside, if we wrongly identify a touchpad as
> a clickpad, then the left button won't work unless the user touches the
> touchpad while clicking the left button.
> 
> If we want we can fix this by doing something like this:
> 1) Making update_hw_button_state return a delay; and
> 2) Tracking that we've delayed BTN_LEFT down transition processing; and
> 3) When we've delayed BTN_LEFT down transition return a small delay value; and
> 4) If when we're called again we still don't have a finger down, just
>    treat the click as a BTN_LEFT
> 
> But this is not worth the trouble IMHO, the proper thing to do in this
> scenario is to fix the mis-identification of the touchpad as a clickpad.

indeed, this is not something we need to hack around.

this series is Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net> except
for the property patch, I'll do some more testing with my clickpad over the
week though.

Cheers,
   Peter

> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/eventcomm.c | 5 +++--
>  src/synaptics.c | 7 +++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/eventcomm.c b/src/eventcomm.c
> index fe57aa8..231afbc 100644
> --- a/src/eventcomm.c
> +++ b/src/eventcomm.c
> @@ -675,8 +675,9 @@ EventReadHwState(InputInfoPtr pInfo,
>  
>      SynapticsResetTouchHwState(hw, FALSE);
>  
> -    /* Reset cumulative values if buttons were not previously pressed */
> -    if (!hw->left && !hw->right && !hw->middle) {
> +    /* Reset cumulative values if buttons were not previously pressed,
> +     * or no finger was previously present. */
> +    if ((!hw->left && !hw->right && !hw->middle) || hw->z < para->finger_low) {
>          hw->cumulative_dx = hw->x;
>          hw->cumulative_dy = hw->y;
>          sync_cumulative = TRUE;
> diff --git a/src/synaptics.c b/src/synaptics.c
> index bd14100..59fcf48 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -2810,6 +2810,12 @@ update_hw_button_state(const InputInfoPtr pInfo, struct SynapticsHwState *hw,
>      if (para->clickpad) {
>          /* hw->left is down, but no other buttons were already down */
>          if (!(priv->lastButtons & 7) && hw->left && !hw->right && !hw->middle) {
> +            /* If the finger down event is delayed, the x and y
> +             * coordinates are stale so we delay processing the click */
> +            if (hw->z < para->finger_low) {
> +                hw->left = 0;
> +                goto out;
> +            }
>              if (is_inside_rightbutton_area(para, hw->x, hw->y)) {
>                  hw->left = 0;
>                  hw->right = 1;
> @@ -2841,6 +2847,7 @@ update_hw_button_state(const InputInfoPtr pInfo, struct SynapticsHwState *hw,
>      if (hw->left && !(priv->lastButtons & 7) && hw->numFingers >= 1)
>          handle_clickfinger(priv, hw);
>  
> +out:
>      /* Two finger emulation */
>      if (hw->numFingers == 1 && hw->z >= para->emulate_twofinger_z &&
>          hw->fingerWidth >= para->emulate_twofinger_w) {
> -- 
> 1.9.0
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>