Allow to undo and redo selection changes #4725
Conversation
This allows a following commit to record selection history inside select_coord() instead of at every call site.
The next commit changes the selections to a history of selections. Today we directly access the selections data member. Let's instead use an accessor method, to reduce the number of changes in the next commit.
To be able to undo selection changes, we want to record selections from all commands that modify selections. Each such command will get its own private copy of the selections object. This copy will live until the command is finished executing. All child commands that are run while the command is executing, will also use the same copy, because to the user it's all just one selection change anyway. Add an RAII object in all places where we might modify selections. The next commit will use this to create the private selections copy in the constructor (if there is none) and remove redundant history items in the destructor. We could avoid the RAII object in some places but that seems worse. For lifetimes that don't correspond to a lexical scope, we use a std::unique_ptr. For lambdas that require conversion to std::function, we use std::shared_ptr because we need something that's copyable.
krobelus
force-pushed
the
undo-selection-change
branch
from
c70d648 to
996efb3
Compare
From the issue: > It often happens to me that I carefully craft a selection with multiple > cursors, ready to make changes elegantly, only to completely mess it > up by pressing a wrong key (by merging the cursors for example). Being > able to undo the last selection change (even if only until the previous > buffer change) would make this much less painful. Fix this by recording selection changes and allowing simple linear undo/redo of selection changes. The preliminary key bindings are <c-h> and <c-k>. Here are some other vacant normal mode keys I considered X Y <backspace> <minus> # ^ = <plus> ' unfortunately none of them is super convenient to type. Maybe we can kick out some other normal mode command? --- This feature has some overlap with the jump list (<c-o>/<c-i>) and with undo (u) but each of the three features have their moment. Currently there's no special integration with either peer feature; the three histories are completely independent. In future we might want to synchronize them so we can implement Sublime Text's "Soft undo" feature. Note that it is possible to restore selections that predate a buffer modification. Depending on the buffer modification, the selections might look different of course. (When trying to apply an old buffer's selection to the new buffer, Kakoune computes a diff of the buffers and updates the selection accordingly. This works quite well for many practical examples.) This makes us record the full history of all selections for each client. This seems wasteful, we could set a limit. I don't expect excessive memory usage in practice (we also keep the full history of buffer changes) but I could be wrong. Closes mawww#898
krobelus
force-pushed
the
undo-selection-change
branch
from
996efb3 to
c2ab5d4
Compare
| @@ -553,7 +559,8 @@ void pipe(Context& context, NormalParams params) | |||
| prompt, {}, default_command, context.faces()["Prompt"], | |||
| PromptFlags::DropHistoryEntriesWithBlankPrefix, '|', | |||
| shell_complete, | |||
| [default_command](StringView cmdline, PromptEvent event, Context& context) | |||
| [default_command, selection_edition=std::make_shared<ScopedSelectionEdition>(context)] | |||
There was a problem hiding this comment.
That make_shared looks suspicious, I guess that means we end-up copying this lambda around, is that something we can avoid ?
Looked it up, this is due to std::function requiring a copyiable target, isn't it... Well maybe its going to be time to replace that with our own, or to use std::move_only_function once we migrate to C++23. I guess for now we can live with the shared_ptr here.
There was a problem hiding this comment.
yeah the commit message tries to mention that but didn't give enough detail.
We could backport std::move_only_function (or perhaps add a template parameter to Prompt but that's probably not worth it)
| if (&history_node(parent).selections.buffer() == &m_context.buffer()) | ||
| select_parent(); | ||
| else | ||
| m_context.change_buffer(history_node(parent).selections.buffer(), { std::move(select_parent) }); |
There was a problem hiding this comment.
Was there an issue with letting change_buffer move the saved selections and overwrite them here ? Its unclear to me why we need to pass that lambda to change_buffer.
There was a problem hiding this comment.
Context::change_buffer calls Client::change_buffer, which creates a new selection in the destination buffer and then runs the WinDisplay hooks.
The original problem was that it would crash inside some WinDisplay hook because of broken invariants in the selection undo-stack.
I think those problems only occurred in a previous version, as I can't reproduce a problem anymore. Will try some more later.
Still, there is a behavior difference that is probably correct:
If we overwrite selections later, then the WinDisplay hooks will be run with whatever selection is returned from client_manager.get_free_window(). By passing the lambda we make sure the selection inside the hook is the same that the user will see. Though the lambda is awkward, would be nice to simplify this somehow.
Current status: there are some open questions for the future UI but basic usage
works well and will hopefully not need change (except maybe the key binding).
From the issue:
Fix this by recording selection changes and allowing simple linear
undo/redo of selection changes.
The preliminary key bindings are
<c-h>and<c-k>.Here are some other vacant normal mode keys I considered
unfortunately none of them is super convenient to type. Maybe we
can kick out some other normal mode command?
This feature has some overlap with the jump list (
<c-o>/<c-i>) andwith undo (u) but each of the three features have their moment.
Currently there's no special integration with either peer feature;
the three histories are completely independent. In future we might
want to synchronize them so we can implement Sublime Text's "Soft
undo" feature.
Note that it is possible to restore selections that predate a buffer
modification. Depending on the buffer modification, the selections
might look different of course. (When trying to apply an old buffer's
selection to the new buffer, Kakoune computes a diff of the buffers
and updates the selection accordingly. This works quite well for
many practical examples.)
This makes us record the full history of all selections for each
client. This seems wasteful, we could set a limit. I don't expect
excessive memory usage in practice (we also keep the full history of
buffer changes) but I could be wrong.
Closes #898