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.hutte...@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 <hdego...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel