Moving to FUTURE because I know it's low, low priority should we even decide to do it. :-)

Created attachment 98299 [details] [review]
take 1

Okay, I'm afraid this has become an "unbound keybindings and then some" patch as I noticed a few things while implementing it.  This patch:

1. Fixes a minor migration nit: The keybindings for Save bookmarks and Go to previous bookmark location had become the same keybinding.

2. Stops the automatic re-sorting of the treeview of keybindings immediately after editing.  For instance, let's say for some wacky reason you decided to re-bind Save bookmarks to an unmodified 'b'.  The minute you press Enter to confirm, Save bookmarks moves to near the very top of the list.  If you want to look at (what was) the next item, you have to work your way back down.  I had not really noticed this behavior before; when deleting keybindings, it's a bit more obvious as you keep flying to the top of the tree.  I addressed it with the addition of a couple of invisible columns which retain a copy of the keybinding prior to pressing the Apply button and sorting on them.  Yes it's a hack.  Suggestions welcome (I looked at using TreeSortable as well as set_sort_func() with varying degrees of success.)

3. Fixes a bug in which pressing the Apply button in an app-specific preferences dialog will duplicate the primary keybinding in the alternate column (if the alternate column is empty) and tack on another copy of the braille keybindings at the end of the tree.  The way I chose to fix it (in large part to help make the unbinding thang happen), was to move the burden of obtaining keybindings to app_gui_prefs.py from orca_gui_prefs.py.  Before app_gui_prefs.py just got its special keystrokes.  But it knows about the rest, including those that have been modified via app-specific settings.  Seems to work anyway.

4. Stops the user from assigning an existing keybinding to another handler.  Most of the time anyway.  If the user tries to assign, say, Orca+f to Increase the speech rate, Orca will now complain, telling the user what the conflict is and putting the binding back to what it was.  (If the user tries to assign Orca+C to two different items before pressing Apply, he/she will still succeed).  I'll fix that in take 2.

5. Speaks the key captured with modifiers (e.g. Orca+f) rather than without as we currently do (e.g. f).

And now for something completely different (the details of the fix for this bug) ;-)

1. Unbound keybindings have a keysymstring of None; modifier_mask and modifiers are 0.  If you unbind something that is bound, it should get written out to the appropriate preference file just like any other keybinding.

2. Unbound Orca keybindings should show up in a new node called "unbound".  Unbound app-unique keybindings stay in their app node.  I'm not sure if that's what is wanted or not....

3. To unbind a keybinding via the dialog box, press Enter on it (like you already do to edit it). If you press Backspace or Delete, instead of being told that the key is captured, you're told that it's being deleted.  The confirmation is the same.  If you press Enter to confirm, you'll be told that the keybinding has been removed (assuming you didn't choose an existing keybinding).

4. This patch puts back the rate and speech commands, but makes them unbound.  Proof of concept, and might as well.

Something I need help on:  If you have deleted your flat review keybindings and try to re-bind those keystrokes, they don't get captured as KP_Blah.  They get captured as if NumLock were pressed (e.g. you get "/" rather than KP_Divide).  How do we get the KP_Blah's back?

Rich, given that you are the master of all things Preference and gtk.tree*.... ;-) Would you mind looking this over and taking it for a spin? Thanks!!

> Rich, given that you are the master of all things Preference and gtk.tree*....
> ;-) Would you mind looking this over and taking it for a spin? Thanks!!

I took it for a mini-spin. It mostly seems fine. Nice job! I did notice
one really minor thing though.

I did a Control-Insert-space with focus on gnome-terminal. Up came the 
application specific preferences dialog. I tried to set the key binding 
for "Increase Speech Rate" and "Decrease Speech Rate" to Orca_Modifier+Right
and Orca_Modifier+Left.

I got to that key binding field 
in the table and pressed Enter to open up the text field.

I then Insert+Left and Insert+Right. In each case it nicely captured
the key binding, but in the text area, you could still see "Enter any key".

Like I said, it's minor, and a blind person wouldn't even know it was
happening.

Just reminding myself to review this one closer.

Hold off a bit.  I worked out how to capture the actual bindings so I'm moving some logic out of orca.py and into orca_gui_prefs.py.  Things will look different. :-)

Created attachment 98525 [details] [review]
Take 2 (AKA When keybindings patches attack)

This version pretty much takes the previous version as it was and adds/addresses the following:

1. It should stop the user from assigning an existing keybinding to another handler *all* of the time, as opposed to some of the time (see patch 1, #4) or none of the time.  I might have missed a case or two, and this will need to be pounded on.  The intent is to check both for existing bindings and "pending" bindings.  In other words, if I have just changed Orca+f to Orca+t but haven't pressed the Apply button, I should still be able to assign Orca+f to something else and should be stopped from assigning Orca+t to anything else.

2. It should enter the actual binding (Orca+f) into the editable cell and do so immediately -- rather than just reflecting what the user typed (f) and doing so only after the user has confirmed it.  This should address Rich's "one really minor thing" (which isn't all that minor, really, as it impacts both low vision users and braille users).

3. It enters lower case letters/symbols when Shift is held down (so you can bind Shift+1 rather than Shift+! as is the case currently).  It also records the KP_Blah's (to give us "KP_Divide" rather than "/").

4. It takes an initial stab at an issue I didn't know we had until I started testing wacky bindings and trying to stop the user from double-binding things:  Our modifier masks are a reflection of our established bindings and not of possible user-created bindings.  If you were to try to add Orca+Control+F as a keybinding for something, any time you attempted to use it you'd be getting formatting information instead because getInputHandler() would find that keybinding first and return it.  The simplest solution seemed to be to have getInputHandler() not bail upon finding the first handler that matched unless the keyboard event's modifiers also matched the keybinding's modifier mask.  Along these same lines, I added a "keysNoMask" typeOfSearch to hasKeyBinding().

5. Quite a few mis-matched quotes in orca_gui_prefs.py confused Emacs's syntax highlighting to the point that it threw up its little electronic arms (or perhaps just threw up, judging from the color) and displayed the bulk of the code in what I'll call "peach."  So my apologies for including even more non-unbound-keybinding stuff in this patch, but I just couldn't take it any longer.  (Had it instead been a lovely shade of red....)

So.... Will, when you get a chance, could you please look this over?  I'm sure it's got issues.  Fire when ready. ;-) ;-) ;-)

My two questions about the user experience are:

1. What do we want to do about the bookmarks and chat room message keybindings?  Because they are all bound to the same handler, the first of the series shows up as the Key Binding, the last in the series shows up as the Alternate.  This makes it hard to rebind via the preferences dialog.  It is also a bit confusing I think as the the latter really isn't an Alternate key binding.

2. Unbound Orca keybindings should show up in a new node called "unbound".  Unbound app-unique keybindings stay in their app node.  I'm not sure if that's
what is wanted or not....

My three questions about the user experience are....  (Forgot one)

3. Mike back during the GNOME summit you had remarked that you found the keybinding prompts too verbose.  As long as we're looking at this functionality so closely, now would be an excellent time for any redesign on this front that you would like to spec out.  <smile>  Thanks!

Wow!  Lots of fixes and nice improvements to keybindings support for a bug that's "just" for showing unbound keybindings!  This is all good, and the patch seems to really improve things.

> bindings.  In other words, if I have just changed Orca+f to Orca+t but haven't
> pressed the Apply button, I should still be able to assign Orca+f to something
> else and should be stopped from assigning Orca+t to anything else.

If this can be avoided, it would be nice, but that can be a separate bug/RFE.

> 4. It takes an initial stab at an issue I didn't know we had until I started
> testing wacky bindings and trying to stop the user from double-binding things: 
> Our modifier masks are a reflection of our established bindings and not of
> possible user-created bindings.  If you were to try to add Orca+Control+F as a
> keybinding for something, any time you attempted to use it you'd be getting
> formatting information instead because getInputHandler() would find that
> keybinding first and return it.  The simplest solution seemed to be to have
> getInputHandler() not bail upon finding the first handler that matched unless
> the keyboard event's modifiers also matched the keybinding's modifier mask. 
> Along these same lines, I added a "keysNoMask" typeOfSearch to hasKeyBinding().

As a general solution to think about for sometime in the future for a separate RFE, we might just consider always considering Control/Shift/Alt/Orca in all of the keybindings.  That is, we always care about the state of Control, Shift, Alt, and Orca for all keybindings.

> My two questions about the user experience are:
> 
> 1. What do we want to do about the bookmarks and chat room message keybindings?
>  Because they are all bound to the same handler, the first of the series shows
> up as the Key Binding, the last in the series shows up as the Alternate.  This
> makes it hard to rebind via the preferences dialog.  It is also a bit confusing
> I think as the the latter really isn't an Alternate key binding.

A separate RFE for this would be a good thing.  I'm not quite sure how to handle it since the keybindings code is almost expecting a 1:1 mapping between keystroke and handler.

> 2. Unbound Orca keybindings should show up in a new node called "unbound". 
> Unbound app-unique keybindings stay in their app node.  I'm not sure if that's
> what is wanted or not....

I'm not sure what you mean by the above.  Can you expand a little more?

In any case, this is a huge patch solving lots of problems.  If you've tested it well and feel comfortable with it, I think you should check it in and then work to solve the other problems one at a time.  Thanks for the hard work!

> If this can be avoided, it would be nice, but that can be a separate bug/RFE.

Sounds good.  I *think* I've already implemented handling for all of the possibilities, but testing will let us know for certain.
 
> As a general solution to think about for sometime in the future for a separate
> RFE, we might just consider always considering Control/Shift/Alt/Orca in all of
> the keybindings.  That is, we always care about the state of Control, Shift,
> Alt, and Orca for all keybindings.

To be honest, that thought crossed my mind more than once this weekend. ;-)

I'll wait on filing a new RFE until we've given it some thought, however.  I mean, maybe we want Orca+F to kick in for Control+Orca+F if Control+Orca+F doesn't exist as a binding.  (I'm not saying we do; just saying we might.  I dunno. :-) )

> A separate RFE for this would be a good thing.  I'm not quite sure how to
> handle it since the keybindings code is almost expecting a 1:1 mapping between
> keystroke and handler.

Bug #493891 has been opened for that.

> I'm not sure what you mean by the above.  Can you expand a little more?

Yup.  Let's say that you never, ever care about the formatting of a document and therefore use the Orca Preferences dialog to delete Orca+f.  When you press the Apply button (or press OK and look at the keybindings later), you should find "Reads the attributes associated with the current text character" in the "Unbound" node because that command is no longer bound.  If you have a similar indifference towards headings at level 1 in Firefox and do a similar deletion, "Goes to next heading at level 1" will NOT appear in the unbound node.  It will remain in the Minefield node -- but at the top because the default sort is by keybinding.

On the one hand, I don't think this is a bad thing because we want the app-specific keybindings at the top.  On the other hand, it's an inconsistency.  Is it an inconsistency we care about?  Hypothetically, we could have an "unbound" node just for apps, but that might be overkill.  Or it might not.  I didn't know what the desired user experience is, so I just left it alone.

> In any case, this is a huge patch solving lots of problems.  If you've tested
> it well and feel comfortable with it, I think you should check it in and then
> work to solve the other problems one at a time.  Thanks for the hard work!

I have tested the heck out of it.  But after a while I got tired, took a nap, and dreamed about peach-colored keybindings.  So I decided to walk away from the patch for a bit. ;-)   I'll test it some more and would ask others (Rich, Mike) to please do the same.  I also still need to pyflakechecklint it.

(In reply to comment #8)
>> As a general solution to think about for sometime in the future for a
>> separate RFE, we might just consider always considering Control/Shift
>> /Alt/Orca in all of the keybindings.  That is, we always care about the
>> state of Control, Shift, Alt, and Orca for all keybindings.
>
> To be honest, that thought crossed my mind more than once this weekend. ;-)

Filed as Bug 494196.
 
>  Is it an inconsistency we care about?  Hypothetically, we could have an
> "unbound" node just for apps, but that might be overkill.  Or it might not.  I
> didn't know what the desired user experience is, so I just left it alone.

With the substantial amount of changes being made by this patch, and considering you've tested this patch pretty well, I'd be more comfortable with leaving things as they are (after you pylint/check) and checking the patch in.  The behavior is substantially better than it was.  We can open a separate bug/rfe if needed to handle the inconsistency, but I'd put that lower on the list than our other bugs right now.  :-)

Thanks! 

I've just been trying the latest patch and seems to work just fine 
(modulo the issues that Joanie mentioned in comment #6).

Created attachment 98682 [details] [review]
take 3

In the spirit of paranoia, I attached the above patch without explanation because one never knows when bugzilla will go down again.

Take 3 has been committed to trunk.

Rich had IMed me with another "minor nit":  You can't use Orca_Modifier + a command that already is assigned to something in the GNOME desktop.  His example was Orca+Control+Alt+Down.  Since Control+Alt+Down is the command to move to workspace down, Orca + that command moves you to workspace down when you try to bind it.  You'll see something similar if you try to bind Orca+Alt+F4.  This was true before the changes related to this bug, and it remains true.  What is happening is this:  We need the keystroke to reach the dialog.  The Orca Modifier is not part of the mask, so when the keystroke hits the dialog, it's as if the Orca Modifier were not held down.  I bet I could find a <strike>hack<strike> way to deal with this, but as Will observed a couple of times this is a huge patch and we can always open a new RFE to allow for bindings such as Orca+Control+Alt+Down and Orca+Alt+F4.

HOWEVER, this wound up being a "minor nit" that wasn't so minor after all.  The way we were identifying that editing had been canceled (and thus that key capturing should be turned off) was if Escape and been pressed.  Turns out that switching workspaces will also cancel editing (but not key capturing).  Therefore, the biggest change in take 3 was to add an event handler for the editable's editing-canceled signal.  That way it doesn't matter how or why editing stopped, we'll do the right thing.  Thanks Rich!!

Created attachment 98699 [details] [review]
In Gecko.py we care about most modifiers in the mask

I *knew* there'd be some side effect of not bailing on the first inputEventHandler we came across on the off chance that a more exact match was just around the corner.  I just didn't know what it would be....

Mike found it.  In laptop layout, structural navigation commands were kicking in when flat review commands should have.  This patch is the first step towards the "we care about all modifiers" RFE.  Right now it checks for most modifiers (Orca, Alt, Control, and Shift).  But since the mask is a variable, we can quickly update our definition of "full" in the future.

I checked this patch in already due to the breakage of flat review in FF3 and Thunderbird for laptop users.  Still needs more testing (clearly). *sigh*  Thanks Mike!

Chatting with Mike at the moment.  He indicated that this patch fixed the broken flat review issue.  Phew!  And sorry!  And thanks Mike!

After further testing I think this one is looking good. 

Thanks Mike.  I'm moving this to [pending] just so that we can give it a bit more time and be on the lookout for oddness that might be related.  It was a pretty large change.  

I still haven't seen any more problems related to this bug.  I think it is perhaps OK to close.  What do you all think?

I'm OK with closing it.  Thanks!

Okie dokie.  Thanks guys!