ENB: Discussion of the fix to #579 re key bindings

[Edward K. Ream]

Rev bc4e2f purports to fix #579: Key bindings don't take for commands defined in plugins. I saw no reason to create a separate git branch for this item because the code should be tested in master.

This is an Engineering Notebook post.  It's got too many details for the general public.  However, the issues raised here could be of general concern because of their potential to create bugs.

tl;dr: The new code looks simple and appears to work, but complications lurk. It should be further tested in master.

Terminology

LM stands for the LoadManager class, of which g.app.loadManager is the singleton instance. g.app is the singleton instance of the LeoApp class.

The problems to be solved

#579 complains that there is no way to bind keys to commands defined in plugins. The cause is clear: the old code made key bindings before plugins were loaded.

However, loading any .leo file later can also cause Leo to load new plugins when they appear for the first time in an @enabled-plugins node in the new .leo file.  Kent (and others) have requested this feature, and such "late loading" of plugins is here to stay.

The solution

In some ways the fix is simple, in some ways, quite ugly. At first glance, the solution looks straightforward.  The solution is to call the new LeoApp.makeAllBindings method in just three places.  Here it is:

def makeAllBindings(self):
    '''
    LeoApp.makeAllBindings:
       
    Call c.k.makeAllBindings for all open commanders c.
    '''
    app = self
    for c in app.commanders():
        c.k.makeAllBindings()

So bc4e2f is safe enough to test because making this method a do-nothing would revert Leo's code exactly to its former state.

All this seems straightforward, but...

Complications

There are two troubling complications that I see no way to avoid:

1. Covering all the bases.  At present, Leo calls g.app.makeAllBindings in three places: late in LM.load and also in c.open and c.openRecentFile. LM.load loads all .leo files during startup.  Leo should call g.app.makeAllBindings in all paths that open a file. Failure to do so could create confusion. #579 will rear its ugly head again, but only under unusual circumstances.

I considered and rejected having g.openWithFileName call g.app.makeAllBindings. That would be way too much redundancy.  Details omitted.

2. Having just complained about redundancy, it's ironic/sad that LM.load must call k.makeAllBindings redundantly and indirectly.  Indeed, the call to k.makeAllBindings in k.finishCreate (called early by LM.load) is essential.  Without this early call to k.makeAllBindings, Leo does not make key bindings in the body pane (and other places)! The late call to g.app.makeAllBindings at the end of LM.load happens too late to make the bindings.  So k.makeAllBindings must be called early to make pane bindings and late to handle bindings made in plugins.

Summary

Happily, it appears that redundant calls to k.makeAllBindings "just work" because k.remove_conflicting_definitions is probably clever enough to ignore duplicate bindings.

Afaik, all paths through Leo's code that open outlines call g.app.makeAllBindings. If not, Leo would fail to make bindings to commands made in plugins.

Please report any new key bindings problems immediately.

Edward

[Edward K. Ream]

On Saturday, November 18, 2017 at 5:17:13 AM UTC-6, Edward K. Ream wrote:

2. Having just complained about redundancy, it's ironic/sad that LM.load must call k.makeAllBindings redundantly and indirectly.  Indeed, the call to k.makeAllBindings in k.finishCreate (called early by LM.load) is essential.  Without this early call to k.makeAllBindings, Leo does not make key bindings in the body pane (and other places)! The late call to g.app.makeAllBindings at the end of LM.load happens too late to make the bindings.  So k.makeAllBindings must be called early to make pane bindings and late to handle bindings made in plugins.

I should add that redundant calls to k.makeAllBindings are fast.  They cause no settings files to be loaded.

EKR

[vitalije]

I am writing this without really looking at code, and I don't remember details since last time I have read the code, so feel free to ignore my comment.

How about calling makeAllBindings from plugin manager after loading any group of plugins? If I understand correctly old code worked fine and the only problem was with the commands defined in the plugins. It seems to me that the only case that needed to be addressed was case of loading plugins. 

Another idea: perhaps registerCommand method can check settings looking for a shortcut that should be bound to the command being registered?

Yet another idea: when Leo first time creates bindings (before plugins have been loaded), it may bind all unknown command names to a single dispatcher function that when invoked will find command by name and execute it. For example: when shortcut to 'quick-search-selected' is read from settings, there is still no function that corresponds to that name because quicksearch plugin has not been loaded yet. Leo should bind shortcut to a function like this one:

def later_defined_command_executor(name):
    f = commandsDict.get(name)
    return lambda *args, **kwargs: f(*args, **kwargs)

# somewhere when binding in the first place before plugins have been loaded
# call the following function for every shortcut setting found in settings
def makeOneBinding(shortcut, command_name):
    f = commandsDict.get(command_name) or \
              later_defined_command_executor(command_name)
    doBindSomehow(shortcut, f)

# I suppose there is somewhere dictionary mapping from command names to
# actual functions (I have named it here commandsDict)
#

This is totally improvised solution, that I haven't tested. Feel free to ignore. 

Vitalije

[Edward K. Ream]

On Saturday, November 18, 2017 at 6:58:46 AM UTC-6, vitalije wrote:

Your questions and suggestions are excellent, and show deep understanding of the code.  However, I think the present code, even with its flaws, is slightly preferable. YMMV.

How about calling makeAllBindings from plugin manager after loading any group of plugins?

It's conceivable, but here DRY (Don't Repeat Yourself) conflicts with "explicit is better than implicit". Plus I'm not sure it would work, though I haven't tried it.  Imo, bindings are best handled in the LoadManager and the KeyHandlerClass classes.  But this is debatable.

 

If I understand correctly old code worked fine and the only problem was with the commands defined in the plugins. It seems to me that the only case that needed to be addressed was case of loading plugins.

Yeah, you would think so. But I don't want the  LeoPluginsController class to get involved in key bindings.  That's not it's job.

So yes, it probably could be made to work, but it doesn't feel right to me because it would pollute the plugins code.

Another idea: perhaps registerCommand method can check settings looking for a shortcut that should be bound to the command being registered?

k.registerCommand is already too complicated.  Again, it doesn't feel like the right place.

Yet another idea: when Leo first time creates bindings (before plugins have been loaded), it may bind all unknown command names to a single dispatcher function that when invoked will find command by name and execute it. For example: when shortcut to 'quick-search-selected' is read from settings, there is still no function that corresponds to that name because quicksearch plugin has not been loaded yet. Leo should bind shortcut to a function like this one:

def later_defined_command_executor(name):
    f = commandsDict.get(name)
    return lambda *args, **kwargs: f(*args, **kwargs)

# somewhere when binding in the first place before plugins have been loaded
# call the following function for every shortcut setting found in settings
def makeOneBinding(shortcut, command_name):
    f = commandsDict.get(command_name) or \
              later_defined_command_executor(command_name)
    doBindSomehow(shortcut, f)

# I suppose there is somewhere dictionary mapping from command names to
# actual functions (I have named it here commandsDict)
#

This is totally improvised solution, that I haven't tested. Feel free to ignore.

I considered something similar to this and rejected it because it adds too much complexity behind the scenes.  I'd rather have the explicit problems than some clever implicit solution.

Leo's command and binding code already suffers from too many wrappers. I just ran across some weird wrapper in k.masterKeyHandler, iirc, and I'd like to simplify the command-dispatching logic by eliminating the wrapper, assuming I can find it again ;-)

Edward

[Edward K. Ream]

On Saturday, November 18, 2017 at 9:20:31 AM UTC-6, Edward K. Ream wrote:

On Saturday, November 18, 2017 at 6:58:46 AM UTC-6, vitalije wrote:

Your questions and suggestions are excellent, and show deep understanding of the code.  However, I think the present code, even with its flaws, is slightly preferable. YMMV.

I truly appreciate such questions.  They challenge me to reconsider my normal way of thinking.

tl;dr: Read the summary

When I awoke this morning I had several addition thoughts.

1. The DRY principle can lead one astray by suggesting that we group statements together that have no other purpose than they happen to be shared in various, possibly unrelated, pieces of code.

The great Glenford J. Meyers warned of this kind of code sharing in various writings, including his 1978 book, Composite/Structured design. Used copies of this book are still available for about $5. This was before any language had classes!  Nowadays I suspect that most people learn design from studying existing classes, but I wonder how well the principles of designing classes are understood.

Meyers advocated that all code have the strongest possible "binding strength". The top two he called "functional" and "informational" strength.  The weakest form of binding is "coincidental" strength. An example is code that just happens to be shared in several places. 

Here are some quotes. In these quotes "module" could mean either a python module or class or function.

Page 35: A functional strength module that performs a single specific function.

Page 36: It is often the case that a program constructed with functional-strength modules still does not have a maximally independent structure...The informational strength module...[hides] some concept, data structure or resource within a single module.

Imo, hiding details is the master design principle behind program design. We want various classes to know as little as possible about all other classes. All will be well when this is true. We must avoid cross-class dependencies whenever possible.

So this is one reason why I am leery of having the plugins manager handle settings.  It creates a cross-class dependency.

2. There is exactly one command method that handles every Leo command. Each of these methods has full and complete responsibility for all aspects of the command's implementation. This responsibility can not really be delegated. In the present discussion, c.open and c.openRecentFile are the command handlers.

From a design standpoint, it really doesn't matter how complicated these methods are. If we can devise well designed helpers that simplify the code then all the better.  But those helpers must have functional/informational binding strength.  And we don't want to pollute any other class with details best handled squarely by the command handlers.

3. The last reason I resist the notion of putting code into the plugins manager is that there arises the possibility that the plugins manager will call the settings-related code too often.  I'm not sure what that possibility is, and that's exactly the point. I don't want to create any kind of hidden dependency or assumption between the plugins manager class and various Leo commands.  Far better, imo, to duplicate the required code in c.open and c.openRecentFile.  Even if initially I haven't covered all the bases, the proper solution is to deal with the complexity in whatever command handler is created or found.

4. Thinking about these issues suggests refactoring k.registerCommand into two pieces.  The new k.registerCommand would reject, with a warning, any attempt to set a binding. This warning is necessary because we don't want to break existing code and we want to provide a clear migration path to the new code.

At present, several pieces of code legitimately set the binding, including the mod_scripting plugin and the code that creates the Open With menu.  The solution would be to have these call a new method, say k.setBindingForCommand.  Details are fuzzy, but I think the refactoring makes sense.  It will greatly simplify k.registerCommand in the typical usage.

5. The question about binding wrappers got me thinking about the IPython traitlets module, and the simplification it brings to their code.  One result of this pattern is that few (no?) jupyter/ipython classes have ctors!  Most ivars can be defined as class ivars.

Leo is not going to use anything like traitlets, for several reasons.  Most importantly, it would be too big a change in the code base for way too little gain.  Still, it's good to think of what might have been...

Summary

Leo's design is sound, flexible and robust because:

1. Classes know nothing about any other class.

2. There is a single command method for each Leo command, and that method is responsible for all implementation details of the command.  Each command method knows everything about the implementation of the command, and no other code anywhere knows anything about such details.

This is likely the clearest summary Leo's design that I have ever devised.  Many thanks for the inspiration to create it.

Edward

[Edward K. Ream]

On Sun, Nov 19, 2017 at 5:33 AM, Edward K. Ream <edre...@gmail.com> wrote:

Leo's design is sound, flexible and robust because:

1. Classes knows nothing about any other class.

2. There is a single command method for each Leo command, and that method is responsible for all implementation details of the command.  Each command method knows everything about the implementation of the command, and no other code anywhere knows anything about such details.

​Some other thoughts arose as the result of writing this.

1. At present c.open and c.openRecentFile call g.app.makeAllBindings(), but it seems dubious to change shortcuts in other commanders. Perhaps these two command methods should call c.k.makeAllBindings() instead.

It's even possible that one should but not the other, which shows that all command methods are responsible for their own complications.

2. The recommended way of defining commands is with the g.command decorator, but there are too many behind-the-scenes complications.  Thinking about wrappers has me wonder whether all command methods should be encapsulated in a new LeoCommand class.

The possible advantages:

A. It would allow Leo to define "legacy" commands outside of leoCommands.py by defining an optional "ivar" keyword argument that would inject the classes command method into c.  This would remove a long-standing hangnail.

B. It might be possible to remove the behind-the-scenes machinery involved in the g.command decorator, and other command-creating decorators.  This machinery includes g.cmd_instance_dict.

I'm not going to do #2 right away, but I have created #581 to memorialize it.

Edward

[vitalije]

Without any wish to insist on any design principle, let me say this. (I still haven't look at the code and former warning still applies.)

I fully agree that calling makeAllBindings from plugins manager, would be a bad idea. Yet, the call doesn't have to be placed inside plugins manager. I assume that methods openLeo and openRecentLeo (or load manager) at some point call plugins manager to load list of enabled plugins. Placing a call to makeAllBindings right after that point will be a single place where this call can be unconditionally made in all cases. There should be no need to repeat it elsewhere.

I was not referring to DRY when I wrote the previous post. It was you Edward, who were not fully satisfied with your new solution that sometimes unnecessary repeats a call to makeAllBindings. Still, without actually reading code, I am guessing that a call to make bindings is not placed where it naturally belongs and I am not referring to a file/class/location but rather about its time location. I am guessing that it used to be placed just after reading all settings when list of all defined shortcuts is known. Well, to fully complete its task makeAllBindings function needs not only a list of all shortcuts to be known, but also the list of ALL commands. As some commands may be defined later in plugins, list of all commands can't be deduced before all plugins have been loaded. Postponing a call to makeAllBindings, after loading all plugins, should allow it to complete job in a single pass. Speaking about its proper place in timeline, I am not advocating any precise location in class-space, although I did mention plugins manager, it was not really my intention. I suppose far better location would be in load manager.

Vitalije

[Edward K. Ream]

On Sun, Nov 19, 2017 at 9:12 AM, vitalije <vita...@gmail.com> wrote:

It was you Edward, who were not fully satisfied with your new solution that sometimes unnecessary repeats a call to makeAllBindings.

​And now I am.  Better, for the first time I know why I am satisfied and what the fundamental issues are.

Edward

[Terry Brown]

On Sun, 19 Nov 2017 07:12:56 -0800 (PST)
vitalije <vita...@gmail.com> wrote:

> I am guessing that a call to make bindings is not placed
> where it naturally belongs and I am not referring to a
> file/class/location but rather about its time location.

I wonder what impact more use of signals would have in terms of
coupling in Leo. Not suggesting a major overhaul, but as an option
going forward. Signals seem to make control flow a lot simpler in many
ways. Code does its stuff (possibly responding to a signal to begin
with) and then just announces "hey, if anyone cares, this just
happened". The program structure doesn't need to encode the flow of
control at all, beyond having the right listeners listening for the
right signals.

If there was interest in this, I've added leo/core/signal_manager.py
being used for leo_edit_pane, but I wonder also about switching to a
more mature signal architecture like
https://github.com/jek/blinker/blob/master/README.md

Cheers -Terry

[Edward K. Ream]

On Sun, Nov 19, 2017 at 10:30 AM, Terry Brown <terry...@gmail.com> wrote:

I wonder what impact more use of signals would have in terms of
coupling in Leo. 

​So do I.  For now, I think the existing settings code, complicated though it is, is good enough.  But I welcome any and all real simplifications to the code.

Edward