Merge lp:~tkluck/grail/coordinate-transform into lp:grail

[Timo Kluck]

See https://bugs.launchpad.net/utouch-grail/+bug/788128.
Depends on https://code.launchpad.net/~tkluck/utouch-frame/coordinate-transform/+merge/73390

[Chase Douglas (chasedouglas)]

Small note: I don't think the callback needs to be in a new deprecated api section. You can add it just below grail_get_contact_frame() in grail.h.

The changes use malloc to allocate memory. malloc is not signal context safe (see "safe functions" in the signal(2) manpage). You probably aren't aware, or might have forgotten if you were (I almost forgot :), that X input module event handling is run in signal context. If we were to merge this code, we would find it works just fine on many machines, usually fine but occasionally crashy on some machines, and very crashy on other machines. It's a very indeterminate bug, and very difficult to try to figure out!

I'm not sure how to get around this. You could try alloca, but it's not an officially "safe" function to call in signal context, and in general alloca use is frowned upon. You could try to compile it with c99 standard variable length arrays, but I don't know if that's safe either, and we would have to switch grail to a c99 library. Alternatively, you could allocate a constant number of touches, but there's no bounds on how many touches there might actually be. If you set it to 10 touches and then ran it on a 20 touch monitor and touched with 11 touches you would crash.

This all begins to feel like this isn't the best place to implement the transformation. Perhaps the transformation should be called from within utouch-frame instead.

I really appreciate the thought and effort you put into this! I don't want you to be discouraged by my disapproval, especially since it is likely due to something you weren't aware of. I look forward to reviewing another iteration of changes :).

review: Disapprove

[Timo Kluck (tkluck)]

Chase: thank you for your extensive comments! I hadn't realized the issue with being in a signal handler at all, so thanks for teaching me.

I moved the transformation code to utouch-frame. The code changes are now minimal and clean. Additionally, the patch doesn't have the issue I mentioned previously, of transformed frames containing pointers to non-transformed frames. Neither does it have the issue of having to call malloc.

[Chase Douglas (chasedouglas)]

The code looks reasonable. However it will depend on a newer version of utouch frame. Please add a commit to check for utouch-frame 1.2.0 in configure.ac.

review: Needs Fixing

[lp:~tkluck/grail/coordinate-transform]

170. By Timo Kluck on 2011-08-31

Bump version (note API extension)

[Timo Kluck (tkluck)]

Done. I also bumped the version because of the API extension, just like in utouch-frame.

I accidently tagged it with both "v1.2.0" and "v2.1.0", but I can't seem to remove the first one from launchpad.

review: Needs Resubmitting

[Stephen M. Webb (bregma)]

This looks OK now. Don;t worry about the tags.

review: Approve

[Chase Douglas (chasedouglas)]

Everything looks good now!

As with utouch-frame, I realize now that we don't want to tag this until we've runtime tested it and everything looks good.

For future reference, if you want to delete a tag on launchpad use the -d flag:

bzr tag -d lp:~tkluck/utouch-grail/coordinate-transform --delete v2.1.0

review: Approve

=== modified file 'configure.ac'
--- configure.ac	2011-08-09 21:52:30 +0000
+++ configure.ac	2011-08-31 13:18:37 +0000
@@ -1,7 +1,7 @@
 # Initialize Autoconf
 AC_PREREQ([2.60])
 AC_INIT([Gesture Recognition And Instantiation Library],
-        [2.0.1],
+        [2.1.0],
         [],
         [utouch-grail])
 AC_CONFIG_SRCDIR([Makefile.am])
@@ -12,7 +12,7 @@
 AM_INIT_AUTOMAKE([foreign dist-bzip2])
 AM_MAINTAINER_MODE
 
-LIB_VERSION=2:0:1
+LIB_VERSION=3:0:2
 AC_SUBST([LIB_VERSION])
 
 # Initialize libtool
@@ -24,7 +24,7 @@
 
 PKG_CHECK_MODULES([MTDEV], [mtdev >= 1.1])
 PKG_CHECK_MODULES([EVEMU], [utouch-evemu >= 1.0.5])
-PKG_CHECK_MODULES([FRAME], [utouch-frame >= 1.0])
+PKG_CHECK_MODULES([FRAME], [utouch-frame >= 1.2])
 
 AC_ARG_WITH([xi], AS_HELP_STRING([--with-xi], [Build with XI2.1 support]))
 AM_CONDITIONAL([HAVE_XI], [test "x$with_xi" != "x"])

=== modified file 'include/grail.h'
--- include/grail.h	2011-05-18 16:41:41 +0000
+++ include/grail.h	2011-08-31 13:18:37 +0000
@@ -468,6 +468,11 @@
 
 #endif
 
+void GRAIL_PUBLIC grail_set_coordinate_transform_callback(struct grail *ge,
+							  utouch_coordinate_transform_cb callback,
+							  void *user_data);
+
+
 #ifdef __cplusplus
 }
 #endif

=== modified file 'src/grail-api.c'
--- src/grail-api.c	2011-08-09 10:03:32 +0000
+++ src/grail-api.c	2011-08-31 13:18:37 +0000
@@ -321,3 +321,9 @@
 
         return count > 0 ? count : ret;
 }
+
+void GRAIL_PUBLIC grail_set_coordinate_transform_callback(struct grail *ge,
+							  utouch_coordinate_transform_cb callback,
+							  void *user_data) {
+	utouch_frame_set_coordinate_transform_callback(ge->impl->fh, callback, user_data);
+}