prompt-toolkit / python-prompt-toolkit

Library for building powerful interactive command line applications in Python
https://python-prompt-toolkit.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
9.33k stars 714 forks source link

Commit 148833e9 (released in 3.0.4) changed undo's behavior in vi mode #1467

Open zardus opened 3 years ago

zardus commented 3 years ago

It looks like undo behavior when undoing inserted bytes changed unexpectedly between 3.0.3 and 3.0.4.

Bisecting the codebase led me to commit 148833e9, and stashing random stuff tracked it to this specific change: https://github.com/prompt-toolkit/python-prompt-toolkit/commit/148833e982cc0ed427a862e0d2725b353af39d04#diff-5324a319d7483c6ee86efe14bfdd6dc1ed139f1b6046461681e209b6fc3b79fdL41

I mirrored it here for completeness:

diff --git a/prompt_toolkit/key_binding/bindings/named_commands.py b/prompt_toolkit/key_binding/bindings/named_commands.py
index 4796cf7c..17c1c5c4 100644
--- a/prompt_toolkit/key_binding/bindings/named_commands.py
+++ b/prompt_toolkit/key_binding/bindings/named_commands.py
@@ -38,8 +38,12 @@ def register(name: str) -> Callable[[_T], _T]:
     """

     def decorator(handler: _T) -> _T:
-        " `handler` is a callable or _Binding. "
-        _readline_commands[name] = handler
+        " `handler` is a callable or Binding. "
+        if isinstance(handler, Binding):
+            _readline_commands[name] = handler
+        else:
+            _readline_commands[name] = key_binding()(cast(_Handler, handler))
+
         return handler

     return decorator

I am not sure of the implication of this change or why it would cause the observed change in behavior, though...

psacawa commented 3 years ago

I have also noticed this change, and it is a significant regression for vi users.

asmeurer commented 2 years ago

This is not specific to vi mode. The issue is that the change makes the self-insert function, which is called for every normal key, apply the save_before setting. This causes each character, when typed, to save itself individually to the undo stack.

The real issue here, is that the undo "stack" is based on keys that were typed, and undoing just undoes this one key at a time (unless the key tells to not save the state). This is a somewhat naive way of handling undo. Even if we revert to the old behavior where each self-insert doesn't save its own state to the undo stack, it still is a very limiting abstraction. It would be impossible, for instance, to undo one word at a time, or one programming language token at a time, or whatever. I would look at how editors handle undoing and try to emulate that. For instance, if you look at how undo is implemented in emacs, it's much more sophisticated (I think it saves state on certain key bindings, similar to prompt-toolkit, but it also seems to save every nth character).

asmeurer commented 2 years ago

This seems to be a usable workaround (needs to be run before the keybindings are loaded)

from prompt_toolkit.key_binding.bindings.named_commands import _readline_commands
_readline_commands['self-insert'] = _readline_commands['self-insert'].handler