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

Proposed by Andrea Azzarone on 2012-08-02
Status: Merged
Approved by: Daniel van Vugt on 2012-08-07
Approved revision: 3300
Merged at revision: 3305
Proposed branch: lp:~azzar1/compiz/fix-960652
Merge into: lp:compiz/0.9.8
Diff against target: 100 lines (+24/-9)
1 file modified
src/event.cpp (+24/-9)
To merge this branch: bzr merge lp:~azzar1/compiz/fix-960652
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-08-02 Approve on 2012-08-07
Review via email: mp+117933@code.launchpad.net

Commit message

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

To post a comment you must log in.
Sam Spilsbury (smspillaz) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

> 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
360 if (!xkbEvent.get() && !mods)360 if (!xkbEvent.get() && !mods)
361 return false;361 return false;
362362
363 bool handled = false;
364
363 foreach (CompOption &option, options)365 foreach (CompOption &option, options)
364 {366 {
365 if (isBound (option, CompAction::BindingTypeKey, state, &action))367 if (isBound (option, CompAction::BindingTypeKey, state, &action))
@@ -373,12 +375,11 @@
373 else if (!xkbEvent.get() && ((mods & modMask & bindMods) != bindMods))375 else if (!xkbEvent.get() && ((mods & modMask & bindMods) != bindMods))
374 match = true;376 match = true;
375377
376 if (match && eventManager.triggerRelease (action, state, arguments))378 handled |= match && eventManager.triggerRelease (action, state, arguments);
377 return true;
378 }379 }
379 }380 }
380381
381 return false;382 return handled;
382}383}
383384
384bool385bool
@@ -417,6 +418,7 @@
417 else if (event->event_type == KeyRelease)418 else if (event->event_type == KeyRelease)
418 {419 {
419 state = CompAction::StateTermKey;420 state = CompAction::StateTermKey;
421 bool handled = false;
420422
421 foreach (CompOption &option, options)423 foreach (CompOption &option, options)
422 {424 {
@@ -430,11 +432,13 @@
430 if ((event->mods && ((event->mods & modMask) != bindMods)) ||432 if ((event->mods && ((event->mods & modMask) != bindMods)) ||
431 (!event->mods && (modKey == bindMods)))433 (!event->mods && (modKey == bindMods)))
432 {434 {
433 if (eventManager.triggerRelease (action, state, arguments))435 handled |= eventManager.triggerRelease (action, state, arguments);
434 return true;
435 }436 }
436 }437 }
437 }438 }
439
440 if (handled)
441 return true;
438 }442 }
439443
440 return false;444 return false;
@@ -757,6 +761,7 @@
757 }761 }
758 break;762 break;
759 case KeyRelease:763 case KeyRelease:
764 {
760 o[0].value ().set ((int) event->xkey.window);765 o[0].value ().set ((int) event->xkey.window);
761 o[1].value ().set ((int) orphanData.activeWindow);766 o[1].value ().set ((int) orphanData.activeWindow);
762 o[2].value ().set ((int) event->xkey.state);767 o[2].value ().set ((int) event->xkey.state);
@@ -770,13 +775,19 @@
770 o[6].value ().set ((int) event->xkey.keycode);775 o[6].value ().set ((int) event->xkey.keycode);
771 o[7].value ().set ((int) event->xkey.time);776 o[7].value ().set ((int) event->xkey.time);
772777
778 bool handled = false;
779
773 foreach (CompPlugin *p, CompPlugin::getPlugins ())780 foreach (CompPlugin *p, CompPlugin::getPlugins ())
774 {781 {
775 CompOption::Vector &options = p->vTable->getOptions ();782 CompOption::Vector &options = p->vTable->getOptions ();
776 if (triggerKeyReleaseBindings (options, &event->xkey, o))783 handled |= triggerKeyReleaseBindings (options, &event->xkey, o);
777 return true;
778 }784 }
785
786 if (handled)
787 return true;
788
779 break;789 break;
790 }
780 case EnterNotify:791 case EnterNotify:
781 if (event->xcrossing.mode != NotifyGrab &&792 if (event->xcrossing.mode != NotifyGrab &&
782 event->xcrossing.mode != NotifyUngrab &&793 event->xcrossing.mode != NotifyUngrab &&
@@ -977,12 +988,16 @@
977 if (stateEvent->event_type == KeyPress)988 if (stateEvent->event_type == KeyPress)
978 eventManager.resetPossibleTap();989 eventManager.resetPossibleTap();
979990
991 bool handled = false;
992
980 foreach (CompPlugin *p, CompPlugin::getPlugins ())993 foreach (CompPlugin *p, CompPlugin::getPlugins ())
981 {994 {
982 CompOption::Vector &options = p->vTable->getOptions ();995 CompOption::Vector &options = p->vTable->getOptions ();
983 if (triggerStateNotifyBindings (options, stateEvent, arg))996 handled |= triggerStateNotifyBindings (options, stateEvent, arg);
984 return true;
985 }997 }
998
999 if (handled)
1000 return true;
986 }1001 }
987 else if (xkbEvent->xkb_type == XkbBellNotify)1002 else if (xkbEvent->xkb_type == XkbBellNotify)
988 {1003 {

Subscribers

People subscribed via source and target branches