[PATCH 2/2] Add test for XIQueryPointer button mask when physical touch is active

[Chase Douglas]

On 05/01/2012 10:21 PM, Peter Hutterer wrote:
> On Wed, Apr 25, 2012 at 06:15:51PM -0700, Chase Douglas wrote:
>> Signed-off-by: Chase Douglas <chase.douglas at canonical.com>
>> ---
>> The functions to wait for specific events should probably be moved to
>> xorg-gtest eventually, but I want to make sure they are generic enough for
>> all use cases before doing that.
> 
> I don't think I'm really qualified to review that properly, I'm not too
> familiar with the xorg-gtest.
> 
> one comment: this patch should probably be split into the actual
> XIQueryPointer part and the misc helper functions beeing added.

Done.

[snip]

>> +/**
>> + * Wait for an event of a specific type on the X connection.
>> + *
>> + * param [in] display   The X display connection
>> + * param [in] type      The X core protocol event type
>> + * param [in] extension The X extension opcode of a generic event, or -1 for any
>> + *                      generic event
>> + * param [in] evtype    The X extension event type of a generic event, or -1 for
>> + *                      any event of the given extension
>> + * param [in] timeout   The timeout in milliseconds
>> + *
>> + * @return Whether an event is available
>> + */
>> +bool wait_for_event_of_type(::Display *display, int type, int extension,
>> +                            int evtype, time_t timeout = 1000)
>> +{
>> +    while (wait_for_event(display)) {
>> +        XEvent event;
>> +        if (!XPeekEvent(display, &event))
>> +            throw std::runtime_error("Failed to peek X event");
>> +
>> +        if (event.type != type) {
>> +            if (XNextEvent(display, &event) != Success)
>> +                throw std::runtime_error("Failed to remove X event");
>> +            continue;
>> +        }
>> +        if (event.type != GenericEvent || extension == -1)
>> +            return true;
>> +
> 
> this could be simplified with XCheckTypedEvent

I looked into this, but it would cause differences in behavior depending
on whether you are looking for a generic event of a specific evtype vs a
normal event type. For example, you have the following event queue:

Exposure
Generic: XI_ButtonPress
Generic: XI_TouchBegin
EnterWindow

If you use XCheckTypedEvent, which removes the returned event from the
queue, the result of the following is different:

get_event_of_type(display, EnterWindow, -1, -1, &event);
Queue is now:
Exposure
Generic: XI_ButtonPress
Generic: XI_TouchBegin

get_event_of_type(display, GenericEvent, xi2_opcode, XI_TouchBegin, &event);
Queue is now:
Exposure
EnterWindow

Notice how we lost the "Generic: XI_ButtonPress" event. This is because
we call XCheckTypedEvent to get the next GenericEvent, but then we need
to discard it because the first event is the XI_ButtonPress. The result
is that sometimes, depending on the event type you request, other event
types are not discarded, but other times some events are discarded.

The current approach always discards events until the first match. It's
more consistent. If the user really needs functionality like
XCheckTypedEvent, they can use it manually.

I will update the comment for this function to make it clear that all
events up to the matched event are discarded.

[snip]

>> +/**
>> + * XIQueryPointer for XInput 2.1 and earlier should report the first button
>> + * pressed if a touch is physically active. For XInput 2.2 and later clients,
>> + * the first button should not be reported.
>> + */
>> +TEST_P(XInput2Test, XIQueryPointerTouchscreen)
>> +{
>> +    XIEventMask mask;
>> +    mask.deviceid = XIAllDevices;
>> +    mask.mask_len = XIMaskLen(XI_HierarchyChanged);
>> +    mask.mask = reinterpret_cast<unsigned char*>(
>> +        calloc(XIMaskLen(XI_HierarchyChanged), 1));
>> +    XISetMask(mask.mask, XI_HierarchyChanged);
>> +
>> +    ASSERT_EQ(Success,
>> +              XISelectEvents(Display(), DefaultRootWindow(Display()), &mask,
>> +              1));
>> +
> 
> odd choice of line breaking. makes the code harder to read, IMO.
> 
>> +    mask.deviceid = XIAllMasterDevices;
>> +    XIClearMask(mask.mask, XI_HierarchyChanged);
>> +    XISetMask(mask.mask, XI_ButtonPress);
>> +
>> +    ASSERT_EQ(Success,
>> +              XISelectEvents(Display(), DefaultRootWindow(Display()), &mask,
>> +              1));
> 
> as above

Thanks for catching these. The 1 should be indented to align inside the
XISelectEvents call.

> rest looks ok (only skimmed it).

Ok. I'll resend with these changes.

-- Chase