[PATCH libinput v2 7/9] touchpad: Fix log_bug_libinput calls on tap enable with fingers down

[Peter Hutterer]

On Sun, Sep 28, 2014 at 01:21:06PM +0200, Hans de Goede wrote:
> Before this commit the tap code deals with enabled being set to false,
> by waiting for tap.state to become IDLE, and then ignoring any events from
> that point on.
> 
> This causes a problem when enabled gets set to true again while fingers are
> down, because when in IDLE no release events are expected, so once a release
> event for one of the fingers is send, log_bug_libinput gets called.

fwiw, the reason for the approach was that one could disable tapping at any
time without any accidental consequences that can be caused by forcing a
release right then and there.

> This commit fixes this by making enabled use the same mechanism for enabled
> state transitions as the tap suspend / resume code.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/evdev-mt-touchpad-tap.c | 39 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/src/evdev-mt-touchpad-tap.c b/src/evdev-mt-touchpad-tap.c
> index 61c0139..b549067 100644
> --- a/src/evdev-mt-touchpad-tap.c
> +++ b/src/evdev-mt-touchpad-tap.c
> @@ -476,9 +476,6 @@ tp_tap_handle_event(struct tp_dispatch *tp,
>  
>  	switch(tp->tap.state) {
>  	case TAP_STATE_IDLE:
> -		if (!tp->tap.enabled)
> -			break;
> -
>  		tp_tap_idle_handle_event(tp, t, event, time);
>  		break;
>  	case TAP_STATE_TOUCH:
> @@ -540,13 +537,19 @@ tp_tap_exceeds_motion_threshold(struct tp_dispatch *tp, struct tp_touch *t)
>  	return dx * dx + dy * dy > threshold * threshold;
>  }
>  
> +static bool
> +tp_tap_enabled(struct tp_dispatch *tp)
> +{
> +	return tp->tap.enabled && !tp->tap.suspended;
> +}
> +
>  int
>  tp_tap_handle_state(struct tp_dispatch *tp, uint64_t time)
>  {
>  	struct tp_touch *t;
>  	int filter_motion = 0;
>  
> -	if (tp->tap.suspended)
> +	if (!tp_tap_enabled(tp))
>  		return 0;
>  
>  	/* Handle queued button pressed events from clickpads. For touchpads
> @@ -623,6 +626,23 @@ tp_tap_handle_timeout(uint64_t time, void *data)
>  	}
>  }
>  
> +static void
> +tp_tap_enabled_updated(struct tp_dispatch *tp, bool old_enabled, uint64_t time)

this is a bit odd, both in naming and the signature. how about passing
suspend and enable in from the callers and then letting this function figure
out the rest. That'd be a bit more obvious I think. So the callers would
be something like:

    tp->tap.enabled = false;
    tp_tap_enabled_update(tp,
                          tp->tap.suspended,
                          tp->tap.enabled,
                          time);

which is a lot more readable than the old_enabled dance.
(btw, for the future: was_enabled is easier to grasp conceptually than
old_enabled)

Cheers,
   Peter

> +{
> +	bool enabled = tp_tap_enabled(tp);
> +
> +	if (enabled == old_enabled)
> +		return;
> +
> +	if (enabled) {
> +		/* Must restart in DEAD if fingers are down atm */
> +		tp->tap.state =
> +			tp->nfingers_down ? TAP_STATE_DEAD : TAP_STATE_IDLE;
> +	} else {
> +		tp_release_all_taps(tp, time);
> +	}
> +}
> +
>  static int
>  tp_tap_config_count(struct libinput_device *device)
>  {
> @@ -641,11 +661,15 @@ tp_tap_config_set_enabled(struct libinput_device *device,
>  {
>  	struct evdev_dispatch *dispatch;
>  	struct tp_dispatch *tp = NULL;
> +	bool old_enabled;
>  
>  	dispatch = ((struct evdev_device *) device)->dispatch;
>  	tp = container_of(dispatch, tp, base);
>  
> +	old_enabled = tp_tap_enabled(tp);
>  	tp->tap.enabled = (enabled == LIBINPUT_CONFIG_TAP_ENABLED);
> +	tp_tap_enabled_updated(tp, old_enabled,
> +			       libinput_now(device->seat->libinput));
>  
>  	return LIBINPUT_CONFIG_STATUS_SUCCESS;
>  }
> @@ -718,14 +742,15 @@ tp_release_all_taps(struct tp_dispatch *tp, uint64_t now)
>  void
>  tp_tap_suspend(struct tp_dispatch *tp, uint64_t time)
>  {
> +	bool old_enabled = tp_tap_enabled(tp);
>  	tp->tap.suspended = true;
> -	tp_release_all_taps(tp, time);
> +	tp_tap_enabled_updated(tp, old_enabled, time);
>  }
>  
>  void
>  tp_tap_resume(struct tp_dispatch *tp, uint64_t time)
>  {
> +	bool old_enabled = tp_tap_enabled(tp);
>  	tp->tap.suspended = false;
> -	/* Must restart in DEAD if fingers are down atm */
> -	tp->tap.state = tp->nfingers_down ? TAP_STATE_DEAD : TAP_STATE_IDLE;
> +	tp_tap_enabled_updated(tp, old_enabled, time);
>  }
> -- 
> 2.1.0