On Wed, Sep 28, 2011 at 01:30:58AM +0100, Daniel Stone wrote: > Hi, > > On 23 September 2011 02:22, Peter Hutterer <peter.hutte...@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 <dan...@fooishbar.org> thanks. follow-up patch in a minute Cheers, Peter _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel