From 7a4fece445a5f169f17a7f45270537b17eef3700 Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Sun, 9 Feb 2020 18:39:14 +0100
Subject: [PATCH] Give reader control of all edits to a command line

So we can do something on every edit, for example repaint the pager (#7318).
This patch fixes pager refiltering and repainting when pressing Control+U
after typing something in the search field.

Implement this by moving the convenience functions from editable_line_t to
the reader, so we have fewer places where we need to refilter.  Essentially we
only have two cases: insertions at the cursor are handled by insert_string(),
and all others go through push_edit().  This should also make it clearer
where we update undo_history.may_coalesce.

This commit was on the history-search-edit-needle branch, so it should
work fine.  I hope it does play well with some recent changes.

In 6d339df61 (Factor repainting decions from readline commands better
in the reader), insert_string() was simplified a lot, mirror that.

The tests for editable_line_t are not that useful anymore since the caller has
to decide whether to coalesce insertions, but I guess they don't hurt either.
We should have more tests for some interactive scenarios like undo and the
pager filtering.
---
 src/fish_tests.cpp |  20 ++++----
 src/reader.cpp     | 121 ++++++++++++++++++++++++---------------------
 src/reader.h       |  12 ++---
 3 files changed, 79 insertions(+), 74 deletions(-)

diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp
index a48b0f78d..074427c47 100644
--- a/src/fish_tests.cpp
+++ b/src/fish_tests.cpp
@@ -3439,7 +3439,7 @@ static void test_undo() {
     do_test(line.text() == L"a b c");
     do_test(line.position() == 5);
     line.set_position(2);
-    line.replace_substring(2, 1, L"B");  // replacement right of cursor
+    line.push_edit(edit_t(2, 1, L"B"));  // replacement right of cursor
     do_test(line.text() == L"a B c");
     line.undo();
     do_test(line.text() == L"a b c");
@@ -3450,7 +3450,7 @@ static void test_undo() {
 
     do_test(!line.redo());  // nothing to redo
 
-    line.erase_substring(0, 2);  // deletion left of cursor
+    line.push_edit(edit_t(0, 2, L""));  // deletion left of cursor
     do_test(line.text() == L"B c");
     do_test(line.position() == 1);
     line.undo();
@@ -3460,27 +3460,27 @@ static void test_undo() {
     do_test(line.text() == L"B c");
     do_test(line.position() == 1);
 
-    line.replace_substring(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"));  // 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.insert_string(L"a");
-    line.insert_string(L"b");
-    line.insert_string(L"c");
+    line.push_edit(edit_t(line.position(), 0, L"a"));
+    line.insert_coalesce(L"b");
+    line.insert_coalesce(L"c");
     do_test(line.undo_history.edits.size() == 1);
-    line.insert_string(L" ");
+    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, bu tdoes not coalesce with the first edit.
-    line.insert_string(L"d");
+    // 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"));
     do_test(line.undo_history.edits.size() == 2);
-    line.insert_string(L"e");
+    line.insert_coalesce(L"e");
     do_test(line.text() == L"abcde");
     line.undo();
     do_test(line.text() == L"abc");
diff --git a/src/reader.cpp b/src/reader.cpp
index 4cc12599a..fe7365f39 100644
--- a/src/reader.cpp
+++ b/src/reader.cpp
@@ -207,32 +207,6 @@ static bool want_to_coalesce_insertion_of(const editable_line_t &el, const wcstr
     return true;
 }
 
-void editable_line_t::insert_string(const wcstring &str, size_t start, size_t len) {
-    // Clamp the range to something valid.
-    size_t string_length = str.size();
-    start = std::min(start, string_length);      //!OCLINT(parameter reassignment)
-    len = std::min(len, string_length - start);  //!OCLINT(parameter reassignment)
-    if (want_to_coalesce_insertion_of(*this, str)) {
-        edit_t &edit = undo_history.edits.back();
-        edit.replacement.append(str);
-        apply_edit(&text_, edit_t(position(), 0, str));
-        set_position(position() + len);
-    } else {
-        push_edit(edit_t(position(), 0, str.substr(start, len)));
-    }
-    undo_history.may_coalesce = (str.size() == 1);
-}
-
-void editable_line_t::erase_substring(size_t offset, size_t length) {
-    push_edit(edit_t(offset, length, L""));
-    undo_history.may_coalesce = false;
-}
-
-void editable_line_t::replace_substring(size_t offset, size_t length, wcstring &&replacement) {
-    push_edit(edit_t(offset, length, replacement));
-    undo_history.may_coalesce = false;
-}
-
 bool editable_line_t::undo() {
     if (undo_history.edits_applied == 0) return false;  // nothing to undo
     const edit_t &edit = undo_history.edits.at(undo_history.edits_applied - 1);
@@ -266,6 +240,13 @@ void editable_line_t::push_edit(edit_t &&edit) {
     undo_history.edits.emplace_back(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() {
     if (undo_history.edits_applied >= undo_history.edits.size()) return false;  // nothing to redo
     const edit_t &edit = undo_history.edits.at(undo_history.edits_applied);
@@ -630,7 +611,14 @@ class reader_data_t : public std::enable_shared_from_this<reader_data_t> {
     void update_buff_pos(editable_line_t *el, maybe_t<size_t> new_pos = none_t());
 
     void kill(editable_line_t *el, size_t begin_idx, size_t length, int mode, int newv);
+    /// Inserts a substring of str given by start, len at the cursor position.
     void insert_string(editable_line_t *el, const wcstring &str);
+    /// Erase @length characters starting at @offset.
+    void erase_substring(editable_line_t *el, size_t offset, size_t length);
+    /// Replace the text of length @length at @offset by @replacement.
+    void replace_substring(editable_line_t *el, size_t offset, size_t length,
+                           wcstring &&replacement);
+    void push_edit(editable_line_t *el, edit_t &&edit);
 
     /// Insert the character into the command line buffer and print it to the screen using syntax
     /// highlighting, etc.
@@ -994,7 +982,7 @@ void reader_data_t::kill(editable_line_t *el, size_t begin_idx, size_t length, i
 
         kill_replace(old, kill_item);
     }
-    el->erase_substring(begin_idx, length);
+    erase_substring(el, begin_idx, length);
 }
 
 // This is called from a signal handler!
@@ -1107,9 +1095,8 @@ bool reader_data_t::expand_abbreviation_as_necessary(size_t cursor_backtrack) {
         size_t cursor_pos = el->position() - std::min(el->position(), cursor_backtrack);
 
         if (auto edit = reader_expand_abbreviation_in_command(el->text(), cursor_pos, vars())) {
-            el->push_edit(std::move(*edit));
+            push_edit(el, std::move(*edit));
             update_buff_pos(el);
-            el->undo_history.may_coalesce = false;
             result = true;
         }
     }
@@ -1374,27 +1361,49 @@ void reader_data_t::delete_char(bool backward) {
         pos--;
         width = fish_wcwidth(el->text().at(pos));
     } while (width == 0 && pos > 0);
-    el->erase_substring(pos, pos_end - pos);
+    erase_substring(el, pos, pos_end - pos);
     update_buff_pos(el);
     suppress_autosuggestion = true;
-    // The pager needs to be refiltered.
-    if (el == &this->pager.search_field_line) {
-        command_line_changed(el);
-    }
 }
 
 /// Insert the characters of the string into the command line buffer and print them to the screen
 /// using syntax highlighting, etc.
 /// Returns true if the string changed.
 void reader_data_t::insert_string(editable_line_t *el, const wcstring &str) {
-    if (!str.empty()) {
-        el->insert_string(str, 0, str.size());
-        if (el == &command_line) suppress_autosuggestion = false;
-        // The pager needs to be refiltered.
-        if (el == &this->pager.search_field_line) {
-            command_line_changed(el);
-        }
+    if (str.empty()) return;
+
+    command_line_has_transient_edit = false;
+    if (!history_search.active() && want_to_coalesce_insertion_of(*el, str)) {
+        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 = (str.size() == 1);
     }
+
+    if (el == &command_line) suppress_autosuggestion = false;
+    // The pager needs to be refiltered.
+    if (el == &this->pager.search_field_line) {
+        command_line_changed(el);
+    }
+}
+
+void reader_data_t::push_edit(editable_line_t *el, edit_t &&edit) {
+    el->push_edit(std::move(edit));
+    el->undo_history.may_coalesce = false;
+    // The pager needs to be refiltered.
+    if (el == &this->pager.search_field_line) {
+        command_line_changed(el);
+    }
+}
+
+void reader_data_t::erase_substring(editable_line_t *el, size_t offset, size_t length) {
+    push_edit(el, edit_t(offset, length, L""));
+}
+
+void reader_data_t::replace_substring(editable_line_t *el, size_t offset, size_t length,
+                                      wcstring &&replacement) {
+    push_edit(el, edit_t(offset, length, replacement));
 }
 
 /// Insert the string in the given command line at the given cursor position. The function checks if
@@ -1670,10 +1679,10 @@ void reader_data_t::accept_autosuggestion(bool full, bool single, move_word_styl
         // Accept the autosuggestion.
         if (full) {
             // Just take the whole thing.
-            command_line.replace_substring(0, command_line.size(), std::move(autosuggestion));
+            replace_substring(&command_line, 0, command_line.size(), std::move(autosuggestion));
         } else if (single) {
-            command_line.replace_substring(command_line.size(), 0,
-                                           autosuggestion.substr(command_line.size(), 1));
+            replace_substring(&command_line, command_line.size(), 0,
+                              autosuggestion.substr(command_line.size(), 1));
         } else {
             // Accept characters according to the specified style.
             move_word_state_machine_t state(style);
@@ -1683,8 +1692,8 @@ void reader_data_t::accept_autosuggestion(bool full, bool single, move_word_styl
                 if (!state.consume_char(wc)) break;
             }
             size_t have = command_line.size();
-            command_line.replace_substring(command_line.size(), 0,
-                                           autosuggestion.substr(have, want - have));
+            replace_substring(&command_line, command_line.size(), 0,
+                              autosuggestion.substr(have, want - have));
         }
     }
 }
@@ -2120,9 +2129,8 @@ static void reader_interactive_destroy() {
 /// Set the specified string as the current buffer.
 void reader_data_t::set_command_line_and_position(editable_line_t *el, wcstring &&new_str,
                                                   size_t pos) {
-    el->push_edit(edit_t(0, el->size(), std::move(new_str)));
+    push_edit(el, edit_t(0, el->size(), std::move(new_str)));
     el->set_position(pos);
-    el->undo_history.may_coalesce = false;
     update_buff_pos(el, pos);
 }
 
@@ -2148,7 +2156,7 @@ void reader_data_t::replace_current_token(wcstring &&new_token) {
 
     size_t offset = begin - buff;
     size_t length = end - begin;
-    el->replace_substring(offset, length, std::move(new_token));
+    replace_substring(el, offset, length, std::move(new_token));
 }
 
 /// Apply the history search to the command line.
@@ -2163,7 +2171,7 @@ void reader_data_t::update_command_line_from_history_search() {
         replace_current_token(std::move(new_text));
     } else {
         assert(history_search.by_line() || history_search.by_prefix());
-        el->replace_substring(0, el->size(), std::move(new_text));
+        replace_substring(&command_line, 0, command_line.size(), std::move(new_text));
     }
     command_line_has_transient_edit = true;
     assert(el == &command_line);
@@ -2229,7 +2237,7 @@ void reader_data_t::set_buffer_maintaining_pager(const wcstring &b, size_t pos,
         }
         command_line_has_transient_edit = true;
     }
-    command_line.replace_substring(0, command_line.size(), wcstring(b));
+    replace_substring(&command_line, 0, command_line.size(), wcstring(b));
     command_line_changed(&command_line);
 
     // Don't set a position past the command line length.
@@ -2526,7 +2534,6 @@ static int read_i(parser_t &parser) {
         conf.right_prompt_cmd = RIGHT_PROMPT_FUNCTION_NAME;
     }
 
-
     std::shared_ptr<reader_data_t> data =
         reader_push_ret(parser, history_session_id(parser.vars()), std::move(conf));
     data->import_history_if_necessary();
@@ -2955,8 +2962,8 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
                 editable_line_t *el = active_edit_line();
                 wcstring yank_str = kill_yank_rotate();
                 size_t new_yank_len = yank_str.size();
-                el->replace_substring(el->position() - rls.yank_len, rls.yank_len,
-                                      std::move(yank_str));
+                replace_substring(el, el->position() - rls.yank_len, rls.yank_len,
+                                  std::move(yank_str));
                 update_buff_pos(el);
                 rls.yank_len = new_yank_len;
                 suppress_autosuggestion = true;
@@ -3359,7 +3366,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
                 }
 
                 replacement.push_back(chr);
-                el->replace_substring(buff_pos, (size_t)1, std::move(replacement));
+                replace_substring(el, buff_pos, (size_t)1, std::move(replacement));
 
                 // Restore the buffer position since replace_substring moves
                 // the buffer position ahead of the replaced text.
@@ -3391,7 +3398,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
                     replacement.push_back(chr);
                 }
 
-                el->replace_substring(start, len, std::move(replacement));
+                replace_substring(el, start, len, std::move(replacement));
 
                 // Restore the buffer position since replace_substring moves
                 // the buffer position ahead of the replaced text.
@@ -3431,7 +3438,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
                 replacement.push_back(chr);
                 capitalized_first = capitalized_first || make_uppercase;
             }
-            el->replace_substring(init_pos, pos - init_pos, std::move(replacement));
+            replace_substring(el, init_pos, pos - init_pos, std::move(replacement));
             update_buff_pos(el);
             break;
         }
diff --git a/src/reader.h b/src/reader.h
index 05e844c44..e0949734b 100644
--- a/src/reader.h
+++ b/src/reader.h
@@ -97,15 +97,13 @@ class editable_line_t {
     }
 
     /// Modify the commandline according to @edit. Most modifications to the
-    /// text should pass through this function. You can use one of the wrappers below.
+    /// text should pass through this function.
     void push_edit(edit_t &&edit);
 
-    /// Erase @length characters starting at @offset.
-    void erase_substring(size_t offset, size_t length);
-    /// Replace the text of length @length at @offset by @replacement.
-    void replace_substring(size_t offset, size_t length, wcstring &&replacement);
-    /// Inserts a substring of str given by start, len at the cursor position.
-    void insert_string(const wcstring &str, size_t start = 0, size_t len = wcstring::npos);
+    /// 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();