[PATCH evdev 3/3] Allow relative scroll valuators on absolute devices
Peter Hutterer
peter.hutterer at who-t.net
Wed Mar 28 22:30:28 PDT 2012
On Wed, Mar 28, 2012 at 08:19:00PM -0700, Chase Douglas wrote:
> On 03/28/2012 05:09 PM, Peter Hutterer wrote:
> > Special-case REL_WHEEL, REL_HWHEEL and REL_DIAL to add scroll valuators
> > for those axes on top of the absolute axes.
> > REL_* and ABS_* overlap, so we need to offset the REL_* axes to avoid
> > overwriting a potential ABS_* mapping. REL_* axes mappings are stored after
> > the last absolute axis detected, for purely relative devices that offset is
> > simply 0.
> >
> > This isn't needed before smooth-scrolling support as REL_WHEEL and friends
> > are special-cased in EvdevProcessRelativeMotionEvent, north of the first
> > hunk in this patch.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=805902
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>
> I think over time we've painted ourselves into a corner with axes in
> evdev. This patch screams *HACK* *HACK* *HACK*, mostly because of having
> one axis array and using a marker for switching between absolute and
> relative axes.
>
> * Would it be possible to use separate axis maps for relative and
> absolute axes?
sure, that makes sense. it's not going to be much prettier though :)
> * Are we going to pay later for having variables like "want_scroll_axes"
> that configure axes like REL_DIAL? The variable is essentially
> controlling a whitelist.
Yes, but not too badly I hope. Changes in scroll axes are few, so I think we
can live with this for now. Given how more and more devices move towards
touch-based interfaces, I doubt we'll see any new axes that are dedicated
towards scrolling in the near future.
> Would we be better off having a default
> blacklist to disable REL_X and REL_Y if ABS_X and ABS_Y are supported,
> and letting everything else through?
evdev has a either rel or absolute axis policy for largely historical
reasons. And the main reason is that I've seen devices that have both,
usually (but not always!) at least one set of axes are partially mute.
This is the main reason for the whole mess in evdev, I'd be glad to get rid
of it.
Current servers _should_ be able to deal with relative events on
absolute axes and vice versa, though this needs more testing and I haven't
found the time to implement this yet.
The next step then is to figure out if clients can deal with it correctly,
having axis ranges for some axes but receiving values outside of it (my
guess is 'yes' since synaptics has been doing this for a while now, but
again this needs testing).
So yeah, this is a hack, but it fixes a serious problem in server 1.12 +
evdev 2.7.0 and the only other solution is quite costly to implement.
> > ---
> > src/evdev.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > src/evdev.h | 3 +-
> > 2 files changed, 99 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/evdev.c b/src/evdev.c
> > index 7848c9d..91e314d 100644
> > --- a/src/evdev.c
> > +++ b/src/evdev.c
> > @@ -864,7 +864,9 @@ EvdevProcessRelativeMotionEvent(InputInfoPtr pInfo, struct input_event *ev)
> > #endif
> > default:
> > /* Ignore EV_REL events if we never set up for them. */
> > - if (!(pEvdev->flags & EVDEV_RELATIVE_EVENTS))
> > + if (!(pEvdev->flags & EVDEV_RELATIVE_EVENTS) &&
> > + ev->code != REL_WHEEL && ev->code != REL_DIAL &&
> > + ev->code != REL_HWHEEL)
> > return;
> >
> > /* Handle mouse wheel emulation */
> > @@ -873,7 +875,7 @@ EvdevProcessRelativeMotionEvent(InputInfoPtr pInfo, struct input_event *ev)
> >
> > pEvdev->rel_queued = 1;
> > pEvdev->delta[ev->code] += value;
> > - map = pEvdev->axis_map[ev->code];
> > + map = pEvdev->axis_map[ev->code + pEvdev->rel_offset];
> > valuator_mask_set(pEvdev->vals, map, value);
> > break;
> > }
> > @@ -1396,7 +1398,7 @@ is_blacklisted_axis(int axis)
> >
> >
> > static int
> > -EvdevAddAbsValuatorClass(DeviceIntPtr device)
> > +EvdevAddAbsValuatorClass(DeviceIntPtr device, int want_scroll_axes)
> > {
> > InputInfoPtr pInfo;
> > EvdevPtr pEvdev;
> > @@ -1444,6 +1446,20 @@ EvdevAddAbsValuatorClass(DeviceIntPtr device)
> > }
> > }
> > #endif
> > +
> > +#ifdef HAVE_SMOOTH_SCROLLING
> > + if (want_scroll_axes && EvdevBitIsSet(pEvdev->bitmask, EV_REL))
> > + {
> > + pEvdev->rel_offset = num_axes;
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_WHEEL))
> > + num_axes++;
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_HWHEEL))
> > + num_axes++;
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_DIAL))
> > + num_axes++;
> > + }
> > +#endif
> > +
> > if (num_axes + num_mt_axes > MAX_VALUATORS) {
> > xf86IDrvMsg(pInfo, X_WARNING, "found %d axes, limiting to %d.\n", num_axes, MAX_VALUATORS);
> > num_axes = MAX_VALUATORS;
> > @@ -1532,6 +1548,33 @@ EvdevAddAbsValuatorClass(DeviceIntPtr device)
> >
> > EvdevInitAxesLabels(pEvdev, Absolute, pEvdev->num_vals + num_mt_axes, atoms);
> >
> > +#ifdef HAVE_SMOOTH_SCROLLING
> > + if (want_scroll_axes)
> > + {
> > + int mapping = pEvdev->rel_offset;
> > + int axis;
> > +
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_HWHEEL))
> > + {
> > + axis = pEvdev->rel_offset + REL_HWHEEL;
> > + pEvdev->axis_map[axis] = mapping++;
> > + EvdevInitOneAxisLabel(pEvdev, axis, rel_labels, REL_HWHEEL, atoms);
> > + }
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_DIAL))
> > + {
> > + axis = pEvdev->rel_offset + REL_DIAL;
> > + pEvdev->axis_map[axis] = mapping++;
> > + EvdevInitOneAxisLabel(pEvdev, axis, rel_labels, REL_DIAL, atoms);
> > + }
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_WHEEL))
> > + {
> > + axis = pEvdev->rel_offset + REL_WHEEL;
> > + pEvdev->axis_map[axis] = mapping++;
> > + EvdevInitOneAxisLabel(pEvdev, axis, rel_labels, REL_WHEEL, atoms);
> > + }
> > + }
> > +#endif
> > +
> > if (!InitValuatorClassDeviceStruct(device, num_axes + num_mt_axes, atoms,
> > GetMotionHistorySize(), Absolute)) {
> > xf86IDrvMsg(pInfo, X_ERROR, "failed to initialize valuator class device.\n");
> > @@ -1623,6 +1666,51 @@ EvdevAddAbsValuatorClass(DeviceIntPtr device)
> > }
> > #endif
> >
> > +#ifdef HAVE_SMOOTH_SCROLLING
> > + if (want_scroll_axes)
> > + {
> > + int axidx;
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_WHEEL))
> > + {
> > + axidx = pEvdev->rel_offset + REL_WHEEL;
>
> What is the meaning of the variable name "axidx"? I think it's supposed
> to mean "axis index", but it was not obvious to me at first glance. I'd
> suggest coming up with a different name. "index" might be good enough
> since it's obvious we're talking about axes here. "axis" is also used
> just above this in the diff without issue.
I'll change it to something more obvious, but yeah, "axis index" was the
intended meaning.
>
> > + xf86InitValuatorAxisStruct(device,
> > + pEvdev->axis_map[axidx],
> > + atoms[pEvdev->axis_map[axidx]],
> > + NO_AXIS_LIMITS, NO_AXIS_LIMITS,
> > + 0, 0, 0, Relative);
> > + SetScrollValuator(device, pEvdev->axis_map[axidx],
> > + SCROLL_TYPE_VERTICAL, -1.0,
> > + SCROLL_FLAG_PREFERRED);
> > + }
> > +
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_HWHEEL))
> > + {
> > + axidx = pEvdev->rel_offset + REL_HWHEEL;
> > + xf86InitValuatorAxisStruct(device,
> > + pEvdev->axis_map[axidx],
> > + atoms[pEvdev->axis_map[axidx]],
> > + NO_AXIS_LIMITS, NO_AXIS_LIMITS,
> > + 0, 0, 0, Relative);
> > + SetScrollValuator(device, pEvdev->axis_map[axidx],
> > + SCROLL_TYPE_HORIZONTAL, 1.0,
> > + SCROLL_FLAG_NONE);
> > + }
> > +
> > + if (EvdevBitIsSet(pEvdev->rel_bitmask, REL_DIAL))
> > + {
> > + axidx = pEvdev->rel_offset + REL_DIAL;
> > + xf86InitValuatorAxisStruct(device,
> > + pEvdev->axis_map[axidx],
> > + atoms[pEvdev->axis_map[axidx]],
> > + NO_AXIS_LIMITS, NO_AXIS_LIMITS,
> > + 0, 0, 0, Relative);
> > + SetScrollValuator(device, pEvdev->axis_map[axidx],
> > + SCROLL_TYPE_HORIZONTAL, 1.0,
> > + SCROLL_FLAG_NONE);
> > + }
> > + }
> > +#endif
> > +
> > free(atoms);
> >
> > for (i = 0; i < ArrayLength(proximity_bits); i++)
> > @@ -1852,12 +1940,16 @@ static void
> > EvdevInitAnyValuators(DeviceIntPtr device, EvdevPtr pEvdev)
> > {
> > InputInfoPtr pInfo = device->public.devicePrivate;
> > + int rel_success = FALSE;
> >
> > if (pEvdev->flags & EVDEV_RELATIVE_EVENTS &&
> > EvdevAddRelValuatorClass(device) == Success)
> > + {
> > + rel_success = TRUE;
>
> The variable "rel_success" doesn't carry much connotation by itself. I
> think "has_rel_axes" would be better.
I'll think of something, thanks.
has_rel_axes is used elsewhere with a different meaning and I don't want to
overload it either. this one here is really "relative axes successfully
initialised?"
Cheers,
Peter
> > xf86IDrvMsg(pInfo, X_INFO, "initialized for relative axes.\n");
> > + }
> > if (pEvdev->flags & EVDEV_ABSOLUTE_EVENTS &&
> > - EvdevAddAbsValuatorClass(device) == Success)
> > + EvdevAddAbsValuatorClass(device, !rel_success) == Success)
> > xf86IDrvMsg(pInfo, X_INFO, "initialized for absolute axes.\n");
> > }
> >
> > @@ -1866,7 +1958,7 @@ EvdevInitAbsValuators(DeviceIntPtr device, EvdevPtr pEvdev)
> > {
> > InputInfoPtr pInfo = device->public.devicePrivate;
> >
> > - if (EvdevAddAbsValuatorClass(device) == Success) {
> > + if (EvdevAddAbsValuatorClass(device, TRUE) == Success) {
> > xf86IDrvMsg(pInfo, X_INFO,"initialized for absolute axes.\n");
> > } else {
> > xf86IDrvMsg(pInfo, X_ERROR,"failed to initialize for absolute axes.\n");
> > diff --git a/src/evdev.h b/src/evdev.h
> > index 309b215..cca5cbe 100644
> > --- a/src/evdev.h
> > +++ b/src/evdev.h
> > @@ -153,7 +153,8 @@ typedef struct {
> > int grabDevice; /* grab the event device? */
> >
> > int num_vals; /* number of valuators */
> > - int axis_map[max(ABS_CNT, REL_CNT)]; /* Map evdev <axis> to index */
> > + int axis_map[ABS_CNT + REL_CNT]; /* Map evdev <axis> to index */
> > + int rel_offset; /* offset for relative axes in absolute device */
> > ValuatorMask *vals; /* new values coming in */
> > ValuatorMask *old_vals; /* old values for calculating relative motion */
> > ValuatorMask *prox; /* last values set while not in proximity */
>
More information about the xorg-devel
mailing list