[PATCH synaptics 2/2] Add a delay between the second button down-up event of double taps

[Peter Hutterer]

On Tue, Jan 27, 2015 at 05:45:48PM +0100, Gabriele Mazzotta wrote:
> On Tuesday 27 January 2015 16:37:44 Peter Hutterer wrote:
> > sorry about the delay, busy here.
> > 
> > On Fri, Jan 16, 2015 at 02:09:38PM +0100, Gabriele Mazzotta wrote:
> > > On Friday 16 January 2015 13:24:31 Peter Hutterer wrote:
> > > > On Thu, Jan 15, 2015 at 10:04:17PM +0100, Gabriele Mazzotta wrote:
> > > > > Some applications ignore the second tap of double taps because of 
> > > the
> > > > > lack of a delay between the button down and button up events.
> > > > > 
> > > > > Prevent this by replacing the transition from TS_2B to TS_START with 
> > > a
> > > > > transition from TS_2B to TS_SINGLETAP that emits only a button down
> > > > > event. The button up event will be emitted when transitioning from
> > > > > TS_SINGLETAP to TS_START.
> > > > > 
> > > > > In addition, decrease the default value of MaxDoubleTapTime from 180 
> > > ms
> > > > > to 100 ms in order to make double taps faster.
> > > > 
> > > > Hmm, the problem here is now that MaxDoubleTapTime is now the sum of
> > > > ClickTime and MaxDoubleTapTime. That's ok if we hadn't exposed those 
> > > as
> > > > toggles but will break user's custom configurations. Even with your 
> > > change
> > > > we go from 180 to 200, anyone who has this option set goes to value +
> > > > ClickTime.
> > > > 
> > > > Easy way around this would be to add clickTime to maxDoubleTapTime's
> > > > default, check the option, then subtract it again after to leave the 
> > > double
> > > > tap timeout as the difference between the two.
> > > > 
> > > >      pars->click_time = xf86SetIntOption(opts, "ClickTime", 100);
> > > >      pars->tap_time_2 = xf86SetIntOption(opts,
> > > >                                          "MaxDoubleTapTime", 
> > > >                                          pars->clicktime + 80);
> > > >      pars->tap_time_2 -= pars->clicktime;
> > > > 
> > > > That should work, no?
> > > 
> > > I was worried about the same thing, but tests showed that the addition 
> > > ClickTime doesn't change things that much.
> > > I have also thought about doing something similar to what you suggest, 
> > > but I didn't like the fact that by doing so the meaning of property 
> > > changes in a non-obvious way.
> > >
> > > My tests showed that the second down->up transition of double taps 
> > > doesn't determine how fast clicks are. The same applies to the only 
> > > down->up transition of single taps. As long as ClickTime is within a 
> > > reasonable range, it has no tangible effect.
> > > 
> > > In case of double taps what really matters is how fast the second button 
> > > up->down transition is (the first one depends on how fast you tap), 
> > > which is what MaxDoubleTapTime determines.
> > 
> > are you sure? applications tend to react to button releases, so with your
> > patch the second release for a doubletap is now on MaxDoubleTapTime +
> > ClickTime, i.e. 100 ms later than before (by default). That would be
> > noticable. You reduced by 80ms in your patch, so a doubletap with timeout
> > would trigger the effect in the application 20ms later than before - which
> > probably feels the same.
> 
> I found that in most of the cases, applications don't wait for the second
> release. For example highlighting words in Firefox works is not affected
> by ClickTime. However in some other cases it matters though, so yes, my
> patch is not OK.
> 
> > > In case of single taps what really matters is MaxTapTime.
> > >
> > > For some reason I think the delay is more obvious for double taps and
> > > in any case lower values of MaxTapTime makes Tap-and-Drag gestures 
> > > impossible, so it shouldn't be changed. This is why I decided to lower 
> > > the default value of MaxDoubleTapTime. My error was to include the 
> > > change here as if it was required.
> > 
> > yeah, I'm pretty sure what you're seeing is the addition of the two
> > timeouts. Have you tried the approach I suggested above? does it make the
> > delay go away?
> 
> The change you suggested can cause some issues. Let's say the current
> config is such that (MaxDoubleTapTime == ClickTime + X). In this case
> tap_time_2 becomes equal to X, which can also be 0. This would make
> proper double taps impossible, even for some non zero values. In my
> opinion it's better to change ClickTime rather than MaxDoubleTapTime.
> 
> To do that, I'd add an additional state that is equal to TS_SINGLETAP
> if not for the timeout or keep track of the previous state to do
> something like the following. In this way double taps will be done
> within MaxDoubleTapTime (+ 10ms when needed).
> 
> diff --git a/src/synaptics.c b/src/synaptics.c
> index 8e27bf9..b41fdf8 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -1966,6 +1966,7 @@ SetTapState(SynapticsPrivate * priv, enum TapState tap_state, CARD32 millis)
>      default:
>          break;
>      }
> +    priv->last_tap_state = priv->tap_state;
>      priv->tap_state = tap_state;
>  }
>  
> @@ -1991,6 +1992,8 @@ GetTimeOut(SynapticsPrivate * priv)
>      case TS_5:
>          return para->tap_time;
>      case TS_SINGLETAP:
> +        if (priv->last_tap_state == TS_2B || priv->last_tap_state == TS_2A)
> +            return max(para->tap_time_2 - para->click_time, 10);
>          return para->click_time;

that can't be right, I'm looking at the state diagram here and single tap
can only be reached from 2B or 2A anyway. So this condition would always be
true (unless the diagram is off).

ok, last idea: how about a check for DoubleTapTime and ClickTime when we
parse the options and then making sure both are above the 10ms threshold you
need. so move the max into the option parsing, lock it as click time if
needed and we're good. Keeps all the current timeouts and avoids the
problem, I think :)

Cheers,
   Peter

>      case TS_2A:
>          return para->single_tap_timeout;
> diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> index 4e913f3..084deda 100644
> --- a/src/synapticsstr.h
> +++ b/src/synapticsstr.h
> @@ -292,6 +292,7 @@ struct _SynapticsPrivateRec {
>      int clickpad_click_millis;  /* Time of last clickpad click */
>  
>      enum TapState tap_state;    /* State of tap processing */
> +    enum TapState last_tap_state;    /* Previous state of tap processing */
>      int tap_max_fingers;        /* Max number of fingers seen since entering start state */
>      int tap_button;             /* Which button started the tap processing */
>      enum TapButtonState tap_button_state;       /* Current tap action */