[PATCH xserver 16/17] Input: Add initial multitouch support from Xi 2.1
Daniel Stone
daniel at fooishbar.org
Fri Jan 7 07:40:55 PST 2011
Hi,
On Fri, Jan 07, 2011 at 08:51:31AM +1000, Peter Hutterer wrote:
> On Thu, Jan 06, 2011 at 05:57:45PM +0000, Daniel Stone wrote:
> > > > +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...
Yep, fair enough.
> > > > +#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.
I do, and yeah, BitIsOn would work fine.
> > > > + /* 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.
OK, I'll introduce that in an earlier patch.
> > > > + 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.
OK, I'll merge the both into DeliverTouchEvents - some minor refactoring
should make it small enough to be tractable.
> > > 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.
Yep, that sounds fine to me, but if a driver tries to reuse a touch ID,
I'd rather scream in the log about a broken driver. :)
> > > > +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.
Fair enough.
> > > > + 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.
Sure.
Thanks again for the review!
Cheers,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110107/f80b57cb/attachment.pgp>
More information about the xorg-devel
mailing list