Merge lp:~azzar1/compiz/fix-960652 into lp:compiz/0.9.8

[Andrea Azzarone]

Always call terminate callbacks for key bindings. ATM we call just the first
callback. (LP: #960652)

[Sam Spilsbury (smspillaz)]

This looks correct, as we discussed. You can probably change statements like

> 19 + handled |= match && eventManager.triggerRelease (action, state, arguments);

changed to

bool success = false;

if (match)
    success = eventManager.triggerRelease (action, state, arguments);

handled |= success;

(|= with bool is safe AIUI)

Are you planning to add any tests for this? Preferably not manual?

[Daniel van Vugt (vanvugt)]

Using a bool in a bitwise expression may work, but is ugly practice. I think "bool" is a semi-opaque type. So its bitwise representation is unknown (other than it is probably != 0). I could be wrong (haven't looked at the C++ spec).

In the very least, bool in a bitwise expression will generate cppcheck warnings (http://sourceforge.net/apps/mediawiki/cppcheck/index.php?title=Main_Page#Other).

[Daniel van Vugt (vanvugt)]

Hmm, the spec says it's OK, kind of... 0 is always converted to false, !=0 always true. true is always converted to 1 and false is always converted to 0. By doing "handled |= x" you're asking x to be converted from a bool to int, handled to be converted from a bool to an int, and the resulting int to be converted back to a bool and stored in handled.

The spec allows this, but it is ugly.

[Daniel van Vugt (vanvugt)]

I can't see any problems. And the fix itself is much simpler and more benign than I initially imagined it would be. I also can't find any regressions in playing with it.

This region of code is very fragile, so any automated tests would have to be external (not modifying code).

I'm happy to ignore the bool/int conversions for now, since the spec does at least allow for it. But not recommended in future.

review: Approve

[Sam Spilsbury (smspillaz)]

> This region of code is very fragile, so any automated tests would have to be external (not modifying code).

I would really prefer it if we did not use this as our reason for bypassing the "you touch it, you test it" policy.

Its possible to safely modify code to get it in a test harness. See the methods used in Feathers' Working with Legacy Code[1]. If people don't have that book, I would highly recommend that they talk to their line manager and expense it and read it.

In this instance, I don't see getting this code into a test harness as that difficult. For example, if you did sprout method here on the KeyRelease handler:

bool
triggerAllReleaseEvents (unsigned int eventMods,
                         unsigned int eventKeyCode,
                         ModifierHandler *modHandler,
                         CompOption::Vector &pptions,
                         CompOption::Vector &arguments,
                         compiz::private_screen::EventManager &eventManager)

{
 state = CompAction::StateTermKey;
 bool handled = false;

 foreach (CompOption &option, options)
 {
     if (isBound (option, CompAction::BindingTypeKey, state, &action))
     {
  bindMods = modHandler->virtualToRealModMask (
      action->key ().modifiers ());
  unsigned int modKey =
      modHandler->keycodeToModifiers (eventKeycode);

  if ((event->mods && ((eventMods & modMask) != bindMods)) ||
      (!event->mods && (modKey == bindMods)))
  {
      handled |= eventManager.triggerRelease (action, state, arguments);
  }
     }
 }

 if (handled)
     return true;
        return false;
}

And then it would be pretty easy to write a test method for whether or not triggerAllReleaseEvents
a) actually triggered all events and
b) set the handled flag and causes the caller to return true;

Please spend some time considering how to test code when modifying it. Not only will you get the code under test, but you'll actually make the code better in the meantime, and you save headache for everyone else.

[1] http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

=== modified file 'src/event.cpp'
--- src/event.cpp	2012-06-24 09:00:27 +0000
+++ src/event.cpp	2012-08-02 15:53:41 +0000
@@ -360,6 +360,8 @@
     if (!xkbEvent.get() && !mods)
 	return false;
 
+    bool handled = false;
+
     foreach (CompOption &option, options)
     {
 	if (isBound (option, CompAction::BindingTypeKey, state, &action))
@@ -373,12 +375,11 @@
 	    else if (!xkbEvent.get() && ((mods & modMask & bindMods) != bindMods))
 	        match = true;
 
-	    if (match && eventManager.triggerRelease (action, state, arguments))
-		return true;
+	    handled |= match && eventManager.triggerRelease (action, state, arguments);
 	}
     }
 
-    return false;
+    return handled;
 }
 
 bool
@@ -417,6 +418,7 @@
     else if (event->event_type == KeyRelease)
     {
 	state = CompAction::StateTermKey;
+	bool handled = false;
 
 	foreach (CompOption &option, options)
 	{
@@ -430,11 +432,13 @@
 		if ((event->mods && ((event->mods & modMask) != bindMods)) ||
 		    (!event->mods && (modKey == bindMods)))
 		{
-		    if (eventManager.triggerRelease (action, state, arguments))
-			return true;
+		    handled |= eventManager.triggerRelease (action, state, arguments);
 		}
 	    }
 	}
+
+	if (handled)
+	    return true;
     }
 
     return false;
@@ -757,6 +761,7 @@
 	}
 	break;
     case KeyRelease:
+    {
 	o[0].value ().set ((int) event->xkey.window);
 	o[1].value ().set ((int) orphanData.activeWindow);
 	o[2].value ().set ((int) event->xkey.state);
@@ -770,13 +775,19 @@
 	o[6].value ().set ((int) event->xkey.keycode);
 	o[7].value ().set ((int) event->xkey.time);
 
+	bool handled = false;
+
 	foreach (CompPlugin *p, CompPlugin::getPlugins ())
 	{
 	    CompOption::Vector &options = p->vTable->getOptions ();
-	    if (triggerKeyReleaseBindings (options, &event->xkey, o))
-		return true;
+	    handled |= triggerKeyReleaseBindings (options, &event->xkey, o);
 	}
+
+        if (handled)
+	    return true;
+
 	break;
+    }
     case EnterNotify:
 	if (event->xcrossing.mode   != NotifyGrab   &&
 	    event->xcrossing.mode   != NotifyUngrab &&
@@ -977,12 +988,16 @@
 		if (stateEvent->event_type == KeyPress)
 		    eventManager.resetPossibleTap();
 
+		bool handled = false;
+
 		foreach (CompPlugin *p, CompPlugin::getPlugins ())
 		{
 		    CompOption::Vector &options = p->vTable->getOptions ();
-		    if (triggerStateNotifyBindings (options, stateEvent, arg))
-			return true;
+		    handled |= triggerStateNotifyBindings (options, stateEvent, arg);
 		}
+
+		if (handled)
+		    return true;
 	    }
 	    else if (xkbEvent->xkb_type == XkbBellNotify)
 	    {