Merge remote-tracking branch 'krobelus/selection-undo-fix-standstill-after-buffer-change'

3 files changed, 42 insertions, 65 deletions

diff --git a/src/context.cc b/src/context.cc
index fe9bd346..0c1b8aad 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
     {
@@ -216,53 +218,36 @@ void Context::SelectionHistory::end_edition()
     m_staging.reset();
 }
 
+template<Direction direction>
 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());
-    begin_edition();
-    auto end = on_scope_end([&] {
-        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;
-        m_staging = current_history_node();
-    };
-    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) });
-                                                                           // });
-}
-
-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) });
+    SelectionList old_selections = selections();
+    HistoryId next;
+    do
+    {
+        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;
+        };
+        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)
@@ -376,15 +361,13 @@ SelectionList& Context::selections(bool update)
     return m_selection_history.selections(update);
 }
 
+template<Direction direction>
 void Context::undo_selection_change()
 {
-    m_selection_history.undo();
-}
-
-void Context::redo_selection_change()
-{
-    m_selection_history.redo();
+    m_selection_history.undo<direction>();
 }
+template void Context::undo_selection_change<Backward>();
+template void Context::undo_selection_change<Forward>();
 
 SelectionList& Context::selections_write_only()
 {
diff --git a/src/context.hh b/src/context.hh
index 3fe28dc2..34bb9e12 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<size_t> index = {});
@@ -91,8 +93,8 @@ public:
     SelectionList& selections_write_only();
 
     void end_selection_edition() { m_selection_history.end_edition(); }
+    template<Direction direction>
     void undo_selection_change();
-    void redo_selection_change();
 
     void change_buffer(Buffer& buffer, Optional<FunctionRef<void()>> set_selection = {});
     void forget_buffer(Buffer& buffer);
@@ -169,8 +171,8 @@ private:
         void end_edition();
         bool in_edition() const { return m_in_edition; }
 
+        template<Direction direction>
         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<Direction direction, bool half = false>
 void scroll(Context& context, NormalParams params)
 {
@@ -2039,18 +2037,12 @@ void move_in_history(Context& context, NormalParams params)
                             history_id, max_history_id));
 }
 
+template<Direction direction>
 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<direction>();
 }
 
 void exec_user_mappings(Context& context, NormalParams params)
@@ -2378,8 +2370,8 @@ static constexpr HashMap<Key, NormalCmd, MemoryDomain::Undefined, KeymapBackend>
     { {alt('u')}, {"move backward in history", move_in_history<Direction::Backward>} },
     { {alt('U')}, {"move forward in history", move_in_history<Direction::Forward>} },
 
-    { {ctrl('h')}, {"undo selection change", undo_selection_change} },
-    { {ctrl('k')}, {"redo selection change", redo_selection_change} },
+    { {ctrl('h')}, {"undo selection change", undo_selection_change<Backward>} },
+    { {ctrl('k')}, {"redo selection change", undo_selection_change<Forward>} },
 
     { {alt('i')}, {"select inner object", select_object<ObjectFlags::ToBegin | ObjectFlags::ToEnd | ObjectFlags::Inner>} },
     { {alt('a')}, {"select whole object", select_object<ObjectFlags::ToBegin | ObjectFlags::ToEnd>} },