[PATCH libinput] touchpad: short-circuit the edge scroll handling when it's not enabled

[Hans de Goede]

Hi,

On 31-05-16 02:16, Peter Hutterer wrote:
> No need to handle events properly in the edge scroll state machine when it's
> not enabled. Just set any beginning touch to state AREA and move on. The rest
> of the code guarantees neutral state when edge scrolling is enabled or
> disabled.
>
> This reduces the debug output produced by libinput-debug-events when edge
> scrolling is disabled, preventing users from seemingly identifying
> bugs where there are none.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>  src/evdev-mt-touchpad-edge-scroll.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad-edge-scroll.c b/src/evdev-mt-touchpad-edge-scroll.c
> index fcc0512..8222bba 100644
> --- a/src/evdev-mt-touchpad-edge-scroll.c
> +++ b/src/evdev-mt-touchpad-edge-scroll.c
> @@ -318,6 +318,15 @@ tp_edge_scroll_handle_state(struct tp_dispatch *tp, uint64_t time)
>  {
>  	struct tp_touch *t;
>
> +	if (tp->scroll.method != LIBINPUT_CONFIG_SCROLL_EDGE) {
> +		tp_for_each_touch(tp, t) {
> +			if (t->state == TOUCH_BEGIN)
> +				t->scroll.edge_state =
> +					EDGE_SCROLL_TOUCH_STATE_AREA;
> +		}
> +		return;
> +	}
> +
>  	tp_for_each_touch(tp, t) {
>  		if (!t->dirty)
>  			continue;

IMHO it would be cleaner to replace this hunk with:

@@ -141,7 +141,8 @@ tp_edge_scroll_handle_none(struct tp_dispatch *tp,

         switch (event) {
         case SCROLL_EVENT_TOUCH:
-           if (tp_touch_get_edge(tp, t)) {
+         if (tp->scroll.method == LIBINPUT_CONFIG_SCROLL_EDGE &&
+             tp_touch_get_edge(tp, t)) {
                         tp_edge_scroll_set_state(tp, t,
                                         EDGE_SCROLL_TOUCH_STATE_EDGE_NEW);
                 } else {

This is the cleanest solution IMHO, but then we still get one log_debug for new touches;
alternatively we could do:

@@ -327,7 +327,13 @@ tp_edge_scroll_handle_state(struct tp_dispatch *tp, uint64_t time)
                 case TOUCH_HOVERING:
                         break;
                 case TOUCH_BEGIN:
-                   tp_edge_scroll_handle_event(tp, t, SCROLL_EVENT_TOUCH);
+                 if (tp->scroll.method == LIBINPUT_CONFIG_SCROLL_EDGE) {
+                         tp_edge_scroll_handle_event(tp, t,
+                                         SCROLL_EVENT_TOUCH);
+                 } else {
+                         tp_edge_scroll_set_state(tp, t,
+                                         EDGE_SCROLL_TOUCH_STATE_AREA);
+                 }
                         break;
                 case TOUCH_UPDATE:
                         tp_edge_scroll_handle_event(tp, t, SCROLL_EVENT_MOTION);

That at least avoids the double tp_for_each_touch(tp, t) {}

I've a slight preference for the first solution, and just living with the
one debug line for a new touch, after all this is for debugging only, and
code-wise it is by far the cleanest.

Anyways all 3 get the job done in the end, so this patch is:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Feel free to pick any of the outlined solutions (including your original one).

> @@ -350,9 +359,6 @@ tp_edge_scroll_post_events(struct tp_dispatch *tp, uint64_t time)
>  	const struct normalized_coords zero = { 0.0, 0.0 };
>  	const struct discrete_coords zero_discrete = { 0.0, 0.0 };
>
> -	if (tp->scroll.method != LIBINPUT_CONFIG_SCROLL_EDGE)
> -		return 0;
> -
>  	tp_for_each_touch(tp, t) {
>  		if (!t->dirty)
>  			continue;
>

Regards,

Hans