Revert "Revert "Factor repainting decions from readline commands better in the reader""

This reverts commit eb86bf23e8.

This reintroduces the refactoring; the next two commits fix the
regressions identified in it.

src/env_dispatch.cpp

@@ -197,7 +197,7 @@ void env_dispatch_var_change(const wcstring &key, env_stack_t &vars) {
 
     // Eww.
     if (string_prefixes_string(L"fish_color_", key)) {
-        reader_react_to_color_change();
+        reader_schedule_prompt_repaint();
     }
 }
 
@@ -220,7 +220,7 @@ void env_universal_callbacks(env_stack_t *stack, const callback_data_list_t &cal
 
 static void handle_fish_term_change(const env_stack_t &vars) {
     update_fish_color_support(vars);
-    reader_react_to_color_change();
+    reader_schedule_prompt_repaint();
 }
 
 static void handle_change_ambiguous_width(const env_stack_t &vars) {

src/flog.h

@@ -99,6 +99,7 @@ class category_list_t {
     category_t term_support{L"term-support", L"Terminal feature detection"};
 
     category_t reader{L"reader", L"The interactive reader/input system"};
+    category_t reader_render{L"reader-render", L"Rendering the command line"};
     category_t complete{L"complete", L"The completion system"};
     category_t path{L"path", L"Searching/using paths"};
 

src/input.cpp

@@ -271,7 +271,7 @@ static maybe_t<char_event_t> interrupt_handler() {
     event_fire_delayed(parser);
     // Reap stray processes, including printing exit status messages.
     // TODO: shouldn't need this parser here.
-    if (job_reap(parser, true)) reader_repaint_needed();
+    if (job_reap(parser, true)) reader_schedule_prompt_repaint();
     // Tell the reader an event occurred.
     if (reader_reading_interrupted()) {
         auto vintr = shell_modes.c_cc[VINTR];

src/pager.cpp

@@ -14,6 +14,7 @@
 #include "common.h"
 #include "complete.h"
 #include "fallback.h"
+#include "flog.h"
 #include "highlight.h"
 #include "pager.h"
 #include "reader.h"
@@ -575,26 +576,28 @@ page_rendering_t pager_t::render() const {
     return rendering;
 }
 
+bool pager_t::rendering_needs_update(const page_rendering_t &rendering) const {
+    // Common case is no pager.
+    if (this->empty() && rendering.screen_data.empty()) return false;
+
+    return rendering.term_width != this->available_term_width ||    //
+           rendering.term_height != this->available_term_height ||  //
+           rendering.selected_completion_idx !=
+               this->visual_selected_completion_index(rendering.rows, rendering.cols) ||    //
+           rendering.search_field_shown != this->search_field_shown ||                      //
+           rendering.search_field_line.text() != this->search_field_line.text() ||          //
+           rendering.search_field_line.position() != this->search_field_line.position() ||  //
+           (rendering.remaining_to_disclose > 0 && this->fully_disclosed);
+}
+
 void pager_t::update_rendering(page_rendering_t *rendering) const {
-    if (rendering->term_width != this->available_term_width ||
+    if (rendering_needs_update(*rendering)) {
-        rendering->term_height != this->available_term_height ||
-        rendering->selected_completion_idx !=
-            this->visual_selected_completion_index(rendering->rows, rendering->cols) ||
-        rendering->search_field_shown != this->search_field_shown ||
-        rendering->search_field_line.text() != this->search_field_line.text() ||
-        rendering->search_field_line.position() != this->search_field_line.position() ||
-        (rendering->remaining_to_disclose > 0 && this->fully_disclosed)) {
         *rendering = this->render();
     }
 }
 
-pager_t::pager_t()
+pager_t::pager_t() = default;
-    : available_term_width(0),
+pager_t::~pager_t() = default;
-      available_term_height(0),
-      selected_completion_idx(PAGER_SELECTION_NONE),
-      suggested_row_start(0),
-      fully_disclosed(false),
-      search_field_shown(false) {}
 
 bool pager_t::empty() const { return unfiltered_completion_infos.empty(); }
 
@@ -855,5 +858,4 @@ size_t pager_t::cursor_position() const {
     return result;
 }
 
-// Constructor
 page_rendering_t::page_rendering_t() = default;

src/pager.h

@@ -62,17 +62,17 @@ enum class selection_motion_t {
 #define PAGER_UNDISCLOSED_MAX_ROWS 4
 
 class pager_t {
-    size_t available_term_width;
+    size_t available_term_width{0};
-    size_t available_term_height;
+    size_t available_term_height{0};
 
-    size_t selected_completion_idx;
+    size_t selected_completion_idx{PAGER_SELECTION_NONE};
-    size_t suggested_row_start;
+    size_t suggested_row_start{0};
 
     // Fully disclosed means that we show all completions.
-    bool fully_disclosed;
+    bool fully_disclosed{false};
 
     // Whether we show the search field.
-    bool search_field_shown;
+    bool search_field_shown{false};
 
     // Returns the index of the completion that should draw selected, using the given number of
     // columns.
@@ -82,19 +82,15 @@ class pager_t {
     /// Data structure describing one or a group of related completions.
     struct comp_t {
         /// The list of all completion strings this entry applies to.
-        wcstring_list_t comp;
+        wcstring_list_t comp{};
         /// The description.
-        wcstring desc;
+        wcstring desc{};
         /// The representative completion.
-        completion_t representative;
+        completion_t representative{L""};
         /// On-screen width of the completion string.
-        size_t comp_width;
+        size_t comp_width{0};
         /// On-screen width of the description information.
-        size_t desc_width;
+        size_t desc_width{0};
-        /// Minimum acceptable width.
-        // size_t min_width;
-
-        comp_t() : comp(), desc(), representative(L""), comp_width(0), desc_width(0) {}
 
         // Our text looks like this:
         // completion  (description)
@@ -112,7 +108,7 @@ class pager_t {
     };
 
    private:
-    typedef std::vector<comp_t> comp_info_list_t;
+    using comp_info_list_t = std::vector<comp_t>;
 
     // The filtered list of completion infos.
     comp_info_list_t completion_infos;
@@ -165,7 +161,10 @@ class pager_t {
     // Produces a rendering of the completions, at the given term size.
     page_rendering_t render() const;
 
-    // Updates the rendering if it's stale.
+    // \return true if the given rendering needs to be updated.
+    bool rendering_needs_update(const page_rendering_t &rendering) const;
+
+    // Updates the rendering.
     void update_rendering(page_rendering_t *rendering) const;
 
     // Indicates if there are no completions, and therefore nothing to render.
@@ -192,8 +191,8 @@ class pager_t {
     // Position of the cursor.
     size_t cursor_position() const;
 
-    // Constructor
     pager_t();
+    ~pager_t();
 };
 
 #endif

src/reader.h

@@ -138,15 +138,9 @@ void reader_change_history(const wcstring &name);
 /// \param reset_cursor_position If set, issue a \r so the line driver knows where we are
 void reader_write_title(const wcstring &cmd, parser_t &parser, bool reset_cursor_position = true);
 
-/// Call this function to tell the reader that a repaint is needed, and should be performed when
+/// Tell the reader that it needs to re-exec the prompt and repaint.
-/// possible.
+/// This may be called in response to e.g. a color variable change.
-void reader_repaint_needed();
+void reader_schedule_prompt_repaint();
-
-/// Call this function to tell the reader that some color has changed.
-void reader_react_to_color_change();
-
-/// Repaint immediately if needed.
-void reader_repaint_if_needed();
 
 /// Enqueue an event to the back of the reader's input queue.
 class char_event_t;