[PATCH libinput] Add right click drag

[Peter Hutterer]

Apologies for the long delay, I had to catch up with too many things and was
stuck on some other bug for longer than I though.

On Wed, Aug 23, 2017 at 12:07:15AM +0200, Corentin Marciau wrote:
> Hi,
> 
> For what it's worth, here is a patch of the touchpad tap state machine to
> handle right click drag.
> 
> Since it's first time I change something in libinput, I have no idea if I
> did things correctly or even if this work is relevant at all. At least, as
> demanded in evdev-mt-touchpad-tap.c, I updated
> touchpad-tap-state-machine.svg accordingly ...

SVGs are a bit bad to visually diff, but afaict aside from the
DRAGGING_TOUCH rename there is only the bit in the top-left corner that's
new, right? The bit that branches off after button 2 press, can you confirm
this?


Let me summarise the behaviour introduced here, to make sure I understand
it. This should be part of the commit message btw, otherwise it's hard to
guess whether it's the intended one or not. Also, I've basically only
reviewed the state diagram so far, because without that the code doesn't
matter anyway :)

Provided tap-drag is enabled:
A two-finger tap doesn't immediately release now but keeps the button down.
A finger down after a right finger now keeps that button held down, the
theory here is that moving that finger will now cause a right-button drag.
If drag-lock is enabled, releasing the finger works like the single-finger
tap thing, so you can reset and continue.

I think I spot a few inconsistencies here though:
* DRAGGING_2_OR_DOUBLETAP is misnamed, there isn't really a doubletap here.
  Well there could be but...

* a two finger tap followed by a one finger tap 
  causes a right-click only and ignores the single finger tap.
  Transition  is TAPPED_2, DRAGGING_2_OR_DOUBLETAP, then to IDLE.
  I think for 'first finger up' after DRAGGING_2_OR_DOUBLETAP you need to
  - set first finger up to idle (already correct)
  - release button 2 (already corrent)
  - basically do the same as the TOUCH->TAPPED transition, send a button 1
    down and go to state TAPPED
 
* what happens in DRAGGING_2_OR_DOUBLETAP when a second finger is put down?
  I can guess (same as single-finger basically) but this needs to be added.

* what happens in DRAGGING_2_OR_DOUBLETAP when a third finger is put down?

* what happens in DRAGGING_2_OR_TAP when the finger is released within the
  timeout? this should loop back to DRAGGING_WAIT, I think

* what happens in DRAGGING_2_OR_TAP when a second finger is put down?


Please make sure that the various connectors are correct too so there's only
ever one line to follow as you go through the states.

> 
> @Peter Hutterer, thanks a lot for your help !
> 
> Enclosed:
> - patch
> - touchpad-tap-state-machine.svg
> - strange file from draw.io
> 
> For now, I just fixed the two tests that were broken by my change. I'll
> probably try to add some additional ones for this feature, but it's not done
> yet.
> 
> Cheers,
> Corentin
> 
> PS: If anyone wonder who in the world would want to right click and drag, I
> use the Firefox plugin FireGestures
> 
> PPS: Yes, I use Firefox. I have a big fat CPU
> 
> PPPS: Yes, I do use a mouse. Call me a noob
 

> From 981005cf5ade719c99b4dc2f117b15d0d4ba71f2 Mon Sep 17 00:00:00 2001
> From: Corentin Marciau <corentin at marciau.fr>
> Date: Tue, 22 Aug 2017 23:25:31 +0200
> Subject: [PATCH libinput 2/2] [TEST] Fix 2fg tap touchpad tests

what's broken here? This needs a bit more information :)

> 
> ---
>  test/test-touchpad-tap.c | 44 ++++++++++----------------------------------
>  1 file changed, 10 insertions(+), 34 deletions(-)
> 
> diff --git a/test/test-touchpad-tap.c b/test/test-touchpad-tap.c
> index bdfab81..974d1b1 100644
> --- a/test/test-touchpad-tap.c
> +++ b/test/test-touchpad-tap.c
> @@ -975,9 +975,6 @@ START_TEST(touchpad_2fg_tap)
>  	struct libinput *li = dev->libinput;
>  	enum libinput_config_tap_button_map map = _i; /* ranged test */
>  	unsigned int button = 0;
> -	struct libinput_event *ev;
> -	struct libinput_event_pointer *ptrev;
> -	uint64_t ptime, rtime;
>  
>  	litest_enable_tap(dev->libinput_device);
>  	litest_set_tap_map(dev->libinput_device, map);
> @@ -1002,20 +999,11 @@ START_TEST(touchpad_2fg_tap)
>  
>  	libinput_dispatch(li);
>  
> -	ev = libinput_get_event(li);
> -	ptrev = litest_is_button_event(ev,
> -				       button,
> -				       LIBINPUT_BUTTON_STATE_PRESSED);
> -	ptime = libinput_event_pointer_get_time_usec(ptrev);
> -	libinput_event_destroy(ev);
> -	ev = libinput_get_event(li);
> -	ptrev = litest_is_button_event(ev,
> -				       button,
> -				       LIBINPUT_BUTTON_STATE_RELEASED);
> -	rtime = libinput_event_pointer_get_time_usec(ptrev);
> -	libinput_event_destroy(ev);
> -
> -	ck_assert_int_lt(ptime, rtime);

don't drop this please, making sure that the timestamps are correct is
suprisingly important. You can move this into a separate test if you don't
want it here, but there should be a test for it.

Cheers,
   Peter

> +	litest_assert_button_event(li, button,
> +				   LIBINPUT_BUTTON_STATE_PRESSED);
> +	litest_timeout_tap();
> +	litest_assert_button_event(li, button,
> +				   LIBINPUT_BUTTON_STATE_RELEASED);
>  
>  	litest_assert_empty_queue(li);
>  }
> @@ -1027,9 +1015,6 @@ START_TEST(touchpad_2fg_tap_inverted)
>  	struct libinput *li = dev->libinput;
>  	enum libinput_config_tap_button_map map = _i; /* ranged test */
>  	unsigned int button = 0;
> -	struct libinput_event *ev;
> -	struct libinput_event_pointer *ptrev;
> -	uint64_t ptime, rtime;
>  
>  	litest_enable_tap(dev->libinput_device);
>  	litest_set_tap_map(dev->libinput_device, map);
> @@ -1054,20 +1039,11 @@ START_TEST(touchpad_2fg_tap_inverted)
>  
>  	libinput_dispatch(li);
>  
> -	ev = libinput_get_event(li);
> -	ptrev = litest_is_button_event(ev,
> -				       button,
> -				       LIBINPUT_BUTTON_STATE_PRESSED);
> -	ptime = libinput_event_pointer_get_time_usec(ptrev);
> -	libinput_event_destroy(ev);
> -	ev = libinput_get_event(li);
> -	ptrev = litest_is_button_event(ev,
> -				       button,
> -				       LIBINPUT_BUTTON_STATE_RELEASED);
> -	rtime = libinput_event_pointer_get_time_usec(ptrev);
> -	libinput_event_destroy(ev);
> -
> -	ck_assert_int_lt(ptime, rtime);
> +	litest_assert_button_event(li, button,
> +				   LIBINPUT_BUTTON_STATE_PRESSED);
> +	litest_timeout_tap();
> +	litest_assert_button_event(li, button,
> +				   LIBINPUT_BUTTON_STATE_RELEASED);
>  
>  	litest_assert_empty_queue(li);
>  }
> -- 
> 2.11.0
>