[PATCH] dix: fix scale_to_desktop for edge ABS events

[Yufeng Shen]

On Tue, Jul 24, 2012 at 10:44 PM, Daniel Kurtz <djkurtz at chromium.org> wrote:

> On Wed, Jul 25, 2012 at 2:04 AM, Jeremy Huddleston Sequoia
> <jeremyhu at apple.com> wrote:
> > Yick.  This is why origin/size are better descriptors =(
> >
> > Reviewed-by: Jeremy Huddleston Sequoia <jeremyhu at apple.com>
> >
> > On Jul 20, 2012, at 16:41, Yufeng Shen <miletus at chromium.org> wrote:
> >
> >> Scale_to_desktop() converts ABS events from device coordinates
> >> to screen coordinates:
> >> [dev_X_min, dev_X_max]  -> [screen_X_min, screen_X_max]
> >> [dev_Y_min, dev_Y_max]  -> [screen_Y_min, screen_Y_max]
> >>
> >> An edge ABS event with X = dev_X_max (e.g., generated from the
> >> edge of a touchscreen) will be converted to have screen X value
> >> = screen_X_max, which, however, will be filterd out when xserver
> >> tries to find proper Window to receive the event, because the
> >> range check for a Window to receive events is
> >>       window_X_min <= event_screen_X < window_X_max
> >> Events with event_screen_X = screen_X_max will fail the test get
> >> and rejected by the Window.
> >>
> >> To fix this, we change the device to screen coordinates mapping to
> >> [dev_X_min, dev_X_max]  -> [screen_X_min, screen_X_max-1]
> >> [dev_Y_min, dev_Y_max]  -> [screen_Y_min, screen_Y_max-1]
> >>
> >> Signed-off-by: Yufeng Shen <miletus at chromium.org>
> >> ---
> >> dix/getevents.c |    4 ++--
> >> 1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/dix/getevents.c b/dix/getevents.c
> >> index b78d5ce..9898c6a 100644
> >> --- a/dix/getevents.c
> >> +++ b/dix/getevents.c
> >> @@ -890,9 +890,9 @@ scale_to_desktop(DeviceIntPtr dev, ValuatorMask
> *mask,
> >>
> >>     /* scale x&y to desktop coordinates */
> >>     *screenx = rescaleValuatorAxis(x, dev->valuator->axes + 0, NULL,
> >> -                                   screenInfo.x, screenInfo.width);
> >> +                                   screenInfo.x, screenInfo.width - 1);
>
> Hi Yufeng,
>
> rescaleValuatorAxis() expects min/max for its last two arguments.
>
> Technically, this patch (and the original code) looks like it is
> assuming that screenInfo.x == 0.  This is probably a good assumption
> (since this is the global origin
> for all screens together).  However, I think something like the
> following is probably safer:
>
>   *screenx = rescaleValuatorAxis(x, dev->valuator->axes + 0, NULL,
> screenInfo.x, screenInfo.x + screenInfo.width - 1);
>   *screeny = rescaleValuatorAxis(y, dev->valuator->axes + 0, NULL,
> screenInfo.y, screenInfo.y + screenInfo.height - 1);
>
>
I think you are correct at pointing out using
[screenInfo.x,  screenInfo.x + screenInfo.width) as [screen_min, screen_max)

But I would like to have another patch for correcting this since this seems
a different
bug than the patch intends to fix (off-by-one when scaling ABS event) and
it is seen
elsewhere through getevents.c (also in updateSlaveDeviceCoords()
and positionSprite() )
so it needs different tests.

Also, can you double check to make sure that the arguments to
> rescaleValuatorAxis() are set correctly elsewhere as well?  A quick
> glance shows that (a) positionSprite() has the exact same issue, and
> (b) other callers seem to be treating them as "origin / width", and
> not "min / max".
>
>
I checked that updateSlaveDeviceCoords() and positionSprite() also use
rescaleValuatorAxis() to map
coordinates from [screenInfo.x, screenInfo.width] to [device_min,
device_max] (which is opposite to scale_to_desktop())

Actually I don't know in this case should we use
[screenInfo.x, screenInfo.width]  -->  [device_min, device_max]
or
[screenInfo.x, screenInfo.width)  -->  [device_min, device_max]

or does it matter (depending on how the device coordinates are used in the
following code path)

--- Yufeng Shen


> Thanks,
> -Daniel
>
> >>     *screeny = rescaleValuatorAxis(y, dev->valuator->axes + 1, NULL,
> >> -                                   screenInfo.y, screenInfo.height);
> >> +                                   screenInfo.y, screenInfo.height -
> 1);
> >>
> >>     *devx = x;
> >>     *devy = y;
> >> --
> >> 1.7.7.3
> >>
> >> _______________________________________________
> >> xorg-devel at lists.x.org: X.Org development
> >> Archives: http://lists.x.org/archives/xorg-devel
> >> Info: http://lists.x.org/mailman/listinfo/xorg-devel
> >>
> >
> > _______________________________________________
> > xorg-devel at lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: http://lists.x.org/mailman/listinfo/xorg-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20120725/45bb1b48/attachment-0001.html>