reader: make undo history private to editable_line_t

reader handles way too much state itself. Let's move the undo handling to
editable_line_t entirely.

No functional change.

src/fish_tests.cpp

@@ -3880,11 +3880,11 @@ static void test_undo() {
     do_test(!line.undo());  // nothing to undo
     do_test(line.text().empty());
     do_test(line.position() == 0);
-    line.push_edit(edit_t(0, 0, L"a b c"));
+    line.push_edit(edit_t(0, 0, L"a b c"), true);
     do_test(line.text() == L"a b c");
     do_test(line.position() == 5);
     line.set_position(2);
-    line.push_edit(edit_t(2, 1, L"B"));  // replacement right of cursor
+    line.push_edit(edit_t(2, 1, L"B"), true);  // replacement right of cursor
     do_test(line.text() == L"a B c");
     line.undo();
     do_test(line.text() == L"a b c");
@@ -3895,7 +3895,7 @@ static void test_undo() {
 
     do_test(!line.redo());  // nothing to redo
 
-    line.push_edit(edit_t(0, 2, L""));  // deletion left of cursor
+    line.push_edit(edit_t(0, 2, L""), true);  // deletion left of cursor
     do_test(line.text() == L"B c");
     do_test(line.position() == 1);
     line.undo();
@@ -3905,27 +3905,23 @@ static void test_undo() {
     do_test(line.text() == L"B c");
     do_test(line.position() == 1);
 
-    line.push_edit(edit_t(0, line.size(), L"a b c"));  // replacement left and right of cursor
+    line.push_edit(edit_t(0, line.size(), L"a b c"), true);  // replacement left and right of cursor
     do_test(line.text() == L"a b c");
     do_test(line.position() == 5);
 
     say(L"Testing undoing coalesced edits.");
     line.clear();
-    line.push_edit(edit_t(line.position(), 0, L"a"));
+    line.push_edit(edit_t(line.position(), 0, L"a"), true);
-    line.insert_coalesce(L"b");
+    line.push_edit(edit_t(line.position(), 0, L"b"), true);
-    line.insert_coalesce(L"c");
+    line.push_edit(edit_t(line.position(), 0, L"c"), true);
-    do_test(line.undo_history.edits.size() == 1);
+    line.push_edit(edit_t(line.position(), 0, L" "), true);
-    line.push_edit(edit_t(line.position(), 0, L" "));
-    do_test(line.undo_history.edits.size() == 2);
     line.undo();
     line.undo();
     line.redo();
     do_test(line.text() == L"abc");
-    do_test(line.undo_history.edits.size() == 2);
     // This removes the space insertion from the history, but does not coalesce with the first edit.
-    line.push_edit(edit_t(line.position(), 0, L"d"));
+    line.push_edit(edit_t(line.position(), 0, L"d"), true);
-    do_test(line.undo_history.edits.size() == 2);
+    line.push_edit(edit_t(line.position(), 0, L"e"), true);
-    line.insert_coalesce(L"e");
     do_test(line.text() == L"abcde");
     line.undo();
     do_test(line.text() == L"abc");

src/reader.cpp

@@ -223,34 +223,17 @@ static size_t cursor_position_after_edit(const edit_t &edit) {
     return cursor > removed ? cursor - removed : 0;
 }
 
-/// Whether we want to append this string to the previous edit.
-static bool want_to_coalesce_insertion_of(const editable_line_t &el, const wcstring &str) {
-    // The previous edit must support coalescing.
-    if (!el.undo_history.may_coalesce) return false;
-    // Only consolidate single character inserts.
-    if (str.size() != 1) return false;
-    // Make an undo group after every space.
-    if (str.at(0) == L' ' && !el.undo_history.try_coalesce) return false;
-    assert(!el.undo_history.edits.empty());
-    const edit_t &last_edit = el.undo_history.edits.back();
-    // Don't add to the last edit if it deleted something.
-    if (last_edit.length != 0) return false;
-    // Must not have moved the cursor!
-    if (cursor_position_after_edit(last_edit) != el.position()) return false;
-    return true;
-}
-
 bool editable_line_t::undo() {
     bool did_undo = false;
     maybe_t<int> last_group_id{-1};
-    while (undo_history.edits_applied != 0) {
+    while (undo_history_.edits_applied != 0) {
-        const edit_t &edit = undo_history.edits.at(undo_history.edits_applied - 1);
+        const edit_t &edit = undo_history_.edits.at(undo_history_.edits_applied - 1);
         if (did_undo && (!edit.group_id.has_value() || edit.group_id != last_group_id)) {
             // We've restored all the edits in this logical undo group
             break;
         }
         last_group_id = edit.group_id;
-        undo_history.edits_applied--;
+        undo_history_.edits_applied--;
         edit_t inverse = edit_t(edit.offset, edit.replacement.size(), L"");
         inverse.replacement = edit.old;
         size_t old_position = edit.cursor_position_before_edit;
@@ -260,18 +243,31 @@ bool editable_line_t::undo() {
     }
 
     end_edit_group();
-    undo_history.may_coalesce = false;
+    undo_history_.may_coalesce = false;
     return did_undo;
 }
 
 void editable_line_t::clear() {
-    undo_history.clear();
+    undo_history_.clear();
     if (empty()) return;
     set_text_bypassing_undo_history(L"");
     set_position(0);
 }
 
-void editable_line_t::push_edit(edit_t &&edit) {
+void editable_line_t::push_edit(edit_t &&edit, bool allow_coalesce) {
+    bool is_insertion = edit.length == 0;
+    /// Coalescing insertion does not create a new undo entry but adds to the last insertion.
+    if (allow_coalesce && is_insertion && want_to_coalesce_insertion_of(edit.replacement)) {
+        assert(edit.offset == position());
+        edit_t &last_edit = undo_history_.edits.back();
+        last_edit.replacement.append(edit.replacement);
+        apply_edit(&text_, edit);
+        set_position(position() + edit.replacement.size());
+
+        assert(undo_history_.may_coalesce);
+        return;
+    }
+
     // Assign a new group id or propagate the old one if we're in a logical grouping of edits
     if (edit_group_level_ != -1) {
         edit.group_id = edit_group_id_;
@@ -279,42 +275,37 @@ void editable_line_t::push_edit(edit_t &&edit) {
 
     bool edit_does_nothing = edit.length == 0 && edit.replacement.empty();
     if (edit_does_nothing) return;
-    if (undo_history.edits_applied != undo_history.edits.size()) {
+    if (undo_history_.edits_applied != undo_history_.edits.size()) {
         // After undoing some edits, the user is making a new edit;
         // we are about to create a new edit branch.
         // Discard all edits that were undone because we only support
         // linear undo/redo, they will be unreachable.
-        undo_history.edits.erase(undo_history.edits.begin() + undo_history.edits_applied,
+        undo_history_.edits.erase(undo_history_.edits.begin() + undo_history_.edits_applied,
-                                 undo_history.edits.end());
+                                  undo_history_.edits.end());
     }
     edit.cursor_position_before_edit = position();
     edit.old = text_.substr(edit.offset, edit.length);
     apply_edit(&text_, edit);
     set_position(cursor_position_after_edit(edit));
-    assert(undo_history.edits_applied == undo_history.edits.size());
+    assert(undo_history_.edits_applied == undo_history_.edits.size());
-    undo_history.edits_applied++;
+    undo_history_.may_coalesce =
-    undo_history.edits.emplace_back(edit);
+        is_insertion && (undo_history_.try_coalesce || edit.replacement.size() == 1);
-}
+    undo_history_.edits_applied++;
-
+    undo_history_.edits.emplace_back(std::move(edit));
-void editable_line_t::insert_coalesce(const wcstring &str) {
-    edit_t &edit = undo_history.edits.back();
-    edit.replacement.append(str);
-    apply_edit(&text_, edit_t(position(), 0, str));
-    set_position(position() + str.size());
 }
 
 bool editable_line_t::redo() {
     bool did_redo = false;
 
     maybe_t<int> last_group_id{-1};
-    while (undo_history.edits_applied < undo_history.edits.size()) {
+    while (undo_history_.edits_applied < undo_history_.edits.size()) {
-        const edit_t &edit = undo_history.edits.at(undo_history.edits_applied);
+        const edit_t &edit = undo_history_.edits.at(undo_history_.edits_applied);
         if (did_redo && (!edit.group_id.has_value() || edit.group_id != last_group_id)) {
             // We've restored all the edits in this logical undo group
             break;
         }
         last_group_id = edit.group_id;
-        undo_history.edits_applied++;
+        undo_history_.edits_applied++;
         apply_edit(&text_, edit);
         set_position(cursor_position_after_edit(edit));
         did_redo = true;
@@ -327,9 +318,9 @@ bool editable_line_t::redo() {
 void editable_line_t::begin_edit_group() {
     if (++edit_group_level_ == 0) {
         // Indicate that the next change must trigger the creation of a new history item
-        undo_history.may_coalesce = false;
+        undo_history_.may_coalesce = false;
         // Indicate that future changes should be coalesced into the same edit if possible.
-        undo_history.try_coalesce = true;
+        undo_history_.try_coalesce = true;
         // Assign a logical edit group id to future edits in this group
         edit_group_id_ += 1;
     }
@@ -343,11 +334,28 @@ void editable_line_t::end_edit_group() {
     }
 
     if (--edit_group_level_ == -1) {
-        undo_history.try_coalesce = false;
+        undo_history_.try_coalesce = false;
-        undo_history.may_coalesce = false;
+        undo_history_.may_coalesce = false;
     }
 }
 
+/// Whether we want to append this string to the previous edit.
+bool editable_line_t::want_to_coalesce_insertion_of(const wcstring &str) const {
+    // The previous edit must support coalescing.
+    if (!undo_history_.may_coalesce) return false;
+    // Only consolidate single character inserts.
+    if (str.size() != 1) return false;
+    // Make an undo group after every space.
+    if (str.at(0) == L' ' && !undo_history_.try_coalesce) return false;
+    assert(!undo_history_.edits.empty());
+    const edit_t &last_edit = undo_history_.edits.back();
+    // Don't add to the last edit if it deleted something.
+    if (last_edit.length != 0) return false;
+    // Must not have moved the cursor!
+    if (cursor_position_after_edit(last_edit) != position()) return false;
+    return true;
+}
+
 // Make the search case-insensitive unless we have an uppercase character.
 static history_search_flags_t smartcase_flags(const wcstring &query) {
     return query == wcstolower(query) ? history_search_ignore_case : 0;
@@ -1696,13 +1704,7 @@ void reader_data_t::delete_char(bool backward) {
 /// Returns true if the string changed.
 void reader_data_t::insert_string(editable_line_t *el, const wcstring &str) {
     if (!str.empty()) {
-        if (!history_search.active() && want_to_coalesce_insertion_of(*el, str)) {
+        el->push_edit(edit_t(el->position(), 0, str), !history_search.active());
-            el->insert_coalesce(str);
-            assert(el->undo_history.may_coalesce);
-        } else {
-            el->push_edit(edit_t(el->position(), 0, str));
-            el->undo_history.may_coalesce = el->undo_history.try_coalesce || (str.size() == 1);
-        }
     }
 
     if (el == &command_line) {
@@ -1713,8 +1715,7 @@ void reader_data_t::insert_string(editable_line_t *el, const wcstring &str) {
 }
 
 void reader_data_t::push_edit(editable_line_t *el, edit_t &&edit) {
-    el->push_edit(std::move(edit));
+    el->push_edit(std::move(edit), false);
-    el->undo_history.may_coalesce = false;
     maybe_refilter_pager(el);
 }
 
@@ -3048,7 +3049,7 @@ void reader_data_t::compute_and_apply_completions(readline_cmd_t c, readline_loo
             rls.complete_did_insert = false;
             size_t tok_off = static_cast<size_t>(token_begin - buff);
             size_t tok_len = static_cast<size_t>(token_end - token_begin);
-            el->push_edit(edit_t{tok_off, tok_len, std::move(wc_expanded)});
+            push_edit(el, edit_t{tok_off, tok_len, std::move(wc_expanded)});
             return;
     }
 

src/reader.h

@@ -80,8 +80,6 @@ struct undo_history_t {
 /// Helper class for storing a command line.
 class editable_line_t {
    public:
-    undo_history_t undo_history;
-
     const wcstring &text() const { return text_; }
     /// Set the text directly without maintaining undo invariants. Use with caution.
     void set_text_bypassing_undo_history(wcstring &&text) { text_ = text; }
@@ -100,12 +98,7 @@ class editable_line_t {
 
     /// Modify the commandline according to @edit. Most modifications to the
     /// text should pass through this function.
-    void push_edit(edit_t &&edit);
+    void push_edit(edit_t &&edit, bool allow_coalesce);
-
-    /// Modify the commandline by inserting a string at the cursor.
-    /// Does not create a new undo point, but adds to the last edit which
-    /// must be an insertion, too.
-    void insert_coalesce(const wcstring &str);
 
     /// Undo the most recent edit that was not yet undone. Returns true on success.
     bool undo();
@@ -119,11 +112,16 @@ class editable_line_t {
     void end_edit_group();
 
    private:
+    /// Whether we want to append this string to the previous edit.
+    bool want_to_coalesce_insertion_of(const wcstring &str) const;
+
     /// The command line.
     wcstring text_;
     /// The current position of the cursor in the command line.
     size_t position_ = 0;
 
+    /// The history of all edits.
+    undo_history_t undo_history_;
     /// The nesting level for atomic edits, so that recursive invocations of start_edit_group()
     /// are not ended by one end_edit_group() call.
     int32_t edit_group_level_ = -1;