[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