[PATCH 2/2] Add test for XIQueryPointer button mask when physical touch is active
Peter Hutterer
peter.hutterer at who-t.net
Tue May 15 15:54:22 PDT 2012
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
More information about the xorg-devel
mailing list