synaptics - double taps are ignored in some applications

[Gabriele Mazzotta]

On Tuesday 13 January 2015 15:55:44 Peter Hutterer wrote:
> On Mon, Jan 12, 2015 at 09:32:13PM +0100, Gabriele Mazzotta wrote:
> > Hi,
> > 
> > I have some problems with double taps in some applications.
> > These applications, e.g. Firefox, often (if not always) ignores double
> > taps. This never happens with double clicks and it happens only with
> > the same applications.
> > 
> > After some tests I found that the problem is that there's no delay
> > between the second ButtonPress and the second ButtonRelease.
> > 
> > The patch here below seems to solve the problem (the delay should be
> > handled better), but I don't know if it can be considered a good
> > solution. Before I try to improve it, does anyone know a better way
> > to address this problem?
> > 
> > I tried to fix the problem from HandleTapProcessing(), but I could only
> > have reliable double taps breaking tap and drag gestures.
> 
> docs/tapndrag.dia has the state diagram, though I'm not 100% it's still
> accurate. Based on that I think there may be a solution: when we transition
> from 3 to 2B, we could send up + down. From 2B we have two transitions, back
> to start (Up) or back to 3 (no event needed).
> Since 2B->start is based on a timeout, that adds the required delay.
> 
> Does that sound sensible? 
> (of course, this could all be bogus if the diagram doesn't reflect the code
> anymore :)

I didn't notice the diagram, thanks. That made things more clear.

> > diff --git a/src/synaptics.c b/src/synaptics.c
> > index 3b11fda..8a3e5c5 100644
> > --- a/src/synaptics.c
> > +++ b/src/synaptics.c
> > @@ -1109,6 +1109,7 @@ SynapticsReset(SynapticsPrivate * priv)
> >      priv->mid_emu_state = MBE_OFF;
> >      priv->nextRepeat = 0;
> >      priv->lastButtons = 0;
> > +    priv->mustRelease = -1;
> 
> fwiw, I'd expect something named "mustRelease" to be a boolean yes/no, not a
> button number. something like pressedButton or activeButton or so would be
> more obvious. also, X doesn't have a button 0, so that'd be fine to use as
> default.

Yes, this was the result of incremental changes, mustRelease was a bool
and I was just too lazy to rename it. As for the button, it was 0, but
I sent the diff with -1 because I didn't bother to check whether 0 was
safe or not, so I used something I was sure of.

> >      priv->prev_z = 0;
> >      priv->prevFingers = 0;
> >      priv->num_active_touches = 0;
> > @@ -3245,6 +3246,12 @@ HandleState(InputInfoPtr pInfo, struct SynapticsHwState *hw, CARD32 now,
> >      Bool using_cumulative_coords = FALSE;
> >      Bool ignore_motion;
> >  
> > +    if (priv->mustRelease != -1) {
> > +        xf86PostButtonEvent(pInfo->dev, FALSE, priv->mustRelease, FALSE, 0, 0);
> > +        priv->mustRelease = -1;
> > +        return 0;
> > +    }
> > +
> >      /* We need both and x/y, the driver can't handle just one of the two
> >       * yet. But since it's possible to hit a phys button on non-clickpads
> >       * without ever getting motion data first, we must continue with 0/0 for
> > @@ -3360,6 +3367,7 @@ HandleState(InputInfoPtr pInfo, struct SynapticsHwState *hw, CARD32 now,
> >  
> >          if (priv->tap_button_state == TBS_BUTTON_DOWN_UP) {
> >              if (tap_mask != (priv->lastButtons & tap_mask)) {
> > +                priv->mustRelease = priv->tap_button;
> >                  xf86PostButtonEvent(pInfo->dev, FALSE, priv->tap_button, TRUE,
> >                                      0, 0);
> >                  priv->lastButtons |= tap_mask;
> > @@ -3387,8 +3395,9 @@ HandleState(InputInfoPtr pInfo, struct SynapticsHwState *hw, CARD32 now,
> >      while (change) {
> >          id = ffs(change);       /* number of first set bit 1..32 is returned */
> >          change &= ~(1 << (id - 1));
> > -        xf86PostButtonEvent(pInfo->dev, FALSE, id, (buttons & (1 << (id - 1))),
> > -                            0, 0);
> > +        if (priv->mustRelease != id)
> > +            xf86PostButtonEvent(pInfo->dev, FALSE, id,
> > +                                (buttons & (1 << (id - 1))), 0, 0);
> >      }
> >  
> >      if (priv->has_scrollbuttons)
> > @@ -3422,6 +3431,9 @@ HandleState(InputInfoPtr pInfo, struct SynapticsHwState *hw, CARD32 now,
> >      if (inside_active_area)
> >          store_history(priv, hw->x, hw->y, hw->millis);
> >  
> > +    if (priv->mustRelease)
> > +        delay = 50;
> 
> I think delay of 50 is too long, that may be noticeable (gut feeling, no
> hard data). I'd probably for for 10 or 20 at max. Whatever is the minimum to
> make the applications work.

I used actual double clicks as reference for this, but I noticed this
is not important. What's really important is the delay between the first
ButtonRelease and the second ButtonPress, as long as there's a delay
after the second ButtonPress and the second ButtonRelease.

Anyway, I think I've managed to handle this from HandleTapProcessing().
I just have to test the changes a bit more.