[PATCH evdev 2/2] Always report all valuators on absolute devices

[Peter Hutterer]

On Sat, Sep 27, 2014 at 10:08:39PM +0200, Éric Brunet wrote:
> When dealing with a device identified as a tablet, both Qt and gtk seem to
> ignore the position as reported by the X server, but rather compute
> themselves the position from the raw valuator values (In order to achieve
> better precision, I imagine). This usually works well because most
> tablets use the wacom driver and wacom always report all the valuators at
> each event, even the unchanged ones. However, evdev reports only the
> valuators that have changed, and it is a source of problems for several
> programs when used with a tablet, see bugs
> 	https://bugs.freedesktop.org/show_bug.cgi?id=82250
> and	https://bugs.freedesktop.org/show_bug.cgi?id=82181
> 
> This patch makes evdev always report all the valuators for devices with
> absolute positionning, which fixes the problems I reported.
> 
> Signed-off-by: Éric Brunet <Eric.Brunet at lps.ens.fr>
> ---
>  src/evdev.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/src/evdev.c b/src/evdev.c
> index 8eb749a..9fa17b5 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -941,11 +941,26 @@ static void EvdevPostQueuedEvents(InputInfoPtr pInfo)
>                                        pEvdev->queue[i].val))
>                  break;
>  
> -            if (pEvdev->abs_queued && pEvdev->in_proximity) {
> -                xf86PostButtonEvent(pInfo->dev, Absolute, pEvdev-
> >queue[i].detail.key,

fwiw, please fix your email client, it mangled the patch as seen
here.

> -                                     pEvdev->queue[i].val, 0, 0);
> -
> -            } else
> +            /*
> +             * We try to report all the valuators with the button events
> +             * because both Qt ang gtk seem to expect them to be present
> +             * for tablets and similar devices. In fact, the wacom driver
> +             * always sends all the valuator, so evdev should do the same.
> +             *
> +             * However, we only do this if
> +             *   a) the device uses absolute values (because sending a
> +             *      relative valuator several time would be akin to moving 
> the
> +             *      cursor twice)
> +             *   b) the device is in proximity (because we don't trust 
> position
> +             *      otherwise)
> +             */
> +            if ((pEvdev->flags & EVDEV_ABSOLUTE_EVENTS) &&
> +                !(pEvdev->flags & EVDEV_RELATIVE_MODE) &&
> +                pEvdev->in_proximity && pEvdev->vals)
> +                xf86PostButtonEventM(pInfo->dev, Absolute, pEvdev-
> >queue[i].detail.key,
> +                                     pEvdev->queue[i].val, pEvdev->vals);
> +            else
> +            /* Either relative or not in proximity */
>                  xf86PostButtonEvent(pInfo->dev, Relative, pEvdev-
> >queue[i].detail.key,
>                                      pEvdev->queue[i].val, 0, 0);
>              break;
> @@ -993,7 +1008,13 @@ EvdevProcessSyncEvent(InputInfoPtr pInfo, struct 
> input_event *ev)
>          /* don't reset the touchMask */
>      }
>  
> -    if (pEvdev->vals)
> +    /*
> +     * Do not reset pEvdev->vals if the device uses absolute events. That
> +     * way, all the valuators (even those that didn't change) are always
> +     * reported 
> +     */
> +    if (pEvdev->vals &&
> +        (pEvdev->flags & (EVDEV_RELATIVE_EVENTS|EVDEV_RELATIVE_MODE)) )
>          valuator_mask_zero(pEvdev->vals);
>      pEvdev->num_queue = 0;
>      pEvdev->abs_queued = 0;

ok, this patch looked good at first, so I applied it locally.
Unfortunately, it breaks a couple of tests in XIT, specifically
EvdevMixedDeviceTest.AbsXYAndRelScroll/*. 

http://cgit.freedesktop.org/xorg/test/xorg-integration-tests/tree/tests/input/evdev.cpp#n1925

These tests set up a device with absolute axes and scroll wheels, then post
a scroll wheel event followed by a abs movement event. The variations are
whether IgnoreAbsoluteAxes or IgnoreRelativeAxes is set, i.e. whether we
expect the abs event to come through or not. It breaks in all cases absolute
axes are not ignored (including the default case, so easy enough to test w/o
config).

Here's the problem: if smooth scrolling is enabled, the wheel events are
set as relative valuators in the mask. so on the first wheel event, the
mask is [0, 0, 1, ...], on the second wheel event the mask is [0, 0, -1,
..].

the device is in absolute mode, so the mask isn't reset after events (scroll
wheels on their own don't set the device to relative, they are special). Now
you get the abs x/y events and the mask is [70, 70, -1, ...]. The server
interprets the -1 as another scroll event. long story short, if you ever get
a scroll wheel event on those devices, you keep scrolling. That's a lot of
fun, but arguably a regression :)

So the mask reset needs to be a bit smarter but from a quick skim of the
code it looks like that's a general problem anyway. if the REL_WHEEL is sent
in the same EV_SYN frame as the ABS_X event, both rel_queued and abs_queued
are set and the mask is submitted twice to the server (once as rel, once as
abs mask) and we get duplicate events. So congratulations I guess: you found
a bug and won the chance to fix that too ;) sorry about that.

Cheers,
   Peter

PS: first patch of this pair pushed, thanks