Merge lp:~vanvugt/compiz-core/fix-925293.2 into lp:compiz-core

[Daniel van Vugt]

Improved the fix for LP: #925293 (add tap support):
  * Add correct tap detection for non-modifier-only key combinations.
  * Avoid getting "stuck" with modifiers apparently locked down.
  * Ungrab the keyboard on key releases, not presses.
  * ReplayKeyboard of release events as well as presses (previously only
    presses, which could cause release events to queue up and block the
    server).

My test case which previously caused modifiers to get "stuck" now works. The test case was "Add correct tap detection for non-modifier-only key combinations", which caused the modifiers-stuck problem in previous revisions.

[Sam Spilsbury (smspillaz)]

> XAllowEvents (priv->dpy, ReplayKeyboard, event->xkey.time);

Do we need to do that if an action event was handled ? I would think that AsyncKeyboard makes more sense here.

[Sam Spilsbury (smspillaz)]

62 - if (priv->grabs.empty ()

You should also change tapGrab to false whenever a plugin takes a grab too.

[Daniel van Vugt (vanvugt)]

Both good points. Though I haven't tested AsyncKeyboard where you say...

[Sam Spilsbury (smspillaz)]

actionEventHandled ? AsyncKeyboard : ReplayKeyboard

This is what I did when I fixed it locally.

[Daniel van Vugt (vanvugt)]

Yes, I understand what you meant thanks. Now testing changes...

[Sam Spilsbury (smspillaz)]

Looks great, thanks!

review: Approve

=== modified file 'src/event.cpp'
--- src/event.cpp	2012-02-15 11:21:07 +0000
+++ src/event.cpp	2012-02-17 09:23:17 +0000
@@ -136,32 +136,24 @@
                              CompAction::State   state,
                              CompOption::Vector &arguments)
 {
-    possibleTap = action;
-    modTapGrab = false;
-
     if (state == CompAction::StateInitKey &&
         grabs.empty () &&
-        action->key ().modifiers () &&
-        action->key ().keycode () == 0)
+        !action->terminate ().empty ())
     {
-        /*
-         * So you're trying to detect taps on a modifier key?
-         * You will need to grab the keyboard SYNCHRONOUSLY.
-         * This is so that later if the user presses another key with
-         * this modifier, we will be able to ReplayKeyboard with
-         * XAllowEvents. Otherwise that modifier+other_key event would
-         * be absorbed and lost.
-         */
-        XGrabKeyboard (dpy, grabWindow, True,
-                       GrabModeAsync, GrabModeSync, CurrentTime);
-        XAllowEvents (dpy, SyncKeyboard, CurrentTime);
-        modTapGrab = true;
+        possibleTap = action;
+        int err = XGrabKeyboard (dpy, grabWindow, True,
+                                 GrabModeAsync, GrabModeSync, CurrentTime);
+        if (err == GrabSuccess)
+        {
+            XAllowEvents (dpy, SyncKeyboard, CurrentTime);
+            tapGrab = true;
+        }
     }
 
-    if (action->initiate ().empty ())
+    if (action->initiate ().empty () && !action->terminate ().empty ())
     {
-        /* Default Initiate implementation for plugins that only provide
-           a Terminate callback (as you might want if responding to taps) */
+        /* Default Initiate implementation for plugins that only
+           provide a Terminate callback */
         if (state & CompAction::StateInitKey)
             action->setState (action->state () | CompAction::StateTermKey);
     }
@@ -180,8 +172,6 @@
     {
         state |= CompAction::StateTermTapped;
         possibleTap = NULL;
-        if (modTapGrab)
-            XUngrabKeyboard (dpy, CurrentTime);
     }
 
     if (!action->terminate ().empty () &&
@@ -1086,14 +1076,17 @@
 	actionEventHandled = true;
     }
 
-    if (priv->grabs.empty () && event->type == KeyPress)
-    {
-	if (priv->modTapGrab)
-	{
-	    XAllowEvents (priv->dpy, ReplayKeyboard, event->xkey.time);
-	    priv->modTapGrab = false;
-	}
+    if (priv->tapGrab &&
+        (event->type == KeyPress || event->type == KeyRelease))
+    {
+	int mode = actionEventHandled ? AsyncKeyboard : ReplayKeyboard;
+	XAllowEvents (priv->dpy, mode, event->xkey.time);
+    }
+
+    if (priv->grabs.empty () && event->type == KeyRelease)
+    {
 	XUngrabKeyboard (priv->dpy, event->xkey.time);
+	priv->tapGrab = false;
     }
 
     if (actionEventHandled)

=== modified file 'src/privatescreen.h'
--- src/privatescreen.h	2012-02-16 01:11:23 +0000
+++ src/privatescreen.h	2012-02-17 09:23:17 +0000
@@ -541,7 +541,7 @@
 	Window	      xdndWindow;
 
 	void *possibleTap;
-	bool  modTapGrab;
+	bool  tapGrab;
 
 	bool initialized;
 };

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-02-16 01:11:23 +0000
+++ src/screen.cpp	2012-02-17 09:23:17 +0000
@@ -2942,6 +2942,8 @@
 		XUngrabPointer (priv->dpy, CurrentTime);
 		return NULL;
 	    }
+	    else
+	        priv->tapGrab = false;
 	}
 	else
 	    return NULL;
@@ -3004,6 +3006,7 @@
 
 	XUngrabPointer (priv->dpy, CurrentTime);
 	XUngrabKeyboard (priv->dpy, CurrentTime);
+	priv->tapGrab = false;
     }
 }
 
@@ -3580,6 +3583,7 @@
 
     XUngrabPointer (priv->dpy, CurrentTime);
     XUngrabKeyboard (priv->dpy, CurrentTime);
+    priv->tapGrab = false;
 
     XSendEvent (priv->dpy, priv->root, false,
 		StructureNotifyMask, &ev);
@@ -4985,7 +4989,7 @@
     edgeWindow (None),
     xdndWindow (None),
     possibleTap (NULL),
-    modTapGrab (false),
+    tapGrab (false),
     initialized (false)
 {
     TimeoutHandler *dTimeoutHandler = new TimeoutHandler ();