[PATCH libinput] evdev: Activate button scrolling only after pointer movement

[Peter Hutterer]

On Wed, Jan 27, 2016 at 09:59:43PM +0100, Norwin Hanghuhn wrote:
> Currently, a timer is used to discriminate between button clicks and
> button scrolling. Therefore, button clicks must finalize before and
> scrolling can only start after the timer has elapsed.
> 
> This patch changes the behaviour such that scrolling starts
> immediately after the scroll button is pressed and button events
> are released consistently if no scrolling takes place after all.
> 
> Signed-off-by: Norwin Hanghuhn <norwinhanghuhn at gmail.com>

Good point, I didn't even realise that we only had the timer there.
that seems like a bug and should be fixed. Alas, the test suite breaks
so the test cases need to be adjusted for the new behaviour.
A few comments to the patch itself below.

> ---
>  src/evdev.c | 34 +++++++++++-----------------------
>  src/evdev.h |  5 +++--
>  2 files changed, 14 insertions(+), 25 deletions(-)
> 
> diff --git a/src/evdev.c b/src/evdev.c
> index 8f0a607..515baff 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -44,7 +44,6 @@
>  #include "libinput-private.h"
>  
>  #define DEFAULT_WHEEL_CLICK_ANGLE 15
> -#define DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT ms2us(200)
>  
>  enum evdev_key_type {
>  	EVDEV_KEY_TYPE_NONE,
> @@ -273,13 +272,12 @@ evdev_post_trackpoint_scroll(struct evdev_device *device,
>  			     uint64_t time)
>  {
>  	if (device->scroll.method != LIBINPUT_CONFIG_SCROLL_ON_BUTTON_DOWN ||
> -	    !hw_is_key_down(device, device->scroll.button))
> +	    !device->scroll.button_down)
>  		return false;
>  
> -	if (device->scroll.button_scroll_active)
> -		evdev_post_scroll(device, time,
> -				  LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS,
> -				  &unaccel);
> +	evdev_post_scroll(device, time,
> +			  LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS,
> +			  &unaccel);
>  
>  	return true;
>  }
> @@ -481,30 +479,20 @@ get_key_type(uint16_t code)
>  }
>  
>  static void
> -evdev_button_scroll_timeout(uint64_t time, void *data)
> -{
> -	struct evdev_device *device = data;
> -
> -	device->scroll.button_scroll_active = true;
> -}
> -
> -static void
>  evdev_button_scroll_button(struct evdev_device *device,
>  			   uint64_t time, int is_press)
>  {
>  	if (is_press) {
> -		libinput_timer_set(&device->scroll.timer,
> -				   time + DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT);
> +		device->scroll.button_down = true;
>  		device->scroll.button_down_time = time;
>  	} else {
> -		libinput_timer_cancel(&device->scroll.timer);
> -		if (device->scroll.button_scroll_active) {
> +		device->scroll.button_down = false;
> +
> +		if (device->scroll.button_scroll_started) {
>  			evdev_stop_scroll(device, time,
>  					  LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS);
> -			device->scroll.button_scroll_active = false;
>  		} else {
> -			/* If the button is released quickly enough emit the
> -			 * button press/release events. */
> +			/* If no scrolling was done, always emit the events */

"If no scrolling was done, emit the button press/release events"

>  			evdev_pointer_notify_physical_button(device,
>  					device->scroll.button_down_time,
>  					device->scroll.button,

this brings up an interesting problem: if you hold the middle button down
for 3 seconds and release it, you now get button events, the first of which
is delayed by 3 sec but still carries the original timestamp. I don't think
that's good behaviour, it may confuse callers that have since received
events with a much newer timestamp.

So I think we should still keep the timeout and toggle to scrolling whenever
the timeout is hit (and thus swallow the button events). There's also the
option of reducing the scroll threshold to 0 when the timeout is hit to make
sure a slow scroll event like that immediately triggers.

I wonder if we even need the scroll threshold for trackpoints,
accidental/wobbly input on those isn't usually an issue.

> @@ -1186,8 +1174,6 @@ static int
>  evdev_init_button_scroll(struct evdev_device *device,
>  			 void (*change_scroll_method)(struct evdev_device *))
>  {
> -	libinput_timer_init(&device->scroll.timer, device->base.seat->libinput,
> -			    evdev_button_scroll_timeout, device);
>  	device->scroll.config.get_methods = evdev_scroll_get_methods;
>  	device->scroll.config.set_method = evdev_scroll_set_method;
>  	device->scroll.config.get_method = evdev_scroll_get_method;
> @@ -2539,6 +2525,7 @@ evdev_start_scrolling(struct evdev_device *device,
>  	       axis == LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL);
>  
>  	device->scroll.direction |= AS_MASK(axis);
> +	device->scroll.button_scroll_started = true;
>  }
>  
>  void
> @@ -2636,6 +2623,7 @@ evdev_stop_scroll(struct evdev_device *device,
>  	device->scroll.buildup.x = 0;
>  	device->scroll.buildup.y = 0;
>  	device->scroll.direction = 0;
> +	device->scroll.button_scroll_started = false;
>  }
>  

both of these need to be moved. evdev_start_scrolling() is called from any
scroll code, not just button scrolling, so you're setting the boolean here
even on a 2-finger scroll event.

>  void
> diff --git a/src/evdev.h b/src/evdev.h
> index 02b5112..f5daaf1 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -154,7 +154,6 @@ struct evdev_device {
>  	struct device_coords rel;
>  
>  	struct {
> -		struct libinput_timer timer;
>  		struct libinput_device_config_scroll_method config;
>  		/* Currently enabled method, button */
>  		enum libinput_config_scroll_method method;
> @@ -167,7 +166,9 @@ struct evdev_device {
>  		uint32_t want_button;
>  		/* Checks if buttons are down and commits the setting */
>  		void (*change_scroll_method)(struct evdev_device *device);
> -		bool button_scroll_active;
> +		bool button_down;
> +		bool button_scroll_started;

imo you should leave the _active instead of replacing it with _started and
just use it for the new use.

Cheers,
   Peter

> +
>  		double threshold;
>  		double direction_lock_threshold;
>  		uint32_t direction;
> -- 
> 2.6.2