[PATCH v2] Input: Add smooth-scrolling support

[Daniel Stone]

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 ==?

> +    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.

> @@ -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.

> +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?

> @@ -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?

> +        /* 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.

(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>

Cheers,
Daniel