GNOME Bugzilla – Bug 494196
Refactor keybindings to always care about Control/Shift/Alt/Orca modifiers
Last modified: 2008-06-17 20:02:24 UTC
The creation of a keybinding looks something like the following, where the 2nd parameter is a mask of the modifiers we care about and the 3rd parameter is a mask of what we want their states to be: keyBindings.add( keybindings.KeyBinding( "KP_Delete", orcaModMask, 0, self.inputEventHandlers["findHandler"])) The main motivation for doing things like this was to allow us to avoid conditions where locking modifiers (e.g., NumLock and CapsLock) could conflict with decisions on whether to process a keystroke or not. As seen in bug 481488, we see that we may often need to go back to existing keybindings and redefine them to include more modifiers in the 2nd parameter. The main reason for this is that new keybindings may end up causing ambiguous keybindings and the additional modifiers are needed for disambiguation. As a possible compatible workaround, we might consider always considering non-locking modifiers in the 2nd parameter. This can be a bit difficult to do without scouring the X Server's keymapping, so we might just consider always adding all of the Shift, Control, Alt, and Orca modifiers (perhaps also the MODx modifiers as well) to the 2nd parameter.
Joanie, Rich, Eitan, Scott: what are your thoughts on this bug? Should we try to address it for GNOME 2.22?
First coarse pass at GNOME 2.24 planning.
Created attachment 112259 [details] [review] revision 1 First pass at this. Pylints to 10.0's and passes the regression tests (Note that you MUST have the very latest macaroon for the latter to be the case. Thanks Eitan!!) This patch puts the various modifier masks (and combinations thereof) which we are using frequently in settings.py, and it causes orca to "always care about" all non-locking modifiers as per team meeting discussion from a couple of weeks ago. Will: If I include pyatspi.MODIFIER_META among those we care about, things fall apart. I haven't worked out exactly why yet, so I merely commented that one modifier out in settings.py (see NON_LOCKING_MODIFIER_MASK). I did notice however, that the very same modifier is commented out in keybindings.py's getModifierNames(): #if mods & (1 << pyatspi.MODIFIER_META): # text += _("Meta") + "+" So.... How badly do we want to ensure that we "care about" Meta in our keybindings? Thanks!
BTW, the commenting out of META in keybindings.py took place in revision 1818: Author: wwalker Date: Wed Dec 13 01:21:31 2006 UTC (17 months, 3 weeks ago) Log Message: Commit Jorge Sandin's keybindings edit feature. http://svn.gnome.org/viewvc/orca/trunk/src/orca/keybindings.py?r1=1715&r2=1818&pathrev=1818
(In reply to comment #3) > So.... How badly do we want to ensure that we "care about" Meta in our > keybindings? I believe it's part of the toxic NUMLOCK world, so let's avoid it. bash-3.2$ python -c "import pyatspi; print pyatspi.MODIFIER_META, pyatspi.MODIFIER_NUMLOCK" 4 7 This says that META is bound to the bit corresponding to 1 shifted left 4 times. I threw NUMLOCK in there just because it points out how inconsistent NUMLOCK support is across the board. bash-3.2$ xmodmap xmodmap: up to 2 keys per modifier, (keycodes in parentheses): shift Shift_L (0x32), Shift_R (0x3e) lock Caps_Lock (0x42) control Control_L (0x25), Control_R (0x6d) mod1 Alt_L (0x40) mod2 Num_Lock (0x4d) mod3 Mode_switch (0x71) mod4 Meta_L (0x73), Meta_R (0x74) mod5 bash-3.2$ grep -i mod2 /usr/include/X11/* /usr/include/X11/X.h:#define Mod2Mask (1<<4) /usr/include/X11/X.h:#define Mod2MapIndex 4 This says that the default X11 keymap binds the Num_Lock key to its notion of mod2, which is the same bit offset as is described by META. So, avoid META.
Created attachment 112445 [details] [review] revision 2 This avoids meta -- both by not adding it to the modifiers we "care about" and also be removing it from the places in which we were attending to it (e.g. when trying to identify if a keypress was a command, and in the __init__ of the KeyboardEvent class in input_event.py). I assume this is the right thing to do. Fortunately, with these things living in settings.py now (assuming that is also the right thing to do), it's easy enough to put Meta back should we change our minds. Pylinted and regression tested. Will please review when you have a chance. Thanks!
(In reply to comment #6) > Will please review when you have a chance. Thanks! Thanks Joanie! Definitely cleans things up and it looks like you fixed some latent bugs in the process. Yeah. I do have some comments, though. :-) My assumption is that we generally want to end up with settings.defaultModifierMask used as the modifier_mask parameter for nearly all (or all?) keybindings. I see several instances where NO_MODIFIER_MASK is used instead. For example: keyBindings.add( keybindings.KeyBinding( "", - None, - None, + settings.NO_MODIFIER_MASK, + settings.NO_MODIFIER_MASK, self.inputEventHandlers["toggleBuddyTypingHandler"])) While I realize all instances of this pattern are for unbound keybindings, there are also some unbound keybindings that use settings.defaultModifierMask. It would be nice to pick just one way and stick with it (I'd opt for settings.defaultModifierMask I think). I think the creation of custom keybindings should probably also use settings.defaultModifierMask. I'm not sure of the exact place to do this, but I think the self.newBinding call around line 3481 in orca_gui_prefs.py might be the spot. As a sanity check, can you also try to do some manual testing with NumLock locked and unlocked just to make sure that toxic modifier doesn't poison the new world order? Thanks again!
Created attachment 112519 [details] [review] revision 3 > While I realize all instances of this pattern are for unbound keybindings, > there are also some unbound keybindings that use settings.defaultModifierMask. > It would be nice to pick just one way and stick with it (I'd opt for > settings.defaultModifierMask I think). Done and done. I also noticed that sometimes we were using an empty string and other times None for the keysymstring of an unbound keybinding. I'm sure I'm at least possibly (and perhaps completely) responsible for that. Since it's a keysymSTRING, we're now consistently using an empty string for unbound keybindings. I tested this both for Orca-wide and app-specific keybindings (and unbindings). > I think the creation of custom keybindings should probably also use > settings.defaultModifierMask. I'm not sure of the exact place to do this, but > I think the self.newBinding call around line 3481 in orca_gui_prefs.py might be > the spot. Yup, there and also in orca_prefs.py (where the things get written to file). Done. > As a sanity check, can you also try to do some manual testing with NumLock > locked and unlocked just to make sure that toxic modifier doesn't poison the > new world order? Done. All (re)pylinted to 10's and (re)regression tested. I think we're good. (Famous last words). As discussed in team meeting today, this patch has been committed. Uh oh. ;-) On a positive note, I'm all caught up on my course stuff for tomorrow, so if we find out I've just broken all of Orca, I'm in a position to do something about it ASAP. :-)
Haven't seen any problems with this one so I think it's OK to close.
Thanks Mike. Closing as fixed.