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

[Peter Hutterer]

On Tue, May 31, 2016 at 10:18:58AM +0200, Hans de Goede wrote:
> 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:

unfortunately in both of your cases we still get the verbose output. this
hunk has no real effect because if edge scrolling is disabled,
tp_touch_get_edge() is always false.
> 
> @@ -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);

and in this case we're still calling into tp_edge_scroll_handle_event() from
TOUCH_UPDATE/END and timeouts. and that is where the log messages come from.

> 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>

thanks, I think I'll stick with the originally proposed patch. it's not as
nice but it keeps things in one place and does the job.

Cheers,
   Peter

> 
> 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