[PATCH xserver 16/17] Input: Add initial multitouch support from Xi 2.1

[Daniel Stone]

Hi,

On Thu, Jan 06, 2011 at 02:30:07PM -0500, Chase Douglas wrote:
> Everything looks pretty good! I made a few comments, none of which are
> major.

Cool. :)

> Two things:
> 
> 1. The protocol document implies that the server generates pointer
> events from touch events. I didn't see any of that here (or maybe I
> misread the protocol?)

I haven't implemented it in the server yet because I'm not sure how the
semantics should look.  This isn't something we can really do without a
workable client stack, so I'm happy to wait until you guys have
something usable that we can at least test against to make sure we're
not implementing something completely useless.

> 2. This is missing the previously discussed ability to request that the
> server buffer touch events until the client becomes the owner of the
> events. My proposal is to rename XIAllowTouchEvents to
> XIModifyTouchEventDelivery, and have it handle touch allowance and
> setting propagation flags like the buffering request. I would suggest
> that we merely rename the call in this patch and add a patch on top to
> implement the buffering.

Ah yes, so it is; this somehow failed to make it to my TODO.  When you
say buffering events though - my recollection was that we were going to
add a flag for clients to specify that they didn't care about
intermediate states, i.e. didn't want to receive any events for
non-owned touches.  Else we're going to have to add an arbitrarily-large
touch buffer if even one client requests buffered events, which will
require a roundtrip to recieve, ugh.

Also, as discussed on IRC, I'm not entirely sure how to implement this
without adding an extra XI_TouchMotionButOnlyIfIOwnTheTouch event type;
I guess we could use the 16-bit padding in both the XISelectEvents and
XIPassiveGrabDevice requests as flags with a TouchOnlyIfOwned flag.
Peter?

> We're almost there!

:)

> > +#define test_xi2_mask(x, d, b, f) ((x)->xi2mask[d][b] & f)
> 
> I like defining macros above a function and undefining them after the
> end of the function. It makes it easier to read the code, imo. A static
> inline function would be even better for type checking.

We can just use BitIsOn here, so it should be OK.

> > +            if (ev->type != ET_TouchBegin) {
> > +                for (j = 0; j < ti->num_listeners; j++)
> > +                {
> > +                    if (ti->listeners[j] == iclients->resource)
> > +                        break;
> > +                }
> > +                if (j == ti->num_listeners)
> > +                {
> > +                    free(xi2);
> > +                    continue;
> > +                }
> > +            }
> 
> Don't we just need to check:
> 
> if (to->listeners[ti->num_listeners - 1] == iclients->resource)
> 
> instead of looping over all the resources? Essentially, isn't the
> selection client always the last client in the list since
> DeliverTouchSelectionEvents is called after DeliverTouchGrabEvents?

Yeah, good catch. :) I'm probably just going to factor it out into a
common function though.

> > +
> > +            if (XaceHook(XACE_RECEIVE_ACCESS, rClient(iclients), win,
> > +                         (xEvent *) xi2, 1) != Success)
> > +                continue;
> > +            deliveries = TryClientEvents(rClient(iclients), deliverdev, xi2, 1,
> > +                                         filter, filter, NullGrab);
> > +            /* As only one event selection can receive events, bail as soon as
> > +             * we find one. */
> > +            if (ev->type == ET_TouchBegin && deliveries > 0)
> > +            {
> > +                ti->listeners[ti->num_listeners++] = iclients->resource;
> > +                free(xi2);
> > +                return;
> > +            }
> > +            else if (ev->type != ET_TouchBegin)
> > +            {
> > +                free(xi2);
> > +                return;
> > +            }
> > +        }
> > +    }
> > +}
> 
> The delivery portion (from my last comment to here) is the same as for
> the grab delivery function. Should it be split out?

Yep.

> > +    if (stuff->mode & XITouchOwnerAccept)
> > +    {
> > +        ProcessInputEvents();
> 
> Shouldn't this ^^ be after we've validated the flags below, as is done
> for the reject case?

Yeah, it should, since we want to honour the accept request as quickly
as possible, rather than flush out events which are guaranteed to have
been generated after the one which made the client decide to receive the
event.

I'm sure I had a reason for running PIE first here specifically, but
can't for the life of me remember what it was.

> > +            /* Only one client per window may select for touch events on the
> > +             * same devices, including master devices.
> > +             * XXX: This breaks if a device goes from floating to attached. */
> 
> Maybe we should prohibit XIAllMasterDevices and any slave device
> selection at the same time, just as is done for XIAllDevices?

That would prevent anyone from ever receiving touch events for a
floating direct touch device if someone had an XIAllMasterDevices grab.
I've been thinking about just allowing both grabs and selections on
different devices (e.g. allowing them on all of the SD, the MD, all MDs,
and all devices), and using the most specific one for delivery.  So,
first check a grab on the specific SD, then the specific MD, then all
MDs, then all devices.

At the moment, I can't think of how to manage the floating -> attached
corner case in that comment (i.e. when you have a grab on a floating
device which later becomes attached to an MD that also has a touch grab,
or if there's an AllMasterDevices touch grab), so I'm inclined to not
pretend we can always ensure exclusive access.

> > +                ProcessInputEvents();
> > +                nev = GetTouchEvents(events, dev, ti->id, mask, 0, 0);
> > +                for (j = 0; j < nev; j++)
> > +                {
> > +                    ev = (InternalEvent *)((events + j)->event);
> > +                    mieqProcessDeviceEvent(dev, ev, NULL);
> > +                }
> 
> Shouldn't we only be sending ownership change events if the owner (the
> first in the listeners array) has its grab removed?

Yep! Good catch - will fix it, thanks.

> Also, we should be adding something to the path for when a client goes
> away to remove its resource from the listeners.

When a client goes away, all its resources (which include both passive
grabs and selections) are deleted.  So DeleteGrab/DeleteTouchGrab will
fix this up for grabs, but InputClientGone needs to handle this for
selections.  I'll fix that.

I'll wait till your patchset comes through and then make all my review
changes on top of those, and post another set of patches, with
changelogs this time.  Peter's already merged everything up to and
including the CheckPassiveGrabsOnWindow commit into his tree, so I'll
just post the ones left on top of that.

Thanks again!

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/82dffd8b/attachment-0001.pgp>