Merge lp:~dandrader/geis/lp967605 into lp:geis

[Daniel d'Andrada]

Fixes bug #967605.

I wonder if I should make a unit test for this guy (mocking XSync*).

Updates:
 * FIxed coding style
 * Fixed issue with querying server time
 * Changed header boilerplate to mention LGPL instead of GPL

[Chase Douglas (chasedouglas)]

* Style:

XSyncIntsToValue(&alarm_attributes.trigger.wait_value, timeout & 0xffffffff,
    timeout & 0xffffffff00000000);

The second line should be indented to match the open parens. The same for the XSyncCreateAlarm() call.

* geis_grail_xsync_query_server_time(GeisGrailXSync self) undoes the fix for bug 962705: https://code.launchpad.net/~chasedouglas/utouch-geis/fix-tap/+merge/98953. Instead of querying the time, we need to use the counter value from the alarm event.

Otherwise, this looks like a great fix for the problem. I don't think there's a test we can come up with that will be determinate, so I'm fine with not having an integration test. A unit test would be nice, but I will leave the call on that up to Stephen. Usually, unit tests are added for these types of additions.

review: Needs Fixing

[Stephen M. Webb (bregma)]

I am not going to comment on the XSync code itself, I'll assume Daniel and Chase know more than I on that topic. The overall structure of the code and use of GeisBag is all OK.

I'm approving this with a single condition: the license terms of libutouch-geis is LGPL-3. Please modify the boilerplate in the new source files to refer to the Lesser General Public License.

review: Approve

[Stephen M. Webb (bregma)]

I don't think any unit test is practical since the functionality is dependent on a working X server (unless we use a mock X lib, and I think it's too late in the game to start adding that sort of thing).

Any reliable integration test would require some carefully handcrafted event files, and even then would be unreliable because of effects such as machine load. I think we'll just have to forego automated testing of this fix.

A manual test case description might be warranted.

[Daniel d'Andrada (dandrader)]

> [...]
> I'm approving this with a single condition: the license terms of libutouch-
> geis is LGPL-3. Please modify the boilerplate in the new source files to
> refer to the Lesser General Public License.

Will do.
I copy-pasted that from geis_grail_backend.c, just changed the copyright year. Looks like several files in utouch-geis are still referring to GPL instead of LGPL.

[Chase Douglas (chasedouglas)]

Looks good to me :).

review: Approve

[Chase Douglas (chasedouglas)]

I missed this: the X connection needs to be flushed after creating the alarm. Otherwise, the request might not reach the server for some time. Make sure to add it in when merging.

[Daniel d'Andrada (dandrader)]

> I missed this: the X connection needs to be flushed after creating the alarm. Otherwise, the request might not reach the server for some time. Make sure to add it in when merging.

I already merged it. Thus I pushed this change now as a separate, subsequent commit.

=== modified file 'libutouch-geis/backend/grail/Makefile.am'
--- libutouch-geis/backend/grail/Makefile.am	2012-01-31 20:46:42 +0000
+++ libutouch-geis/backend/grail/Makefile.am	2012-03-30 16:56:28 +0000
@@ -22,6 +22,7 @@
 	geis_grail_token.h          geis_grail_token.c \
 	geis_grail_backend.h        geis_grail_backend.c  \
 	geis_grail_window_grab.h    geis_grail_window_grab.c \
+	geis_grail_xsync.h          geis_grail_xsync.c \
 	geis_ugsubscription_store.h geis_ugsubscription_store.c
 
 libutouch_geis_grail_backend_la_CPPFLAGS = \

=== modified file 'libutouch-geis/backend/grail/geis_grail_backend.c'
--- libutouch-geis/backend/grail/geis_grail_backend.c	2012-03-29 19:41:33 +0000
+++ libutouch-geis/backend/grail/geis_grail_backend.c	2012-03-30 16:56:28 +0000
@@ -29,6 +29,7 @@
 #include "geis_event.h"
 #include "geis_grail_token.h"
 #include "geis_grail_window_grab.h"
+#include "geis_grail_xsync.h"
 #include "geis_group.h"
 #include "geis_logging.h"
 #include "geis_private.h"
@@ -41,7 +42,6 @@
 #include <string.h>
 #include <utouch/frame_x11.h>
 #include <utouch/grail.h>
-#include <X11/extensions/sync.h>
 #include <X11/extensions/XInput2.h>
 #include <X11/Xlib.h>
 
@@ -84,7 +84,7 @@
   Geis                      geis;         /**< The parent GEIS instance. */
   Display                  *display;      /**< The X11 connection. */
   Window                    root_window;  /**< The X11 root window. */
-  XSyncCounter              x11_timer;    /**< The X11 timer. */
+  GeisGrailXSync            xsync;        /**< XSync handler for setting timers. */
   UFHandle                  frame;        /**< A utouch-frame instance. */
   UGHandle                  grail;        /**< A utouch-grail instance. */
   GeisBag                   devices;      /**< The list of known devices. */
@@ -373,22 +373,7 @@
   /* Set a timeout on the X server, if necessary. */
   uint64_t timeout = grail_next_timeout(gbe->grail);
   if (timeout)
-  {
-    XSyncWaitCondition condition;
-
-    condition.trigger.counter = gbe->x11_timer;
-    condition.trigger.value_type = XSyncAbsolute;
-    XSyncIntsToValue(&condition.trigger.wait_value,
-                     timeout & 0x00000000ffffffff,
-                     timeout & 0xffffffff00000000);
-    condition.trigger.test_type = XSyncPositiveComparison;
-    XSyncIntToValue(&condition.event_threshold, 0);
-    if (!XSyncAwait(gbe->display, &condition, 1))
-    {
-      geis_warning("failed to set wait trigger");
-    }
-    XFlush(gbe->display);
-  }
+    geis_grail_xsync_set_timeout(gbe->xsync, timeout);
 }
 
 static void
@@ -929,14 +914,11 @@
   {
     XNextEvent(gbe->display, &event);
 
-    /* Non-Generic events should be timer events. */
-    if (event.type != GenericEvent)
+    if (geis_grail_xsync_is_timeout(gbe->xsync, &event))
     {
-      XSyncCounterNotifyEvent *sync_event = (XSyncCounterNotifyEvent*)&event;
-      XSyncValue value = sync_event->counter_value;
-      uint64_t grail_time = (uint64_t)value.hi << 32 | value.lo;
-
-      grail_update_time(gbe->grail, grail_time);
+      uint64_t server_time = geis_grail_xsync_get_server_time(&event);
+      if (server_time)
+        grail_update_time(gbe->grail, server_time);
       continue;
     }
 
@@ -996,57 +978,6 @@
   return has_xi2;
 }
 
-
-/**
- * Gets a synchronization timer from the X11 server (if one is available).
- *
- * @param[in] gbe  The grail back end
- *
- * @returns a valid syncronization timer from the X11 server is an appropriate
- * one is available, otherwise returns a value < 0.
- */
-static XSyncCounter
-_geis_grail_x11_get_timer(GeisGrailBackend gbe)
-{
-  XSyncCounter x11_timer = -1;
-
-  /* check for the presence of the XSync extension */
-  int event;
-  int error;
-  if (XSyncQueryExtension(gbe->display, &event, &error) != True)
-  {
-    geis_warning("XSync extension is not available");
-    goto final_exit;
-  }
-
-  /* initialize the XSync extension */
-  int major;
-  int minor;
-  if (XSyncInitialize(gbe->display, &major, &minor) != True)
-  {
-    geis_warning("failed to initialize XSync extension");
-    goto final_exit;
-  }
-
-  /* find the server-time counter */
-  int num_system_counters;
-  XSyncSystemCounter *counters;
-  counters = XSyncListSystemCounters(gbe->display, &num_system_counters);
-  for (int i = 0; i < num_system_counters; ++i)
-  {
-    if (0 == strcmp(counters[i].name, "SERVERTIME"))
-    {
-      x11_timer = counters[i].counter;
-      break;
-    }
-  }
-  XSyncFreeSystemCounterList(counters);
-
-final_exit:
-  return x11_timer;
-}
-
-
 /**
  * Connects to the X11 server and sets up processing on X11 events.
  *
@@ -1074,7 +1005,6 @@
   }
 
   gbe->root_window = DefaultRootWindow(gbe->display);
-  gbe->x11_timer = _geis_grail_x11_get_timer(gbe);
 
   /* install an X11 event callback */
   geis_multiplex_fd(gbe->geis,
@@ -1222,13 +1152,17 @@
 
   if (_geis_grail_open_x11_connection(gbe))
   {
+    gbe->xsync = geis_grail_xsync_new(gbe->display);
+    if (!gbe->xsync)
+      goto unwind_x11;
+
     _geis_grail_subscribe_x11_device_events(gbe);
 
     UFStatus frame_status = frame_x11_new(gbe->display, &gbe->frame);
     if (frame_status != UFStatusSuccess)
     {
       geis_error("failed to create utouch frame instance");
-      goto unwind_x11;
+      goto unwind_xsync;
     }
     geis_multiplex_fd(gbe->geis,
                       frame_get_fd(gbe->frame),
@@ -1300,6 +1234,8 @@
 unwind_frame:
     geis_demultiplex_fd(gbe->geis, frame_get_fd(gbe->frame));
     frame_x11_delete(gbe->frame);
+unwind_xsync:
+    geis_grail_xsync_delete(gbe->xsync);
 unwind_x11:
     geis_demultiplex_fd(gbe->geis, ConnectionNumber(gbe->display));
     XCloseDisplay(gbe->display);
@@ -1338,6 +1274,7 @@
   grail_delete_v3(gbe->grail);
   geis_demultiplex_fd(gbe->geis, frame_get_fd(gbe->frame));
   frame_x11_delete(gbe->frame);
+  geis_grail_xsync_delete(gbe->xsync);
   geis_demultiplex_fd(gbe->geis, ConnectionNumber(gbe->display));
 
   XErrorHandler old_x_handler = XSetErrorHandler(_grail_be_x_error_handler);

=== added file 'libutouch-geis/backend/grail/geis_grail_xsync.c'
--- libutouch-geis/backend/grail/geis_grail_xsync.c	1970-01-01 00:00:00 +0000
+++ libutouch-geis/backend/grail/geis_grail_xsync.c	2012-03-30 16:56:28 +0000
@@ -0,0 +1,226 @@
+/**
+ * @file geis_grail_xsync.c
+ * @brief Handles XSync lib usage for the GEIS grail backend
+ */
+/*
+ * Copyright 2012 Canonical Ltd.
+ *
+ * This file is part of UTouch GEIS.
+ *
+ * UTouch GEIS is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published by the
+ * Free Software Foundation, either version 3 of the License, or (at your
+ * option) any later version.
+ *
+ * UTouch GEIS is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with UTouch GEIS.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "geis_grail_xsync.h"
+
+#include "geis_bag.h"
+#include "geis_logging.h"
+#include "geis/geis.h"
+#include <X11/extensions/sync.h>
+#include <string.h>
+
+struct GeisGrailAlarm {
+  XSyncAlarm xsync_alarm;
+  uint64_t timeout;
+};
+
+struct GeisGrailXSync
+{
+  Display *display;
+  XSyncCounter   server_time_counter;
+  int xsync_event_base;
+  GeisBag alarms; /* List of GeisGrailAlarm, describing existing alarms */
+};
+
+GeisGrailXSync
+geis_grail_xsync_new(Display *display)
+{
+  GeisGrailXSync self = malloc(sizeof(struct GeisGrailXSync));
+  if (!self)
+  {
+    geis_error("failed to allocate new GeisGrailXSync");
+    goto return_failure;
+  }
+
+  self->display = display;
+  self->xsync_event_base = -1;
+
+  self->alarms = geis_bag_new(sizeof(struct GeisGrailAlarm), 4, 4.0f);
+  if (!self->alarms)
+  {
+    geis_error("failed to create GeisGrailXSync.alarms bag");
+    goto return_failure;
+  }
+
+  /* check for the presence of the XSync extension */
+  int error_base;
+  if (XSyncQueryExtension(self->display, &self->xsync_event_base, &error_base) != True)
+  {
+    geis_warning("XSync extension is not available");
+    goto return_failure;
+  }
+
+  /* initialize the XSync extension */
+  int major;
+  int minor;
+  if (XSyncInitialize(self->display, &major, &minor) != True)
+  {
+    geis_warning("failed to initialize XSync extension");
+    goto return_failure;
+  }
+
+  /* find the server-time counter */
+  int num_system_counters;
+  XSyncSystemCounter *counters;
+  counters = XSyncListSystemCounters(self->display, &num_system_counters);
+  GeisBoolean found_counter = GEIS_FALSE;
+  for (int i = 0; i < num_system_counters; ++i)
+  {
+    if (0 == strcmp(counters[i].name, "SERVERTIME"))
+    {
+      self->server_time_counter = counters[i].counter;
+      found_counter = GEIS_TRUE;
+      break;
+    }
+  }
+  XSyncFreeSystemCounterList(counters);
+  if (!found_counter)
+    geis_warning("couldn't find SERVERTIME XSyncCounter");
+
+  return self;
+
+return_failure:
+  if (self->alarms)
+    geis_bag_delete(self->alarms);
+
+  if (self)
+    free(self);
+
+  return NULL;
+}
+
+void
+geis_grail_xsync_delete(GeisGrailXSync self)
+{
+  free(self);
+}
+
+/**
+ * Creates and returns an XSyncAlarm configured with the given timeout or
+ * None on failure
+ */
+static XSyncAlarm
+_geis_grail_xsync_create_alarm(GeisGrailXSync self, uint64_t timeout)
+{
+  XSyncAlarmAttributes alarm_attributes;
+  alarm_attributes.trigger.counter = self->server_time_counter;
+  alarm_attributes.trigger.value_type = XSyncAbsolute;
+  XSyncIntsToValue(&alarm_attributes.trigger.wait_value, timeout & 0xffffffff,
+                   timeout & 0xffffffff00000000);
+  alarm_attributes.trigger.test_type = XSyncPositiveComparison;
+  alarm_attributes.events = True;
+
+  return XSyncCreateAlarm(self->display,
+                          XSyncCACounter | XSyncCAValueType | XSyncCAValue
+                          | XSyncCATestType | XSyncCAEvents,
+                          &alarm_attributes);
+}
+
+/**
+ * Returns GEIS_TRUE if an XSyncAlarm exists for the given timeout or
+ * GEIS_FALSE otherwise
+ */
+static GeisBoolean
+_geis_grail_xsync_alarm_exists_for_timeout(GeisGrailXSync self, uint64_t timeout)
+{
+  for (GeisSize i = 0, alarms_count = geis_bag_count(self->alarms);
+       i < alarms_count; ++i)
+  {
+    struct GeisGrailAlarm *alarm = geis_bag_at(self->alarms, i);
+    if (alarm->timeout == timeout)
+      return GEIS_TRUE;
+  }
+  return GEIS_FALSE;
+}
+
+void
+geis_grail_xsync_set_timeout(GeisGrailXSync self, uint64_t timeout)
+{
+  if (_geis_grail_xsync_alarm_exists_for_timeout(self, timeout))
+    return;
+
+  struct GeisGrailAlarm alarm;
+
+  alarm.xsync_alarm = _geis_grail_xsync_create_alarm(self, timeout);
+  if (alarm.xsync_alarm == None)
+  {
+    geis_error("failed to create an XSync alarm.");
+    return;
+  }
+
+  alarm.timeout = timeout;
+
+  geis_bag_append(self->alarms, &alarm);
+}
+
+/**
+ * If the given XSyncAlarm exists, it's destroyed, removed from the
+ * list of alarms and the function returns GEIS_TRUE.
+ *
+ * If the given XSyncAlarm doesn't exist, the function returns GEIS_FALSE.
+ */
+static GeisBoolean
+_geis_grail_xsync_destroy_alarm(GeisGrailXSync self, XSyncAlarm xsync_alarm)
+{
+  for (GeisSize i = 0, alarms_count = geis_bag_count(self->alarms);
+       i < alarms_count; ++i)
+  {
+    struct GeisGrailAlarm *alarm = geis_bag_at(self->alarms, i);
+    if (alarm->xsync_alarm == xsync_alarm)
+    {
+      if (XSyncDestroyAlarm(self->display, xsync_alarm) == False)
+        geis_error("failed to destroy XSync alarm");
+
+      geis_bag_remove(self->alarms, i);
+      return GEIS_TRUE;
+    }
+  }
+  return GEIS_FALSE;
+}
+
+GeisBoolean
+geis_grail_xsync_is_timeout(GeisGrailXSync self, const XEvent *event)
+{
+  if (event->type != (self->xsync_event_base + XSyncAlarmNotify))
+    return GEIS_FALSE;
+
+  const XSyncAlarmNotifyEvent *alarm_notify =
+      (const XSyncAlarmNotifyEvent *) event;
+
+  /* If the destruction isn't successful it means that this alarm has already
+     been destroyed due to a previous XSyncAlarmNotifyEvent. Therefore its
+     corresponding timeout has already happened and we shouldn't confirm it
+     again. */
+  return _geis_grail_xsync_destroy_alarm(self, alarm_notify->alarm);
+}
+
+uint64_t
+geis_grail_xsync_get_server_time(const XEvent *event)
+{
+  const XSyncAlarmNotifyEvent *alarm_notify =
+      (const XSyncAlarmNotifyEvent *) event;
+
+  XSyncValue time = alarm_notify->counter_value;
+
+  return (uint64_t)XSyncValueHigh32(time) << 32 | XSyncValueLow32(time);
+}

=== added file 'libutouch-geis/backend/grail/geis_grail_xsync.h'
--- libutouch-geis/backend/grail/geis_grail_xsync.h	1970-01-01 00:00:00 +0000
+++ libutouch-geis/backend/grail/geis_grail_xsync.h	2012-03-30 16:56:28 +0000
@@ -0,0 +1,89 @@
+/**
+ * @file geis_grail_xsync.h
+ * @brief Handles XSync lib usage for the GEIS grail backend
+ */
+/*
+ * Copyright 2012 Canonical Ltd.
+ *
+ * This file is part of UTouch GEIS.
+ *
+ * UTouch GEIS is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published by the
+ * Free Software Foundation, either version 3 of the License, or (at your
+ * option) any later version.
+ *
+ * UTouch GEIS is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with UTouch GEIS.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GEIS_BACKEND_GRAIL_XSYNC_H_
+#define GEIS_BACKEND_GRAIL_XSYNC_H_
+
+#include <X11/Xlib.h> // For Display and XEvent
+#include <stdint.h> // For uint64_t
+#include "geis/geisimpl.h" // For GeisBoolean
+
+/**
+ * The opaque "X Synchronization Extension Library" handler
+ *
+ * It facilitates and encapsulates the use of the XSync extension. Specially
+ * by managing the XSyncAlarm instances needed to implement timeouts.
+ */
+typedef struct GeisGrailXSync *GeisGrailXSync;
+
+/**
+ * Constructs a new GeisGrailXSync instance.
+ *
+ * It returns NULL if the initialization wasn't successful.
+ *
+ * OBS: Having multiple GeisGrailXSync instances is not supported and doesn't
+ * make sense.
+ */
+GeisGrailXSync
+geis_grail_xsync_new(Display *display);
+
+/**
+ * Destroys the given GeisGrailXSync
+ */
+void
+geis_grail_xsync_delete(GeisGrailXSync xsync);
+
+
+/**
+ * Creates an XSyncAlarm with the given timeout
+ *
+ * If there's already an XSyncAlarm with that timeout,
+ * nothing is done.
+ *
+ * An XSyncAlarmNotifyEvent will be sent once that timeout
+ * is reached on the server.
+ */
+void
+geis_grail_xsync_set_timeout(GeisGrailXSync xsync, uint64_t timeout);
+
+/**
+ * Returns true if the given XEvent is an XSyncAlarmNotifyEvent for
+ * one of the existing XSyncAlarm instances and false otherwise.
+ *
+ * It also takes care of destroying the XSyncAlarm that had its
+ * timeout reached.
+ */
+GeisBoolean
+geis_grail_xsync_is_timeout(GeisGrailXSync xsync, const XEvent *event);
+
+/**
+ * Gets the server time contained in the given event.
+ *
+ * That event must be an XSyncAlarmNotifyEvent. It will be the case when
+ * geis_grail_xsync_is_timeout() returns GeisTrue for it.
+ *
+ */
+uint64_t
+geis_grail_xsync_get_server_time(const XEvent *event);
+
+#endif // GEIS_BACKEND_GRAIL_XSYNC_H_