[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