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

[Peter Hutterer]

On Tue, May 15, 2012 at 10:44:51AM -0700, Chase Douglas wrote:
> 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.

right, looks like what we really need is a generic event-capable version for
XCheckTypedEvent so that we can have this sort of code with predictable
effects.

with the updated comment this code makes more sense now. thanks.

Cheers,
  Peter

> 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