[PATCH 2/2] touchpad: don't apply tap config until all fingers are up
Hans de Goede
hdegoede at redhat.com
Thu Apr 30 01:01:55 PDT 2015
Hi,
On 30-04-15 08:00, Peter Hutterer wrote:
> If tapping is enabled while at least one finger is down, we underrun
> tp->tap.tap_finger_count on touch release. Avoid this by only switching
> tap config whenever there are no fingers down.
The immediate applying of tapping enabled changes is deliberate because
we also go through this code path when suspending tapping on trackpoint
activity and then we want to enable tapping directly even though a
finger may be resting on the touchpad (think top soft buttons).
We could delay the enabling and not the disabling but that seems complex.
I think that the best solution is actually to completely remove the
tap.tap_finger_count variable, it is only used in one place:
static void
tp_tap_dead_handle_event(struct tp_dispatch *tp,
struct tp_touch *t,
enum tap_event event,
uint64_t time)
{
switch (event) {
case TAP_EVENT_RELEASE:
if (tp->tap.tap_finger_count == 0)
tp->tap.state = TAP_STATE_IDLE;
break;
We can simply replace that with:
static void
tp_tap_dead_handle_event(struct tp_dispatch *tp,
struct tp_touch *t,
enum tap_event event,
uint64_t time)
{
switch (event) {
case TAP_EVENT_RELEASE:
if (tp->nfingers_down == 0)
tp->tap.state = TAP_STATE_IDLE;
break;
And completely remove tap.tap_finger_count. As a bonus this new
code is also consistent with how we set tp->tap.state on
enable/disable.
Regards,
Hans
>
> Reported-by: Rui Matos <tiagomatos at gmail.com>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> src/evdev-mt-touchpad-tap.c | 47 +++++++++++++++++++++++++++++++++++----------
> src/evdev-mt-touchpad.h | 1 +
> test/touchpad.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 85 insertions(+), 10 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad-tap.c b/src/evdev-mt-touchpad-tap.c
> index bd58bb1..0962899 100644
> --- a/src/evdev-mt-touchpad-tap.c
> +++ b/src/evdev-mt-touchpad-tap.c
> @@ -622,12 +622,33 @@ tp_tap_enabled(struct tp_dispatch *tp)
> return tp->tap.enabled && !tp->tap.suspended;
> }
>
> +static void
> +tp_tap_apply_config(struct tp_dispatch *tp)
> +{
> + struct tp_touch *t;
> + bool finger_down = false;
> +
> + if (tp->tap.enabled == tp->tap.want_enabled)
> + return;
> +
> + tp_for_each_touch(tp, t) {
> + if (t->state != TOUCH_NONE) {
> + finger_down = true;
> + break;
> + }
> + }
> +
> + if (!finger_down)
> + tp->tap.enabled = tp->tap.want_enabled;
> +}
> +
> int
> tp_tap_handle_state(struct tp_dispatch *tp, uint64_t time)
> {
> struct tp_touch *t;
> int filter_motion = 0;
>
> + tp_tap_apply_config(tp);
> if (!tp_tap_enabled(tp))
> return 0;
>
> @@ -647,6 +668,7 @@ tp_tap_handle_state(struct tp_dispatch *tp, uint64_t time)
>
> if (t->state == TOUCH_BEGIN) {
> tp->tap.tap_finger_count++;
> + assert(tp->tap.tap_finger_count > 0);
> t->tap.state = TAP_TOUCH_STATE_TOUCH;
> t->tap.initial = t->point;
> tp_tap_handle_event(tp, t, TAP_EVENT_TOUCH, time);
> @@ -659,6 +681,7 @@ tp_tap_handle_state(struct tp_dispatch *tp, uint64_t time)
> tp_tap_handle_event(tp, t, TAP_EVENT_MOTION, time);
>
> } else if (t->state == TOUCH_END) {
> + assert(tp->tap.tap_finger_count > 0);
> tp->tap.tap_finger_count--;
> tp_tap_handle_event(tp, t, TAP_EVENT_RELEASE, time);
> t->tap.state = TAP_TOUCH_STATE_IDLE;
> @@ -719,12 +742,14 @@ tp_tap_handle_timeout(uint64_t time, void *data)
> }
>
> static void
> -tp_tap_enabled_update(struct tp_dispatch *tp, bool suspended, bool enabled, uint64_t time)
> +tp_tap_enabled_update(struct tp_dispatch *tp,
> + bool suspended,
> + uint64_t time)
> {
> bool was_enabled = tp_tap_enabled(tp);
>
> + tp_tap_apply_config(tp);
> tp->tap.suspended = suspended;
> - tp->tap.enabled = enabled;
>
> if (tp_tap_enabled(tp) == was_enabled)
> return;
> @@ -758,9 +783,8 @@ tp_tap_config_set_enabled(struct libinput_device *device,
> struct tp_dispatch *tp = NULL;
>
> tp = container_of(dispatch, tp, base);
> - tp_tap_enabled_update(tp, tp->tap.suspended,
> - (enabled == LIBINPUT_CONFIG_TAP_ENABLED),
> - libinput_now(device->seat->libinput));
> + tp->tap.want_enabled = (enabled == LIBINPUT_CONFIG_TAP_ENABLED),
> + tp_tap_apply_config(tp);
>
> return LIBINPUT_CONFIG_STATUS_SUCCESS;
> }
> @@ -774,8 +798,9 @@ tp_tap_config_is_enabled(struct libinput_device *device)
> dispatch = ((struct evdev_device *) device)->dispatch;
> tp = container_of(dispatch, tp, base);
>
> - return tp->tap.enabled ? LIBINPUT_CONFIG_TAP_ENABLED :
> - LIBINPUT_CONFIG_TAP_DISABLED;
> + return tp->tap.want_enabled ?
> + LIBINPUT_CONFIG_TAP_ENABLED :
> + LIBINPUT_CONFIG_TAP_DISABLED;
> }
>
> static enum libinput_config_tap_state
> @@ -818,7 +843,9 @@ tp_init_tap(struct tp_dispatch *tp)
> tp->device->base.config.tap = &tp->tap.config;
>
> tp->tap.state = TAP_STATE_IDLE;
> - tp->tap.enabled = tp_tap_default(tp->device);
> + tp->tap.want_enabled = tp_tap_default(tp->device);
> + tp->tap.enabled = !tp->tap.want_enabled;
> + tp_tap_apply_config(tp);
>
> libinput_timer_init(&tp->tap.timer,
> tp->device->base.seat->libinput,
> @@ -849,13 +876,13 @@ tp_release_all_taps(struct tp_dispatch *tp, uint64_t now)
> void
> tp_tap_suspend(struct tp_dispatch *tp, uint64_t time)
> {
> - tp_tap_enabled_update(tp, true, tp->tap.enabled, time);
> + tp_tap_enabled_update(tp, true, time);
> }
>
> void
> tp_tap_resume(struct tp_dispatch *tp, uint64_t time)
> {
> - tp_tap_enabled_update(tp, false, tp->tap.enabled, time);
> + tp_tap_enabled_update(tp, false, time);
> }
>
> bool
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 97b17cd..7f90f2b 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -253,6 +253,7 @@ struct tp_dispatch {
> struct {
> struct libinput_device_config_tap config;
> bool enabled;
> + bool want_enabled;
> bool suspended;
> struct libinput_timer timer;
> enum tp_tap_state state;
> diff --git a/test/touchpad.c b/test/touchpad.c
> index 830e589..c25a5d0 100644
> --- a/test/touchpad.c
> +++ b/test/touchpad.c
> @@ -2944,6 +2944,51 @@ START_TEST(touchpad_tap_invalid)
> }
> END_TEST
>
> +START_TEST(touchpad_tap_enable_during_tap)
> +{
> + struct litest_device *dev = litest_current_device();
> + struct libinput *li = dev->libinput;
> +
> + libinput_device_config_tap_set_enabled(dev->libinput_device,
> + LIBINPUT_CONFIG_TAP_DISABLED);
> + litest_drain_events(li);
> +
> + litest_touch_down(dev, 0, 50, 50);
> + libinput_dispatch(li);
> + libinput_device_config_tap_set_enabled(dev->libinput_device,
> + LIBINPUT_CONFIG_TAP_ENABLED);
> + litest_touch_up(dev, 0);
> + libinput_dispatch(li);
> +
> + litest_assert_empty_queue(li);
> +}
> +END_TEST
> +
> +START_TEST(touchpad_tap_disable_during_tap)
> +{
> + struct litest_device *dev = litest_current_device();
> + struct libinput *li = dev->libinput;
> +
> + libinput_device_config_tap_set_enabled(dev->libinput_device,
> + LIBINPUT_CONFIG_TAP_ENABLED);
> + litest_drain_events(li);
> +
> + litest_touch_down(dev, 0, 50, 50);
> + libinput_dispatch(li);
> + libinput_device_config_tap_set_enabled(dev->libinput_device,
> + LIBINPUT_CONFIG_TAP_DISABLED);
> + litest_touch_up(dev, 0);
> + libinput_dispatch(li);
> +
> + litest_assert_button_event(li,
> + BTN_LEFT,
> + LIBINPUT_BUTTON_STATE_PRESSED);
> + litest_assert_button_event(li,
> + BTN_LEFT,
> + LIBINPUT_BUTTON_STATE_RELEASED);
> +}
> +END_TEST
> +
> static int
> touchpad_has_palm_detect_size(struct litest_device *dev)
> {
> @@ -4163,6 +4208,8 @@ int main(int argc, char **argv) {
> litest_add("touchpad:tap", touchpad_tap_invalid, LITEST_TOUCHPAD, LITEST_ANY);
> litest_add("touchpad:tap", touchpad_tap_is_available, LITEST_TOUCHPAD, LITEST_ANY);
> litest_add("touchpad:tap", touchpad_tap_is_not_available, LITEST_ANY, LITEST_TOUCHPAD);
> + litest_add("touchpad:tap", touchpad_tap_enable_during_tap, LITEST_TOUCHPAD, LITEST_ANY);
> + litest_add("touchpad:tap", touchpad_tap_disable_during_tap, LITEST_TOUCHPAD, LITEST_ANY);
>
> litest_add("touchpad:tap", clickpad_1fg_tap_click, LITEST_CLICKPAD, LITEST_ANY);
> litest_add("touchpad:tap", clickpad_2fg_tap_click, LITEST_CLICKPAD, LITEST_SINGLE_TOUCH|LITEST_APPLE_CLICKPAD);
>
More information about the wayland-devel
mailing list