From a50cb5f6e7969183c69b56578ad63cb61fe06227 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sat, 3 Dec 2022 16:50:29 +0100 Subject: [PATCH 1/6] Share logic for undo/redo selection changes I will suggest a few changes to this code. No functional change. --- src/context.cc | 61 +++++++++++++++++--------------------------------- src/context.hh | 6 +++-- src/normal.cc | 16 ++++--------- 3 files changed, 29 insertions(+), 54 deletions(-) diff --git a/src/context.cc b/src/context.cc index fe9bd346..3446c2e4 100644 --- a/src/context.cc +++ b/src/context.cc @@ -216,8 +216,10 @@ void Context::SelectionHistory::end_edition() m_staging.reset(); } +template void Context::SelectionHistory::undo() { + static constexpr bool backward = direction == Backward; if (in_edition()) throw runtime_error("selection undo is only supported at top-level"); kak_assert(not empty()); @@ -226,43 +228,24 @@ void Context::SelectionHistory::undo() kak_assert(current_history_node().selections == m_staging->selections); end_edition(); }); - HistoryId parent = current_history_node().parent; - if (parent == HistoryId::Invalid) - throw runtime_error("no selection change to undo"); - auto select_parent = [&, parent] { - HistoryId before_undo = m_history_id; - m_history_id = parent; - current_history_node().redo_child = before_undo; + HistoryId next; + if constexpr (backward) + next = current_history_node().parent; + else + next = current_history_node().redo_child; + if (next == HistoryId::Invalid) + throw runtime_error(backward ? "no selection change to undo" : "no selection change to redo"); + auto select_next = [&, next] { + HistoryId previous_id = m_history_id; + m_history_id = next; + if constexpr (backward) + current_history_node().redo_child = previous_id; m_staging = current_history_node(); }; - if (&history_node(parent).selections.buffer() == &m_context.buffer()) - select_parent(); + if (&history_node(next).selections.buffer() == &m_context.buffer()) + select_next(); else - m_context.change_buffer(history_node(parent).selections.buffer(), { std::move(select_parent) }); - // }); -} - -void Context::SelectionHistory::redo() -{ - if (in_edition()) - throw runtime_error("selection redo is only supported at top-level"); - kak_assert(not empty()); - begin_edition(); - auto end = on_scope_end([&] { - kak_assert(current_history_node().selections == m_staging->selections); - end_edition(); - }); - HistoryId child = current_history_node().redo_child; - if (child == HistoryId::Invalid) - throw runtime_error("no selection change to redo"); - auto select_child = [&, child] { - m_history_id = child; - m_staging = current_history_node(); - }; - if (&history_node(child).selections.buffer() == &m_context.buffer()) - select_child(); - else - m_context.change_buffer(history_node(child).selections.buffer(), { std::move(select_child) }); + m_context.change_buffer(history_node(next).selections.buffer(), { std::move(select_next) }); } void Context::SelectionHistory::forget_buffer(Buffer& buffer) @@ -376,15 +359,13 @@ SelectionList& Context::selections(bool update) return m_selection_history.selections(update); } +template void Context::undo_selection_change() { - m_selection_history.undo(); -} - -void Context::redo_selection_change() -{ - m_selection_history.redo(); + m_selection_history.undo(); } +template void Context::undo_selection_change(); +template void Context::undo_selection_change(); SelectionList& Context::selections_write_only() { diff --git a/src/context.hh b/src/context.hh index 5ef3c42c..cf7b7a67 100644 --- a/src/context.hh +++ b/src/context.hh @@ -19,6 +19,8 @@ class DisplayLine; class KeymapManager; class AliasRegistry; +enum Direction { Backward = -1, Forward = 1 }; + struct JumpList { void push(SelectionList jump, Optional index = {}); @@ -91,8 +93,8 @@ public: SelectionList& selections_write_only(); void end_selection_edition() { m_selection_history.end_edition(); } + template void undo_selection_change(); - void redo_selection_change(); void change_buffer(Buffer& buffer, Optional> set_selection = {}); void forget_buffer(Buffer& buffer); @@ -170,8 +172,8 @@ private: void end_edition(); bool in_edition() const { return m_in_edition; } + template void undo(); - void redo(); void forget_buffer(Buffer& buffer); private: enum class HistoryId : size_t { First = 0, Invalid = (size_t)-1 }; diff --git a/src/normal.cc b/src/normal.cc index 1142d871..7dc14382 100644 --- a/src/normal.cc +++ b/src/normal.cc @@ -1417,8 +1417,6 @@ void select_object(Context& context, NormalParams params) {{alt(';')}, "run command in object context"}})); } -enum Direction { Backward = -1, Forward = 1 }; - template void scroll(Context& context, NormalParams params) { @@ -2039,18 +2037,12 @@ void move_in_history(Context& context, NormalParams params) history_id, max_history_id)); } +template void undo_selection_change(Context& context, NormalParams params) { int count = std::max(1, params.count); while (count--) - context.undo_selection_change(); -} - -void redo_selection_change(Context& context, NormalParams params) -{ - int count = std::max(1, params.count); - while (count--) - context.redo_selection_change(); + context.undo_selection_change(); } void exec_user_mappings(Context& context, NormalParams params) @@ -2378,8 +2370,8 @@ static constexpr HashMap { {alt('u')}, {"move backward in history", move_in_history} }, { {alt('U')}, {"move forward in history", move_in_history} }, - { {ctrl('h')}, {"undo selection change", undo_selection_change} }, - { {ctrl('k')}, {"redo selection change", redo_selection_change} }, + { {ctrl('h')}, {"undo selection change", undo_selection_change} }, + { {ctrl('k')}, {"redo selection change", undo_selection_change} }, { {alt('i')}, {"select inner object", select_object} }, { {alt('a')}, {"select whole object", select_object} }, From 02d0584e0f29abab29a0397f582c0c521c6c0470 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sat, 3 Dec 2022 16:52:08 +0100 Subject: [PATCH 2/6] Extract variable in selection undo No functional change. --- src/context.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/context.cc b/src/context.cc index 3446c2e4..2afa278a 100644 --- a/src/context.cc +++ b/src/context.cc @@ -242,10 +242,11 @@ void Context::SelectionHistory::undo() current_history_node().redo_child = previous_id; m_staging = current_history_node(); }; - if (&history_node(next).selections.buffer() == &m_context.buffer()) + Buffer& destination_buffer = history_node(next).selections.buffer(); + if (&destination_buffer == &m_context.buffer()) select_next(); else - m_context.change_buffer(history_node(next).selections.buffer(), { std::move(select_next) }); + m_context.change_buffer(destination_buffer, { std::move(select_next) }); } void Context::SelectionHistory::forget_buffer(Buffer& buffer) From 016e1be77fd34610cae082fd8d90e846ed8af28e Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sat, 3 Dec 2022 12:36:33 +0100 Subject: [PATCH 3/6] Extract variable and add comment in selection change recording No functional change. --- src/context.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/context.cc b/src/context.cc index 2afa278a..568142c6 100644 --- a/src/context.cc +++ b/src/context.cc @@ -204,9 +204,11 @@ void Context::SelectionHistory::end_edition() if (m_history_id != HistoryId::Invalid and current_history_node().selections == m_staging->selections) { - auto& sels = m_history[(size_t)m_history_id].selections; - sels.force_timestamp(m_staging->selections.timestamp()); - sels.set_main_index(m_staging->selections.main_index()); + // No change, except maybe the index of the main selection. + // Update timestamp to potentially improve interaction with content undo. + auto& node = current_history_node(); + node.selections.force_timestamp(m_staging->selections.timestamp()); + node.selections.set_main_index(m_staging->selections.main_index()); } else { From 8e8c2fb46d418236918c0dbddad8e48ae9e4e20d Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sat, 3 Dec 2022 19:23:24 +0100 Subject: [PATCH 4/6] Add tests for selection undo --- test/normal/selection-undo/redo/cmd | 1 + test/normal/selection-undo/redo/in | 4 ++++ test/normal/selection-undo/redo/out | 4 ++++ test/normal/selection-undo/redo/script | 2 ++ test/normal/selection-undo/undo/cmd | 1 + test/normal/selection-undo/undo/in | 4 ++++ test/normal/selection-undo/undo/out | 4 ++++ test/normal/selection-undo/undo/script | 2 ++ 8 files changed, 22 insertions(+) create mode 100644 test/normal/selection-undo/redo/cmd create mode 100644 test/normal/selection-undo/redo/in create mode 100644 test/normal/selection-undo/redo/out create mode 100644 test/normal/selection-undo/redo/script create mode 100644 test/normal/selection-undo/undo/cmd create mode 100644 test/normal/selection-undo/undo/in create mode 100644 test/normal/selection-undo/undo/out create mode 100644 test/normal/selection-undo/undo/script diff --git a/test/normal/selection-undo/redo/cmd b/test/normal/selection-undo/redo/cmd new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/test/normal/selection-undo/redo/cmd @@ -0,0 +1 @@ + diff --git a/test/normal/selection-undo/redo/in b/test/normal/selection-undo/redo/in new file mode 100644 index 00000000..94ebaf90 --- /dev/null +++ b/test/normal/selection-undo/redo/in @@ -0,0 +1,4 @@ +1 +2 +3 +4 diff --git a/test/normal/selection-undo/redo/out b/test/normal/selection-undo/redo/out new file mode 100644 index 00000000..76bef247 --- /dev/null +++ b/test/normal/selection-undo/redo/out @@ -0,0 +1,4 @@ +1 +2 +here3 +4 diff --git a/test/normal/selection-undo/redo/script b/test/normal/selection-undo/redo/script new file mode 100644 index 00000000..c92d82d7 --- /dev/null +++ b/test/normal/selection-undo/redo/script @@ -0,0 +1,2 @@ +ui_out -ignore 4 +ui_in '{ "jsonrpc": "2.0", "method": "keys", "params": [ "2jjihere" ] }' diff --git a/test/normal/selection-undo/undo/cmd b/test/normal/selection-undo/undo/cmd new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/test/normal/selection-undo/undo/cmd @@ -0,0 +1 @@ + diff --git a/test/normal/selection-undo/undo/in b/test/normal/selection-undo/undo/in new file mode 100644 index 00000000..94ebaf90 --- /dev/null +++ b/test/normal/selection-undo/undo/in @@ -0,0 +1,4 @@ +1 +2 +3 +4 diff --git a/test/normal/selection-undo/undo/out b/test/normal/selection-undo/undo/out new file mode 100644 index 00000000..00a7e5fd --- /dev/null +++ b/test/normal/selection-undo/undo/out @@ -0,0 +1,4 @@ +here1 +2 +3 +4 diff --git a/test/normal/selection-undo/undo/script b/test/normal/selection-undo/undo/script new file mode 100644 index 00000000..2b00d75e --- /dev/null +++ b/test/normal/selection-undo/undo/script @@ -0,0 +1,2 @@ +ui_out -ignore 4 +ui_in '{ "jsonrpc": "2.0", "method": "keys", "params": [ "j2jihere" ] }' From 8427379a5dfcf35af35fd5a7b26803e8727476e6 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Thu, 22 Dec 2022 18:09:45 +0100 Subject: [PATCH 5/6] Tweak selection-undo interaction with WinDisplay hooks Each selection undo operation is surrounded by pair of begin_edition()/end_edition() calls. The original reason for adding these was that in one of my preliminary versions, a WinDisplay hook could break an undo chain, even if the hook did not affect selections at all. This has since been fixed. By surrounding the undo with begin_edition()/end_edition(), try to ensure that any selection modification that happens in a WinDisplay hook would not break the undo chain. Essentially this means that, after using to undo a buffer change, this was meant to make sure that could redo that buffer change. However, it turns out this actually doesn't work. The attached test case triggers an assertion. As described in the first paragraph, the only real-world motivation for this is gone, so let's simplify the behavior. The assertion fix means that we can test the next commit better. --- src/context.cc | 6 ------ test/normal/selection-undo/windisplay-hook/cmd | 1 + test/normal/selection-undo/windisplay-hook/in | 3 +++ test/normal/selection-undo/windisplay-hook/out | 3 +++ test/normal/selection-undo/windisplay-hook/rc | 1 + test/normal/selection-undo/windisplay-hook/script | 2 ++ 6 files changed, 10 insertions(+), 6 deletions(-) create mode 100644 test/normal/selection-undo/windisplay-hook/cmd create mode 100644 test/normal/selection-undo/windisplay-hook/in create mode 100644 test/normal/selection-undo/windisplay-hook/out create mode 100644 test/normal/selection-undo/windisplay-hook/rc create mode 100644 test/normal/selection-undo/windisplay-hook/script diff --git a/src/context.cc b/src/context.cc index 568142c6..3a0aadb4 100644 --- a/src/context.cc +++ b/src/context.cc @@ -225,11 +225,6 @@ void Context::SelectionHistory::undo() if (in_edition()) throw runtime_error("selection undo is only supported at top-level"); kak_assert(not empty()); - begin_edition(); - auto end = on_scope_end([&] { - kak_assert(current_history_node().selections == m_staging->selections); - end_edition(); - }); HistoryId next; if constexpr (backward) next = current_history_node().parent; @@ -242,7 +237,6 @@ void Context::SelectionHistory::undo() m_history_id = next; if constexpr (backward) current_history_node().redo_child = previous_id; - m_staging = current_history_node(); }; Buffer& destination_buffer = history_node(next).selections.buffer(); if (&destination_buffer == &m_context.buffer()) diff --git a/test/normal/selection-undo/windisplay-hook/cmd b/test/normal/selection-undo/windisplay-hook/cmd new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/test/normal/selection-undo/windisplay-hook/cmd @@ -0,0 +1 @@ + diff --git a/test/normal/selection-undo/windisplay-hook/in b/test/normal/selection-undo/windisplay-hook/in new file mode 100644 index 00000000..01e79c32 --- /dev/null +++ b/test/normal/selection-undo/windisplay-hook/in @@ -0,0 +1,3 @@ +1 +2 +3 diff --git a/test/normal/selection-undo/windisplay-hook/out b/test/normal/selection-undo/windisplay-hook/out new file mode 100644 index 00000000..92243f2b --- /dev/null +++ b/test/normal/selection-undo/windisplay-hook/out @@ -0,0 +1,3 @@ +1 +2 +here3 diff --git a/test/normal/selection-undo/windisplay-hook/rc b/test/normal/selection-undo/windisplay-hook/rc new file mode 100644 index 00000000..b157a2ce --- /dev/null +++ b/test/normal/selection-undo/windisplay-hook/rc @@ -0,0 +1 @@ +hook global WinDisplay .*/out %{exec j} diff --git a/test/normal/selection-undo/windisplay-hook/script b/test/normal/selection-undo/windisplay-hook/script new file mode 100644 index 00000000..d2fd1c40 --- /dev/null +++ b/test/normal/selection-undo/windisplay-hook/script @@ -0,0 +1,2 @@ +ui_out -ignore 4 +ui_in '{ "jsonrpc": "2.0", "method": "keys", "params": [ "j:buffer *debug*ihere" ] }' From 516759bb2fd95d02134bc84130bf9060e0354310 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Thu, 22 Dec 2022 18:09:45 +0100 Subject: [PATCH 6/6] Make selection undo skip over entries that are nop after buffer change After buffer modification - in particular after deletion - adjacent selection history entries may correspond to the same effective selection when applied to the current buffer. This means that we sometimes need to press multiple times to make one visible change. This is not what the user expects, so let's keep walking the selection history until we hit an actual change. Alternatively, we could minimize the selection history after buffer changes but I think that would make the it worse after content undo+redo. --- src/context.cc | 37 +++++++++++-------- .../selection-undo/fold-redundant-entries/cmd | 1 + .../selection-undo/fold-redundant-entries/in | 4 ++ .../selection-undo/fold-redundant-entries/out | 3 ++ .../fold-redundant-entries/script | 2 + 5 files changed, 31 insertions(+), 16 deletions(-) create mode 100644 test/normal/selection-undo/fold-redundant-entries/cmd create mode 100644 test/normal/selection-undo/fold-redundant-entries/in create mode 100644 test/normal/selection-undo/fold-redundant-entries/out create mode 100644 test/normal/selection-undo/fold-redundant-entries/script diff --git a/src/context.cc b/src/context.cc index 3a0aadb4..0c1b8aad 100644 --- a/src/context.cc +++ b/src/context.cc @@ -225,24 +225,29 @@ void Context::SelectionHistory::undo() if (in_edition()) throw runtime_error("selection undo is only supported at top-level"); kak_assert(not empty()); + SelectionList old_selections = selections(); HistoryId next; - if constexpr (backward) - next = current_history_node().parent; - else - next = current_history_node().redo_child; - if (next == HistoryId::Invalid) - throw runtime_error(backward ? "no selection change to undo" : "no selection change to redo"); - auto select_next = [&, next] { - HistoryId previous_id = m_history_id; - m_history_id = next; + do + { if constexpr (backward) - current_history_node().redo_child = previous_id; - }; - Buffer& destination_buffer = history_node(next).selections.buffer(); - if (&destination_buffer == &m_context.buffer()) - select_next(); - else - m_context.change_buffer(destination_buffer, { std::move(select_next) }); + next = current_history_node().parent; + else + next = current_history_node().redo_child; + if (next == HistoryId::Invalid) + throw runtime_error(backward ? "no selection change to undo" : "no selection change to redo"); + auto select_next = [&, next] { + HistoryId previous_id = m_history_id; + m_history_id = next; + if constexpr (backward) + current_history_node().redo_child = previous_id; + }; + Buffer& destination_buffer = history_node(next).selections.buffer(); + if (&destination_buffer == &m_context.buffer()) + select_next(); + else + m_context.change_buffer(destination_buffer, { std::move(select_next) }); + } + while (selections() == old_selections); } void Context::SelectionHistory::forget_buffer(Buffer& buffer) diff --git a/test/normal/selection-undo/fold-redundant-entries/cmd b/test/normal/selection-undo/fold-redundant-entries/cmd new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/test/normal/selection-undo/fold-redundant-entries/cmd @@ -0,0 +1 @@ + diff --git a/test/normal/selection-undo/fold-redundant-entries/in b/test/normal/selection-undo/fold-redundant-entries/in new file mode 100644 index 00000000..94ebaf90 --- /dev/null +++ b/test/normal/selection-undo/fold-redundant-entries/in @@ -0,0 +1,4 @@ +1 +2 +3 +4 diff --git a/test/normal/selection-undo/fold-redundant-entries/out b/test/normal/selection-undo/fold-redundant-entries/out new file mode 100644 index 00000000..239c8675 --- /dev/null +++ b/test/normal/selection-undo/fold-redundant-entries/out @@ -0,0 +1,3 @@ +2 +3 +here4 diff --git a/test/normal/selection-undo/fold-redundant-entries/script b/test/normal/selection-undo/fold-redundant-entries/script new file mode 100644 index 00000000..456f92f2 --- /dev/null +++ b/test/normal/selection-undo/fold-redundant-entries/script @@ -0,0 +1,2 @@ +ui_out -ignore 4 +ui_in '{ "jsonrpc": "2.0", "method": "keys", "params": [ "gjgkxdihere" ] }'