[PATCH 20/42] dix: handle DIX-submitted touch events

Chase Douglas chase.douglas at canonical.com
Tue Dec 20 17:19:15 PST 2011


On 12/20/2011 05:14 PM, Peter Hutterer wrote:
> On Mon, Dec 19, 2011 at 06:33:06PM -0800, Chase Douglas wrote:
>> On Dec 14, 2011, at 7:01 PM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
>>> The DIX needs to submit touch events for e.g. TouchEnd after an
>>> acceptance/rejection. These have the TOUCH_CLIENT_ID flag set.
>>>
>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>> ---
>>> dix/getevents.c |   77 ++++++++++++++++++++++++++++++++++++++++--------------
>>> 1 files changed, 57 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/dix/getevents.c b/dix/getevents.c
>>> index 4038309..819880c 100644
>>> --- a/dix/getevents.c
>>> +++ b/dix/getevents.c
>>> @@ -1703,7 +1703,10 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
>>>     int i;
>>>     int num_events = 0;
>>>     RawDeviceEvent *raw;
>>> -    DDXTouchPointInfoPtr ti;
>>> +    union touch {
>>> +        TouchPointInfoPtr dix_ti;
>>> +        DDXTouchPointInfoPtr ti;
>>> +    } touchpoint;
>>>     int need_rawevent = TRUE;
>>>     Bool emulate_pointer = FALSE;
>>>
>>> @@ -1711,15 +1714,39 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
>>>         return 0;
>>>
>>>     /* Find and/or create the DDX touch info */
>>> -    ti = TouchFindByDDXID(dev, ddx_touchid, (type == XI_TouchBegin));
>>> -    if (!ti)
>>> +
>>> +    if (flags & TOUCH_CLIENT_ID) /* A DIX-submitted TouchEnd */
>>>     {
>>> -        ErrorF("[dix] %s: unable to %s touch point %x\n", dev->name,
>>> -                type == XI_TouchBegin ? "begin" : "find", ddx_touchid);
>>> -        return 0;
>>> +        touchpoint.dix_ti = TouchFindByClientID(dev, ddx_touchid);
>>> +        BUG_WARN(!touchpoint.dix_ti);
>>> +
>>> +        if (!touchpoint.dix_ti)
>>> +            return 0;
>>> +
>>> +        if (!mask_in ||
>>> +            !valuator_mask_isset(mask_in, 0) ||
>>> +            !valuator_mask_isset(mask_in, 1))
>>> +        {
>>> +            ErrorF("[dix] dix-submitted events must have x/y valuator information.\n");
>>> +            return 0;
>>> +        }
>>
>> I don't see why this is required. We're also inconsistent because we
>> require x and y, but we don't copy any other valuators later on in this
>> function for dix submitted events. If x and y are required, why not copy
>> everything?
> 
> we mainly need x/y for sprite positioning and valuator scaling.
> (abs events should have valuator information in any case, some (older)
> clients can't easily deal with them) It's either requiring the DIX to submit
> this information or adding hacks here to get the right data out. 
> There's a bit of history as well, when I added the requirement for x/y on
> emulated TouchEnd events, GetTouchEvents didn't have access to 
> touchpoint.dix_ti->valuators yet. We can clean this up, but then we also
> need the code to alloc/free mask_in here on demand. in short, we're mostly
> just moving the code around and this is stuff I'd like to clean up later
> when we got the features in and we're stabilising and polishing.
>>
>>> +
>>> +        need_rawevent = FALSE;
>>
>> Ahh, the answer to my question from patch 19 :).
>>
>>> +    } else /* a DDX-submitted touch */
>>> +    {
>>> +        touchpoint.ti = TouchFindByDDXID(dev, ddx_touchid, (type == XI_TouchBegin));
>>> +        if (!touchpoint.ti)
>>> +        {
>>> +            ErrorF("[dix] %s: unable to %s touch point %x\n", dev->name,
>>> +                    type == XI_TouchBegin ? "begin" : "find", ddx_touchid);
>>> +            return 0;
>>> +        }
>>>     }
>>>
>>> -    emulate_pointer =  ti->emulate_pointer;
>>> +    if (!(flags & TOUCH_CLIENT_ID))
>>> +        emulate_pointer =  touchpoint.ti->emulate_pointer;
>>> +    else
>>> +        emulate_pointer = !!(flags & TOUCH_POINTER_EMULATED);
>>>
>>>    if (!IsMaster(dev))
>>>         events = UpdateFromMaster(events, dev, DEVCHANGE_POINTER_EVENT, &num_events);
>>> @@ -1731,7 +1758,7 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
>>>         raw = &events->raw_event;
>>>         events++;
>>>         num_events++;
>>> -        init_raw(dev, raw, ms, type, ti->client_id);
>>> +        init_raw(dev, raw, ms, type, touchpoint.ti->client_id);
>>>         set_raw_valuators(raw, &mask, raw->valuators.data_raw);
>>>     }
>>>
>>> @@ -1739,6 +1766,12 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
>>>     num_events++;
>>>
>>>     init_event(dev, event, ms);
>>> +    /* if submitted for master device, get the sourceid from there */
>>> +    if (flags & TOUCH_CLIENT_ID)
>>> +    {
>>> +        event->sourceid = touchpoint.dix_ti->sourceid;
>>> +        /* TOUCH_CLIENT_ID implies norawevent */
>>> +    }
>>>
>>>     switch (type) {
>>>     case XI_TouchBegin:
>>> @@ -1765,17 +1798,19 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
>>>         event->type = ET_TouchEnd;
>>>         /* We can end the DDX touch here, since we don't use the active
>>>          * field below */
>>> -        TouchEndDDXTouch(dev, ti);
>>> +        if (!(flags & TOUCH_CLIENT_ID))
>>> +            TouchEndDDXTouch(dev, touchpoint.ti);
>>>         break;
>>>     default:
>>>         return 0;
>>>     }
>>> -
>>> -    if (!valuator_mask_isset(&mask, 0))
>>> -        valuator_mask_set_double(&mask, 0, valuator_mask_get_double(ti->valuators, 0));
>>> -    if (!valuator_mask_isset(&mask, 1))
>>> -        valuator_mask_set_double(&mask, 1, valuator_mask_get_double(ti->valuators, 1));
>>> -
>>> +    if (!(flags & TOUCH_CLIENT_ID))
>>> +    {
>>> +        if (!valuator_mask_isset(&mask, 0))
>>> +            valuator_mask_set_double(&mask, 0, valuator_mask_get_double(touchpoint.ti->valuators, 0));
>>> +        if (!valuator_mask_isset(&mask, 1))
>>> +            valuator_mask_set_double(&mask, 1, valuator_mask_get_double(touchpoint.ti->valuators, 1));
>>> +    }
>>
>> Above, we guarantee that the x and y axes were set for dix submitted
>> touches. It wouldn't be harmful to recheck if the valuators were set.
>> Thus, imo it would be easier to read and understand without the check for
>> a ddx submitted touch event here.
> 
> touchpoint.ti and touchpoint.dix_ti are different and we mustn't access the
> latter for DDX-submitted touchpoints. Removing the condition relies to much
> on the implicit assumption that 0/1 is always set for dix-submitted events.
> 
> or we force the two to be binary compatible. Right now they only need to be for
> the client_id element and I've got a patch to remove requirement, it's a
> leftover from earlier versions.
>  
>>>     /* Get our screen event co-ordinates (root_x/root_y/event_x/event_y):
>>>      * these come from the touchpoint in Absolute mode, or the sprite in
>>> @@ -1783,10 +1818,12 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
>>>     if (t->mode == XIDirectTouch) {
>>>         transformAbsolute(dev, &mask);
>>>
>>> -        for (i = 0; i < valuator_mask_size(&mask); i++) {
>>> -            if (valuator_mask_isset(&mask, i))
>>> -                valuator_mask_set_double(ti->valuators, i,
>>> -                        valuator_mask_get_double(&mask, i));
>>> +        if (!(flags & TOUCH_CLIENT_ID)) {
>>> +            for (i = 0; i < valuator_mask_size(&mask); i++) {
>>> +                if (valuator_mask_isset(&mask, i))
>>> +                    valuator_mask_set_double(touchpoint.ti->valuators, i,
>>> +                            valuator_mask_get_double(&mask, i));
>>> +            }
>>
>> Why not copy the rest of the valuators, leaving the code a bit simpler to
>> understand?
> 
> see above.
> 
> fwiw, this should use valuator_mask_fetch_double() for easier reading, I've
> replaced this part with the code below.
> 
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 486c40a..b60ddc0 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -1849,9 +1849,9 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> 
>          if (!(flags & TOUCH_CLIENT_ID)) {
>              for (i = 0; i < valuator_mask_size(&mask); i++) {
> -                if (valuator_mask_isset(&mask, i))
> -                    valuator_mask_set_double(touchpoint.ti->valuators, i,
> -                            valuator_mask_get_double(&mask, i));
> +                double val;
> +                if (valuator_mask_fetch_double(&mask, i, &val))
> +                    valuator_mask_set_double(touchpoint.ti->valuators, i, val);
>              }
>          }

I'm ok with all of the above.

Reviewed-by: Chase Douglas <chase.douglas at canonical.com>


More information about the xorg-devel mailing list