[PATCH 2/2] touchpad: don't apply tap config until all fingers are up
Peter Hutterer
peter.hutterer at who-t.net
Thu Apr 30 19:48:17 PDT 2015
On Thu, Apr 30, 2015 at 10:01:55AM +0200, Hans de Goede wrote:
> 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).
is this much of an issue though? afaict the only drawback is that we require
the user to lift all fingers once tapping is enabled to allow tapping. I
don't think that's a problem.
> 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.
right, the problem is that nfingers_down is not useful for us here. this
variable was only introduced recently, from 591a41f:
"tp->nfingers_down gives us the current state of the touchpad but in
the case of the tapping state we need the touchpoints separately. If all
touchpoints end in the same SYN_REPORT frame, tp->nfingers_down is 0 when we
handle the touch releases."
Cheers,
Peter
> >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