Re: [PATCH libinput] touchpad: don't check clickfinger distance for three fingers

[Hans de Goede]

Hi,

On 24-07-15 02:35, Peter Hutterer wrote:

It's reasonable to expect a thumb (or the other hand's index finger) to click
a button while a finger is down for movement. It's less reasonable to expect
this when two fingers are interacting with the touchpad, or when two fingers
click the touchpad (even on a large touchpad that's an awkward position).

Simplify the clickfinger detection mechanism - if we have three touches down,
it's always a three-finger click. Two fingers may be a right click or a index
+ thumb click.

Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
---
  src/evdev-mt-touchpad-buttons.c | 23 ++++---------
  test/touchpad-buttons.c         | 74 +++++++++++++++++++++++++++++++++++++++++
  2 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c
index a2fb4b4..0e6426c 100644
--- a/src/evdev-mt-touchpad-buttons.c
+++ b/src/evdev-mt-touchpad-buttons.c
@@ -856,11 +856,9 @@ tp_clickfinger_set_button(struct tp_dispatch *tp)
        unsigned int nfingers = tp->nfingers_down;
        struct tp_touch *t;
        struct tp_touch *first = NULL,
-                       *second = NULL,
-                       *third = NULL;
-       uint32_t close_touches = 0;
+                       *second = NULL;

-       if (nfingers < 2 || nfingers > 3)
+       if (nfingers < 2 || nfingers >= 3)
                goto out;

        /* two or three fingers down on the touchpad. Check for distance

This comment should now be:
        /* two fingers down on the touchpad. Check for distance

And you should also simplify the test above the comment to match and make it:

        if (nfingers != 2)
                goto out;

Which is what the code in your patch also does in a somewhat complicated manner.

With this fixed: Reviewed-by: Hans de Goede <hdego...@redhat.com>

Regards,

Hans

@@ -873,10 +871,6 @@ tp_clickfinger_set_button(struct tp_dispatch *tp)
                        first = t;
                else if (!second)
                        second = t;
-               else if (!third) {
-                       third = t;
-                       break;
-               }
        }

        if (!first || !second) {
@@ -884,15 +878,10 @@ tp_clickfinger_set_button(struct tp_dispatch *tp)
                goto out;
        }

-       close_touches |= tp_check_clickfinger_distance(tp, first, second) << 0;
-       close_touches |= tp_check_clickfinger_distance(tp, second, third) << 1;
-       close_touches |= tp_check_clickfinger_distance(tp, first, third) << 2;
-
-       switch(__builtin_popcount(close_touches)) {
-       case 0: nfingers = 1; break;
-       case 1: nfingers = 2; break;
-       default: nfingers = 3; break;
-       }
+       if (tp_check_clickfinger_distance(tp, first, second))
+               nfingers = 2;
+       else
+               nfingers = 1;

  out:
        switch (nfingers) {
diff --git a/test/touchpad-buttons.c b/test/touchpad-buttons.c
index ccc3b88..5767114 100644
--- a/test/touchpad-buttons.c
+++ b/test/touchpad-buttons.c
@@ -444,6 +444,78 @@ START_TEST(touchpad_2fg_clickfinger_distance)
  }
  END_TEST

+START_TEST(touchpad_3fg_clickfinger_distance)
+{
+       struct litest_device *dev = litest_current_device();
+       struct libinput *li = dev->libinput;
+
+       if (libevdev_get_num_slots(dev->evdev) < 3)
+               return;
+
+       enable_clickfinger(dev);
+
+       litest_drain_events(li);
+
+       litest_touch_down(dev, 0, 90, 90);
+       litest_touch_down(dev, 1, 10, 15);
+       litest_touch_down(dev, 2, 10, 15);
+       litest_event(dev, EV_SYN, SYN_REPORT, 0);
+       litest_event(dev, EV_KEY, BTN_LEFT, 1);
+       litest_event(dev, EV_SYN, SYN_REPORT, 0);
+       litest_event(dev, EV_KEY, BTN_LEFT, 0);
+       litest_event(dev, EV_SYN, SYN_REPORT, 0);
+       litest_event(dev, EV_SYN, SYN_REPORT, 0);
+       litest_touch_up(dev, 0);
+       litest_touch_up(dev, 1);
+       litest_touch_up(dev, 2);
+
+       litest_assert_button_event(li,
+                                  BTN_MIDDLE,
+                                  LIBINPUT_BUTTON_STATE_PRESSED);
+       litest_assert_button_event(li,
+                                  BTN_MIDDLE,
+                                  LIBINPUT_BUTTON_STATE_RELEASED);
+}
+END_TEST
+
+START_TEST(touchpad_3fg_clickfinger_distance_btntool)
+{
+       struct litest_device *dev = litest_current_device();
+       struct libinput *li = dev->libinput;
+
+       if (libevdev_get_num_slots(dev->evdev) > 2)
+               return;
+
+       enable_clickfinger(dev);
+
+       litest_drain_events(li);
+
+       litest_touch_down(dev, 0, 90, 90);
+       litest_touch_down(dev, 1, 10, 15);
+       libinput_dispatch(li);
+       litest_event(dev, EV_KEY, BTN_TOOL_DOUBLETAP, 0);
+       litest_event(dev, EV_KEY, BTN_TOOL_TRIPLETAP, 1);
+       litest_event(dev, EV_SYN, SYN_REPORT, 0);
+       libinput_dispatch(li);
+       litest_event(dev, EV_KEY, BTN_LEFT, 1);
+       litest_event(dev, EV_SYN, SYN_REPORT, 0);
+       litest_event(dev, EV_KEY, BTN_LEFT, 0);
+       litest_event(dev, EV_SYN, SYN_REPORT, 0);
+       litest_event(dev, EV_KEY, BTN_TOOL_DOUBLETAP, 1);
+       litest_event(dev, EV_KEY, BTN_TOOL_TRIPLETAP, 0);
+       litest_event(dev, EV_SYN, SYN_REPORT, 0);
+       litest_touch_up(dev, 0);
+       litest_touch_up(dev, 1);
+
+       litest_assert_button_event(li,
+                                  BTN_MIDDLE,
+                                  LIBINPUT_BUTTON_STATE_PRESSED);
+       litest_assert_button_event(li,
+                                  BTN_MIDDLE,
+                                  LIBINPUT_BUTTON_STATE_RELEASED);
+}
+END_TEST
+
  START_TEST(touchpad_2fg_clickfinger_bottom)
  {
        struct litest_device *dev = litest_current_device();
@@ -1415,6 +1487,8 @@ litest_setup_tests(void)
        litest_add("touchpad:clickfinger", 
touchpad_4fg_clickfinger_btntool_2slots, LITEST_CLICKPAD, LITEST_ANY);
        litest_add("touchpad:clickfinger", 
touchpad_4fg_clickfinger_btntool_3slots, LITEST_CLICKPAD, LITEST_ANY);
        litest_add("touchpad:clickfinger", touchpad_2fg_clickfinger_distance, 
LITEST_CLICKPAD, LITEST_ANY);
+       litest_add("touchpad:clickfinger", touchpad_3fg_clickfinger_distance, 
LITEST_CLICKPAD, LITEST_ANY);
+       litest_add("touchpad:clickfinger", 
touchpad_3fg_clickfinger_distance_btntool, LITEST_CLICKPAD, LITEST_ANY);
        litest_add_for_device("touchpad:clickfinger", 
touchpad_2fg_clickfinger_bottom, LITEST_SYNAPTICS_TOPBUTTONPAD);
        litest_add("touchpad:clickfinger", touchpad_clickfinger_to_area_method, 
LITEST_CLICKPAD, LITEST_ANY);
        litest_add("touchpad:clickfinger",

_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel