[PATCH 1/2] mi: fix cursor warping screens

[Vignatti Tiago (Nokia-D/Helsinki)]

On Thu, Aug 06, 2009 at 03:11:16AM +0200, Peter Hutterer wrote:
> On Wed, Aug 05, 2009 at 09:18:55PM +0300, y at mgw-mx09.nokia.com wrote:
> > From: Tiago Vignatti <tiago.vignatti at nokia.com>
> > 
> > The server was processing ET_RawMotion type when the cursor was wrapping to
> > another screen and getting wrong valuator values. This fix such issue
> > considering only ET_Motion, ET_KeyPress, ET_KeyRelease, ET_ButtonPress and
> > ET_ButtonRelease types when the cursor detects a new screen.
> > 
> > Signed-off-by: Tiago Vignatti <tiago.vignatti at nokia.com>
> > ---
> >  mi/mieq.c |   24 ++++++++++++++++--------
> >  1 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/mi/mieq.c b/mi/mieq.c
> > index 6ec2dba..b52ed84 100644
> > --- a/mi/mieq.c
> > +++ b/mi/mieq.c
> > @@ -367,14 +367,22 @@ mieqProcessDeviceEvent(DeviceIntPtr dev,
> >      /* Custom event handler */
> >      handler = miEventQueue.handlers[event->any.type];
> >  
> > -    if (dev && screen && screen != DequeueScreen(dev) && !handler) {
> > -        /* Assumption - screen switching can only occur on motion events. */
> > -        DequeueScreen(dev) = screen;
> > -        x = event->device.root_x;
> > -        y = event->device.root_y;
> > -        NewCurrentScreen (dev, DequeueScreen(dev), x, y);
> > -    }
> > -    else {
> > +    switch (event->any.type) {
> > +        /* Catch events that include valuator information and check if they
> > +         * are changing the screen */
> > +        case ET_Motion:
> > +        case ET_KeyPress:
> > +        case ET_KeyRelease:
> > +        case ET_ButtonPress:
> > +        case ET_ButtonRelease:
> > +            if (dev && screen && screen != DequeueScreen(dev) && !handler) {
> > +                DequeueScreen(dev) = screen;
> > +                x = event->device.root_x;
> > +                y = event->device.root_y;
> > +                NewCurrentScreen (dev, DequeueScreen(dev), x, y);
> > +                break;
> > +            }
> 
> please always add a comment when an intentional fallthrough is used in a
> switch statement.

I agree with you Peter. The readability of this chunk (and all
mieqProcessInputEvents) is pretty bad :(


> anyway, having a return instead of a break after the dequeue stuff means we
> wouldn't need the large default block. 

yeah, return there makes more sense. But I guess you meant we indeed need the
default block, but not for catch that kinds of events (ET_Motion, ET_KeyPress,
etc).

 
> This whole thing looks odd though. If the screen switch happens on a button
> or key release event, the event isn't processed and we'd end up with a stuck
> button or key.

agreed.


> I think the return is only supposed to happen on a ET_Motion, with the other
> events falling through to the default case.

What about this patched attached? This fix the cursor warping screens and keep
processing those kind of events (it doesn't return on ET_Motion as you stated).


            Tiago
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mi-fix-cursor-warping-screens.patch
Type: text/x-diff
Size: 0 bytes
Desc: not available
Url : http://lists.x.org/archives/xorg-devel/attachments/20090806/d0156f02/attachment-0001.patch