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

[Chase Douglas]

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>

Thanks!