[PATCH 2/2] touchpad: don't apply tap config until all fingers are up
Hans de Goede
hdegoede at redhat.com
Fri May 1 01:19:59 PDT 2015
Hi,
On 01-05-15 04:48, Peter Hutterer wrote:
> 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."
I see, so if we use tp->nfingers_down then the first released touch would
move us to idle, and the next one would trigger:
static void
tp_tap_idle_handle_event(struct tp_dispatch *tp,
struct tp_touch *t,
enum tap_event event, uint64_t time)
{
struct libinput *libinput = tp->device->base.seat->libinput;
switch (event) {
...
case TAP_EVENT_RELEASE:
case TAP_EVENT_MOTION:
log_bug_libinput(libinput,
"invalid tap event, no fingers are down\n");
break;
We can easily fix that though by simply ignoring release events in idle.
This way we can remove finger_count simplifying the code, where as your
patch just adds additional code.
Regards,
Hans
>
> 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