[PATCH (diff only) libinput] touchpad: track touch sequences that ended

[Hans de Goede]

Hi,

On 13-01-15 02:38, Peter Hutterer wrote:
> Note: this patch is just for easier review, see below for the real patch (with
> this one squashed in)
> http://lists.freedesktop.org/archives/wayland-devel/2015-January/019352.html
>
> A "touch" in the touchpad code refers to one on the touchpad. When we hover,
> the touch ends but the touch sequence may still be active. Store whether the
> HW sequence is still active in t->has_ended and then decide which touches to
> hover based on that.
>
> Not 100% happy with it, it doesn't quite feel right. But it does the job for
> now and we'll likely have to revisit all this if/once we start handling
> pressure thresholds.
> ---
>   src/evdev-mt-touchpad.c | 38 ++++++++++++++++++++++++++++++++------
>   src/evdev-mt-touchpad.h |  1 +
>   2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index a1d2274..066bbba 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -190,6 +190,7 @@ tp_new_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
>   	 * after ABS_MT_TRACKING_ID */
>   	tp_motion_history_reset(t);
>   	t->dirty = true;
> +	t->has_ended = false;
>   	t->state = TOUCH_HOVERING;
>   	t->pinned.is_pinned = false;
>   	t->millis = time;
> @@ -206,12 +207,17 @@ tp_begin_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
>   	assert(tp->nfingers_down >= 1);
>   }
>
> +/**
> + * End a touch, even if the touch sequence is still active.
> + */
>   static inline void
>   tp_end_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
>   {
>   	switch (t->state) {
>   	case TOUCH_HOVERING:
> +		t->has_ended = true;

Why ?

>   		t->state = TOUCH_NONE;
> +		/* fallthough */
>   	case TOUCH_NONE:
>   	case TOUCH_END:
>   		return;
> @@ -232,6 +238,16 @@ tp_end_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
>   	tp->queued |= TOUCHPAD_EVENT_MOTION;
>   }
>
> +/**
> + * End the touch sequence on ABS_MT_TRACKING_ID -1 or when the BTN_TOOL_* 0 is received.
> + */
> +static inline void
> +tp_end_sequence(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> +{
> +	t->has_ended = true;
> +	tp_end_touch(tp, t, time);
> +}
> +
>   static double
>   tp_estimate_delta(int x0, int x1, int x2, int x3)
>   {
> @@ -284,7 +300,7 @@ tp_process_absolute(struct tp_dispatch *tp,
>   		if (e->value != -1)
>   			tp_new_touch(tp, t, time);
>   		else
> -			tp_end_touch(tp, t, time);
> +			tp_end_sequence(tp, t, time);
>   	}
>   }
>
> @@ -330,7 +346,7 @@ tp_process_fake_touch(struct tp_dispatch *tp,
>   		if (i < nfake_touches)
>   			tp_new_touch(tp, t, time);
>   		else
> -			tp_end_touch(tp, t, time);
> +			tp_end_sequence(tp, t, time);
>   	}
>   }
>
> @@ -706,7 +722,13 @@ static void
>   tp_post_process_state(struct tp_dispatch *tp, uint64_t time)
>   {
>   	struct tp_touch *t;
> -	unsigned int i;
> +	unsigned int i, active_real_touches = 0;
> +
> +	for (i = 0; i < tp->real_touches; i++) {
> +		t = tp_get_touch(tp, i);
> +		if (!t->has_ended)
> +			active_real_touches++;
> +	}
>
>   	for (i = 0; i < tp->ntouches; i++) {
>   		t = tp_get_touch(tp, i);
> @@ -716,10 +738,13 @@ tp_post_process_state(struct tp_dispatch *tp, uint64_t time)
>
>   		if (t->state == TOUCH_END) {
>   			if (!tp_fake_finger_is_touching(tp) &&
> -			    tp_fake_finger_count(tp) >= i + 1)
> +			    active_real_touches > 0 &&
> +			    tp_fake_finger_count(tp) >= active_real_touches) {
>   				t->state = TOUCH_HOVERING;
> -			else
> +			} else {
>   				t->state = TOUCH_NONE;
> +				t->has_ended = true;
> +			}

This feels wrong, you're still switching arbitrary (based on slot number)
touches here, why not simply do:

			if (t->has_ended)
				t->state = TOUCH_NONE;
			else
				t->state = TOUCH_HOVERING;

That is much simpler, and ends up actually ending all the right touches, you
already made sure the finger count is correct in unhover() so all you need to do
here is do the usual post_process state update AFAICT.



>   		} else if (t->state == TOUCH_BEGIN) {
>   			t->state = TOUCH_UPDATE;
>   		}
> @@ -903,7 +928,7 @@ tp_clear_state(struct tp_dispatch *tp)
>   	tp_release_all_taps(tp, now);
>
>   	tp_for_each_touch(tp, t) {
> -		tp_end_touch(tp, t, now);
> +		tp_end_sequence(tp, t, now);
>   	}
>
>   	tp_handle_state(tp, now);
> @@ -1064,6 +1089,7 @@ tp_init_touch(struct tp_dispatch *tp,
>   	      struct tp_touch *t)
>   {
>   	t->tp = tp;
> +	t->has_ended = true;
>   }
>
>   static int
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 0de2e62..7fe152a 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -131,6 +131,7 @@ struct tp_motion {
>   struct tp_touch {
>   	struct tp_dispatch *tp;
>   	enum touch_state state;
> +	bool has_ended;				/* TRACKING_ID == -1 */
>   	bool dirty;
>   	bool is_pointer;			/* the pointer-controlling touch */
>   	int32_t x;
>

Regards,

Hans