Patch: some refactoring in evdev-mt-touchpad-tap.c
Peter Hutterer
peter.hutterer at who-t.net
Mon Apr 27 15:14:08 PDT 2015
On Mon, Apr 27, 2015 at 12:36:20PM +0200, Lucas Baudin wrote:
> Hi,
>
> Well, I was not aware of Hans de Goede's patchset, so I am not sure this is
> still relevant, see in text replies.
ah, fair enough, that makes sense then.
> On 27/04/2015 03:35, Peter Hutterer wrote:
> >Hi Lucas,
> >
> >On Fri, Apr 24, 2015 at 11:50:58PM +0200, Lucas Baudin wrote:
> >>Here is a patch that factorizes some code from edev-mt-touchpad-tap.c, that
> >>uses more the structures that are already updated by libinput.
> >>
> >>This patch is a first step towards multi-touch touchpad event, such as three
> >>finger scrolling or 4-5 fingers click.
> >can you expand on why you'd want all this?
> >we already have two-finger scrolling and that's unlikely to ever change,
> >it's too ingrained in virtually everything today. So adding 3 finger
> >scrolling doesn't seem to add much.
> 3 finger scrolling would be used as a gesture (to trigger a WM expose event
> or whatever), but I suppose it should rather be a gesture in Hans's patchset
> than in a different file.
right, this will be a 3fg swipe gesture then, the exact behaviour is then
up to the caller.
> >We don't have 4/5 finger tap because not only are they hard to trigger on
> >many touchpads, it's also not clear what should happen when tapped. which
> >button event should be generated, what will the caller do with this event,
> >etc.
> >
> >In regards to the patch there are two comments: one is that the state
> >machine diagram should be updated whenever you change the state machine.
> >the second is: the state diagram is quite complex but in exchange the code
> >itself contains virtually no logic other than posting the events and
> >setting/clearing timers. That's intentional, it makes debugging a lot
> >easier since there is no side effects. I'd prefer to keep it that way for 4
> >and 5-finger taps.
> In fact, there is no new side effects, since the fingers_count is already
> used in some other part of the code, so it has to be accurate anyway.
yeah, but the tap handling kicks in at some specific time of the event
processing. and the actual event processing doesn't rely on or modify other
state, so I'd like to keep it simple.
> Regarding the state diagram, I am not sure, how is it built ? by hand in
> Inkscape or something like that ?
it was created with http://draw.io, the link is in the top of the file.
that's currently set to view only though, if you think we still need changes
with the gesture stuff I can give you write access.
Cheers,
Peter
> >> From 91b5c43fc43a6f12b86c00f55f57267a4e27c866 Mon Sep 17 00:00:00 2001
> >>From: Lucas Baudin <lucas.baudin at ens.fr>
> >>Date: Fri, 24 Apr 2015 23:37:19 +0200
> >>Subject: [PATCH] Factorize the tap_touch*_handle_event
> >>
> >>---
> >> src/evdev-mt-touchpad-tap.c | 147 +++++---------------------------------------
> >> src/evdev-mt-touchpad.h | 4 --
> >> 2 files changed, 16 insertions(+), 135 deletions(-)
> >>
> >>diff --git a/src/evdev-mt-touchpad-tap.c b/src/evdev-mt-touchpad-tap.c
> >>index 0f25e26..5ae8aab 100644
> >>--- a/src/evdev-mt-touchpad-tap.c
> >>+++ b/src/evdev-mt-touchpad-tap.c
> >>@@ -66,10 +66,6 @@ tap_state_to_str(enum tp_tap_state state) {
> >> CASE_RETURN_STRING(TAP_STATE_HOLD);
> >> CASE_RETURN_STRING(TAP_STATE_TOUCH);
> >> CASE_RETURN_STRING(TAP_STATE_TAPPED);
> >>- CASE_RETURN_STRING(TAP_STATE_TOUCH_2);
> >>- CASE_RETURN_STRING(TAP_STATE_TOUCH_2_HOLD);
> >>- CASE_RETURN_STRING(TAP_STATE_TOUCH_3);
> >>- CASE_RETURN_STRING(TAP_STATE_TOUCH_3_HOLD);
> >> CASE_RETURN_STRING(TAP_STATE_DRAGGING);
> >> CASE_RETURN_STRING(TAP_STATE_DRAGGING_WAIT);
> >> CASE_RETURN_STRING(TAP_STATE_DRAGGING_OR_DOUBLETAP);
> >>@@ -167,13 +163,22 @@ tp_tap_touch_handle_event(struct tp_dispatch *tp,
> >> switch (event) {
> >> case TAP_EVENT_TOUCH:
> >>- tp->tap.state = TAP_STATE_TOUCH_2;
> >> tp_tap_set_timer(tp, time);
> >> break;
> >> case TAP_EVENT_RELEASE:
> >>- tp->tap.state = TAP_STATE_TAPPED;
> >>- tp_tap_notify(tp, time, 1, LIBINPUT_BUTTON_STATE_PRESSED);
> >>- tp_tap_set_timer(tp, time);
> >>+ if (tp->tap.tap_finger_count == 0) {
> >>+ tp->tap.state = TAP_STATE_TAPPED;
> >>+ tp_tap_notify(tp, time, 1, LIBINPUT_BUTTON_STATE_PRESSED);
> >>+ tp_tap_set_timer(tp, time);
> >>+ }
> >>+ else if (tp->tap.tap_finger_count > 0) {
> >>+ tp->tap.state = TAP_STATE_HOLD;
> >>+ if (t->tap.state == TAP_TOUCH_STATE_TOUCH) {
> >>+ tp_tap_notify(tp, time, tp->tap.tap_finger_count + 1, LIBINPUT_BUTTON_STATE_PRESSED);
> >>+ tp_tap_notify(tp, time, tp->tap.tap_finger_count + 1, LIBINPUT_BUTTON_STATE_RELEASED);
> >>+ }
> >>+ tp_tap_clear_timer(tp);
> >>+ }
> >> break;
> >> case TAP_EVENT_TIMEOUT:
> >> case TAP_EVENT_MOTION:
> >>@@ -194,11 +199,12 @@ tp_tap_hold_handle_event(struct tp_dispatch *tp,
> >> switch (event) {
> >> case TAP_EVENT_TOUCH:
> >>- tp->tap.state = TAP_STATE_TOUCH_2;
> >> tp_tap_set_timer(tp, time);
> >> break;
> >> case TAP_EVENT_RELEASE:
> >>- tp->tap.state = TAP_STATE_IDLE;
> >>+ if (tp->tap.tap_finger_count <= 0) {
> >>+ tp->tap.state = TAP_STATE_IDLE;
> >>+ }
> >> break;
> >> case TAP_EVENT_MOTION:
> >> case TAP_EVENT_TIMEOUT:
> >>@@ -237,113 +243,6 @@ tp_tap_tapped_handle_event(struct tp_dispatch *tp,
> >> }
> >> static void
> >>-tp_tap_touch2_handle_event(struct tp_dispatch *tp,
> >>- struct tp_touch *t,
> >>- enum tap_event event, uint64_t time)
> >>-{
> >>-
> >>- switch (event) {
> >>- case TAP_EVENT_TOUCH:
> >>- tp->tap.state = TAP_STATE_TOUCH_3;
> >>- tp_tap_set_timer(tp, time);
> >>- break;
> >>- case TAP_EVENT_RELEASE:
> >>- tp->tap.state = TAP_STATE_HOLD;
> >>- if (t->tap.state == TAP_TOUCH_STATE_TOUCH) {
> >>- tp_tap_notify(tp, time, 2, LIBINPUT_BUTTON_STATE_PRESSED);
> >>- tp_tap_notify(tp, time, 2, LIBINPUT_BUTTON_STATE_RELEASED);
> >>- }
> >>- tp_tap_clear_timer(tp);
> >>- break;
> >>- case TAP_EVENT_MOTION:
> >>- tp_tap_clear_timer(tp);
> >>- /* fallthrough */
> >>- case TAP_EVENT_TIMEOUT:
> >>- tp->tap.state = TAP_STATE_TOUCH_2_HOLD;
> >>- break;
> >>- case TAP_EVENT_BUTTON:
> >>- tp->tap.state = TAP_STATE_DEAD;
> >>- break;
> >>- }
> >>-}
> >>-
> >>-static void
> >>-tp_tap_touch2_hold_handle_event(struct tp_dispatch *tp,
> >>- struct tp_touch *t,
> >>- enum tap_event event, uint64_t time)
> >>-{
> >>-
> >>- switch (event) {
> >>- case TAP_EVENT_TOUCH:
> >>- tp->tap.state = TAP_STATE_TOUCH_3;
> >>- tp_tap_set_timer(tp, time);
> >>- break;
> >>- case TAP_EVENT_RELEASE:
> >>- tp->tap.state = TAP_STATE_HOLD;
> >>- break;
> >>- case TAP_EVENT_MOTION:
> >>- case TAP_EVENT_TIMEOUT:
> >>- tp->tap.state = TAP_STATE_TOUCH_2_HOLD;
> >>- break;
> >>- case TAP_EVENT_BUTTON:
> >>- tp->tap.state = TAP_STATE_DEAD;
> >>- break;
> >>- }
> >>-}
> >>-
> >>-static void
> >>-tp_tap_touch3_handle_event(struct tp_dispatch *tp,
> >>- struct tp_touch *t,
> >>- enum tap_event event, uint64_t time)
> >>-{
> >>-
> >>- switch (event) {
> >>- case TAP_EVENT_TOUCH:
> >>- tp->tap.state = TAP_STATE_DEAD;
> >>- tp_tap_clear_timer(tp);
> >>- break;
> >>- case TAP_EVENT_MOTION:
> >>- case TAP_EVENT_TIMEOUT:
> >>- tp->tap.state = TAP_STATE_TOUCH_3_HOLD;
> >>- tp_tap_clear_timer(tp);
> >>- break;
> >>- case TAP_EVENT_RELEASE:
> >>- tp->tap.state = TAP_STATE_TOUCH_2_HOLD;
> >>- if (t->tap.state == TAP_TOUCH_STATE_TOUCH) {
> >>- tp_tap_notify(tp, time, 3, LIBINPUT_BUTTON_STATE_PRESSED);
> >>- tp_tap_notify(tp, time, 3, LIBINPUT_BUTTON_STATE_RELEASED);
> >>- }
> >>- break;
> >>- case TAP_EVENT_BUTTON:
> >>- tp->tap.state = TAP_STATE_DEAD;
> >>- break;
> >>- }
> >>-}
> >>-
> >>-static void
> >>-tp_tap_touch3_hold_handle_event(struct tp_dispatch *tp,
> >>- struct tp_touch *t,
> >>- enum tap_event event, uint64_t time)
> >>-{
> >>-
> >>- switch (event) {
> >>- case TAP_EVENT_TOUCH:
> >>- tp->tap.state = TAP_STATE_DEAD;
> >>- tp_tap_set_timer(tp, time);
> >>- break;
> >>- case TAP_EVENT_RELEASE:
> >>- tp->tap.state = TAP_STATE_TOUCH_2_HOLD;
> >>- break;
> >>- case TAP_EVENT_MOTION:
> >>- case TAP_EVENT_TIMEOUT:
> >>- break;
> >>- case TAP_EVENT_BUTTON:
> >>- tp->tap.state = TAP_STATE_DEAD;
> >>- break;
> >>- }
> >>-}
> >>-
> >>-static void
> >> tp_tap_dragging_or_doubletap_handle_event(struct tp_dispatch *tp,
> >> struct tp_touch *t,
> >> enum tap_event event, uint64_t time)
> >>@@ -560,18 +459,6 @@ tp_tap_handle_event(struct tp_dispatch *tp,
> >> case TAP_STATE_TAPPED:
> >> tp_tap_tapped_handle_event(tp, t, event, time);
> >> break;
> >>- case TAP_STATE_TOUCH_2:
> >>- tp_tap_touch2_handle_event(tp, t, event, time);
> >>- break;
> >>- case TAP_STATE_TOUCH_2_HOLD:
> >>- tp_tap_touch2_hold_handle_event(tp, t, event, time);
> >>- break;
> >>- case TAP_STATE_TOUCH_3:
> >>- tp_tap_touch3_handle_event(tp, t, event, time);
> >>- break;
> >>- case TAP_STATE_TOUCH_3_HOLD:
> >>- tp_tap_touch3_hold_handle_event(tp, t, event, time);
> >>- break;
> >> case TAP_STATE_DRAGGING_OR_DOUBLETAP:
> >> tp_tap_dragging_or_doubletap_handle_event(tp, t, event, time);
> >> break;
> >>@@ -687,8 +574,6 @@ tp_tap_handle_state(struct tp_dispatch *tp, uint64_t time)
> >> case TAP_STATE_TOUCH:
> >> case TAP_STATE_TAPPED:
> >> case TAP_STATE_DRAGGING_OR_DOUBLETAP:
> >>- case TAP_STATE_TOUCH_2:
> >>- case TAP_STATE_TOUCH_3:
> >> case TAP_STATE_MULTITAP_DOWN:
> >> filter_motion = 1;
> >> break;
> >>diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> >>index 97b17cd..1661c56 100644
> >>--- a/src/evdev-mt-touchpad.h
> >>+++ b/src/evdev-mt-touchpad.h
> >>@@ -89,10 +89,6 @@ enum tp_tap_state {
> >> TAP_STATE_TOUCH,
> >> TAP_STATE_HOLD,
> >> TAP_STATE_TAPPED,
> >>- TAP_STATE_TOUCH_2,
> >>- TAP_STATE_TOUCH_2_HOLD,
> >>- TAP_STATE_TOUCH_3,
> >>- TAP_STATE_TOUCH_3_HOLD,
> >> TAP_STATE_DRAGGING_OR_DOUBLETAP,
> >> TAP_STATE_DRAGGING,
> >> TAP_STATE_DRAGGING_WAIT,
> >>--
> >>2.3.5
> >>
> >>_______________________________________________
> >>wayland-devel mailing list
> >>wayland-devel at lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >_______________________________________________
> >wayland-devel mailing list
> >wayland-devel at lists.freedesktop.org
> >http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list