dix: switch scroll button emulation to multiples of increment

The current algorithm triggers a bug in Xwayland when two devices have
different granularity of scrolling. In Xwayland, the scroll increment is
1 and all physical devices scroll through the same (x)wayland pointer
device.

This may cause events to get lost when changing devices:
- mouse scrolls by full increment, current value is 1.0
  last scroll button was sent for valuator value 0.0,
  delta is 1.0 and we emulate a button event.
- touchpad scrolls by partial increment, current value is 1.3
  last scroll button was sent for valuator value 1.0, delta is 0.3
  and no button event is emulated
- mouse scrolls by full increment, current value is -0.7,
  last scroll button was sent for valuator value 1.0, delta is -0.7
  and no button event is emulated

Thus the wheel event appears to get lost. Xwayland cannot reliably
detect this case because we don't see the physical devices.

We can work around this by instead emulating buttons whenever we cross
a multiple of increment. However, this has a drawback:
high-resolution scroll devices can now trigger a button event storm by
jittering across the multiple of increment. e.g. in the example above
the touchpad moving from 1.3 to 1.0 would cause a click, despite this
being a third of an increment.

Fixes #1339

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Acked-by: Olivier Fourdan <ofourdan@redhat.com>

dix/getevents.c

@@ -1500,7 +1500,8 @@ fill_pointer_events(InternalEvent *events, DeviceIntPtr pDev, int type,
  * @param type The real type of the event
  * @param axis The axis number to generate events for
  * @param mask State before this event in absolute coords
- * @param[in,out] last Last scroll state posted in absolute coords (modified
+ * @param last_valuators The device's last valuators value
+ * @param[in,out] lastScroll Last scroll state posted in absolute coords (modified
  * in-place)
  * @param ms Current time in ms
  * @param max_events Max number of events to be generated
@@ -1512,15 +1513,19 @@ emulate_scroll_button_events(InternalEvent *events,
                              int type,
                              int axis,
                              const ValuatorMask *mask,
-                             ValuatorMask *last, CARD32 ms, int max_events)
+                             const ValuatorMask *last_valuators,
+                             ValuatorMask *lastScroll, CARD32 ms, int max_events)
 {
     AxisInfoPtr ax;
     double delta;
     double incr;
+    int direction = 0; /* -1 for up, 1 for down */
     int num_events = 0;
-    double total;
     int b;
     int flags = 0;
+    double last_val,    /* abs axis value from previous event */
+           current_val, /* abs axis value for this event */
+           last_scroll_val; /* abs axis value we sent out the last scroll button for */
 
     if (dev->valuator->axes[axis].scroll.type == SCROLL_TYPE_NONE)
         return 0;
@@ -1538,25 +1543,73 @@ emulate_scroll_button_events(InternalEvent *events,
     if (type != ButtonPress && type != ButtonRelease)
         flags |= POINTER_EMULATED;
 
-    if (!valuator_mask_isset(last, axis))
+    if (!valuator_mask_isset(lastScroll, axis))
-        valuator_mask_set_double(last, axis, 0);
+        valuator_mask_set_double(lastScroll, axis, 0);
+
+    /* The delta between the last value we sent a scroll button event for
+     * and the current event value (which has been applied already in
+     * fill_pointer_events). This tells us the scroll direction. */
+    delta = valuator_mask_get_double(mask, axis) - valuator_mask_get_double(lastScroll, axis);
+    direction = delta * incr > 0 ? 1 : -1;
 
-    delta =
-        valuator_mask_get_double(mask, axis) - valuator_mask_get_double(last,
-                                                                        axis);
-    total = delta;
     b = (ax->scroll.type == SCROLL_TYPE_VERTICAL) ? 5 : 7;
-
+    if (direction < 0)
-    if ((incr > 0 && delta < 0) || (incr < 0 && delta > 0))
         b--;                    /* we're scrolling up or left → button 4 or 6 */
 
-    while (fabs(delta) >= fabs(incr)) {
+    /* Note: we emulate scroll on multiples of the increment, regardless of the
-        int nev_tmp;
+     * current delta, mostly for the benefit of Xwayland which doesn't (cannot)
+     * distinguish between devices, see #1339 and #1414.
+     *
+     * Where a device scrolls a fraction of an increment, a subsequent scroll in
+     * the other direction did not trigger a scroll event. For example, where
+     * the increment is 1.0, the current axis value is 3.0 and a device scrolls
+     * down by 0.7 (a), then up by -1.0 (b), no scroll event was emitted:
+     *      -----|------b--|------a--|----
+     *          2.0       3.0       4.0
+     * For both events, the last button was sent at 3.0 and since the delta
+     * from that is never a full increment, no events were generated.
+     * With Xwayland this can happen when we switch between smooth-scroll
+     * devices and discrete devices.
+     *
+     * To avoid this, we now emulate button events whenever we cross a multiple
+     * of the scroll increment. For example, for a scroll increment of 1.0
+     * we expect events at -2.0, -1.0, 0.0, 1.0, 2.0,...
+     *
+     * In the above example, we go from 3.0 to 3.7 (no scroll button event),
+     * then from 3.7 to 2.7 which triggers a scroll button event because we
+     * cross 3.0.
+     *
+     * This trades off one bug for another. Previously, the first scroll button
+     * event after changing direction was always between
+     * [increment, 2 * increment). Above example again: the first event would be
+     * emulated at 2.0 so the full movement before a button event was actually
+     * -1.7.
+     *
+     * Now, the first scroll button event is always between (0.0, increment).
+     * Above example again: the first event would be emulated at 3.0
+     * so the full movement before a button event was actually -0.7.
+     *
+     * This only affects changes of directions. Above example again: the next
+     * button event in-direction would've been emulated at 4.0 so only 0.3
+     * from the current position.
+     */
+    last_val = valuator_mask_get_double(last_valuators, axis);
+    last_scroll_val = valuator_mask_get_double(lastScroll, axis);
+    current_val = valuator_mask_get_double(mask, axis);
 
-        if (delta > 0)
+    /* We're crossing an increment multiple? */
-            delta -= fabs(incr);
+    if ((current_val < last_scroll_val && last_scroll_val < last_val) ||
-        else if (delta < 0)
+        (last_val < last_scroll_val && last_scroll_val < current_val)) {
-            delta += fabs(incr);
+        last_scroll_val -= direction * incr;
+    }
+
+    while (TRUE) {
+        /* The next value we want to send out a button event for */
+        double next_val = last_scroll_val + direction * incr;
+
+        if ((direction > 0 && next_val > current_val) ||
+            (direction < 0 && next_val < current_val))
+                break;
 
         /* fill_pointer_events() generates four events: one normal and one raw
          * event for button press and button release.
@@ -1564,6 +1617,8 @@ emulate_scroll_button_events(InternalEvent *events,
          * for. In that case, we keep decreasing delta, but skip events.
          */
         if (num_events + 4 < max_events) {
+            int nev_tmp;
+
             if (type != ButtonRelease) {
                 nev_tmp = fill_pointer_events(events, dev, ButtonPress, b, ms,
                                               flags, NULL);
@@ -1577,14 +1632,11 @@ emulate_scroll_button_events(InternalEvent *events,
                 num_events += nev_tmp;
             }
         }
+        /* send out scroll event */
+        last_scroll_val = next_val;
     }
 
-    /* We emulated, update last.scroll */
+    valuator_mask_set_double(lastScroll, axis, last_scroll_val);
-    if (total != delta) {
-        total -= delta;
-        valuator_mask_set_double(last, axis,
-                                 valuator_mask_get_double(last, axis) + total);
-    }
 
     return num_events;
 }
@@ -1614,6 +1666,7 @@ GetPointerEvents(InternalEvent *events, DeviceIntPtr pDev, int type,
 {
     CARD32 ms = GetTimeInMillis();
     int num_events = 0, nev_tmp;
+    ValuatorMask last_valuators;
     ValuatorMask mask;
     ValuatorMask scroll;
     int i;
@@ -1642,6 +1695,12 @@ GetPointerEvents(InternalEvent *events, DeviceIntPtr pDev, int type,
 
     valuator_mask_copy(&mask, mask_in);
 
+    /* Back up the current value of last.valuators. fill_pointer_events()
+     * overwrites those but we need them for scroll button emulation */
+    valuator_mask_zero(&last_valuators);
+    for (size_t idx = 0; idx < pDev->last.numValuators; idx++)
+        valuator_mask_set_double(&last_valuators, idx, pDev->last.valuators[idx]);
+
     /* Turn a scroll button press into a smooth-scrolling event if
      * necessary. This only needs to cater for the XIScrollFlagPreferred
      * axis (if more than one scrolling axis is present) */
@@ -1712,7 +1771,7 @@ GetPointerEvents(InternalEvent *events, DeviceIntPtr pDev, int type,
 
         nev_tmp =
             emulate_scroll_button_events(events, pDev, realtype, i, &scroll,
-                                         pDev->last.scroll, ms,
+                                         &last_valuators, pDev->last.scroll, ms,
                                          GetMaximumEventsNum() - num_events);
         events += nev_tmp;
         num_events += nev_tmp;