[PATCH xserver 16/17] Input: Add initial multitouch support from Xi 2.1
Peter Hutterer
peter.hutterer at who-t.net
Thu Jan 6 14:51:31 PST 2011
On Thu, Jan 06, 2011 at 05:57:45PM +0000, Daniel Stone wrote:
> > > /**
> > > + * Ensure a window trace is present in ti->sprite, constructing one for
> > > + * TouchBegin events.
> > > + */
> > > +static Bool
> > > +EnsureTouchSprite(DeviceIntPtr sourcedev, TouchPointInfoPtr ti, DeviceEvent *ev)
> > > +{
> > > + TouchClassPtr t = sourcedev->touch;
> > > + SpritePtr sprite = &ti->sprite;
> > > + int i;
> > > +
> > > + if (ev->type != ET_TouchBegin)
> > > + {
> > > + if (ev->type == ET_TouchMotion && sprite->spriteTraceGood <= 0)
> > > + return FALSE;
> >
> > shouldn't this check for ET_TouchEnd too?
>
> Not really. It's a bit more subtle than it needs to be -
> ProcessTouchEnd _must_ call FinishTouchPoint for TouchEnd events, even
> if there's no sprite. A sprite trace can be empty if nothing has
> selected for it (or the selections have disappeared), and nothing has
> grabbed for it, or the grabs have disappeared, or the grab owners have
> all rejected the touch. So when this happens, we want to throw motion
> events on the floor, but continue processing TouchEnd events.
that sounds like an excellent comment to put in the code...
> > > + for (i = sprite->spriteTraceGood; i >= 0; i--)
> > > + {
> > > + /* First check whether anyone has selected for touch events on this
> > > + * window at all. */
> > > + win = sprite->spriteTrace[i];
> > > + inputMasks = wOtherInputMasks(win);
> > > + if (!inputMasks)
> > > + continue;
> > > + iclients = inputMasks->inputClients;
> > > + if (!iclients)
> > > + continue;
> > > +#define test_xi2_mask(x, d, b, f) ((x)->xi2mask[d][b] & f)
> >
> > this seems like something that's useful in other functions as well.
>
> Do you want it introduced and exported in another event, with all other
> users converted?
event? do you man macro? if so, yes, but see your BitIsOn comment, if that
works.
> > > + /* Now deliver first to grabbing clients, then to clients with an
> > > + * applicable event selection. */
> > > + child = sprite->spriteTrace[sprite->spriteTraceGood - 1]->drawable.id;
> >
> > this cries out for a macro with some nice, comforting, heart-warming name..
>
> GetIDOfDeepestWindowInSpriteTrace()?!
GetDeepestWindow(sprite)->drawable.id would be acceptable I think.
> > > + DeliverTouchGrabEvents(sourcedev, ti, ev, child);
> > > + DeliverTouchSelectionEvents(sourcedev, ti, ev, child);
> >
> > I don't think using "Selection" is a good name choise. everywhere else we
> > either use "mask" or nothing (DeliverGrabbedEvent vs DeliverDeviceEvents)..
> > "Selection" is imo too close to the copy/paste method and every time I read
> > it somewhere it throws me out for a second.
>
> TBH, I still don't think that clears anything up (being that grabs have
> masks too, bitmasks in general which are surely much more applicable to
> event delivery than copy & paste, etc), but I've changed it to
> DeliverTouchMaskEvents anyway.
fwiw, DeliverTouchEvents would be good enough, given that's what the others
use too. but either way is fine.
>
> > > + if (nev == 0)
> > > + return BadAlloc;
> > > + for (i = 0; i < nev; i++)
> > > + {
> > > + ev = (DeviceEvent *)((events + i)->event);
> > > + mieqProcessDeviceEvent(dev, (InternalEvent *) ev, NULL);
> > > + }
> >
> > this could be interesting. you're injecting into the event stream here
> > directly, so there's a race condition where you end up with touch motion
> > events after sending a touch end event. note that when the request arrives,
> > a touch motion event may still be in the EQ, so we need some handling in
> > ProcessTouchEvent to discard those that are XITouchOwnerAccepted when they
> > go through the processing path.
>
> If a TouchEnd event arrives and isn't processed before the
> TouchOwnerAccepted, then I fail to see how that would break anything.
> If the TouchOwnerAccepted arrives after the TouchEnd event, then that's
> also not a problem because of the ti->pending_finish handling which
> doesn't _actually_ finish a touch event on TouchEnd until all grab
> owners have either accepted or rejected the touch.
>
> > > + if (BitIsOn(bits, XI_TouchBegin))
> > > + {
> > > + OtherInputMasks *inputMasks = wOtherInputMasks(win);
> > > + InputClients *iclient = NULL;
> > > + if (inputMasks)
> > > + iclient = inputMasks->inputClients;
> > > + for (; iclient; iclient = iclient->next)
> > > + {
> > > + if (CLIENT_ID(iclient->resource) == client->index)
> > > + continue;
> > > + if (BitIsOn(iclient->xi2mask[evmask->deviceid],
> > > + XI_TouchBegin) ||
> > > + BitIsOn(iclient->xi2mask[XIAllDevices],
> > > + XI_TouchBegin) ||
> > > + (dev && (IsMaster(dev) || dev->u.master) &&
> > > + BitIsOn(iclient->xi2mask[XIAllMasterDevices],
> > > + XI_TouchBegin)))
> > > + return BadAccess;
> > > + }
> > > + }
> > > + }
> > > +
> >
> > please split this out into a helper function. it's self-contained anyway.
>
> Come to think of it, why am I not using BitIsOn rather than the
> test_xi2_mask macro?!
heh, good point.
> > > + /* Touch IDs must increase monotonically.
> > > + * XXX: Deal with this so the drivers don't have to. */
> >
> > change the xf86 api to xf86CreateTouchEvent() for the first (returning the
> > touchid) and then xf86PostTouchEvent() for subsequent calls.
>
> Fair enough, though this does require slightly more active tracking in
> the driver, and I'd rather have more dumb than more smart drivers.
> Hence the comment: if the touchid isn't monotonically increasing, I'd
> prefer to just have a server-internal mapping, rather than drivers
> having to reinvent the same thing over and over.
drivers _must_ have tracking anyway, so they know when a new touchpoint was
created. we could have xf86CreateTouchEvent() that only sets up the
server-internal mapping. so a driver would do
if (new_touch)
xf86CreateTouchEvent(non-monotonic-touch-id)
xf86PostTouchEvent(non-monotonic-touch-id)
which would avoid any touch id tracking other than what the hardware does in
the driver. plus, bonus point, if a driver re-uses a tracking ID but didn't
send the TouchEnd event you can emulate one in xf86CreateTouchEvent() before
you get ID collision.
does this make sense? just thinking aloud here.
> > > +typedef struct _TouchPointInfo {
> > > + Bool active; /* whether or not the touch is active */
> > > + Bool pending_finish; /* true if the touch is physically inactive
> > > + * but still owned by a grab */
> > > + uint32_t id;
> > > + SpriteRec sprite; /* window trace for delivery */
> > > + int *valuators; /* last-recorded valuators */
> >
> > how big is valuators?
> > wouldn't hurt to have a nvaluators field here, or just use a valuator mask?
> > right now, if I just get passed a TPI I wouldn't know the size of the array.
>
> TouchClassRec::num_axes.
i strongly recommend against having the array size in a separate struct, one
that we may at any time not even have a pointer to. just add a size field.
> > > +typedef struct _TouchClassRec {
> > > + TouchAxisInfoPtr axes;
> > > + unsigned short num_axes;
> > > + TouchPointInfoPtr touches;
> > > + unsigned short num_touches;
> > > + CARD8 mode; /* ::XIDirectTouch, XIDependentTouch */
> > > + unsigned int last_touchid;
> > > + int x_axis; /* axis number */
> > > + int y_axis; /* axis number */
> >
> > axis number?? is this to hold the axis index for the x/y axis?
>
> Yeah, it is.
>
> > why not force the implementation to have x/y on 0/1, that's what we do for
> > pointer events too.
>
> *shrug*, it was from Chase's original patch, but to be honest I don't
> really see the harm in being slightly more flexible.
already replied in the other email, leave it like this, thanks. maybe "axis
number of x axis" though. silly as it sounds, it actually read like a
copy/paste error when I looked at this first. the code clarifies it, but the
commend could easily be expanded.
Cheers,
Peter
>
> > I don't see anything fundamentally wrong with the patch and I think my
> > comments are more on style and general cleanups. I guess there will still be
> > a bit of churn on the patch though. and I haven't actually tested it yet
> > either.
>
> Ha. Well, make sure you test the latest stuff out of my branch on
> people.fd.o, which is what's in Chase's PPA, and what the Ubuntu guys
> are testing against.
>
> Cheers,
> Daniel
More information about the xorg-devel
mailing list