[PATCH] dix: only transform valuators when we need them.

[Peter Hutterer]

On Wed, May 04, 2011 at 10:15:22AM +0200, Chase Douglas wrote:
> On 05/03/2011 03:28 AM, Peter Hutterer wrote:
> > On Mon, Apr 25, 2011 at 12:58:17PM -0400, Chase Douglas wrote:
> >> On 04/21/2011 03:35 AM, Peter Hutterer wrote:
> >>> Unconditionally drop the valuators back into the mask when they were there
> >>> in the first place. Otherwise, sending identical coordinates from the driver
> >>> on a translated device causes the valuator mask to be alternatively
> >>> overwritten with the translated value or left as-is. This leads to the
> >>> device jumping around between the translated and the original position.
> >>>
> >>> The same could be achieved with a valuator_mask_unset() combination.
> >>>
> >>> Testcase:
> >>> xsetwacom set "device name" MapToOutput VGA1
> >>> Then press a button on the device, cursor jumps between the two positions.
> >>>
> >>> Introduced in 31737fff08ec19b394837341d5e358ec401f5cd8
> >>>
> >>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >>> ---
> >>>  dix/getevents.c |    5 +++--
> >>>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/dix/getevents.c b/dix/getevents.c
> >>> index 0fa8046..7afd330 100644
> >>> --- a/dix/getevents.c
> >>> +++ b/dix/getevents.c
> >>> @@ -1065,9 +1065,10 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
> >>>  
> >>>      pixman_f_transform_point(&dev->transform, &p);
> >>>  
> >>> -    if (lround(p.v[0]) != dev->last.valuators[0])
> >>> +    if (valuator_mask_isset(mask, 0))
> >>>          valuator_mask_set(mask, 0, lround(p.v[0]));
> >>> -    if (lround(p.v[1]) != dev->last.valuators[1])
> >>> +
> >>> +    if (valuator_mask_isset(mask, 1))
> >>>          valuator_mask_set(mask, 1, lround(p.v[1]));
> >>>  }
> >>>  
> >>
> >> Why don't we change this to:
> >>
> >> if (lround(p.v[0]) != dev->last.valuators[0])
> >>     valuator_mask_set(mask, 0, lround(p.v[0]));
> >> else
> >>     valuator_mask_unset(mask, 0);
> >>
> >> The proposed fix will cause valuators to be sent with repeated values if
> >> they haven't actually changed.
> > 
> > I think this would generally need a bigger patch set than just the above
> > (which was my first attempt at the patch, see also the commit message) to be
> > addressed properly.
> 
> Yeah... We received a bug report for this and I started diving into it
> and realized it will require a bigger patch.
> 
> > We don't have any guidelines for drivers regarding sending identical
> > coordinates and the server does a pretty poor job in filtering them,
> > especially for core events.
> > https://bugs.freedesktop.org/show_bug.cgi?id=23985
> > 
> > Plus the need for continuous valuator ranges in XI1 also means that even
> > when unsetting valuators in the generation path, they may (should!) get
> > added later anyway.
> 
> Yeah.
> 
> > At least in the Wacom driver we need the coordinates to not be filtered when
> > identical, for the clients' sake. the specific use-case here is sending a
> > motion event followed by a button event. They have identical coordinates,
> > but if a client only listens to button events and the coordinates are
> > filtered, they cannot get the valuator values from the button event only.
> 
> Good point.
> 
> Reviewed-by: Chase Douglas <chase.douglas at canonical.com>

heh, I just noticed that you had also fixed this in the commit below that
was in daniels' for-keith tree. I'll skip the one above and just merge yours
in then.

commit 65b54548dce80c8e8ff5ff91fc4f0659e9b2d921
Author: Chase Douglas <chase.douglas at canonical.com>
Date:   Tue Jan 18 20:08:09 2011 +0000

    Input: Pass co-ordinates by reference to transformAbsolute


Cheers,
  Peter