GNOME Bugzilla – Bug 756284
Unable to use two finger scroll on Wayland
Last modified: 2015-10-12 22:43:21 UTC
When using the Wayland session, the screen shield does not raise with a two-finger scroll. Two finger scrolls also don't work in the Applications grid, nor in the workspace stack.
The four finger swipe gesture to switch workspace works, on the other hand.
This seems down to Clutter evdev backend only emitting smooth scroll events, there seems to be a bunch of event handlers in gnome-shell relying on up/.../right direction in scroll events. I'm moving to Clutter, and attaching a patch to emulate these on the evdev backend, this fixes scroll behavior in all these places.
Created attachment 312973 [details] [review] evdev: Emulate discrete scroll events out of smooth scroll ones. There's handlers around relying on up/down/left/right scroll events, which won't work as expected if only smooth scroll events are sent. In order to work properly there, we have to retrofit discrete scroll events on the evdev backend. So implement this by accumulating dx/dy, and emit (possibly multiple) discrete scroll events when thresholds are reached. When the last 0/0 event is sent, the accumulators will be reset for the next scrolling batch.
*** Bug 729839 has been marked as a duplicate of this bug. ***
Review of attachment 312973 [details] [review]: ::: clutter/evdev/clutter-device-manager-evdev.c @@ +104,3 @@ + /* Emulation of non-smooth scroll events */ + gfloat accum_dx; + gfloat accum_dy; Maybe have "smooth scroll" somehow in the name? @@ +1249,3 @@ + gint i; + +#define DISCRETE_SCROLL_STEP 10 Can you just make this a file wide macro and replace the other magic 10's with it? @@ +1258,3 @@ + } + + for (i = 0; i < (int) ABS (seat->accum_dy/ DISCRETE_SCROLL_STEP); i++) nit: space before / @@ +1431,2 @@ notify_scroll (device, time, dx, dy); + check_notify_discrete_scroll (manager_evdev, device, time); I think you should only emulate the discrete scroll events if the source is not a wheel. For when it's a wheel, you should just use the discrete value directly.
(In reply to Jonas Ådahl from comment #5) > Review of attachment 312973 [details] [review] [review]: > > ::: clutter/evdev/clutter-device-manager-evdev.c > @@ +104,3 @@ > + /* Emulation of non-smooth scroll events */ > + gfloat accum_dx; > + gfloat accum_dy; > > Maybe have "smooth scroll" somehow in the name? Sure. > > @@ +1249,3 @@ > + gint i; > + > +#define DISCRETE_SCROLL_STEP 10 > > Can you just make this a file wide macro and replace the other magic 10's > with it? Yeah, good idea. > > @@ +1258,3 @@ > + } > + > + for (i = 0; i < (int) ABS (seat->accum_dy/ DISCRETE_SCROLL_STEP); i++) > > nit: space before / Oops :) > > @@ +1431,2 @@ > notify_scroll (device, time, dx, dy); > + check_notify_discrete_scroll (manager_evdev, device, time); > > I think you should only emulate the discrete scroll events if the source is > not a wheel. For when it's a wheel, you should just use the discrete value > directly. TBH I started doing that, but I came up with a "simple" if (wheel)... branch that was roughly as long as check_notify_discrete_scroll(), and I was finding out the direction out of dx/dy too, so I ended up thinking that dx/dy is already friendly to that function on all scroll sources.
(In reply to Carlos Garnacho from comment #6) > (In reply to Jonas Ådahl from comment #5) > > Review of attachment 312973 [details] [review] [review] [review]: > > > > @@ +1431,2 @@ > > notify_scroll (device, time, dx, dy); > > + check_notify_discrete_scroll (manager_evdev, device, time); > > > > I think you should only emulate the discrete scroll events if the source is > > not a wheel. For when it's a wheel, you should just use the discrete value > > directly. > > TBH I started doing that, but I came up with a "simple" if (wheel)... branch > that was roughly as long as check_notify_discrete_scroll(), and I was > finding out the direction out of dx/dy too, so I ended up thinking that > dx/dy is already friendly to that function on all scroll sources. What if you made notify_scroll take discrete_x/y, then have a if (wheel) discrete = get_discrete() else discrete = emulate_discrete() and then just pass the discrete_x/y to notify_scroll? It'd make it more clear that we actually just use the discrete values when we can and emulate it when we can't.
Created attachment 313112 [details] [review] evdev: Emulate discrete scroll events out of smooth scroll ones. There's handlers around relying on up/down/left/right scroll events, which won't work as expected if only smooth scroll events are sent. In order to work properly there, we have to retrofit discrete scroll events on the evdev backend. Fix this by implementing emission (on devices with a wheel) and emulation (on anything else) of discrete scroll events. On the former both smooth and discrete events are set, for the latter we do accumulate the dx/dy of the latest scroll events, and emit discrete ones when we accumulated enough. The ending 0/0 event will reset the accumulators for the next scrolling batch.
Review of attachment 313112 [details] [review]: Thanks for looking at this. Looks generally good to me, and I'd like to get this into 3.18.1, at least for selfish purposes. ;-) ::: clutter/evdev/clutter-device-manager-evdev.c @@ +56,3 @@ #include "clutter-device-manager-evdev.h" +#define DISCRETE_SCROLL_STEP 10 Use 10.0. @@ +467,3 @@ + * associated with the device yet. */ + stage = _clutter_input_device_get_stage (input_device); + Please, use an explicit `== NULL` equality test for pointer values. @@ +1270,3 @@ + gint i; + +{ ABS will recompute this all the time. Use something like: int n_xscrolls = floorf (fabsf (seat->accum_scroll_dx) / DISCRETE_SCROLL_STEP); int n_yscrolls = floorf (fabsf (seat->accum_scroll_dy) / DISCRETE_SCROLL_STEP); instead. @@ +1425,3 @@ + dy = libinput_event_pointer_get_axis_value (axis_event, axis); + + if (wheel || dy == 0) I'm not really sure that `dy == 0` will work; it should work for single precision values, but double precision ones may still have floating point fluctuations. You should use `fabs (dy) < DBL_EPSILON`. @@ +1437,3 @@ + dx = libinput_event_pointer_get_axis_value (axis_event, axis); + + if (wheel || dx == 0) Same as above. @@ +1449,3 @@ + discrete_y * DISCRETE_SCROLL_STEP); + notify_discrete_scroll (device, time, + { No need to break the line, here.
Created attachment 313138 [details] [review] evdev: Emulate discrete scroll events out of smooth scroll ones. v2: Fixed the small issues found in code review; left the attribution alone.
Pushed attachment 313138 [details] [review] to master, and released 1.24.2 with it. Let's see how it goes. :-)