Merge lp:~bregma/geis/lp-937021 into lp:geis

[Stephen M. Webb]

Check he success of grabbing multi-touch input on a window and propagates the result up the stack.

Fixes lp:937021.

Fix can be verified by running the geistest tool without any command-line arguments so that it attempts to grab input from the root window. This will now fail, additional error messages are show if the environment variable GEIS_DEBUG is set to an integer value greater than or equal to 1.

[Jussi Pakkanen (jpakkane)]

Having final_exit and goto would make more sense if some resource were freed there. Personally I would replace them with an explicit "return GEIS_STATUS_UNKNOWN_ERROR;". This is not a blocker for merging, though.

[Chase Douglas (chasedouglas)]

This needs an automated test that runs at 'make check'.

review: Needs Fixing

[lp:~bregma/geis/lp-937021]

210. By Stephen M. Webb on 2012-03-06

synched with trunk

211. By Stephen M. Webb on 2012-03-06

Refactored GEIS test fixture.

212. By Stephen M. Webb on 2012-03-06

Removed warnings from configure.

213. By Stephen M. Webb on 2012-03-06

Added test case for lp:937021

214. By Stephen M. Webb on 2012-03-07

Got lp:937021 test case passing (when evemu works).

215. By Stephen M. Webb on 2012-03-07

Eliminated transient evemy device creation failures.

[Stephen M. Webb (bregma)]

OK, test cases seem to pass every time. Ready for full review.

[Chase Douglas (chasedouglas)]

Looks good to me :). The GeisSubscriptionTests class doesn't need the SetUp or TearDown methods since they just call the superclass methods. After deleting them, feel free to merge.

review: Approve

[lp:~bregma/geis/lp-937021]

216. By Stephen M. Webb on 2012-03-07

Removed unnecessary fixture SetUp() and TearDown() in gtest-based tests.

=== modified file '.bzrignore'
--- .bzrignore	2012-02-21 20:31:39 +0000
+++ .bzrignore	2012-03-07 18:15:24 +0000
@@ -10,9 +10,9 @@
 *Makefile.in
 aclocal.m4
 autom4te.cache
-check_geis1_api
-check_geis2_api
-check_geis2_internals
+check-*-compile
+check_*_api
+check_*_internals
 check_geis_util
 config.*
 configure

=== modified file 'configure.ac'
--- configure.ac	2012-02-23 17:07:26 +0000
+++ configure.ac	2012-03-07 18:15:24 +0000
@@ -32,6 +32,7 @@
 
 # Checks for programs.
 AM_PROG_CC_C_O
+AC_PROG_CXX
 AM_PATH_PYTHON([2.7])
 
 LT_PREREQ([2.2.6b])
@@ -99,7 +100,7 @@
             [with_integration_tests=check])
 AC_MSG_RESULT([$with_integration_tests])
 AS_IF([test "x$with_integration_tests" != xno],
-      [AC_PROG_CXX
+      [AC_LANG_PUSH(C++)
        AC_CHECK_LIB([gtest], [main], [have_gtest=yes])
        AS_IF([test "x$have_gtest" = xyes],
              [PKG_CHECK_MODULES([XORG_GTEST],
@@ -109,6 +110,7 @@
                           unit testing disabled])])
               PKG_CHECK_MODULES([EVEMU],[utouch-evemu >= 1.0.5])
              ])
+       AC_LANG_POP
        XORG_GTEST_LIBS="$XORG_GTEST_LIBS -lgtest -lpthread"])
 AM_CONDITIONAL([HAVE_XORG_GTEST],[test "x$have_xorg_gtest" = xyes])
 

=== modified file 'libutouch-geis/backend/grail/geis_grail_backend.c'
--- libutouch-geis/backend/grail/geis_grail_backend.c	2012-02-24 17:12:40 +0000
+++ libutouch-geis/backend/grail/geis_grail_backend.c	2012-03-07 18:15:24 +0000
@@ -1437,7 +1437,13 @@
                (void *)ugsub,
                grail_use_atomic_gestures);
 
-  geis_grail_window_grab_store_grab(gbe->window_grabs, window_id);
+  status = geis_grail_window_grab_store_grab(gbe->window_grabs, window_id);
+  if (status != GEIS_STATUS_SUCCESS)
+  {
+    geis_error("failed to grab input on window 0x%08x", window_id);
+    goto final_exit;
+  }
+
   ugstatus = grail_subscription_activate(gbe->grail, ugsub);
   if (ugstatus != UGStatusSuccess)
   {
@@ -1486,6 +1492,10 @@
                 device,
                 window_id,
                 subscription);
+            if (status != GEIS_STATUS_SUCCESS)
+            {
+              goto final_exit;
+            }
           }
           else
           {
@@ -1506,6 +1516,8 @@
                                                       subscription);
     }
   }
+
+final_exit:
   return status;
 }
 
@@ -1558,6 +1570,8 @@
       status = GEIS_STATUS_SUCCESS;
     }
     geis_device_bag_delete(selected_devices);
+    if (status != GEIS_STATUS_SUCCESS)
+      break;
   }
 
   return status;

=== modified file 'libutouch-geis/backend/grail/geis_grail_window_grab.c'
--- libutouch-geis/backend/grail/geis_grail_window_grab.c	2012-03-02 17:54:50 +0000
+++ libutouch-geis/backend/grail/geis_grail_window_grab.c	2012-03-07 18:15:24 +0000
@@ -20,7 +20,6 @@
 #include "geis_config.h"
 #include "geis_grail_window_grab.h"
 
-#include "geis/geis.h"
 #include "geis_bag.h"
 #include "geis_logging.h"
 #include <X11/extensions/XInput2.h>
@@ -124,9 +123,10 @@
 /*
  * Grabs a window through a window grab store.
  */
-void
+GeisStatus
 geis_grail_window_grab_store_grab(GeisGrailWindowGrabStore wgs, Window window_id)
 {
+  GeisStatus status = GEIS_STATUS_UNKNOWN_ERROR;
   GeisGrailWindowGrab grab = _window_grab_store_find(wgs, window_id);
   if (!grab)
   {
@@ -147,16 +147,27 @@
     int xstat = XIGrabTouchBegin(wgs->display, XIAllMasterDevices,
                                  window_id,
                                  0, &mask, 1, &mods);
+    free(mask.mask);
     if (xstat)
     {
       geis_error("error %d returned from XIGrabTouchBegin()", xstat);
-    }
-    free(mask.mask);
+      goto final_exit;
+    }
+    else if (mods.status != XIGrabSuccess)
+    {
+      geis_error("status %d returned from XIGrabTouchBegin()", mods.status);
+      goto final_exit;
+    }
+    status = GEIS_STATUS_SUCCESS;
   }
   else
   {
     ++grab->grab_count;
+    status = GEIS_STATUS_SUCCESS;
   }
+
+final_exit:
+  return status;
 }
 
 

=== modified file 'libutouch-geis/backend/grail/geis_grail_window_grab.h'
--- libutouch-geis/backend/grail/geis_grail_window_grab.h	2012-01-31 20:46:42 +0000
+++ libutouch-geis/backend/grail/geis_grail_window_grab.h	2012-03-07 18:15:24 +0000
@@ -20,6 +20,7 @@
 #ifndef GEIS_BACKEND_GRAIL_WINDOW_GRAB_H_
 #define GEIS_BACKEND_GRAIL_WINDOW_GRAB_H_
 
+#include "geis/geis.h"
 #include <X11/Xlib.h>
 
 
@@ -56,9 +57,10 @@
  * @param store  A window grab store.
  * @param window The window to grab.
  *
- * @returns a reference to the grab.
+ * @returns GEIS_STATUS_SUCCESS if the multi-touch for the window was grabbed
+ * successfully, GEIS_STATUS_UNKNOWN_ERROR otherwise.
  */
-void
+GeisStatus
 geis_grail_window_grab_store_grab(GeisGrailWindowGrabStore store,
                                   Window window);
 

=== modified file 'testsuite/geis2/Makefile.am'
--- testsuite/geis2/Makefile.am	2012-03-05 14:22:20 +0000
+++ testsuite/geis2/Makefile.am	2012-03-07 18:15:24 +0000
@@ -44,7 +44,9 @@
 
 gtest_geis2_api_SOURCES = \
 	gtest_attrs.cpp \
-	gtest_evemu_device.h   gtest_evemu_device.cpp
+	gtest_subscriptions.cpp \
+	gtest_evemu_device.h   gtest_evemu_device.cpp \
+	gtest_geis_fixture.h   gtest_geis_fixture.cpp
 
 gtest_geis2_api_CPPFLAGS = \
 	--std=c++0x \

=== modified file 'testsuite/geis2/gtest_attrs.cpp'
--- testsuite/geis2/gtest_attrs.cpp	2012-03-05 17:22:19 +0000
+++ testsuite/geis2/gtest_attrs.cpp	2012-03-07 18:15:24 +0000
@@ -7,13 +7,11 @@
 #include <functional>
 #include "geis/geis.h"
 #include "gtest_evemu_device.h"
+#include "gtest_geis_fixture.h"
 #include <gtest/gtest.h>
-#include <memory>
 #include <mutex>
 #include <sys/select.h>
 #include <sys/time.h>
-#include <X11/extensions/XInput2.h>
-#include <xorg/gtest/test.h>
 
 
 namespace
@@ -23,52 +21,19 @@
 static const std::string TEST_DEVICE_EVENTS_FILE("touchscreen_a_rotate90.events");
 
 /**
- * Fixture for testing expected attributes.
+ * Fixture for testing expected attributes.  This has to be a separate class
+ * because of the way Java reflection is used in jUnit.
  */
 class GeisAttributeTests
-: public xorg::testing::Test
+: public GTestGeisFixture
 {
 public:
-  void
-  SetUp()
-  {
-    // Chain up the static class heirarchy, as if the test framework was
-    // designed by a Java trainee with some exposure to glib but has never used
-    // C++.
-    ASSERT_NO_FATAL_FAILURE(xorg::testing::Test::SetUp());
-
-    // Verify that whatever X server is in use supports the required XInput
-    // version.
-    int xi_major = 2;
-    int xi_minor = 2;
-    ASSERT_EQ(Success, XIQueryVersion(Display(), &xi_major, &xi_minor));
-    ASSERT_GE(xi_major, 2);
-    ASSERT_GE(xi_minor, 2);
-
-    evemu_device_.reset(new Testsuite::EvemuDevice(TEST_DEVICE_PROP_FILE));
-
-    geis_ = geis_new(GEIS_INIT_SYNCHRONOUS_START, GEIS_INIT_NO_ATOMIC_GESTURES, NULL);
-  }
-
-  void
-  TearDown()
-  {
-    geis_delete(geis_);
-    xorg::testing::Test::TearDown(); // NVI ftw.
-    evemu_device_.reset();
-  }
-
-  int
-  geis_fd()
-  {
-    GeisInteger fd;
-    geis_get_configuration(geis_, GEIS_CONFIGURATION_FD, &fd);
-    return fd;
-  }
+  GeisAttributeTests()
+  : evemu_device_(TEST_DEVICE_PROP_FILE)
+  { }
 
 protected:
-  std::unique_ptr<Testsuite::EvemuDevice> evemu_device_;
-  Geis                                    geis_;
+  Testsuite::EvemuDevice evemu_device_;
 };
 
 
@@ -114,7 +79,7 @@
     {
       if (geis_event_type(event) == GEIS_EVENT_INIT_COMPLETE)
       {
-        evemu_device_->play(TEST_DEVICE_EVENTS_FILE);
+        evemu_device_.play(TEST_DEVICE_EVENTS_FILE);
       }
       else if (geis_event_type(event) == GEIS_EVENT_GESTURE_END)
       {

=== added file 'testsuite/geis2/gtest_geis_fixture.cpp'
--- testsuite/geis2/gtest_geis_fixture.cpp	1970-01-01 00:00:00 +0000
+++ testsuite/geis2/gtest_geis_fixture.cpp	2012-03-07 18:15:24 +0000
@@ -0,0 +1,64 @@
+/**
+ * @file gtest_geis_fixture.cpp
+ * @brief A GTest fixture for testing the full uTouch stack through GEIS.
+ */
+/*
+ *  Copyright 2012 Canonical Ltd.
+ *
+ *  This program is free software: you can redistribute it and/or modify it 
+ *  under the terms of the GNU General Public License version 3, as published 
+ *  by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful, but 
+ *  WITHOUT ANY WARRANTY; without even the implied warranties of 
+ *  MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR 
+ *  PURPOSE.  See the GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along 
+ *  with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include "gtest_geis_fixture.h"
+
+#include <unistd.h>
+#include <X11/extensions/XInput2.h>
+
+
+void GTestGeisFixture::
+SetUp()
+{
+  // wait for things to settle
+  usleep(1000000);
+
+  // Chain up the static class heirarchy, as if the test framework was
+  // designed by a Java trainee with some exposure to glib but has never used
+  // C++.
+  ASSERT_NO_FATAL_FAILURE(xorg::testing::Test::SetUp());
+
+  // Verify that whatever X server is in use supports the required XInput
+  // version.
+  int xi_major = 2;
+  int xi_minor = 2;
+  ASSERT_EQ(Success, XIQueryVersion(Display(), &xi_major, &xi_minor));
+  ASSERT_GE(xi_major, 2);
+  ASSERT_GE(xi_minor, 2);
+
+  geis_ = geis_new(GEIS_INIT_SYNCHRONOUS_START, GEIS_INIT_NO_ATOMIC_GESTURES, NULL);
+}
+
+void GTestGeisFixture::
+TearDown()
+{
+  geis_delete(geis_);
+  xorg::testing::Test::TearDown(); // NVI ftw.
+  usleep(1000000);
+}
+
+int GTestGeisFixture::
+geis_fd()
+{
+  GeisInteger fd;
+  geis_get_configuration(geis_, GEIS_CONFIGURATION_FD, &fd);
+  return fd;
+}
+
+

=== added file 'testsuite/geis2/gtest_geis_fixture.h'
--- testsuite/geis2/gtest_geis_fixture.h	1970-01-01 00:00:00 +0000
+++ testsuite/geis2/gtest_geis_fixture.h	2012-03-07 18:15:24 +0000
@@ -0,0 +1,50 @@
+/**
+ * @file gtest_geis_fixture.h
+ * @brief A GTest fixture for testing the full uTouch stack through GEIS.
+ */
+/*
+ *  Copyright 2012 Canonical Ltd.
+ *
+ *  This program is free software: you can redistribute it and/or modify it 
+ *  under the terms of the GNU General Public License version 3, as published 
+ *  by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful, but 
+ *  WITHOUT ANY WARRANTY; without even the implied warranties of 
+ *  MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR 
+ *  PURPOSE.  See the GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along 
+ *  with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef GTEST_GEIS_FIXTURE_H_
+#define GTEST_GEIS_FIXTURE_H_
+
+#include "geis/geis.h"
+#include <gtest/gtest.h>
+#include <xorg/gtest/test.h>
+
+
+/**
+ * Fixture for testing expected attributes.
+ */
+class GTestGeisFixture
+: public xorg::testing::Test
+{
+public:
+  void
+  SetUp();
+
+  void
+  TearDown();
+
+  int
+  geis_fd();
+
+protected:
+  Geis  geis_;
+};
+
+
+#endif /* GTEST_GEIS_FIXTURE_H_ */
+

=== added file 'testsuite/geis2/gtest_subscriptions.cpp'
--- testsuite/geis2/gtest_subscriptions.cpp	1970-01-01 00:00:00 +0000
+++ testsuite/geis2/gtest_subscriptions.cpp	2012-03-07 18:15:24 +0000
@@ -0,0 +1,95 @@
+/**
+ * GTest-based test suite for GEIS subscription actions.
+ *
+ * Copyright 2012 Canonical Ltd.
+ */
+
+#include "gtest_evemu_device.h"
+#include "gtest_geis_fixture.h"
+
+
+static const std::string TEST_DEVICE_PROP_FILE("touchscreen_a.prop");
+
+
+/**
+ * Tests need to make sure at least one multi-touch device is available.
+ */
+class GeisSubscriptionTests
+: public GTestGeisFixture
+{
+public:
+  GeisSubscriptionTests()
+  : evemu_device_(TEST_DEVICE_PROP_FILE)
+  { }
+
+private:
+  Testsuite::EvemuDevice evemu_device_;
+};
+
+
+/**
+ * Regression test for lp:937021: Geis subscription touch grabs need to check
+ * for failure.
+ *
+ * This test creates two subscriptions, both attached to the root window but
+ * each on a different X client.  Since multiple subscriptions for the same
+ * window but a different client are expected to fail using the XI2.2-based
+ * grail back end, activating the second subscription is extected to resut in
+ * a failure.
+ *
+ * This behaviour is very dependent on the particular back end used and internal
+ * implementattion details of that back end.  It is not a unit test or a
+ * reliable group or system test.
+ */
+TEST_F(GeisSubscriptionTests, duplicate_window_subscription)
+{
+  Geis geis2 = geis_new(GEIS_INIT_SYNCHRONOUS_START,
+                        GEIS_INIT_NO_ATOMIC_GESTURES,
+                        NULL);
+  EXPECT_TRUE(geis2 != NULL) << "can not create second geis instance";
+
+  GeisSubscription sub1 = geis_subscription_new(geis_,
+                                                "subscription 1",
+                                                GEIS_SUBSCRIPTION_NONE);
+  EXPECT_TRUE(sub1 != NULL) << "can not create first subscription";
+
+  GeisSubscription sub2 = geis_subscription_new(geis2,
+                                                "subscription 2",
+                                                GEIS_SUBSCRIPTION_NONE);
+  EXPECT_TRUE(sub1 != NULL) << "can not create second subscription";
+
+  GeisFilter filter1 = geis_filter_new(geis_, "root window 1");
+  EXPECT_TRUE(filter1 != NULL) << "can not create filter 1";
+
+  GeisStatus fs = geis_filter_add_term(filter1,
+               GEIS_FILTER_CLASS,
+               GEIS_CLASS_ATTRIBUTE_NAME, GEIS_FILTER_OP_EQ, GEIS_GESTURE_ROTATE,
+               GEIS_GESTURE_ATTRIBUTE_TOUCHES, GEIS_FILTER_OP_GT, 1,
+               NULL);
+  EXPECT_EQ(fs, GEIS_STATUS_SUCCESS) << "can not add class to filter 1";
+
+  GeisFilter filter2 = geis_filter_new(geis_, "root window 2");
+  EXPECT_TRUE(filter2 != NULL) << "can not create filter 2";
+
+  fs = geis_filter_add_term(filter2,
+               GEIS_FILTER_CLASS,
+               GEIS_CLASS_ATTRIBUTE_NAME, GEIS_FILTER_OP_EQ, GEIS_GESTURE_ROTATE,
+               GEIS_GESTURE_ATTRIBUTE_TOUCHES, GEIS_FILTER_OP_GT, 1,
+               NULL);
+  EXPECT_EQ(fs, GEIS_STATUS_SUCCESS) << "can not add class to filter 2";
+
+  fs = geis_subscription_add_filter(sub1, filter1);
+  EXPECT_EQ(fs, GEIS_STATUS_SUCCESS) << "can not subscribe filter 1";
+  fs = geis_subscription_add_filter(sub2, filter2);
+  EXPECT_EQ(fs, GEIS_STATUS_SUCCESS) << "can not subscribe filter 2";
+
+  EXPECT_EQ(GEIS_STATUS_SUCCESS, geis_subscription_activate(sub1))
+              << "can not activate subscription 1";
+  EXPECT_NE(GEIS_STATUS_SUCCESS, geis_subscription_activate(sub2))
+              << "mistakenly activated subscription 2";
+
+  geis_subscription_delete(sub2);
+  geis_subscription_delete(sub1);
+  geis_delete(geis2);
+}
+