[PATCH 27/27] Input: Add smooth-scrolling support to GetPointerEvents

[Daniel Stone]

Hi,

On Tue, Jun 07, 2011 at 04:29:45PM +1000, Peter Hutterer wrote:
> On Fri, Jun 03, 2011 at 04:00:03PM +0100, Daniel Stone wrote:
> > @@ -60,11 +61,13 @@
> >  #include <X11/extensions/XI.h>
> >  #include <X11/extensions/XI2.h>
> >  #include <X11/extensions/XIproto.h>
> > +#include <X11/extensions/XI2.h>
> 
> does it scroll smoother if you include it twice? :)

Chuckle.  Monday rebase really.

> > +
> > +    valuator_mask_copy(&mask, mask_in);
> > +
> > +    /* Find the vertical and horizontal scroll axes, if any. */
> > +    for (i = 0; i < pDev->valuator->numAxes; i++)
> > +    {
> > +        if (h_scroll_label && pDev->valuator->axes[i].label == h_scroll_label)
> > +            h_scroll_axis = i;
> > +        else if (v_scroll_label &&
> > +                 pDev->valuator->axes[i].label == v_scroll_label)
> > +            v_scroll_axis = i;
> > +    }
> 
> this may be better cached in the DeviceIntRec instead of recalculated on
> every single pointer event.

Done - I was worried about extra DeviceIntRec usage, but eh.

> > +
> > +    /* Turn a scroll button press into a smooth-scrolling event. */
> > +    if (type == ButtonPress &&
> > +        ((v_scroll_axis != -1 && (buttons == 4 || buttons == 5)) ||
> > +         (h_scroll_axis != -1 && (buttons == 6 || buttons == 7))))
> > +    {
> > +        double val;
> > +
> > +        type = MotionNotify;
> > +        buttons = 0;
> > +        if (buttons == 4)
> 
> how about switch(buttons) {...}

Sure.  And, while I was at it, I also moved the buttons = 0 assignment
below the test for buttons. :P

> > +        if (v_scroll_axis != -1 && pDev->last.valuators[v_scroll_axis] <= -1.0)
> > +        {
> > +            pDev->last.valuators[v_scroll_axis] += 1.0;
> > +            b = 4;
> > +        }
> > +        else if (v_scroll_axis != -1 &&
> > +                 pDev->last.valuators[v_scroll_axis] >= 1.0)
> > +        {
> > +            pDev->last.valuators[v_scroll_axis] -= 1.0;
> > +            b = 5;
> > +        }
> > +        else if (h_scroll_axis != -1 &&
> > +                 pDev->last.valuators[h_scroll_axis] <= -1.0)
> > +        {
> > +            pDev->last.valuators[h_scroll_axis] += 1.0;
> > +            b = 6;
> > +        }
> > +        else if (h_scroll_axis != -1 &&
> > +                 pDev->last.valuators[h_scroll_axis] >= 1.0)
> > +        {
> > +            pDev->last.valuators[h_scroll_axis] -= 1.0;
> > +            b = 7;
> > +        }
> 
> I'd prefer it nested as
>     if (v_scroll_axis != 1)
>     {
>             if (last.valuators < -1)
>             else if (...)
>     } else if (h_scroll_axis != 1)
>     {
>     }

Well, it can't be an else if because otherwise you completely lose
horizontal scrolling if you have a vertical axis set up, but I have
nested them now.

> Also, I'd rather you move this whole while block into a new function.

Sure.

> a bit of a side-issue with this approach is that the scroll events lose
> valuator information. we hit that one with the wacom driver where one of my
> patches started submitting button events with first/num_valuators as 0/0. it
> is then impossible for the client to get valuator information without
> tracking motion events (which may be quite costly and I'm not sure all
> clients do).
> 
> so we went back to submitting the current valuator state with _every_ button
> event. This patch suffers from the same issue. If you scroll, there is no
> information about the other axes. Which for relative devices doesn't matter
> much but for absolute devices it certainly does.

Hmph, so is that just a matter of dropping the current x and y in the
mask then?

Cheers,
Daniel