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.doug...@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 _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel