Created attachment 160832 [details] [review]
Rev 1

Joanie, sorry for the large patch, but cant be helped.

Tested and pylinted to 10 bar recursion issues.
almost a thousand lines removed from default.py, reduced from 
6405 to 5491

broken into {common,desktop,laptop}_keyboardmap.py
a helper function in keybindings.py

Tried with laptop and desktop layouts, and everything seems to be working fine.

Does this look reasonable?

-Jon

Review of attachment 160832 [details] [review]:

I think it does look reasonable. And a shorter default.py is a happy default.py. :-) Nice!

Questions:

1. We still have a (comparatively short) getBrailleBindings() method in default.py. In the spirit of being consistent, would it be worth making a braille_keyboardmap.py?  I'm thinking the answer is yes, but I'm willing to listen to arguments to the contrary. <smile> If you agree that it is worth doing, I think that would still fall under the umbrella of this bug.

2. What do we want to do about scripts which still have the old-school bindings?
   a. If they only have a few bindings (e.g. Evolution, OpenOffice)
   b. If they have quite a few bindings (e.g. Gecko)
   Handling this would be a new/separate bug, but I think it's worth thinking about now.

3. What do we want to do about how we write out (and import) keybindings which the user has bound/rebound? (This would be a new/separate bug as well. But I'm wondering if that issue would have any impact on your solution here. That's not a trick question. It's 6 AM, I haven't had coffee yet. <grin> A valid answer very well may be, "There's no impact whatsoever on this bug." I'm just asking you to think about it and confirm to me if that is indeed the case.)

Another pre-coffee question: Should the input event handlers also be pulled out? (again, if so it would be another bug). Just something to ponder....

(In reply to comment #2)> 
> I think it does look reasonable. And a shorter default.py is a happy
> default.py. :-) Nice!

:) I also have hopes for moving all the handlers to a handlers file and doing some more on the fly function correlation.. but thats another matter
Just saw that you posted another comment :) also thinking of the handlers.

> Questions:
> 
> 1. We still have a (comparatively short) getBrailleBindings() method in
> default.py. In the spirit of being consistent, would it be worth making a
> braille_keyboardmap.py?  I'm thinking the answer is yes, but I'm willing to
> listen to arguments to the contrary. <smile> If you agree that it is worth
> doing, I think that would still fall under the umbrella of this bug.

the code wouldnt get any simpler, and all that would be done would be moving the try-except block, and then having to re-import it here. (maybe not worth it, too much segmentation for small amount of code me thinks)

> 2. What do we want to do about scripts which still have the old-school
> bindings?
>    a. If they only have a few bindings (e.g. Evolution, OpenOffice)
>    b. If they have quite a few bindings (e.g. Gecko)
>    Handling this would be a new/separate bug, but I think it's worth thinking
> about now.

Two alternatives:
* Externalize the keybindings as what was done above
* handle it inline. 

(example of inline) This would be part of the solution if we did it for soffice:


    def getKeyBindings(self):
        """Defines the key bindings for this script. Setup the default
        key bindings, then add one in for reading the input line.

        Returns an instance of keybindings.KeyBindings.
        """

        keyBindings = default.Script.getKeyBindings(self)
        soffice_keymap = ( 
          ("a", settings.defaultModifierMask, settings.ORCA_MODIFIER_MASK,
          "presentInputLineHandler"),
          ...
        )
        keyBindings.load(soffice_keymap, self.inputEventHandlers)
        ...
        return keyBindings

hmm, dont know if indentation comes out ok in these comments.

> 3. What do we want to do about how we write out (and import) keybindings which
> the user has bound/rebound? (This would be a new/separate bug as well. But I'm
> wondering if that issue would have any impact on your solution here. That's not
> a trick question. It's 6 AM, I haven't had coffee yet. <grin> A valid answer
> very well may be, "There's no impact whatsoever on this bug." I'm just asking
> you to think about it and confirm to me if that is indeed the case.)

At the moment what is written out and being read back in is exactly what it has been before.
We are just making code cleaner for our purposes (the actual orca code, not the code that user might have in their customizations)
But you are right, it could be simplified too.

Okay, based on your observations, I propose the following:

1. Commit the work you've done to master. Were it me, I'd also give the list a heads-up. But this change isn't so huge that I think doing so is essential.

2. Tackle the things which you think are worth tackling (in particular, handlers and other scripts) in new bugs. As I mentioned before, this is the sort of work I want to knock out earlier rather than later in the cycle. Thus if you'd like to do it for 3.0, I'd prefer the aim be to get it done in the weeks to come.

Thanks again! Default.py definitely looks a lot more sane. :-)

Thanks! I will finish off the keybindings for scripts before moving onto handlers.
 
Committed to master:
http://git.gnome.org/browse/orca/commit/?id=49c7a5faad85c07b7bf368e3a2280ef169c3454b

Awesome thanks!

BTW, right now the Pidgin script has keybindings. However, once I convert that to use the generic chat support, the generic chat support will handle the bindings. Therefore, I wouldn't bother to change the Pidgin script unless you're just really, really bored. <smile>