Need to optimize color redrawing to reduce lag and flashes

[mqudsi]

I've been meaning to file this issue for years but needed to find the time to ascertain that it wasn't specific to my terminal emulator and just put it off repeatedly.

The current way that we handle edits in the middle of commandlines wrapping across multiple lines results in an extremely suboptimal experience that causes the colors to lag and shift while the commandline is being edited. This is best illustrated with an example, so here's a video showing the issue:

2022-09-05-161450.mp4

This screencast was taken with kitty as my terminal emulator, which is significantly faster than conhost -- the issue is even more visible there. It seems the colors are persisting in their location (which cell) while the text shifts, then fish recalculates the colors after the edit debounces?

[faho]

I have to confess I have literally never seen this.

[krobelus]

I can reproduce something similar on WSL2, or by emulating the slowness:

diff --git a/src/reader.cpp b/src/reader.cpp
index 0d49b2236..c30dbcef5 100644
--- a/src/reader.cpp
+++ b/src/reader.cpp
@@ -2702,6 +2702,7 @@ static std::function<highlight_result_t(void)> get_highlight_performer(parser_t
         operation_context_t ctx = get_bg_context(vars, generation_count);
         std::vector<highlight_spec_t> colors(text.size(), highlight_spec_t{});
         highlight_shell(text, colors, ctx, io_ok);
+        usleep(200 * 1000);
         return highlight_result_t{std::move(colors), text};
     };
 }

I think we can fix this easily by shifting the old highlighting vector when inserting/deleting text.

[mqudsi]

Thanks, @krobelus. A number of projects use this forced slowness approach (neovim was the one to introduce me to it) to track down inefficiencies in the screen update routines. Unfortunately we don't have just one place to insert them.

[krobelus]

Here's a quick & dirty fix. It needs some clean-up.
I'm not sure how to best simplify the reader; it has a mutual dependency to the undo logic in editable_line_t.
With callbacks we can make it so the undo logic doesn't need to know anything about the reader state.

Details

commit 21fc3fcd4dbd38fde7b13f1dd1eae62a3f407d7e
Author: Johannes Altmanninger <aclopte@gmail.com>
Date:   Tue Sep 6 20:39:54 2022 +0200

    When updating commandline, also update rendered highlighting
    
    After editing the command line, we repaint it using potentially stale
    syntax highlighting.  At the same time we kick off a background thread that
    recomputes syntax highlighting. On some systems, syntax highlighting is slow,
    so the stale syntax highlighting is visible.
    This is especially bad when editing somewhere in the middle of a long
    command line, because that invalidates the highlighting right of the edit.
    
    We can fix this by updating the old highlighting whenever the command line
    changes. This means that old text will keep its highlighting until a new
    one is computed.

diff --git a/src/reader.cpp b/src/reader.cpp
index 0d49b2236..8cc387d51 100644
--- a/src/reader.cpp
+++ b/src/reader.cpp
@@ -240,7 +240,7 @@ static bool want_to_coalesce_insertion_of(const editable_line_t &el, const wcstr
     return true;
 }
 
-bool editable_line_t::undo() {
+bool editable_line_t::undo(const std::function<void(const edit_t &)> &on_edit) {
     bool did_undo = false;
     maybe_t<int> last_group_id{-1};
     while (undo_history.edits_applied != 0) {
@@ -254,6 +254,7 @@ bool editable_line_t::undo() {
         edit_t inverse = edit_t(edit.offset, edit.replacement.size(), L"");
         inverse.replacement = edit.old;
         size_t old_position = edit.cursor_position_before_edit;
+        on_edit(inverse);
         apply_edit(&text_, inverse);
         set_position(old_position);
         did_undo = true;
@@ -296,7 +297,7 @@ void editable_line_t::insert_coalesce(const wcstring &str) {
     set_position(position() + str.size());
 }
 
-bool editable_line_t::redo() {
+bool editable_line_t::redo(const std::function<void(const edit_t &)> &on_edit) {
     bool did_redo = false;
 
     maybe_t<int> last_group_id{-1};
@@ -308,6 +309,7 @@ bool editable_line_t::redo() {
         }
         last_group_id = edit.group_id;
         undo_history.edits_applied++;
+        on_edit(edit);
         apply_edit(&text_, edit);
         set_position(cursor_position_after_edit(edit));
         did_redo = true;
@@ -816,6 +818,9 @@ class reader_data_t : public std::enable_shared_from_this<reader_data_t> {
     /// 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);
+    bool undo(editable_line_t &el);
+    bool redo(editable_line_t &el);
+    void on_edit(const editable_line_t &el, const edit_t &edit);
 
     /// Insert the character into the command line buffer and print it to the screen using syntax
     /// highlighting, etc.
@@ -1129,8 +1134,14 @@ layout_data_t reader_data_t::make_layout_data(maybe_t<highlight_list_t> mcolors)
     result.text = command_line.text();
 
     if (mcolors.has_value()) {
+        // Ensure our color list has the same length as the command line, by extending it with the last
+        // color. This typically reduces redraws; e.g. if the user continues types into an argument, we
+        // guess it's still an argument, while the highlighting proceeds in the background.
+        highlight_spec_t last_color = result.colors.empty() ? highlight_spec_t{} : result.colors.back();
         result.colors = mcolors.acquire();
+        result.colors.resize(result.text.size(), last_color);
     } else {
+        assert(rendered_layout.colors.size() == result.text.size());
         result.colors = rendered_layout.colors;
     }
 
@@ -1143,11 +1154,6 @@ layout_data_t reader_data_t::make_layout_data(maybe_t<highlight_list_t> mcolors)
     result.mode_prompt_buff = mode_prompt_buff;
     result.right_prompt_buff = right_prompt_buff;
 
-    // Ensure our color list has the same length as the command line, by extending it with the last
-    // color. This typically reduces redraws; e.g. if the user continues types into an argument, we
-    // guess it's still an argument, while the highlighting proceeds in the background.
-    highlight_spec_t last_color = result.colors.empty() ? highlight_spec_t{} : result.colors.back();
-    result.colors.resize(result.text.size(), last_color);
     return result;
 }
 
@@ -1693,6 +1699,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()) {
+        on_edit(*el, edit_t(el->position(), 0, str));
         if (!history_search.active() && want_to_coalesce_insertion_of(*el, str)) {
             el->insert_coalesce(str);
             assert(el->undo_history.may_coalesce);
@@ -1710,6 +1717,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) {
+    on_edit(*el, edit);
     el->push_edit(std::move(edit));
     el->undo_history.may_coalesce = false;
     maybe_refilter_pager(el);
@@ -1724,6 +1732,26 @@ void reader_data_t::replace_substring(editable_line_t *el, size_t offset, size_t
     push_edit(el, edit_t(offset, length, std::move(replacement)));
 }
 
+bool reader_data_t::undo(editable_line_t &el) {
+    return el.undo([this, &el](const edit_t &edit) { on_edit(el, edit); });
+}
+
+bool reader_data_t::redo(editable_line_t &el) {
+    return el.redo([this, &el](const edit_t &edit) { on_edit(el, edit); });
+}
+
+void reader_data_t::on_edit(const editable_line_t &el, const edit_t &edit) {
+    if (&el != &command_line)
+        return;
+    size_t offset = edit.offset;
+    size_t length = edit.length;
+    auto &replacement = edit.replacement;
+    auto &colors = rendered_layout.colors;
+    colors.erase(colors.begin() + offset, colors.begin() + offset + length);
+    highlight_spec_t last_color = offset < 1 ? highlight_spec_t{} : colors.at(offset - 1);
+    colors.insert(colors.begin() + offset, replacement.size(), last_color);
+}
+
 /// Insert the string in the given command line at the given cursor position. The function checks if
 /// the string is quoted or not and correctly escapes the string.
 ///
@@ -2514,7 +2542,7 @@ void reader_data_t::clear_transient_edit() {
     if (!command_line_has_transient_edit) {
         return;
     }
-    command_line.undo();
+    undo(command_line);
     update_buff_pos(&command_line);
     command_line_has_transient_edit = false;
 }
@@ -2540,7 +2568,7 @@ void reader_data_t::update_command_line_from_history_search() {
                                                    : history_search.current_result().text;
     editable_line_t *el = active_edit_line();
     if (command_line_has_transient_edit) {
-        el->undo();
+        undo(*el);
     }
     if (history_search.by_token()) {
         replace_current_token(std::move(new_text));
@@ -2606,7 +2634,7 @@ void reader_data_t::set_buffer_maintaining_pager(const wcstring &b, size_t pos,
     size_t command_line_len = b.size();
     if (transient) {
         if (command_line_has_transient_edit) {
-            command_line.undo();
+            undo(command_line);
         }
         command_line_has_transient_edit = true;
     }
@@ -3041,7 +3069,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;
     }
 
@@ -3122,6 +3150,7 @@ static int read_i(parser_t &parser) {
 
             data->update_buff_pos(&data->command_line, 0);
             data->command_line.clear();
+            data->rendered_layout.colors.clear();
             data->command_line_changed(&data->command_line);
             event_fire_generic(parser, L"fish_preexec", {command});
             auto eval_res = reader_run_command(parser, command);
@@ -3372,7 +3401,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
             if (rls.complete_did_insert &&
                 (rls.last_cmd == rl::complete || rls.last_cmd == rl::complete_and_search)) {
                 editable_line_t *el = active_edit_line();
-                el->undo();
+                undo(*el);
                 update_buff_pos(el);
             }
             break;
@@ -4109,7 +4138,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
         case rl::undo:
         case rl::redo: {
             editable_line_t *el = active_edit_line();
-            bool ok = (c == rl::undo) ? el->undo() : el->redo();
+            bool ok = (c == rl::undo) ? undo(*el) : redo(*el);
             if (ok) {
                 if (el == &command_line) {
                     clear_pager();
diff --git a/src/reader.h b/src/reader.h
index f22e9a378..1ab69f9bb 100644
--- a/src/reader.h
+++ b/src/reader.h
@@ -124,10 +124,10 @@ class editable_line_t {
     void insert_coalesce(const wcstring &str);
 
     /// Undo the most recent edit that was not yet undone. Returns true on success.
-    bool undo();
+    bool undo(const std::function<void(const edit_t &)> &on_edit = [](const edit_t&){});
 
     /// Redo the most recent undo. Returns true on success.
-    bool redo();
+    bool redo(const std::function<void(const edit_t &)> &on_edit = [](const edit_t&){});
 
     /// Start a logical grouping of command line edits that should be undone/redone together.
     void begin_edit_group();

[mqudsi]

Thanks, @krobelus. You beat me to the refactor.