<div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 24, 2012 at 10:44 PM, Daniel Kurtz <span dir="ltr"><<a href="mailto:djkurtz@chromium.org" target="_blank">djkurtz@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><div class="h5">On Wed, Jul 25, 2012 at 2:04 AM, Jeremy Huddleston Sequoia<br>
<<a href="mailto:jeremyhu@apple.com">jeremyhu@apple.com</a>> wrote:<br>
> Yick. This is why origin/size are better descriptors =(<br>
><br>
> Reviewed-by: Jeremy Huddleston Sequoia <<a href="mailto:jeremyhu@apple.com">jeremyhu@apple.com</a>><br>
><br>
> On Jul 20, 2012, at 16:41, Yufeng Shen <<a href="mailto:miletus@chromium.org">miletus@chromium.org</a>> wrote:<br>
><br>
>> Scale_to_desktop() converts ABS events from device coordinates<br>
>> to screen coordinates:<br>
>> [dev_X_min, dev_X_max] -> [screen_X_min, screen_X_max]<br>
>> [dev_Y_min, dev_Y_max] -> [screen_Y_min, screen_Y_max]<br>
>><br>
>> An edge ABS event with X = dev_X_max (e.g., generated from the<br>
>> edge of a touchscreen) will be converted to have screen X value<br>
>> = screen_X_max, which, however, will be filterd out when xserver<br>
>> tries to find proper Window to receive the event, because the<br>
>> range check for a Window to receive events is<br>
>> window_X_min <= event_screen_X < window_X_max<br>
>> Events with event_screen_X = screen_X_max will fail the test get<br>
>> and rejected by the Window.<br>
>><br>
>> To fix this, we change the device to screen coordinates mapping to<br>
>> [dev_X_min, dev_X_max] -> [screen_X_min, screen_X_max-1]<br>
>> [dev_Y_min, dev_Y_max] -> [screen_Y_min, screen_Y_max-1]<br>
>><br>
>> Signed-off-by: Yufeng Shen <<a href="mailto:miletus@chromium.org">miletus@chromium.org</a>><br>
>> ---<br>
>> dix/getevents.c | 4 ++--<br>
>> 1 files changed, 2 insertions(+), 2 deletions(-)<br>
>><br>
>> diff --git a/dix/getevents.c b/dix/getevents.c<br>
>> index b78d5ce..9898c6a 100644<br>
>> --- a/dix/getevents.c<br>
>> +++ b/dix/getevents.c<br>
>> @@ -890,9 +890,9 @@ scale_to_desktop(DeviceIntPtr dev, ValuatorMask *mask,<br>
>><br>
>> /* scale x&y to desktop coordinates */<br>
>> *screenx = rescaleValuatorAxis(x, dev->valuator->axes + 0, NULL,<br>
>> - screenInfo.x, screenInfo.width);<br>
>> + screenInfo.x, screenInfo.width - 1);<br>
<br>
</div></div>Hi Yufeng,<br>
<br>
rescaleValuatorAxis() expects min/max for its last two arguments.<br>
<br>
Technically, this patch (and the original code) looks like it is<br>
assuming that screenInfo.x == 0. This is probably a good assumption<br>
(since this is the global origin<br>
for all screens together). However, I think something like the<br>
following is probably safer:<br>
<div class="im"><br>
*screenx = rescaleValuatorAxis(x, dev->valuator->axes + 0, NULL,<br>
</div>screenInfo.x, screenInfo.x + screenInfo.width - 1);<br>
*screeny = rescaleValuatorAxis(y, dev->valuator->axes + 0, NULL,<br>
screenInfo.y, screenInfo.y + screenInfo.height - 1);<br>
<br></blockquote><div><br></div><div>I think you are correct at pointing out using</div><div>[screenInfo.x, screenInfo.x + screenInfo.width) as [screen_min, screen_max)</div><div><br></div><div>But I would like to have another patch for correcting this since this seems a different</div>
<div>bug than the patch intends to fix (off-by-one when scaling ABS event) and it is seen</div><div>elsewhere through getevents.c (also in updateSlaveDeviceCoords() and positionSprite() )</div><div>so it needs different tests. </div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, can you double check to make sure that the arguments to<br>
rescaleValuatorAxis() are set correctly elsewhere as well? A quick<br>
glance shows that (a) positionSprite() has the exact same issue, and<br>
(b) other callers seem to be treating them as "origin / width", and<br>
not "min / max".<br>
<br></blockquote><div><br></div><div>I checked that updateSlaveDeviceCoords() and positionSprite() also use rescaleValuatorAxis() to map</div><div>coordinates from [screenInfo.x, screenInfo.width] to [device_min, device_max] (which is opposite to scale_to_desktop()) </div>
<div><br></div><div>Actually I don't know in this case should we use </div><div>[screenInfo.x, screenInfo.width] --> [device_min, device_max]<br></div><div>or </div><div>[screenInfo.x, screenInfo.width) --> [device_min, device_max]<br>
</div><div><br></div><div>or does it matter (depending on how the device coordinates are used in the following code path)</div><div><br></div><div>--- Yufeng Shen</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks,<br>
-Daniel<br>
<div class=""><div class="h5"><br>
>> *screeny = rescaleValuatorAxis(y, dev->valuator->axes + 1, NULL,<br>
>> - screenInfo.y, screenInfo.height);<br>
>> + screenInfo.y, screenInfo.height - 1);<br>
>><br>
>> *devx = x;<br>
>> *devy = y;<br>
>> --<br>
>> 1.7.7.3<br>
>><br>
>> _______________________________________________<br>
>> <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>: X.Org development<br>
>> Archives: <a href="http://lists.x.org/archives/xorg-devel" target="_blank">http://lists.x.org/archives/xorg-devel</a><br>
>> Info: <a href="http://lists.x.org/mailman/listinfo/xorg-devel" target="_blank">http://lists.x.org/mailman/listinfo/xorg-devel</a><br>
>><br>
><br>
> _______________________________________________<br>
> <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>: X.Org development<br>
> Archives: <a href="http://lists.x.org/archives/xorg-devel" target="_blank">http://lists.x.org/archives/xorg-devel</a><br>
> Info: <a href="http://lists.x.org/mailman/listinfo/xorg-devel" target="_blank">http://lists.x.org/mailman/listinfo/xorg-devel</a><br>
</div></div></blockquote></div><br></div>