[PATCH xserver 08/10] Input: Add initial multitouch support from Xi 2.1
Daniel Stone
daniel at fooishbar.org
Wed Dec 22 03:56:30 PST 2010
Hi,
On Wed, Dec 22, 2010 at 03:10:07PM +1000, Peter Hutterer wrote:
> On Fri, Dec 17, 2010 at 05:13:33PM +0000, Daniel Stone wrote:
> > /**
> > + * Processes and delivers a TouchBegin, TouchMotion or a TouchEnd event.
> > + *
> > + * Due to having rather different delivery semantics (see the Xi 2.1 protocol
> > + * spec for more information), this implements its own grab and selection
> > + * delivery logic.
> > + */
> > +static void
> > +ProcessTouchEvent(DeviceEvent *ev, DeviceIntPtr sourcedev)
> > +{
> > + TouchClassPtr t;
> > + TouchPointInfoPtr ti;
> > + DeviceIntPtr masterdev = NULL, deliverdev = NULL;
> > + Window child;
> > + WindowPtr win;
> > + SpritePtr sprite;
> > + xXIDeviceEvent *xi2;
> > + Mask filter;
> > + int err, touch, i, j, deliveries;
> > +
> > + /* We handle deliveries to MDs through the SD, rather than copying
> > + * the event and processing it twice. */
>
> no. if the current hierarchy event processing isn't good enough, fix it but
> don't shortcut it like this.
Well, we can only have a single delivery tree for each touchpoint: if we
deliver once through the SD and once through the MD, we run the risk of
a single touch being delivered in parallel to two sets of clients (e.g.
one tree which has grabs on the device ID, and another which has grabs
on XIAllMasterDevices), or at best, delivering the same touch twice to
the same clients.
The if (!dev->touch) test will reject MDs anyway, and we could put in a
test to ignore TouchBegins if they already have a delivery tree, but I
don't see how that's any different. So I thought it'd be better to make
it explicit up top.
Do you have any suggestions here? I'm taking the 'touch must only be
delivered once' thing as axiomatic, so ...
> > + if (ev->sourceid != ev->deviceid)
> > + return;
> > +
> > + if (sourcedev->u.master)
> > + masterdev = sourcedev->u.master;
>
> needs an IsMaster() check. u.master is also used as u.lastSlave for master
> devices. the sourceid != deviceid check will eliminate master devices, but
> the IsMaster() check is still the explicit preferred check.
Fair.
> > + else
> > + {
> > + WindowPtr *trace;
> > + SpritePtr srcsprite;
> > +
>
> please add a comment what this loop and the other conditions do.
Sure. The window construction probably wants to be a separate function
anyway, so I'll break it out.
> > + /* Mark which grabs/selections we're delivering to: max one grab per
> > + * window plus the bottom-most selection. */
>
> s/selection/event mask/ or event selection maybe? selection has multiple
> meanings in X.
OK, I'll go with event selection.
> > + /* First search the stack going from root to child looking for grabs,
> > + * delivering to every grab we find ... */
> > + ti->num_grabs = 0;
> > + for (i = 0; i < sprite->spriteTraceGood; i++)
> > + {
> > + GrabPtr grab;
> > + enum EventType saved_evtype = ev->type;
> > +
> > + win = sprite->spriteTrace[i];
> > +
> > + /* Grabs can only be established on one type, so fool
> > + * CheckPassiveGrabsOnWindow into thinking that it's a TouchBegin. */
>
> no. fix CheckPassiveGrabsOnWindow that it works with other touch events too.
So something like this in CheckPassiveGrabsOnWindow:
if (ev->evtype == ET_TouchMotion || ev->evtype == ET_TouchEnd)
evtype = ET_TouchBegin;
else
evtype = ev->evtype;
?
The other option is to look up the saved resource IDs for TouchMotion
and TouchEnd, but that involves verifying that the grabs haven't
changed, etc, etc. I'm happy to do that if you think that's the best
way though.
> > + ev->type = ET_TouchBegin;
> > + deliverdev = sourcedev;
> > + grab = CheckPassiveGrabsOnWindow(win, deliverdev, ev, FALSE, FALSE);
> > + if (!grab && masterdev)
> > + {
> > + deliverdev = masterdev;
> > + grab = CheckPassiveGrabsOnWindow(win, deliverdev, ev, FALSE, FALSE);
> > + }
> > + ev->type = saved_evtype;
> > + if (!grab)
> > + continue;
> > + xi2->deviceid = deliverdev->id;
>
> I really don't like calling EventToXI2() above and then messing with the
> fields manually down here. the main reason we have internal and protocol
> events is that we can just pass the internal event to the conversion func
> and not worry about the details.
I don't particularly like it either, but given that the flags need to
change during delivery - should we be calling EventToXI2() again? If so,
we need to make sure that it's completely deterministic.
> > + /* Stash the list of currently-active listeners away at TouchBegin time,
> > + * then subsequently check against that list to make sure we only
> > + * deliver to that same list later on, so we don't deliver TouchMotion
> > + * events to clients which have never seen the corresponding
> > + * TouchBegin. */
> > + if (ev->type != ET_TouchBegin)
> > + {
> > + for (j = 0; j < ti->num_listeners; j++)
> > + {
> > + if (ti->listeners[j] == grab->resource)
> > + break;
> > + }
> > + if (j == ti->num_listeners)
> > + continue;
> > + }
>
> I don't quite understand the need for this. Once the sprite trace is
> established, anything in listeners should be ok to deliver to, right?
> if so, we don't need this for loop and can just run it on TouchBegin.
Sure, but grabs can (and will) change at any time, so if one client
ungrabs and then another client regrabs mid-stream,
CheckPassiveGrabsOnWindow will hand us back a grab for a client which
has never seen that TouchBegin.
> > + if (XaceHook(XACE_RECEIVE_ACCESS, rClient(grab), win,
> > + (xEvent *) xi2, 1) != Success)
> > + continue;
> > + FixUpEventFromWindow(sprite, (xEvent *) xi2, win, child, FALSE);
> > + deliveries = TryClientEvents(rClient(grab), deliverdev,
> > + (xEvent *) xi2, 1, filter, filter,
> > + NullGrab);
> > + if (deliveries > 0)
> > + {
> > + ti->num_grabs++;
> > + xi2->flags &= ~XITouchOwner;
> > + if (ev->type == ET_TouchBegin)
> > + ti->listeners[ti->num_listeners++] = grab->resource;
> > + }
> > + }
>
> here would be an awesome spot for deciding that we need to move into another
> function...
Fair.
> > +int
> > +ProcXIAllowTouchEvents(ClientPtr client)
> > +{
> > + DeviceIntPtr dev;
> > + TouchPointInfoPtr ti;
> > + int ret, touch, i, nev;
> > + ValuatorMask *mask = valuator_mask_new(0);
> > + EventList *events = InitEventList(GetMaximumEventsNum());
>
> size of 1 seems to be sufficient? unless you call GPE from GetTouchEvent(),
> but in this case you need to bump the value returned by
> GetMaximumEventsNum().
Yeah, it was done like that to allow for future pointer emulation. Mind
you, it would be even better if GetTouchEvent/GetPointerEvents and
friends took an 'enqueue or process now' parameter, and we didn't have
to do the whole event list/for loop/process events dance in every
caller ...
> > + DeviceEvent *ev;
> > +
> > + REQUEST(xXIAllowTouchEventsReq);
> > + REQUEST_SIZE_MATCH(xXIAllowTouchEventsReq);
> > +
> > + if (!mask || !events)
> > + return BadAlloc;
> > +
> > + ret = dixLookupDevice(&dev, stuff->deviceid, client, DixGetAttrAccess);
> > + if (ret != Success)
> > + return ret;
>
> indentation
Oops.
> > + /* Only one client per window may select for touch events on the
> > + * same devices, including master devices.
> > + * XXX: This breaks if a client goes from floating to attached. */
>
> s/client/device?
Er, yeah.
> > - if (wasRealized && !fromConfigure)
> > + if (wasRealized && !fromConfigure) {
> > WindowsRestructured ();
> > + WindowGone(pWin);
>
> indentation
>
> > + }
> > return Success;
> > }
> >
> > @@ -2953,8 +2956,10 @@ UnmapSubwindows(WindowPtr pWin)
> > if (anyMarked && pScreen->PostValidateTree)
> > (*pScreen->PostValidateTree)(pLayerWin->parent, pHead, VTUnmap);
> > }
> > - if (wasRealized)
> > + if (wasRealized) {
> > WindowsRestructured ();
> > + WindowGone(pWin);
>
> indentation
Guh.
> > @@ -90,6 +93,7 @@ struct _DeviceEvent
> > union {
> > uint32_t button; /**< Button number */
> > uint32_t key; /**< Key code */
> > + uint32_t touch; /**< Touch ID */
> > } detail;
> > int16_t root_x; /**< Pos relative to root window in integral data */
> > float root_x_frac; /**< Pos relative to root window in frac part */
> > @@ -117,6 +121,7 @@ struct _DeviceEvent
> > Window root; /**< Root window of the event */
> > int corestate; /**< Core key/button state BEFORE the event */
> > int key_repeat; /**< Internally-generated key repeat event */
> > + uint32_t flags; /**< Flags to pass into the generated event */
> > };
> >
>
> this and the matching hunk to eventToDeviceEvent() should be a separate
> patch. we could merge this now.
Yep, will do.
> > + */
> > + ScreenPtr pEnqueueScreen;
> > + ScreenPtr pDequeueScreen;
> > +
> > +} SpriteRec;
> > +
> > typedef struct _KeyClassRec {
> > int sourceid;
> > CARD8 down[DOWN_LENGTH];
>
> please split this hunk into a separate commit
Sure.
> > @@ -243,6 +286,36 @@ typedef struct _ValuatorClassRec {
> > ValuatorAccelerationRec accelScheme;
> > } ValuatorClassRec, *ValuatorClassPtr;
> >
> > +typedef struct _TouchPointInfo {
> > + Bool active;
> > + Bool pending_finish;
> > + uint32_t id;
> > + SpriteRec sprite;
> > + int *valuators;
> > + XID *listeners;
> > + int num_listeners;
> > + int num_grabs;
> > + Bool emulate_pointer;
> > +} TouchPointInfoRec, *TouchPointInfoPtr;
> > +
> please add short comments to all fields. at least pending_finish, valuators
> and liteners are not self-explanatory enough.
Yep.
> I admit that towards the end my concentration let off so I've probably
> missed some things there. splitting the functions into more bite-sized ones
> would be much appreciated. a 200 line ProcessTouchEvent() that does
> everything from generating sprite traces to delivering events is a bit
> heavy.
Yeah, it's a tad rough. :) I'll break sprite construction, grab
delivery, and event-selection delivery out into separate events, which
should make it a great deal more tractable.
> some test cases for xi2-protocol swapping would be appreciated (as
> follow-up, this patch is long enough as it is). I'd also like to see some
> test cases for all the CreateTouch() and friends so we know when we
> accidentally break it in the future.
Sure. Most of the fancy footwork is in PTE though, as you saw, so
unfortunately it'll be pretty hard to get good test coverage.
Thanks again for the detailed review - will try to get another patchset
out soon. Any other comments welcome!
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/20101222/fa8251f3/attachment.pgp>
More information about the xorg-devel
mailing list