[PATCH v2] Input: Add smooth-scrolling support

[Peter Hutterer]

On Wed, Sep 28, 2011 at 01:30:58AM +0100, Daniel Stone wrote:
> Hi,
> 
> On 23 September 2011 02:22, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> > +    if (*current_ax != -1 && axnum != *current_ax)
> > +    {
> > +        ax = &dev->valuator->axes[*current_ax];
> > +        if (ax->scroll.type == type &&
> > +            (flags & SCROLL_FLAG_PREFERRED) == (ax->scroll.flags & SCROLL_FLAG_PREFERRED))
> > +            return FALSE;
> > +    }
> 
> This will fail if you try to add two non-preferred axes; did you mean
> && instead of ==?

amended
> 
> > +    info->increment.integral = (int)axis->scroll.increment;
> > +    info->increment.frac = (unsigned int)(axis->scroll.increment * (1UL << 32));
> > +    info->sourceid = v->sourceid;
> 
> This needs the same * (1UL << 16) * (1UL << 16) treatment, I believe.

amended
> 
> > @@ -603,8 +604,10 @@ GetMaximumEventsNum(void) {
> >     /* One raw event
> >      * One device event
> >      * One possible device changed event
> > +     * Lots of possible separate button scroll events (horiz + vert)
> > +     * Lots of possible separate raw button scroll events (horiz + vert)
> >      */
> > -    return 3;
> > +    return 51;
> >  }
> 
> Hmm, I've seen up to 100 fairly easily.

fwiw, this is still from your original patch. I've amended to 100 now.

> > +static int
> > +emulate_scroll_button_events(InternalEvent *events,
> > +                             DeviceIntPtr dev,
> > +                             int axis,
> > +                             const ValuatorMask *mask,
> > +                             ValuatorMask *last,
> > +                             CARD32 ms,
> > +                             int max_events)
> > +{
> > +    AxisInfoPtr ax;
> > +    double delta;
> > +    double incr;
> > +    int num_events = 0;
> > +    double total;
> > +    int b;
> > +
> > +    if (dev->valuator->axes[axis].scroll.type == SCROLL_TYPE_NONE)
> > +        return 0;
> > +
> > +    if (!valuator_mask_isset(mask, axis))
> > +        return 0;
> > +
> > +    ax = &dev->valuator->axes[axis];
> > +    incr = ax->scroll.increment;
> > +
> > +    if (!valuator_mask_isset(last, axis))
> > +    {
> > +        valuator_mask_set_double(last, axis, valuator_mask_get_double(mask, axis));
> > +        return 0;
> > +    }
> > +
> > +    delta = valuator_mask_get_double(mask, axis) - valuator_mask_get_double(last, axis);
> 
> I think this really needs to be:
> delta = valuator_mask_get_double(mask, axis);
> if (valuator_mask_isset(last, axis))
>     delta -= valuator_mask_get_double(last, axis);
> 
> On the grounds that a single event with a scroll axis with an incr of
> 1.0 and a value of 10.0 should generate 10 scroll button events.  But
> with this, we won't generate anything until the second event comes in,
> no?

By the time we get here, we're in absolute coordinates. So what we need to
do is 

if (!valuator_mask_isset(last, axis))
    valuator_mask_set(last, axis, 0);

This way, the very first event on this axis is handled as you described and
for any future events we're fine.

> > @@ -1193,7 +1288,12 @@ GetPointerEvents(InternalEvent *events, DeviceIntPtr pDev, int type,
> >                  int buttons, int flags, const ValuatorMask *mask_in)
> >  {
> >     CARD32 ms = GetTimeInMillis();
> > -    int num_events = 0;
> > +    int num_events = 0, nev_tmp;
> > +    int h_scroll_axis = pDev->valuator->h_scroll_axis;
> > +    int v_scroll_axis = pDev->valuator->v_scroll_axis;
> > +    ValuatorMask mask;
> > +    ValuatorMask scroll;
> > +    int i;
> > [...]
> > +
> > +    /* Now turn the smooth-scrolling axes back into emulated button presses
> > +     * for legacy clients, based on the integer delta between before and now */
> > +    for (i = 0; i < valuator_mask_size(&mask); i++) {
> > +        if (!valuator_mask_isset(&mask, i))
> > +            continue;
> > +
> > +        valuator_mask_set_double(&scroll, i, pDev->last.valuators[i]);
> 
> Don't you need a valuator_mask_zero(&scroll) before using it?

done.

> 
> > +        /* fill_pointer_events() generates four events: one normal and one raw
> > +         * event each for the emulated button press and release both. */
> > +        if (num_events + 4 >= GetMaximumEventsNum())
> > +            break;
> > +
> > +        nev_tmp = emulate_scroll_button_events(events, pDev, i, &scroll,
> > +                                               pDev->last.scroll, ms,
> > +                                               GetMaximumEventsNum() - num_events);
> 
> So with this comment, you should probably either
> s/fill_pointer_events/emulate_scroll_button_events/, or just bin the
> comment and the conditional as well, and have
> emulate_scroll_button_events catch max_events <= 0 and just do
> nothing.

removed the condition because I think strictly speaking we may end up with
some weird delta. So better to loop one time to many (but not generate any
events).
 
> (I think the comment in emulate_scroll_button_events about
> fill_pointer_events wants revisiting too, strictly speaking.)
> 
> Other than that:
> Reviewed-by: Daniel Stone <daniel at fooishbar.org>

thanks. follow-up patch in a minute

Cheers,
  Peter