Re: [PATCH 2/2] Add test for XIQueryPointer button mask when physical touch is active
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] evtypeThe 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_castunsigned 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
Re: [PATCH 2/2] Add test for XIQueryPointer button mask when physical touch is active
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] evtypeThe 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_castunsigned 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
Re: [PATCH 2/2] Add test for XIQueryPointer button mask when physical touch is active
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. configure.ac | 14 + test/integration/.gitignore|1 + test/integration/Makefile.am | 24 ++ .../recordings/ntrig_dell_xt2/1_begin.record | 11 + .../recordings/ntrig_dell_xt2/1_end.record |3 + .../recordings/ntrig_dell_xt2/device.prop | 32 +++ test/integration/xi2.cpp | 267 7 files changed, 352 insertions(+), 0 deletions(-) create mode 100644 test/integration/.gitignore create mode 100644 test/integration/recordings/ntrig_dell_xt2/1_begin.record create mode 100644 test/integration/recordings/ntrig_dell_xt2/1_end.record create mode 100644 test/integration/recordings/ntrig_dell_xt2/device.prop create mode 100644 test/integration/xi2.cpp diff --git a/configure.ac b/configure.ac index fe350c9..bc9f46f 100644 --- a/configure.ac +++ b/configure.ac @@ -2141,6 +2141,20 @@ if [test x$XORG = xyes test x$enable_integration_tests != xno]; then fi fi +PKG_CHECK_MODULES(TEST_X11, x11, have_test_x11=yes, have_test_x11=no) +PKG_CHECK_MODULES(TEST_XINPUT, [xi = 1.6], have_test_xinput=yes, have_test_xinput=no) + +if [test x$have_test_x11 != xyes]; then +AC_MSG_NOTICE([libX11 not available, skipping input tests]) +elif [test x$have_test_xinput != xyes]; then +AC_MSG_NOTICE([libXi 1.6 not available, skipping input tests]) +elif [test x$have_xorg_gtest_evemu != xyes]; then +AC_MSG_NOTICE([uTouch-Evemu not available, skipping input tests]) +else +enable_input_tests=yes +fi +AM_CONDITIONAL(ENABLE_XORG_GTEST_INPUT_TESTS, [test x$enable_input_tests = xyes]) + dnl and the rest of these are generic, so they're in config.h dnl dnl though, thanks to the passing of some significant amount of time, the diff --git a/test/integration/.gitignore b/test/integration/.gitignore new file mode 100644 index 000..ab86ebf --- /dev/null +++ b/test/integration/.gitignore @@ -0,0 +1 @@ +gtest-tests diff --git a/test/integration/Makefile.am b/test/integration/Makefile.am index e70d642..3b7c858 100644 --- a/test/integration/Makefile.am +++ b/test/integration/Makefile.am @@ -1,4 +1,28 @@ +TESTS = + if ENABLE_XORG_GTEST_TESTS include $(top_srcdir)/test/integration/Makefile-xorg-gtest.am check_LIBRARIES = $(XORG_GTEST_BUILD_LIBS) + +if ENABLE_XORG_GTEST_INPUT_TESTS +TESTS += gtest-tests endif +endif + +check_PROGRAMS = $(TESTS) + +AM_CPPFLAGS = \ + -DDEFAULT_XORG_SERVER=\$(abs_top_builddir)/hw/xfree86/Xorg\ \ + -DTEST_ROOT_DIR=\$(abs_top_srcdir)/test/integration/\ \ + $(GTEST_CPPFLAGS) \ + $(XORG_GTEST_CPPFLAGS) + +AM_CXXFLAGS = $(GTEST_CXXFLAGS) $(XORG_GTEST_CXXFLAGS) + +gtest_tests_SOURCES = xi2.cpp +gtest_tests_LDADD = \ + $(TEST_XINPUT_LIBS) \ + $(TEST_X11_LIBS) \ + $(XORG_GTEST_LIBS) \ + $(EVEMU_LIBS) \ + $(XORG_GTEST_MAIN_LIBS) diff --git a/test/integration/recordings/ntrig_dell_xt2/1_begin.record b/test/integration/recordings/ntrig_dell_xt2/1_begin.record new file mode 100644 index 000..28a849b --- /dev/null +++ b/test/integration/recordings/ntrig_dell_xt2/1_begin.record @@ -0,0 +1,11 @@ +E: 1327542640.244087 0003 2745 +E: 1327542640.244089 0003 0001 1639 +E: 1327542640.244090 0003 0035 2745 +E: 1327542640.244091 0003 0036 1639 +E: 1327542640.244092 0003 0034 0 +E: 1327542640.244093 0003 0030 468 +E: 1327542640.244094 0003 0031 306 +E: 1327542640.244095 0002 0 +E: 1327542640.244251 0001 014d 1 +E: 1327542640.244251 0001 014a 1 +E: 1327542640.244253 0 diff --git a/test/integration/recordings/ntrig_dell_xt2/1_end.record b/test/integration/recordings/ntrig_dell_xt2/1_end.record new file mode 100644 index 000..cd6a9d9 --- /dev/null +++ b/test/integration/recordings/ntrig_dell_xt2/1_end.record @@ -0,0 +1,3 @@ +E: 1327542642.244253 0001 014d 0 +E: 1327542642.244253 0001 014a 0 +E: 1327542642.244253 0 I really recommend renaming these as touch_1_begin and touch_1_end. 1 could otherwise refer to buttons as well. diff --git a/test/integration/recordings/ntrig_dell_xt2/device.prop b/test/integration/recordings/ntrig_dell_xt2/device.prop new file mode 100644 index 000..2738c04 --- /dev/null +++ b/test/integration/recordings/ntrig_dell_xt2/device.prop @@ -0,0 +1,32 @@ +N: N-Trig MultiTouch (Virtual Test Device)
[PATCH 2/2] Add test for XIQueryPointer button mask when physical touch is active
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. configure.ac | 14 + test/integration/.gitignore|1 + test/integration/Makefile.am | 24 ++ .../recordings/ntrig_dell_xt2/1_begin.record | 11 + .../recordings/ntrig_dell_xt2/1_end.record |3 + .../recordings/ntrig_dell_xt2/device.prop | 32 +++ test/integration/xi2.cpp | 267 7 files changed, 352 insertions(+), 0 deletions(-) create mode 100644 test/integration/.gitignore create mode 100644 test/integration/recordings/ntrig_dell_xt2/1_begin.record create mode 100644 test/integration/recordings/ntrig_dell_xt2/1_end.record create mode 100644 test/integration/recordings/ntrig_dell_xt2/device.prop create mode 100644 test/integration/xi2.cpp diff --git a/configure.ac b/configure.ac index fe350c9..bc9f46f 100644 --- a/configure.ac +++ b/configure.ac @@ -2141,6 +2141,20 @@ if [test x$XORG = xyes test x$enable_integration_tests != xno]; then fi fi +PKG_CHECK_MODULES(TEST_X11, x11, have_test_x11=yes, have_test_x11=no) +PKG_CHECK_MODULES(TEST_XINPUT, [xi = 1.6], have_test_xinput=yes, have_test_xinput=no) + +if [test x$have_test_x11 != xyes]; then +AC_MSG_NOTICE([libX11 not available, skipping input tests]) +elif [test x$have_test_xinput != xyes]; then +AC_MSG_NOTICE([libXi 1.6 not available, skipping input tests]) +elif [test x$have_xorg_gtest_evemu != xyes]; then +AC_MSG_NOTICE([uTouch-Evemu not available, skipping input tests]) +else +enable_input_tests=yes +fi +AM_CONDITIONAL(ENABLE_XORG_GTEST_INPUT_TESTS, [test x$enable_input_tests = xyes]) + dnl and the rest of these are generic, so they're in config.h dnl dnl though, thanks to the passing of some significant amount of time, the diff --git a/test/integration/.gitignore b/test/integration/.gitignore new file mode 100644 index 000..ab86ebf --- /dev/null +++ b/test/integration/.gitignore @@ -0,0 +1 @@ +gtest-tests diff --git a/test/integration/Makefile.am b/test/integration/Makefile.am index e70d642..3b7c858 100644 --- a/test/integration/Makefile.am +++ b/test/integration/Makefile.am @@ -1,4 +1,28 @@ +TESTS = + if ENABLE_XORG_GTEST_TESTS include $(top_srcdir)/test/integration/Makefile-xorg-gtest.am check_LIBRARIES = $(XORG_GTEST_BUILD_LIBS) + +if ENABLE_XORG_GTEST_INPUT_TESTS +TESTS += gtest-tests endif +endif + +check_PROGRAMS = $(TESTS) + +AM_CPPFLAGS = \ + -DDEFAULT_XORG_SERVER=\$(abs_top_builddir)/hw/xfree86/Xorg\ \ + -DTEST_ROOT_DIR=\$(abs_top_srcdir)/test/integration/\ \ + $(GTEST_CPPFLAGS) \ + $(XORG_GTEST_CPPFLAGS) + +AM_CXXFLAGS = $(GTEST_CXXFLAGS) $(XORG_GTEST_CXXFLAGS) + +gtest_tests_SOURCES = xi2.cpp +gtest_tests_LDADD = \ + $(TEST_XINPUT_LIBS) \ + $(TEST_X11_LIBS) \ + $(XORG_GTEST_LIBS) \ + $(EVEMU_LIBS) \ + $(XORG_GTEST_MAIN_LIBS) diff --git a/test/integration/recordings/ntrig_dell_xt2/1_begin.record b/test/integration/recordings/ntrig_dell_xt2/1_begin.record new file mode 100644 index 000..28a849b --- /dev/null +++ b/test/integration/recordings/ntrig_dell_xt2/1_begin.record @@ -0,0 +1,11 @@ +E: 1327542640.244087 0003 2745 +E: 1327542640.244089 0003 0001 1639 +E: 1327542640.244090 0003 0035 2745 +E: 1327542640.244091 0003 0036 1639 +E: 1327542640.244092 0003 0034 0 +E: 1327542640.244093 0003 0030 468 +E: 1327542640.244094 0003 0031 306 +E: 1327542640.244095 0002 0 +E: 1327542640.244251 0001 014d 1 +E: 1327542640.244251 0001 014a 1 +E: 1327542640.244253 0 diff --git a/test/integration/recordings/ntrig_dell_xt2/1_end.record b/test/integration/recordings/ntrig_dell_xt2/1_end.record new file mode 100644 index 000..cd6a9d9 --- /dev/null +++ b/test/integration/recordings/ntrig_dell_xt2/1_end.record @@ -0,0 +1,3 @@ +E: 1327542642.244253 0001 014d 0 +E: 1327542642.244253 0001 014a 0 +E: 1327542642.244253 0 diff --git a/test/integration/recordings/ntrig_dell_xt2/device.prop b/test/integration/recordings/ntrig_dell_xt2/device.prop new file mode 100644 index 000..2738c04 --- /dev/null +++ b/test/integration/recordings/ntrig_dell_xt2/device.prop @@ -0,0 +1,32 @@ +N: N-Trig MultiTouch (Virtual Test Device) +I: 0003 1b96 0001 0110 +P: 00 00 00 00 00 00 00 00 +B: 00 0b 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 04 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 01 00 00 00 00 00 00 00 00 +B: 02 00 00 00 00 00 00 00 00 +B: 03 03 00 00 00 00 01 73 00
