From 5429b542581ce48caaab20a2ccb82c93d4b0b67e Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 13:18:51 -0700 Subject: [PATCH 01/11] Remove k_invalid_termsize --- src/common.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common.cpp b/src/common.cpp index 05f824ea2..5fd3b228f 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -102,8 +102,8 @@ static relaxed_atomic_t initial_fg_process_group{-1}; /// This struct maintains the current state of the terminal size. It is updated on demand after /// receiving a SIGWINCH. Use common_get_width()/common_get_height() to read it lazily. -static constexpr struct winsize k_invalid_termsize = {USHRT_MAX, USHRT_MAX, USHRT_MAX, USHRT_MAX}; -static owning_lock s_termsize{k_invalid_termsize}; +static owning_lock s_termsize{ + (struct winsize){USHRT_MAX, USHRT_MAX, USHRT_MAX, USHRT_MAX}}; static relaxed_atomic_bool_t s_termsize_valid{false}; From 812cc1dbafba5ec20d1846b93d0f535b3d0ee8aa Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 13:53:41 -0700 Subject: [PATCH 02/11] Clean up line_t Use a single allocation instead of two for text and colors. Comment and tighten up its methods. --- src/fish_tests.cpp | 5 ++++- src/screen.cpp | 15 +++++++++++---- src/screen.h | 40 +++++++++++++++++++++++----------------- 3 files changed, 38 insertions(+), 22 deletions(-) diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index b08346c50..e1ce29851 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2294,7 +2294,10 @@ struct pager_layout_testcase_t { std::replace(expected.begin(), expected.end(), L'\x2026', ellipsis_char); } - wcstring text = sd.line(0).to_string(); + wcstring text; + for (const auto &p : sd.line(0).text) { + text.push_back(p.first); + } if (text != expected) { std::fwprintf(stderr, L"width %zu got %zu<%ls>, expected %zu<%ls>\n", this->width, text.length(), text.c_str(), expected.length(), expected.c_str()); diff --git a/src/screen.cpp b/src/screen.cpp index 57be553e2..39678b63e 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -84,6 +84,14 @@ static size_t next_tab_stop(size_t current_line_width) { /// Like fish_wcwidth, but returns 0 for control characters instead of -1. static int fish_wcwidth_min_0(wchar_t widechar) { return std::max(0, fish_wcwidth(widechar)); } +int line_t::wcswidth_min_0(size_t max) const { + int result = 0; + for (size_t idx = 0, end = std::min(max, text.size()); idx < end; idx++) { + result += fish_wcwidth_min_0(text[idx].first); + } + return result; +} + /// Whether we permit soft wrapping. If so, in some cases we don't explicitly move to the second /// physical line on a wrapped logical line; instead we just output it. static bool allow_soft_wrap() { @@ -767,9 +775,8 @@ static void s_update(screen_t *scr, const wcstring &left_prompt, const wcstring // over the shared prefix of what we want to output now, and what we output before, to // avoid repeatedly outputting it. if (skip_prefix > 0) { - size_t skip_width = shared_prefix < skip_prefix - ? skip_prefix - : fish_wcswidth(&o_line.text.at(0), shared_prefix); + size_t skip_width = + shared_prefix < skip_prefix ? skip_prefix : o_line.wcswidth_min_0(shared_prefix); if (skip_width > skip_remaining) skip_remaining = skip_width; } @@ -846,7 +853,7 @@ static void s_update(screen_t *scr, const wcstring &left_prompt, const wcstring // Only do it if the previous line could conceivably be wider. // That means if it is a prefix of the current one we can skip it. if (s_line.text.size() != shared_prefix) { - int prev_width = fish_wcswidth(&s_line.text.at(0), s_line.text.size()); + int prev_width = s_line.wcswidth_min_0(); clear_remainder = prev_width > current_width; } } diff --git a/src/screen.h b/src/screen.h index 6a33ecfb2..177f17e01 100644 --- a/src/screen.h +++ b/src/screen.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include "common.h" @@ -30,42 +31,47 @@ class page_rendering_t; /// A class representing a single line of a screen. struct line_t { - std::vector text; - std::vector colors; - bool is_soft_wrapped; - size_t indentation; + /// A pair of a character, and the color with which to draw it. + using highlighted_char_t = std::pair; + std::vector text{}; + bool is_soft_wrapped{false}; + size_t indentation{0}; - line_t() : text(), colors(), is_soft_wrapped(false), indentation(0) {} + line_t() = default; + /// Clear the line's contents. void clear(void) { text.clear(); - colors.clear(); } - void append(wchar_t txt, highlight_spec_t color) { - text.push_back(txt); - colors.push_back(color); - } + /// Append a single character \p txt to the line with color \p c. + void append(wchar_t c, highlight_spec_t color) { text.push_back({c, color}); } + /// Append a nul-terminated string \p txt to the line, giving each character \p color. void append(const wchar_t *txt, highlight_spec_t color) { for (size_t i = 0; txt[i]; i++) { - text.push_back(txt[i]); - colors.push_back(color); + text.push_back({txt[i], color}); } } - size_t size(void) const { return text.size(); } + /// \return the number of characters. + size_t size() const { return text.size(); } - wchar_t char_at(size_t idx) const { return text.at(idx); } + /// \return the character at a char index. + wchar_t char_at(size_t idx) const { return text.at(idx).first; } - highlight_spec_t color_at(size_t idx) const { return colors.at(idx); } + /// \return the color at a char index. + highlight_spec_t color_at(size_t idx) const { return text.at(idx).second; } + /// Append the contents of \p line to this line. void append_line(const line_t &line) { text.insert(text.end(), line.text.begin(), line.text.end()); - colors.insert(colors.end(), line.colors.begin(), line.colors.end()); } - wcstring to_string() const { return wcstring(this->text.begin(), this->text.end()); } + /// \return the width of this line, counting up to no more than \p max characters. + /// This follows fish_wcswidth() semantics, except that characters whose width would be -1 are + /// treated as 0. + int wcswidth_min_0(size_t max = std::numeric_limits::max()) const; }; /// A class representing screen contents. From 0673d86242df592f5a6cf84253379ccb7f5ae406 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 14:28:44 -0700 Subject: [PATCH 03/11] Remove a dead overload of s_reset This function was not actually implemented. --- src/screen.h | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/screen.h b/src/screen.h index 177f17e01..2f85a5d75 100644 --- a/src/screen.h +++ b/src/screen.h @@ -186,23 +186,6 @@ void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_pro const std::vector &colors, const std::vector &indent, size_t cursor_pos, const page_rendering_t &pager_data, bool cursor_is_within_pager); -/// This function resets the screen buffers internal knowledge about the contents of the screen. Use -/// this function when some other function than s_write has written to the screen. -/// -/// \param s the screen to reset -/// \param reset_cursor whether the line on which the cursor has changed should be assumed to have -/// changed. If \c reset_cursor is false, the library will attempt to make sure that the screen area -/// does not seem to move up or down on repaint. -/// \param reset_prompt whether to reset the prompt as well. -/// -/// If reset_cursor is incorrectly set to false, this may result in screen contents being erased. If -/// it is incorrectly set to true, it may result in one or more lines of garbage on screen on the -/// next repaint. If this happens during a loop, such as an interactive resizing, there will be one -/// line of garbage for every repaint, which will quickly fill the screen. -void s_reset(screen_t *s, bool reset_cursor, bool reset_prompt = true); - -/// Stat stdout and stderr and save result as the current timestamp. -void s_save_status(screen_t *s); enum class screen_reset_mode_t { /// Do not make a new line, do not repaint the prompt. @@ -215,8 +198,21 @@ enum class screen_reset_mode_t { abandon_line_and_clear_to_end_of_screen }; +/// This function resets the screen buffers internal knowledge about the contents of the screen. Use +/// this function when some other function than s_write has written to the screen. +/// +/// \param s the screen to reset +/// \param mode the sort of screen reset to apply +/// +/// If reset_cursor is incorrectly set to false, this may result in screen contents being erased. If +/// it is incorrectly set to true, it may result in one or more lines of garbage on screen on the +/// next repaint. If this happens during a loop, such as an interactive resizing, there will be one +/// line of garbage for every repaint, which will quickly fill the screen. void s_reset(screen_t *s, screen_reset_mode_t mode); +/// Stat stdout and stderr and save result as the current timestamp. +void s_save_status(screen_t *s); + /// Issues an immediate clr_eos. void screen_force_clear_to_end(); From fc42516dfbb860faa07dcc641600f11643f40b68 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 14:53:42 -0700 Subject: [PATCH 04/11] Unwind some calls to common_get_width from inside screen common_get_width will "lazily" decide the screen width, which means changing the environment variable stack. This is a surprising thing to do from the middle of screen rendering. Switch to passing in widths explicitly to screen. --- src/reader.cpp | 17 +++++++++-------- src/screen.cpp | 36 ++++++++++++++++++------------------ src/screen.h | 19 ++++++++++++------- 3 files changed, 39 insertions(+), 33 deletions(-) diff --git a/src/reader.cpp b/src/reader.cpp index 3fa37496f..152e06dd0 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -799,8 +799,9 @@ void reader_data_t::repaint() { size_t cursor_position = focused_on_pager ? pager.cursor_position() : cmd_line->position(); // Prepend the mode prompt to the left prompt. - s_write(&screen, mode_prompt_buff + left_prompt_buff, right_prompt_buff, full_line, - cmd_line->size(), colors, indents, cursor_position, current_page_rendering, + int screen_width = common_get_width(); + s_write(&screen, screen_width, mode_prompt_buff + left_prompt_buff, right_prompt_buff, + full_line, cmd_line->size(), colors, indents, cursor_position, current_page_rendering, focused_on_pager); repaint_needed = false; @@ -974,7 +975,7 @@ void reader_data_t::repaint_if_needed() { if (needs_reset) { exec_prompt(); - s_reset(&screen, screen_reset_mode_t::current_line_and_prompt); + s_reset(&screen, common_get_width(), screen_reset_mode_t::current_line_and_prompt); screen_reset_needed = false; } @@ -2327,7 +2328,7 @@ void reader_pop() { reader_interactive_destroy(); } else { s_end_current_loop = false; - s_reset(&new_reader->screen, screen_reset_mode_t::abandon_line); + s_reset(&new_reader->screen, common_get_width(), screen_reset_mode_t::abandon_line); } } @@ -2666,7 +2667,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat // elsewhere, we detect if the mode output is empty. exec_mode_prompt(); if (!mode_prompt_buff.empty()) { - s_reset(&screen, screen_reset_mode_t::current_line_and_prompt); + s_reset(&screen, common_get_width(), screen_reset_mode_t::current_line_and_prompt); screen_reset_needed = false; repaint(); break; @@ -2679,7 +2680,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat if (!rls.coalescing_repaints) { rls.coalescing_repaints = true; exec_prompt(); - s_reset(&screen, screen_reset_mode_t::current_line_and_prompt); + s_reset(&screen, common_get_width(), screen_reset_mode_t::current_line_and_prompt); screen_reset_needed = false; repaint(); } @@ -2962,7 +2963,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat // already be printed, all we need to do is repaint. wcstring_list_t argv(1, el->text()); event_fire_generic(parser(), L"fish_posterror", &argv); - s_reset(&screen, screen_reset_mode_t::abandon_line); + s_reset(&screen, common_get_width(), screen_reset_mode_t::abandon_line); mark_repaint_needed(); } @@ -3483,7 +3484,7 @@ maybe_t reader_data_t::readline(int nchars_or_0) { history_search.reset(); - s_reset(&screen, screen_reset_mode_t::abandon_line); + s_reset(&screen, common_get_width(), screen_reset_mode_t::abandon_line); event_fire_generic(parser(), L"fish_prompt"); exec_prompt(); diff --git a/src/screen.cpp b/src/screen.cpp index 39678b63e..e23ce17c2 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -459,7 +459,7 @@ static void s_check_status(screen_t *s) { // modeled cursor y-pos to its earlier value. int prev_line = s->actual.cursor.y; write_loop(STDOUT_FILENO, "\r", 1); - s_reset(s, screen_reset_mode_t::current_line_and_prompt); + s_reset(s, common_get_width(), screen_reset_mode_t::current_line_and_prompt); s->actual.cursor.y = prev_line; } } @@ -488,7 +488,7 @@ static void s_desired_append_char(screen_t *s, wchar_t b, highlight_spec_t c, in current.clear(); s->desired.cursor.x = 0; } else { - int screen_width = common_get_width(); + int screen_width = s->desired.screen_width; int cw = bwidth; s->desired.create_line(line_no); @@ -531,7 +531,7 @@ static void s_move(screen_t *s, int new_x, int new_y) { // If we are at the end of our window, then either the cursor stuck to the edge or it didn't. We // don't know! We can fix it up though. - if (s->actual.cursor.x == common_get_width()) { + if (s->actual.cursor.x == s->actual.screen_width) { // Either issue a cr to go back to the beginning of this line, or a nl to go to the // beginning of the next one, depending on what we think is more efficient. if (new_y <= s->actual.cursor.y) { @@ -617,7 +617,7 @@ static void s_write_char(screen_t *s, wchar_t c, size_t width) { scoped_buffer_t outp(*s); s->actual.cursor.x += width; s->outp().writech(c); - if (s->actual.cursor.x == s->actual_width && allow_soft_wrap()) { + if (s->actual.cursor.x == s->actual.screen_width && allow_soft_wrap()) { s->soft_wrap_location = screen_data_t::cursor_t{0, s->actual.cursor.y + 1}; // Note that our cursor position may be a lie: Apple Terminal makes the right cursor stick @@ -699,8 +699,6 @@ static void s_update(screen_t *scr, const wcstring &left_prompt, const wcstring const size_t right_prompt_width = cached_layouts.calc_prompt_layout(right_prompt).last_line_width; - int screen_width = common_get_width(); - // Figure out how many following lines we need to clear (probably 0). size_t actual_lines_before_reset = scr->actual_lines_before_reset; scr->actual_lines_before_reset = 0; @@ -709,17 +707,19 @@ static void s_update(screen_t *scr, const wcstring &left_prompt, const wcstring bool need_clear_screen = scr->need_clear_screen; bool has_cleared_screen = false; - if (scr->actual_width != screen_width) { + const int screen_width = scr->desired.screen_width; + + if (scr->actual.screen_width != screen_width) { // Ensure we don't issue a clear screen for the very first output, to avoid issue #402. - if (scr->actual_width > 0) { + if (scr->actual.screen_width > 0) { need_clear_screen = true; s_move(scr, 0, 0); - s_reset(scr, screen_reset_mode_t::current_line_contents); + s_reset(scr, screen_width, screen_reset_mode_t::current_line_contents); need_clear_lines = need_clear_lines || scr->need_clear_lines; need_clear_screen = need_clear_screen || scr->need_clear_screen; } - scr->actual_width = screen_width; + scr->actual.screen_width = screen_width; } scr->need_clear_lines = false; @@ -792,7 +792,7 @@ static void s_update(screen_t *scr, const wcstring &left_prompt, const wcstring } if (next_line_will_change) { skip_remaining = - std::min(skip_remaining, static_cast(scr->actual_width - 2)); + std::min(skip_remaining, static_cast(scr->actual.screen_width - 2)); } } } @@ -1097,8 +1097,8 @@ static screen_layout_t compute_layout(screen_t *s, size_t screen_width, return result; } -void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_prompt, - const wcstring &commandline, size_t explicit_len, +void s_write(screen_t *s, int screen_width, const wcstring &left_prompt, + const wcstring &right_prompt, const wcstring &commandline, size_t explicit_len, const std::vector &colors, const std::vector &indent, size_t cursor_pos, const page_rendering_t &pager, bool cursor_is_within_pager) { static relaxed_atomic_t s_repaints{0}; @@ -1123,7 +1123,6 @@ void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_pro } s_check_status(s); - const size_t screen_width = common_get_width(); // Completely ignore impossibly small screens. if (screen_width < 4) { @@ -1138,7 +1137,8 @@ void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_pro s->autosuggestion_is_truncated = !autosuggestion.empty() && autosuggestion != layout.autosuggestion; - // Clear the desired screen. + // Clear the desired screen and set its width. + s->desired.screen_width = screen_width; s->desired.resize(0); s->desired.cursor.x = s->desired.cursor.y = 0; @@ -1189,7 +1189,8 @@ void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_pro s_update(s, layout.left_prompt, layout.right_prompt); s_save_status(s); } -void s_reset(screen_t *s, screen_reset_mode_t mode) { + +void s_reset(screen_t *s, int screen_width, screen_reset_mode_t mode) { assert(s && "Null screen"); bool abandon_line = false, repaint_prompt = false, clear_to_eos = false; @@ -1245,9 +1246,8 @@ void s_reset(screen_t *s, screen_reset_mode_t mode) { if (abandon_line) { // Do the PROMPT_SP hack. - int screen_width = common_get_width(); wcstring abandon_line_string; - abandon_line_string.reserve(screen_width + 32); // should be enough + abandon_line_string.reserve(screen_width + 32); // Don't need to check for fish_wcwidth errors; this is done when setting up // omitted_newline_char in common.cpp. diff --git a/src/screen.h b/src/screen.h index 2f85a5d75..6805f44eb 100644 --- a/src/screen.h +++ b/src/screen.h @@ -79,12 +79,18 @@ class screen_data_t { std::vector line_datas; public: + /// The width of the screen in this rendering. + /// -1 if not set, i.e. we have not rendered before. + int screen_width{-1}; + + /// Where the cursor is in (x, y) coordinates. struct cursor_t { int x{0}; int y{0}; cursor_t() = default; cursor_t(int a, int b) : x(a), y(b) {} - } cursor; + }; + cursor_t cursor; line_t &add_line(void) { line_datas.resize(line_datas.size() + 1); @@ -135,9 +141,6 @@ class screen_t { wcstring actual_left_prompt{}; /// Last right prompt width. size_t last_right_prompt_width{0}; - /// The actual width of the screen at the time of the last screen write, or negative if not yet - /// set. - int actual_width{-1}; /// If we support soft wrapping, we can output to this location without any cursor motion. maybe_t soft_wrap_location{}; /// Whether the last-drawn autosuggestion (if any) is truncated, or hidden entirely. @@ -171,6 +174,7 @@ class screen_t { /// screen in order to render the desired output using as few terminal commands as possible. /// /// \param s the screen on which to write +/// \param int screen_width the width of the screen to render /// \param left_prompt the prompt to prepend to the command line /// \param right_prompt the right prompt, or NULL if none /// \param commandline the command line @@ -181,8 +185,8 @@ class screen_t { /// \param cursor_pos where the cursor is /// \param pager_data any pager data, to append to the screen /// \param cursor_is_within_pager whether the position is within the pager line (first line) -void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_prompt, - const wcstring &commandline, size_t explicit_len, +void s_write(screen_t *s, int screen_width, const wcstring &left_prompt, + const wcstring &right_prompt, const wcstring &commandline, size_t explicit_len, const std::vector &colors, const std::vector &indent, size_t cursor_pos, const page_rendering_t &pager_data, bool cursor_is_within_pager); @@ -202,13 +206,14 @@ enum class screen_reset_mode_t { /// this function when some other function than s_write has written to the screen. /// /// \param s the screen to reset +/// \param screen_width the current width of the screen /// \param mode the sort of screen reset to apply /// /// If reset_cursor is incorrectly set to false, this may result in screen contents being erased. If /// it is incorrectly set to true, it may result in one or more lines of garbage on screen on the /// next repaint. If this happens during a loop, such as an interactive resizing, there will be one /// line of garbage for every repaint, which will quickly fill the screen. -void s_reset(screen_t *s, screen_reset_mode_t mode); +void s_reset(screen_t *s, int screen_width, screen_reset_mode_t mode); /// Stat stdout and stderr and save result as the current timestamp. void s_save_status(screen_t *s); From 7b486b4634c925bd725fe1a63775bce45fd9c501 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 15:16:27 -0700 Subject: [PATCH 05/11] Factor s_reset better Prior to this fix, s_reset would attempt to reset the screen, optionally using the PROMPT_SP hack to go to the next line. This in turn required passing in the screen width even if it wasn't needed (because we were not going to abandon the line). Factor this into two functions: - s_reset_line which does not apply the hack - s_reset_abandoning_line which applies the PROMPT_SP hack --- src/reader.cpp | 12 +-- src/screen.cpp | 194 +++++++++++++++++++++---------------------------- src/screen.h | 32 +++----- 3 files changed, 99 insertions(+), 139 deletions(-) diff --git a/src/reader.cpp b/src/reader.cpp index 152e06dd0..29151ebf0 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -975,7 +975,7 @@ void reader_data_t::repaint_if_needed() { if (needs_reset) { exec_prompt(); - s_reset(&screen, common_get_width(), screen_reset_mode_t::current_line_and_prompt); + s_reset_line(&screen, true /* repaint prompt */); screen_reset_needed = false; } @@ -2328,7 +2328,7 @@ void reader_pop() { reader_interactive_destroy(); } else { s_end_current_loop = false; - s_reset(&new_reader->screen, common_get_width(), screen_reset_mode_t::abandon_line); + s_reset_abandoning_line(&new_reader->screen, common_get_width()); } } @@ -2667,7 +2667,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat // elsewhere, we detect if the mode output is empty. exec_mode_prompt(); if (!mode_prompt_buff.empty()) { - s_reset(&screen, common_get_width(), screen_reset_mode_t::current_line_and_prompt); + s_reset_line(&screen, true /* redraw prompt */); screen_reset_needed = false; repaint(); break; @@ -2680,7 +2680,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat if (!rls.coalescing_repaints) { rls.coalescing_repaints = true; exec_prompt(); - s_reset(&screen, common_get_width(), screen_reset_mode_t::current_line_and_prompt); + s_reset_line(&screen, true /* redraw prompt */); screen_reset_needed = false; repaint(); } @@ -2963,7 +2963,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat // already be printed, all we need to do is repaint. wcstring_list_t argv(1, el->text()); event_fire_generic(parser(), L"fish_posterror", &argv); - s_reset(&screen, common_get_width(), screen_reset_mode_t::abandon_line); + s_reset_abandoning_line(&screen, common_get_width()); mark_repaint_needed(); } @@ -3484,7 +3484,7 @@ maybe_t reader_data_t::readline(int nchars_or_0) { history_search.reset(); - s_reset(&screen, common_get_width(), screen_reset_mode_t::abandon_line); + s_reset_abandoning_line(&screen, common_get_width()); event_fire_generic(parser(), L"fish_prompt"); exec_prompt(); diff --git a/src/screen.cpp b/src/screen.cpp index e23ce17c2..dc3da77df 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -458,8 +458,7 @@ static void s_check_status(screen_t *s) { // move to the beginning of the line, reset the modelled screen contents, and then set the // modeled cursor y-pos to its earlier value. int prev_line = s->actual.cursor.y; - write_loop(STDOUT_FILENO, "\r", 1); - s_reset(s, common_get_width(), screen_reset_mode_t::current_line_and_prompt); + s_reset_line(s, true /* repaint prompt */); s->actual.cursor.y = prev_line; } } @@ -714,7 +713,7 @@ static void s_update(screen_t *scr, const wcstring &left_prompt, const wcstring if (scr->actual.screen_width > 0) { need_clear_screen = true; s_move(scr, 0, 0); - s_reset(scr, screen_width, screen_reset_mode_t::current_line_contents); + s_reset_line(scr); need_clear_lines = need_clear_lines || scr->need_clear_lines; need_clear_screen = need_clear_screen || scr->need_clear_screen; @@ -1190,44 +1189,15 @@ void s_write(screen_t *s, int screen_width, const wcstring &left_prompt, s_save_status(s); } -void s_reset(screen_t *s, int screen_width, screen_reset_mode_t mode) { +void s_reset_line(screen_t *s, bool repaint_prompt) { assert(s && "Null screen"); - bool abandon_line = false, repaint_prompt = false, clear_to_eos = false; - switch (mode) { - case screen_reset_mode_t::current_line_contents: { - break; - } - case screen_reset_mode_t::current_line_and_prompt: { - repaint_prompt = true; - break; - } - case screen_reset_mode_t::abandon_line: { - abandon_line = true; - repaint_prompt = true; - break; - } - case screen_reset_mode_t::abandon_line_and_clear_to_end_of_screen: { - abandon_line = true; - repaint_prompt = true; - clear_to_eos = true; - break; - } - } + // Remember how many lines we had output to, so we can clear the remaining lines in the next + // call to s_update. This prevents leaving junk underneath the cursor when resizing a window + // wider such that it reduces our desired line count. + s->actual_lines_before_reset = std::max(s->actual_lines_before_reset, s->actual.line_count()); - // If we're abandoning the line, we must also be repainting the prompt. - assert(!abandon_line || repaint_prompt); - - // If we are not abandoning the line, we need to remember how many lines we had output to, so we - // can clear the remaining lines in the next call to s_update. This prevents leaving junk - // underneath the cursor when resizing a window wider such that it reduces our desired line - // count. - if (!abandon_line) { - s->actual_lines_before_reset = - std::max(s->actual_lines_before_reset, s->actual.line_count()); - } - - if (repaint_prompt && !abandon_line) { + if (repaint_prompt) { // If the prompt is multi-line, we need to move up to the prompt's initial line. We do this // by lying to ourselves and claiming that we're really below what we consider "line 0" // (which is the last line of the prompt). This will cause us to move up to try to get back @@ -1235,94 +1205,98 @@ void s_reset(screen_t *s, int screen_width, screen_reset_mode_t mode) { const size_t prompt_line_count = calc_prompt_lines(s->actual_left_prompt); assert(prompt_line_count >= 1); s->actual.cursor.y += (prompt_line_count - 1); - } else if (abandon_line) { - s->actual.cursor.y = 0; + s->actual_left_prompt.clear(); } - - if (repaint_prompt) s->actual_left_prompt.clear(); s->actual.resize(0); s->need_clear_lines = true; - s->need_clear_screen = s->need_clear_screen || clear_to_eos; - if (abandon_line) { - // Do the PROMPT_SP hack. - wcstring abandon_line_string; - abandon_line_string.reserve(screen_width + 32); + // This should prevent resetting the cursor position during the next repaint. + write_loop(STDOUT_FILENO, "\r", 1); + s->actual.cursor.x = 0; - // Don't need to check for fish_wcwidth errors; this is done when setting up - // omitted_newline_char in common.cpp. - int non_space_width = get_omitted_newline_width(); - // We do `>` rather than `>=` because the code below might require one extra space. - if (screen_width > non_space_width) { - bool justgrey = true; - if (cur_term && enter_dim_mode) { - std::string dim = tparm(const_cast(enter_dim_mode)); - if (!dim.empty()) { - // Use dim if they have it, so the color will be based on their actual normal - // color and the background of the termianl. - abandon_line_string.append(str2wcstring(dim)); - justgrey = false; - } + fstat(STDOUT_FILENO, &s->prev_buff_1); + fstat(STDERR_FILENO, &s->prev_buff_2); +} + +void s_reset_abandoning_line(screen_t *s, int screen_width) { + assert(s && "Null screen"); + + s->actual.cursor.y = 0; + s->actual.resize(0); + s->actual_left_prompt.clear(); + s->need_clear_lines = true; + + // Do the PROMPT_SP hack. + wcstring abandon_line_string; + abandon_line_string.reserve(screen_width + 32); + + // Don't need to check for fish_wcwidth errors; this is done when setting up + // omitted_newline_char in common.cpp. + int non_space_width = get_omitted_newline_width(); + // We do `>` rather than `>=` because the code below might require one extra space. + if (screen_width > non_space_width) { + bool justgrey = true; + if (cur_term && enter_dim_mode) { + std::string dim = tparm(const_cast(enter_dim_mode)); + if (!dim.empty()) { + // Use dim if they have it, so the color will be based on their actual normal + // color and the background of the termianl. + abandon_line_string.append(str2wcstring(dim)); + justgrey = false; } - if (cur_term && justgrey && set_a_foreground) { - if (max_colors >= 238) { - // draw the string in a particular grey - abandon_line_string.append( - str2wcstring(tparm(const_cast(set_a_foreground), 237))); - } else if (max_colors >= 9) { - // bright black (the ninth color, looks grey) - abandon_line_string.append( - str2wcstring(tparm(const_cast(set_a_foreground), 8))); - } else if (max_colors >= 2 && enter_bold_mode) { - // we might still get that color by setting black and going bold for bright - abandon_line_string.append( - str2wcstring(tparm(const_cast(enter_bold_mode)))); - abandon_line_string.append( - str2wcstring(tparm(const_cast(set_a_foreground), 0))); - } + } + if (cur_term && justgrey && set_a_foreground) { + if (max_colors >= 238) { + // draw the string in a particular grey + abandon_line_string.append( + str2wcstring(tparm(const_cast(set_a_foreground), 237))); + } else if (max_colors >= 9) { + // bright black (the ninth color, looks grey) + abandon_line_string.append( + str2wcstring(tparm(const_cast(set_a_foreground), 8))); + } else if (max_colors >= 2 && enter_bold_mode) { + // we might still get that color by setting black and going bold for bright + abandon_line_string.append( + str2wcstring(tparm(const_cast(enter_bold_mode)))); + abandon_line_string.append( + str2wcstring(tparm(const_cast(set_a_foreground), 0))); } - - abandon_line_string.append(get_omitted_newline_str()); - - if (cur_term && exit_attribute_mode) { - abandon_line_string.append(str2wcstring(tparm( - const_cast(exit_attribute_mode)))); // normal text ANSI escape sequence - } - - int newline_glitch_width = term_has_xn ? 0 : 1; - abandon_line_string.append(screen_width - non_space_width - newline_glitch_width, L' '); } - abandon_line_string.push_back(L'\r'); abandon_line_string.append(get_omitted_newline_str()); - // Now we are certainly on a new line. But we may have dropped the omitted newline char on - // it. So append enough spaces to overwrite the omitted newline char, and then clear all the - // spaces from the new line. - abandon_line_string.append(non_space_width, L' '); - abandon_line_string.push_back(L'\r'); - // Clear entire line. Zsh doesn't do this. Fish added this with commit 4417a6ee: If you have - // a prompt preceded by a new line, you'll get a line full of spaces instead of an empty - // line above your prompt. This doesn't make a difference in normal usage, but copying and - // pasting your terminal log becomes a pain. This commit clears that line, making it an - // actual empty line. - if (!is_dumb() && clr_eol) { - abandon_line_string.append(str2wcstring(clr_eol)); + + if (cur_term && exit_attribute_mode) { + abandon_line_string.append(str2wcstring(tparm( + const_cast(exit_attribute_mode)))); // normal text ANSI escape sequence } - const std::string narrow_abandon_line_string = wcs2string(abandon_line_string); - write_loop(STDOUT_FILENO, narrow_abandon_line_string.c_str(), - narrow_abandon_line_string.size()); - s->actual.cursor.x = 0; + int newline_glitch_width = term_has_xn ? 0 : 1; + abandon_line_string.append(screen_width - non_space_width - newline_glitch_width, L' '); } - if (!abandon_line) { - // This should prevent resetting the cursor position during the next repaint. - write_loop(STDOUT_FILENO, "\r", 1); - s->actual.cursor.x = 0; + abandon_line_string.push_back(L'\r'); + abandon_line_string.append(get_omitted_newline_str()); + // Now we are certainly on a new line. But we may have dropped the omitted newline char on + // it. So append enough spaces to overwrite the omitted newline char, and then clear all the + // spaces from the new line. + abandon_line_string.append(non_space_width, L' '); + abandon_line_string.push_back(L'\r'); + // Clear entire line. Zsh doesn't do this. Fish added this with commit 4417a6ee: If you have + // a prompt preceded by a new line, you'll get a line full of spaces instead of an empty + // line above your prompt. This doesn't make a difference in normal usage, but copying and + // pasting your terminal log becomes a pain. This commit clears that line, making it an + // actual empty line. + if (!is_dumb() && clr_eol) { + abandon_line_string.append(str2wcstring(clr_eol)); } - fstat(1, &s->prev_buff_1); - fstat(2, &s->prev_buff_2); + const std::string narrow_abandon_line_string = wcs2string(abandon_line_string); + write_loop(STDOUT_FILENO, narrow_abandon_line_string.c_str(), + narrow_abandon_line_string.size()); + s->actual.cursor.x = 0; + + fstat(STDOUT_FILENO, &s->prev_buff_1); + fstat(STDERR_FILENO, &s->prev_buff_2); } void screen_force_clear_to_end() { diff --git a/src/screen.h b/src/screen.h index 6805f44eb..04d1b721a 100644 --- a/src/screen.h +++ b/src/screen.h @@ -190,30 +190,16 @@ void s_write(screen_t *s, int screen_width, const wcstring &left_prompt, const std::vector &colors, const std::vector &indent, size_t cursor_pos, const page_rendering_t &pager_data, bool cursor_is_within_pager); +/// Resets the screen buffer's internal knowledge about the contents of the screen, +/// optionally repainting the prompt as well. +/// This function assumes that the current line is still valid. +void s_reset_line(screen_t *s, bool repaint_prompt = false); -enum class screen_reset_mode_t { - /// Do not make a new line, do not repaint the prompt. - current_line_contents, - /// Do not make a new line, do repaint the prompt. - current_line_and_prompt, - /// Abandon the current line, go to the next one, repaint the prompt. - abandon_line, - /// Abandon the current line, go to the next one, clear the rest of the screen. - abandon_line_and_clear_to_end_of_screen -}; - -/// This function resets the screen buffers internal knowledge about the contents of the screen. Use -/// this function when some other function than s_write has written to the screen. -/// -/// \param s the screen to reset -/// \param screen_width the current width of the screen -/// \param mode the sort of screen reset to apply -/// -/// If reset_cursor is incorrectly set to false, this may result in screen contents being erased. If -/// it is incorrectly set to true, it may result in one or more lines of garbage on screen on the -/// next repaint. If this happens during a loop, such as an interactive resizing, there will be one -/// line of garbage for every repaint, which will quickly fill the screen. -void s_reset(screen_t *s, int screen_width, screen_reset_mode_t mode); +/// Resets the screen buffer's internal knowldge about the contents of the screen, +/// abandoning the current line and going to the next line. +/// If clear_to_eos is set, +/// The screen width must be provided for the PROMPT_SP hack. +void s_reset_abandoning_line(screen_t *s, int screen_width); /// Stat stdout and stderr and save result as the current timestamp. void s_save_status(screen_t *s); From df618a076854110a17001af8a3cd581536606a06 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 15:46:54 -0700 Subject: [PATCH 06/11] Migrate DFLT_TERM from common.h to env.cpp There's no reason every .cpp file needs to see these values. --- src/common.h | 5 ----- src/env.cpp | 26 ++++++++++++-------------- src/env.h | 3 --- 3 files changed, 12 insertions(+), 22 deletions(-) diff --git a/src/common.h b/src/common.h index d12750cb5..c7fa396b6 100644 --- a/src/common.h +++ b/src/common.h @@ -774,11 +774,6 @@ void redirect_tty_output(); std::string get_path_to_tmp_dir(); -// Default terminal size -#define DFLT_TERM_COL 80 -#define DFLT_TERM_ROW 24 -#define DFLT_TERM_COL_STR L"80" -#define DFLT_TERM_ROW_STR L"24" void invalidate_termsize(bool invalidate_vars = false); struct winsize get_current_winsize(); diff --git a/src/env.cpp b/src/env.cpp index 67debe426..c947d1a8d 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -46,8 +46,12 @@ extern char **environ; /// The character used to delimit path and non-path variables in exporting and in string expansion. -static const wchar_t PATH_ARRAY_SEP = L':'; -static const wchar_t NONPATH_ARRAY_SEP = L' '; +static constexpr wchar_t PATH_ARRAY_SEP = L':'; +static constexpr wchar_t NONPATH_ARRAY_SEP = L' '; + +// Default terminal sizes. +static constexpr size_t DFLT_TERM_COL = 80; +static constexpr size_t DFLT_TERM_ROW = 24; bool curses_initialized = false; @@ -357,7 +361,12 @@ void env_init(const struct config_paths_t *paths /* or NULL */) { } else { vars.set_pwd_from_getcwd(); } - vars.set_termsize(); // initialize the terminal size variables + + // Initialize termsize variables. + if (vars.get(L"COLUMNS").missing_or_empty()) + vars.set_one(L"COLUMNS", ENV_GLOBAL, to_string(DFLT_TERM_COL)); + if (vars.get(L"LINES").missing_or_empty()) + vars.set_one(L"LINES", ENV_GLOBAL, to_string(DFLT_TERM_ROW)); // Set fish_bind_mode to "default". vars.set_one(FISH_BIND_MODE_VAR, ENV_GLOBAL, DEFAULT_BIND_MODE); @@ -1216,17 +1225,6 @@ void env_stack_t::set_last_statuses(statuses_t s) { acquire_impl()->perproc_data().statuses = std::move(s); } -/// If they don't already exist initialize the `COLUMNS` and `LINES` env vars to reasonable -/// defaults. They will be updated later by the `get_current_winsize()` function if they need to be -/// adjusted. -void env_stack_t::set_termsize() { - auto cols = get(L"COLUMNS"); - if (cols.missing_or_empty()) set_one(L"COLUMNS", ENV_GLOBAL, DFLT_TERM_COL_STR); - - auto rows = get(L"LINES"); - if (rows.missing_or_empty()) set_one(L"LINES", ENV_GLOBAL, DFLT_TERM_ROW_STR); -} - /// Update the PWD variable directory from the result of getcwd(). void env_stack_t::set_pwd_from_getcwd() { wcstring cwd = wgetcwd(); diff --git a/src/env.h b/src/env.h index 273ce9c5c..5cf907e2f 100644 --- a/src/env.h +++ b/src/env.h @@ -294,9 +294,6 @@ class env_stack_t final : public environment_t { int get_last_status() const; void set_last_statuses(statuses_t s); - /// Update the termsize variable. - void set_termsize(); - /// Sets up argv as the given list of strings. void set_argv(wcstring_list_t argv); From d5a239e59ec0e84a47d499270f618e432abd747f Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 17:21:57 -0700 Subject: [PATCH 07/11] Bravely stop attempting to modify the terminal size Prior to this fix, fish would attempt to resize the terminal via TIOCSWINSZ, which was added as part of #3740. In practice this probably never did anything useful since generally only the tty master can use this. Remove the support and note it in the changelog. --- CHANGELOG.rst | 1 + src/common.cpp | 7 ------- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index cbfeccc66..6e1b2a4fd 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -136,6 +136,7 @@ Completions Deprecations and removed features --------------------------------- +- fish no longer attempts to modify the terminal size via `TIOCSWINSZ`. For distributors and developers ------------------------------- diff --git a/src/common.cpp b/src/common.cpp index 5fd3b228f..61c5482e0 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -1801,13 +1801,6 @@ static void export_new_termsize(struct winsize *new_termsize, env_stack_t &vars) auto lines = vars.get(L"LINES", ENV_EXPORT); vars.set_one(L"LINES", ENV_GLOBAL | (lines.missing_or_empty() ? ENV_DEFAULT : ENV_EXPORT), std::to_wstring(int(new_termsize->ws_row))); - -#ifdef HAVE_WINSIZE - // Only write the new terminal size if we are in the foreground (#4477) - if (tcgetpgrp(STDOUT_FILENO) == getpgrp()) { - ioctl(STDOUT_FILENO, TIOCSWINSZ, new_termsize); - } -#endif } /// Get the current termsize, lazily computing it. Return by reference if it changed. From 340c8490f669ce2456e8b84de4d933135a0e520a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 16:05:52 -0700 Subject: [PATCH 08/11] Introduce termsize_container_t fish's handling of terminal sizes is currently rather twisted. The essential problem is that the terminal size may change at any point from a SIGWINCH, and common_get_{width,height} may modify it and post variable change events from arbitrary locations. Tighten up the semantics. Assign responsibility for managing the tty size to a new class, `termsize_container_t`. Rationalize locking and reentrancy. Explicitly nail down the relationship between $COLUMNS/$LINES and the tty size. The new semantics are: whatever changed most recently takes precendence. --- CMakeLists.txt | 2 +- src/env.cpp | 10 ++-- src/fish_tests.cpp | 69 ++++++++++++++++++++++++ src/signal.cpp | 4 +- src/termsize.cpp | 127 +++++++++++++++++++++++++++++++++++++++++++++ src/termsize.h | 110 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 314 insertions(+), 8 deletions(-) create mode 100644 src/termsize.cpp create mode 100644 src/termsize.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 5f9774ab0..e5284f092 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -121,7 +121,7 @@ set(FISH_SRCS src/wcstringutil.cpp src/wgetopt.cpp src/wildcard.cpp src/wutil.cpp src/future_feature_flags.cpp src/redirection.cpp src/topic_monitor.cpp src/flog.cpp src/trace.cpp src/timer.cpp src/null_terminated_array.cpp - src/operation_context.cpp src/fd_monitor.cpp + src/operation_context.cpp src/fd_monitor.cpp src/termsize.cpp ) # Header files are just globbed. diff --git a/src/env.cpp b/src/env.cpp index c947d1a8d..784258252 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -32,6 +32,7 @@ #include "path.h" #include "proc.h" #include "reader.h" +#include "termsize.h" #include "wutil.h" // IWYU pragma: keep /// Some configuration path environment variables. @@ -49,10 +50,6 @@ extern char **environ; static constexpr wchar_t PATH_ARRAY_SEP = L':'; static constexpr wchar_t NONPATH_ARRAY_SEP = L' '; -// Default terminal sizes. -static constexpr size_t DFLT_TERM_COL = 80; -static constexpr size_t DFLT_TERM_ROW = 24; - bool curses_initialized = false; /// Does the terminal have the "eat_newline_glitch". @@ -363,10 +360,11 @@ void env_init(const struct config_paths_t *paths /* or NULL */) { } // Initialize termsize variables. + auto termsize = termsize_container_t::shared().initialize(vars); if (vars.get(L"COLUMNS").missing_or_empty()) - vars.set_one(L"COLUMNS", ENV_GLOBAL, to_string(DFLT_TERM_COL)); + vars.set_one(L"COLUMNS", ENV_GLOBAL, to_string(termsize.width)); if (vars.get(L"LINES").missing_or_empty()) - vars.set_one(L"LINES", ENV_GLOBAL, to_string(DFLT_TERM_ROW)); + vars.set_one(L"LINES", ENV_GLOBAL, to_string(termsize.height)); // Set fish_bind_mode to "default". vars.set_one(FISH_BIND_MODE_VAR, ENV_GLOBAL, DEFAULT_BIND_MODE); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index e1ce29851..b54572cbc 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -73,6 +73,7 @@ #include "redirection.h" #include "screen.h" #include "signal.h" +#include "termsize.h" #include "timer.h" #include "tnode.h" #include "tokenizer.h" @@ -5743,6 +5744,72 @@ Executed in 500.00 micros fish external free(saved_locale); } +struct termsize_tester_t { + static void test(); +}; + +void termsize_tester_t::test() { + say(L"Testing termsize"); + + parser_t &parser = parser_t::principal_parser(); + env_stack_t &vars = parser.vars(); + + // Use a static variable so we can pretend we're the kernel exposing a terminal size. + static maybe_t stubby_termsize{}; + termsize_container_t ts([] { return stubby_termsize; }); + + // Initially default value. + do_test(ts.last() == termsize_t::defaults()); + + // Haha we change the value, it doesn't even know. + stubby_termsize = termsize_t{42, 84}; + do_test(ts.last() == termsize_t::defaults()); + + // Ok let's tell it. But it still doesn't update right away. + ts.handle_winch(); + do_test(ts.last() == termsize_t::defaults()); + + // Ok now we tell it to update. + ts.updating(parser); + do_test(ts.last() == *stubby_termsize); + do_test(vars.get(L"COLUMNS")->as_string() == L"42"); + do_test(vars.get(L"LINES")->as_string() == L"84"); + + // Wow someone set COLUMNS and LINES to a weird value. + // Now the tty's termsize doesn't matter. + vars.set(L"COLUMNS", ENV_GLOBAL, {L"75"}); + vars.set(L"LINES", ENV_GLOBAL, {L"150"}); + ts.handle_columns_lines_var_change(vars); + do_test(ts.last() == termsize_t(75, 150)); + do_test(vars.get(L"COLUMNS")->as_string() == L"75"); + do_test(vars.get(L"LINES")->as_string() == L"150"); + + vars.set(L"COLUMNS", ENV_GLOBAL, {L"33"}); + ts.handle_columns_lines_var_change(vars); + do_test(ts.last() == termsize_t(33, 150)); + + // Oh it got SIGWINCH, now the tty matters again. + ts.handle_winch(); + do_test(ts.last() == termsize_t(33, 150)); + do_test(ts.updating(parser) == *stubby_termsize); + do_test(vars.get(L"COLUMNS")->as_string() == L"42"); + do_test(vars.get(L"LINES")->as_string() == L"84"); + + // Test initialize(). + vars.set(L"COLUMNS", ENV_GLOBAL, {L"83"}); + vars.set(L"LINES", ENV_GLOBAL, {L"38"}); + ts.initialize(vars); + do_test(ts.last() == termsize_t(83, 38)); + + // initialize() even beats the tty reader until a sigwinch. + termsize_container_t ts2([] { return stubby_termsize; }); + ts.initialize(vars); + ts2.updating(parser); + do_test(ts.last() == termsize_t(83, 38)); + ts2.handle_winch(); + do_test(ts2.updating(parser) == *stubby_termsize); +} + /// Main test. int main(int argc, char **argv) { UNUSED(argc); @@ -5876,6 +5943,8 @@ int main(int argc, char **argv) { if (should_test_function("timer_format")) test_timer_format(); // history_tests_t::test_history_speed(); + if (should_test_function("termsize")) termsize_tester_t::test(); + say(L"Encountered %d errors in low-level tests", err_count); if (s_test_run_count == 0) say(L"*** No Tests Were Actually Run! ***"); diff --git a/src/signal.cpp b/src/signal.cpp index 932ea594d..179d7d339 100644 --- a/src/signal.cpp +++ b/src/signal.cpp @@ -16,6 +16,7 @@ #include "proc.h" #include "reader.h" #include "signal.h" +#include "termsize.h" #include "topic_monitor.h" #include "wutil.h" // IWYU pragma: keep @@ -219,7 +220,8 @@ static void fish_signal_handler(int sig, siginfo_t *info, void *context) { switch (sig) { #ifdef SIGWINCH case SIGWINCH: - /// Respond to a winch signal by checking the terminal size. + /// Respond to a winch signal by invalidating the terminal size. + termsize_container_t::handle_winch(); common_handle_winch(sig); break; #endif diff --git a/src/termsize.cpp b/src/termsize.cpp new file mode 100644 index 000000000..22144a290 --- /dev/null +++ b/src/termsize.cpp @@ -0,0 +1,127 @@ +// Support for exposing the terminal size. + +#include "termsize.h" + +#include "maybe.h" +#include "parser.h" +#include "wutil.h" + +// A counter which is incremented every SIGWINCH. +// This is only updated from termsize_handle_winch(). +static volatile uint32_t s_sigwinch_gen_count{0}; + +/// \return a termsize from ioctl, or none on error or if not supported. +static maybe_t read_termsize_from_tty() { + maybe_t result{}; +#ifdef HAVE_WINSIZE + struct winsize winsize = {0, 0, 0, 0}; + if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &winsize) >= 0) { + result = termsize_t{winsize.ws_col, winsize.ws_row}; + } +#endif + return result; +} + +// static +termsize_container_t &termsize_container_t::shared() { + // Heap-allocated to avoid runtime dtor registration. + static termsize_container_t *res = new termsize_container_t(read_termsize_from_tty); + return *res; +} + +termsize_t termsize_container_t::data_t::current() const { + // This encapsulates our ordering logic. If we have a termsize from a tty, use it; otherwise use + // what we have seen from the environment. + if (this->last_from_tty) return *this->last_from_tty; + if (this->last_from_env) return *this->last_from_env; + return termsize_t::defaults(); +} + +void termsize_container_t::data_t::mark_override_from_env(termsize_t ts) { + // Here we pretend to have an up-to-date tty value so that we will prefer the environment value. + this->last_from_env = ts; + this->last_from_tty.reset(); + this->last_winch_gen_count = s_sigwinch_gen_count; +} + +termsize_t termsize_container_t::last() const { return this->data_.acquire()->current(); } + +termsize_t termsize_container_t::updating(parser_t &parser) { + termsize_t new_size = termsize_t::defaults(); + termsize_t prev_size = termsize_t::defaults(); + + // Take the lock in a local region. + // Capture the size before and the new size. + { + auto data = data_.acquire(); + prev_size = data->current(); + + // Critical read of signal-owned variable. + // This must happen before the TIOCGWINSZ ioctl. + const uint32_t sigwinch_gen_count = s_sigwinch_gen_count; + if (data->last_winch_gen_count != sigwinch_gen_count) { + // We have received a sigwinch (or we have not yet computed the value). + // Apply any updates. + data->last_winch_gen_count = sigwinch_gen_count; + data->last_from_tty = this->tty_size_reader_(); + } + new_size = data->current(); + } + + // Announce any updates. + if (new_size != prev_size) set_columns_lines_vars(new_size, parser); + return new_size; +} + +void termsize_container_t::set_columns_lines_vars(termsize_t val, parser_t &parser) { + const bool saved = setting_env_vars_; + setting_env_vars_ = true; + parser.set_var_and_fire(L"COLUMNS", ENV_GLOBAL, to_string(val.width)); + parser.set_var_and_fire(L"LINES", ENV_GLOBAL, to_string(val.height)); + setting_env_vars_ = saved; +} + +/// Convert an environment variable to an int, or return a default value. +/// The int must be >0 and &var, int def) { + if (var.has_value() && !var->empty()) { + errno = 0; + int proposed = fish_wcstoi(var->as_string().c_str()); + if (errno == 0 && proposed > 0 && proposed <= USHRT_MAX) { + return proposed; + } + } + return def; +} + +termsize_t termsize_container_t::initialize(const environment_t &vars) { + termsize_t new_termsize{ + var_to_int_or(vars.get(L"COLUMNS", ENV_GLOBAL), -1), + var_to_int_or(vars.get(L"LINES", ENV_GLOBAL), -1), + }; + auto data = data_.acquire(); + if (new_termsize.width > 0 && new_termsize.height > 0) { + data->mark_override_from_env(new_termsize); + } else { + data->last_winch_gen_count = s_sigwinch_gen_count; + data->last_from_tty = this->tty_size_reader_(); + } + return data->current(); +} + +void termsize_container_t::handle_columns_lines_var_change(const environment_t &vars) { + // Do nothing if we are the ones setting it. + if (setting_env_vars_) return; + + // Construct a new termsize from COLUMNS and LINES, then set it in our data. + termsize_t new_termsize{ + var_to_int_or(vars.get(L"COLUMNS", ENV_GLOBAL), termsize_t::DEFAULT_WIDTH), + var_to_int_or(vars.get(L"LINES", ENV_GLOBAL), termsize_t::DEFAULT_HEIGHT), + }; + + // Store our termsize as an environment override. + data_.acquire()->mark_override_from_env(new_termsize); +} + +// static +void termsize_container_t::handle_winch() { s_sigwinch_gen_count += 1; } diff --git a/src/termsize.h b/src/termsize.h new file mode 100644 index 000000000..750d8f5c0 --- /dev/null +++ b/src/termsize.h @@ -0,0 +1,110 @@ +// Support for exposing the terminal size. + +#include "config.h" // IWYU pragma: keep +#ifndef FISH_TERMSIZE_H +#define FISH_TERMSIZE_H + +#include + +#include "common.h" +#include "global_safety.h" + +class environment_t; +class parser_t; +struct termsize_tester_t; + +/// A simple value type wrapping up a terminal size. +struct termsize_t { + /// Default width and height. + static constexpr int DEFAULT_WIDTH = 80; + static constexpr int DEFAULT_HEIGHT = 24; + + /// width of the terminal, in columns. + int width{DEFAULT_WIDTH}; + + /// height of the terminal, in rows. + int height{DEFAULT_HEIGHT}; + + /// Construct from width and height. + termsize_t(int w, int h) : width(w), height(h) {} + + /// Return a default-sized termsize. + static termsize_t defaults() { return termsize_t{DEFAULT_WIDTH, DEFAULT_HEIGHT}; } + + bool operator==(termsize_t rhs) const { + return this->width == rhs.width && this->height == rhs.height; + } + + bool operator!=(termsize_t rhs) const { return !(*this == rhs); } +}; + +/// Termsize monitoring is more complicated than one may think. +/// The main source of complexity is the interaction between the environment variables COLUMNS/ROWS, +/// the WINCH signal, and the TIOCGWINSZ ioctl. +/// Our policy is "last seen wins": if COLUMNS or LINES is modified, we respect that until we get a +/// SIGWINCH. +struct termsize_container_t { + /// \return the termsize without applying any updates. + /// Return the default termsize if none. + termsize_t last() const; + + /// If our termsize is stale, update it, using \p parser firing any events that may be + /// registered for COLUMNS and LINES. + /// \return the updated termsize. + termsize_t updating(parser_t &parser); + + /// Initialize our termsize, using the given environment stack. + /// This will prefer to use COLUMNS and LINES, but will fall back to the tty size reader. + /// This does not change any variables in the environment. + termsize_t initialize(const environment_t &vars); + + /// Note that a WINCH signal is received. + /// Naturally this may be called from within a signal handler. + static void handle_winch(); + + /// Note that COLUMNS and/or LINES global variables changed. + void handle_columns_lines_var_change(const environment_t &vars); + + /// \return the singleton shared container. + static termsize_container_t &shared(); + + private: + /// A function used for accessing the termsize from the tty. This is only exposed for testing. + using tty_size_reader_func_t = maybe_t (*)(); + + struct data_t { + // The last termsize returned by TIOCGWINSZ, or none if none. + maybe_t last_from_tty{}; + + // The last termsize seen from the environment (COLUMNS/LINES), or none if none. + maybe_t last_from_env{}; + + // The last-seen winch generation count. + // Set to a huge value so it's initially stale. + uint32_t last_winch_gen_count{UINT32_MAX}; + + /// \return the current termsize from this data. + termsize_t current() const; + + /// Mark that our termsize is (for the time being) from the environment, not the tty. + void mark_override_from_env(termsize_t ts); + }; + + // Construct from a reader function. + explicit termsize_container_t(tty_size_reader_func_t func) : tty_size_reader_(func) {} + + // Update COLUMNS and LINES in the parser's stack. + void set_columns_lines_vars(termsize_t val, parser_t &parser); + + // Our lock-protected data. + mutable owning_lock data_{}; + + // An indication that we are currently in the process of setting COLUMNS and LINES, and so do + // not react to any changes. + relaxed_atomic_bool_t setting_env_vars_{false}; + const tty_size_reader_func_t tty_size_reader_; + + friend termsize_tester_t; +}; + +#endif // FISH_TERMSIZE_H From db909605b8ae365b8c73d45a3b06af3f7e20c1eb Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 18:47:27 -0700 Subject: [PATCH 09/11] Migrate reformat_for_screen to new termsize container --- src/builtin_functions.cpp | 3 ++- src/common.cpp | 5 +++-- src/common.h | 6 ++++-- src/termsize.h | 3 +++ 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/builtin_functions.cpp b/src/builtin_functions.cpp index 790f7582c..f0fb7bca2 100644 --- a/src/builtin_functions.cpp +++ b/src/builtin_functions.cpp @@ -27,6 +27,7 @@ #include "parser_keywords.h" #include "proc.h" #include "signal.h" +#include "termsize.h" #include "wcstringutil.h" #include "wgetopt.h" #include "wutil.h" // IWYU pragma: keep @@ -371,7 +372,7 @@ int builtin_functions(parser_t &parser, io_streams_t &streams, wchar_t **argv) { buff.append(name); buff.append(L", "); } - streams.out.append(reformat_for_screen(buff)); + streams.out.append(reformat_for_screen(buff, termsize_last())); } else { for (const auto &name : names) { streams.out.append(name.c_str()); diff --git a/src/common.cpp b/src/common.cpp index 61c5482e0..afc11cb58 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -62,6 +62,7 @@ #include "parser.h" #include "proc.h" #include "signal.h" +#include "termsize.h" #include "wcstringutil.h" #include "wildcard.h" #include "wutil.h" // IWYU pragma: keep @@ -778,10 +779,10 @@ void narrow_string_safe(char buff[64], const wchar_t *s) { buff[idx] = '\0'; } -wcstring reformat_for_screen(const wcstring &msg) { +wcstring reformat_for_screen(const wcstring &msg, const termsize_t &termsize) { wcstring buff; int line_width = 0; - int screen_width = common_get_width(); + int screen_width = termsize.width; if (screen_width) { const wchar_t *start = msg.c_str(); diff --git a/src/common.h b/src/common.h index c7fa396b6..985e7b8ea 100644 --- a/src/common.h +++ b/src/common.h @@ -38,6 +38,8 @@ typedef std::wstring wcstring; typedef std::vector wcstring_list_t; +struct termsize_t; + // Maximum number of bytes used by a single utf-8 character. #define MAX_UTF8_BYTES 6 @@ -662,8 +664,8 @@ int common_get_height(); /// variable used by common_get_wisth and common_get_height(). void common_handle_winch(int signal); -/// Write the given paragraph of output, redoing linebreaks to fit the current screen. -wcstring reformat_for_screen(const wcstring &msg); +/// Write the given paragraph of output, redoing linebreaks to fit \p termsize. +wcstring reformat_for_screen(const wcstring &msg, const termsize_t &termsize); /// Print a short message about how to file a bug report to stderr. void bugreport(); diff --git a/src/termsize.h b/src/termsize.h index 750d8f5c0..e85257d6d 100644 --- a/src/termsize.h +++ b/src/termsize.h @@ -107,4 +107,7 @@ struct termsize_container_t { friend termsize_tester_t; }; +/// Convenience helper to return the last known termsize. +inline termsize_t termsize_last() { return termsize_container_t::shared().last(); } + #endif // FISH_TERMSIZE_H From 6bdbe732e40c2e325aa15fcf0f28ad0dedb3a551 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 18:59:15 -0700 Subject: [PATCH 10/11] Adopt termsize_t in the pager --- src/fish_tests.cpp | 6 +++--- src/pager.cpp | 6 +++--- src/pager.h | 5 +++-- src/reader.cpp | 5 +++-- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index b54572cbc..9c45ad749 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2201,7 +2201,7 @@ static void test_pager_navigation() { pager_t pager; pager.set_completions(completions); - pager.set_term_size(80, 24); + pager.set_term_size(termsize_t::defaults()); page_rendering_t render = pager.render(); if (render.term_width != 80) err(L"Wrong term width"); @@ -2275,14 +2275,14 @@ static void test_pager_navigation() { } struct pager_layout_testcase_t { - size_t width; + int width; const wchar_t *expected; // Run ourselves as a test case. // Set our data on the pager, and then check the rendering. // We should have one line, and it should have our expected text. void run(pager_t &pager) const { - pager.set_term_size(this->width, 24); + pager.set_term_size(termsize_t{this->width, 24}); page_rendering_t rendering = pager.render(); const screen_data_t &sd = rendering.screen_data; do_test(sd.line_count() == 1); diff --git a/src/pager.cpp b/src/pager.cpp index 78f3bb091..006062036 100644 --- a/src/pager.cpp +++ b/src/pager.cpp @@ -393,9 +393,9 @@ void pager_t::set_completions(const completion_list_t &raw_completions) { void pager_t::set_prefix(const wcstring &pref) { prefix = pref; } -void pager_t::set_term_size(size_t w, size_t h) { - available_term_width = w; - available_term_height = h; +void pager_t::set_term_size(termsize_t ts) { + available_term_width = ts.width > 0 ? ts.width : 0; + available_term_height = ts.height > 0 ? ts.height : 0; } /// Try to print the list of completions lst with the prefix prefix using cols as the number of diff --git a/src/pager.h b/src/pager.h index e6a8bcfc4..8160df1ce 100644 --- a/src/pager.h +++ b/src/pager.h @@ -12,6 +12,7 @@ #include "complete.h" #include "reader.h" #include "screen.h" +#include "termsize.h" #define PAGER_SELECTION_NONE static_cast(-1) @@ -146,8 +147,8 @@ class pager_t { // Sets the prefix. void set_prefix(const wcstring &pref); - // Sets the terminal width and height. - void set_term_size(size_t w, size_t h); + // Sets the terminal size. + void set_term_size(termsize_t ts); // Changes the selected completion in the given direction according to the layout of the given // rendering. Returns true if the selection changed. diff --git a/src/reader.cpp b/src/reader.cpp index 29151ebf0..68127f60b 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -791,8 +791,9 @@ void reader_data_t::repaint() { // term size, minus the number of lines consumed by our string. (Note this doesn't yet consider // wrapping). int full_line_count = 1 + std::count(full_line.cbegin(), full_line.cend(), L'\n'); - pager.set_term_size(std::max(1, common_get_width()), - std::max(1, common_get_height() - full_line_count)); + termsize_t curr_termsize = termsize_last(); + pager.set_term_size(termsize_t{std::max(1, curr_termsize.width), + std::max(1, curr_termsize.height - full_line_count)}); pager.update_rendering(¤t_page_rendering); bool focused_on_pager = active_edit_line() == &pager.search_field_line; From c7160d7cb4970c2a03df34547f357721cb5e88db Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 7 Jun 2020 19:35:47 -0700 Subject: [PATCH 11/11] Eliminate the termsize handling from common.h Finish the transition to termsize.h. Remove the scary termsize bits from common.cpp, which can throw off events at arbitrary calls and are dangerously reentrant. Migrate everyone to the new termsize.h. --- src/common.cpp | 113 ------------------------------------------- src/common.h | 18 ------- src/env_dispatch.cpp | 4 +- src/reader.cpp | 29 ++++++----- src/signal.cpp | 3 +- src/termsize.cpp | 22 +++++---- src/termsize.h | 7 ++- 7 files changed, 38 insertions(+), 158 deletions(-) diff --git a/src/common.cpp b/src/common.cpp index afc11cb58..9609d5698 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -28,12 +28,6 @@ #ifdef HAVE_EXECINFO_H #include #endif -#ifdef HAVE_SIGINFO_H -#include -#endif -#ifdef HAVE_SYS_IOCTL_H -#include -#endif #ifdef __linux__ // Includes for WSL detection @@ -101,13 +95,6 @@ int get_debug_stack_frames() { return debug_stack_frames; } /// This is set during startup and not modified after. static relaxed_atomic_t initial_fg_process_group{-1}; -/// This struct maintains the current state of the terminal size. It is updated on demand after -/// receiving a SIGWINCH. Use common_get_width()/common_get_height() to read it lazily. -static owning_lock s_termsize{ - (struct winsize){USHRT_MAX, USHRT_MAX, USHRT_MAX, USHRT_MAX}}; - -static relaxed_atomic_bool_t s_termsize_valid{false}; - static char *wcs2str_internal(const wchar_t *in, char *out); static void debug_shared(wchar_t msg_level, const wcstring &msg); @@ -1747,106 +1734,6 @@ bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t e return success; } -/// Used to invalidate our idea of having a valid window size. This can occur when either the -/// COLUMNS or LINES variables are changed. This is also invoked when the shell regains control of -/// the tty since it is possible the terminal size changed while an external command was running. -void invalidate_termsize(bool invalidate_vars) { - s_termsize_valid = false; - if (invalidate_vars) { - auto termsize = s_termsize.acquire(); - termsize->ws_col = termsize->ws_row = USHRT_MAX; - } -} - -/// Handle SIGWINCH. This is also invoked when the shell regains control of the tty since it is -/// possible the terminal size changed while an external command was running. -void common_handle_winch(int signal) { - (void)signal; - s_termsize_valid = false; -} - -/// Validate the new terminal size. Fallback to the env vars if necessary. -static void validate_new_termsize(struct winsize *new_termsize, const environment_t &vars) { - if (new_termsize->ws_col == 0 || new_termsize->ws_row == 0) { -#ifdef HAVE_WINSIZE - // Highly hackish. This seems like it should be moved. - if (is_main_thread() && parser_t::principal_parser().is_interactive()) { - FLOGF(warning, _(L"Current terminal parameters have rows and/or columns set to zero.")); - FLOGF(warning, _(L"The stty command can be used to correct this " - L"(e.g., stty rows 80 columns 24).")); - } -#endif - // Fallback to the environment vars. - maybe_t col_var = vars.get(L"COLUMNS"); - maybe_t row_var = vars.get(L"LINES"); - if (!col_var.missing_or_empty() && !row_var.missing_or_empty()) { - // Both vars have to have valid values. - int col = fish_wcstoi(col_var->as_string().c_str()); - bool col_ok = errno == 0 && col > 0 && col <= USHRT_MAX; - int row = fish_wcstoi(row_var->as_string().c_str()); - bool row_ok = errno == 0 && row > 0 && row <= USHRT_MAX; - if (col_ok && row_ok) { - new_termsize->ws_col = col; - new_termsize->ws_row = row; - } - } - } -} - -/// Export the new terminal size as env vars and to the kernel if possible. -static void export_new_termsize(struct winsize *new_termsize, env_stack_t &vars) { - auto cols = vars.get(L"COLUMNS", ENV_EXPORT); - vars.set_one(L"COLUMNS", ENV_GLOBAL | (cols.missing_or_empty() ? ENV_DEFAULT : ENV_EXPORT), - std::to_wstring(int(new_termsize->ws_col))); - - auto lines = vars.get(L"LINES", ENV_EXPORT); - vars.set_one(L"LINES", ENV_GLOBAL | (lines.missing_or_empty() ? ENV_DEFAULT : ENV_EXPORT), - std::to_wstring(int(new_termsize->ws_row))); -} - -/// Get the current termsize, lazily computing it. Return by reference if it changed. -static struct winsize get_current_winsize_prim(bool *changed, const environment_t &vars) { - auto termsize = s_termsize.acquire(); - if (s_termsize_valid) return *termsize; - - struct winsize new_termsize = {0, 0, 0, 0}; -#ifdef HAVE_WINSIZE - errno = 0; - if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &new_termsize) != -1 && - new_termsize.ws_col == termsize->ws_col && new_termsize.ws_row == termsize->ws_row) { - s_termsize_valid = true; - return *termsize; - } -#endif - validate_new_termsize(&new_termsize, vars); - termsize->ws_col = new_termsize.ws_col; - termsize->ws_row = new_termsize.ws_row; - *changed = true; - s_termsize_valid = true; - return *termsize; -} - -/// Updates termsize as needed, and returns a copy of the winsize. -struct winsize get_current_winsize() { - bool changed = false; - auto &vars = env_stack_t::globals(); - struct winsize termsize = get_current_winsize_prim(&changed, vars); - if (changed) { - // TODO: this may call us reentrantly through the environment dispatch mechanism. We need to - // rationalize this. - export_new_termsize(&termsize, vars); - // Hack: due to the dispatch the termsize may have just become invalid. Stomp it back to - // valid. What a mess. - *s_termsize.acquire() = termsize; - s_termsize_valid = true; - } - return termsize; -} - -int common_get_width() { return get_current_winsize().ws_col; } - -int common_get_height() { return get_current_winsize().ws_row; } - /// Returns true if seq, represented as a subsequence, is contained within string. static bool subsequence_in_string(const wcstring &seq, const wcstring &str) { // Impossible if seq is larger than string. diff --git a/src/common.h b/src/common.h index 985e7b8ea..ed842a576 100644 --- a/src/common.h +++ b/src/common.h @@ -648,21 +648,6 @@ bool unescape_string(const wchar_t *input, wcstring *output, unescape_flags_t es bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t escape_special, escape_string_style_t style = STRING_STYLE_SCRIPT); -/// Returns the width of the terminal window, so that not all functions that use these values -/// continually have to keep track of it separately. -/// -/// Only works if common_handle_winch is registered to handle winch signals. -int common_get_width(); - -/// Returns the height of the terminal window, so that not all functions that use these values -/// continually have to keep track of it separatly. -/// -/// Only works if common_handle_winch is registered to handle winch signals. -int common_get_height(); - -/// Handle a window change event by looking up the new window size and saving it in an internal -/// variable used by common_get_wisth and common_get_height(). -void common_handle_winch(int signal); /// Write the given paragraph of output, redoing linebreaks to fit \p termsize. wcstring reformat_for_screen(const wcstring &msg, const termsize_t &termsize); @@ -776,9 +761,6 @@ void redirect_tty_output(); std::string get_path_to_tmp_dir(); -void invalidate_termsize(bool invalidate_vars = false); -struct winsize get_current_winsize(); - bool valid_var_name_char(wchar_t chr); bool valid_var_name(const wcstring &str); bool valid_func_name(const wcstring &str); diff --git a/src/env_dispatch.cpp b/src/env_dispatch.cpp index 1f0d68b8c..2db92817d 100644 --- a/src/env_dispatch.cpp +++ b/src/env_dispatch.cpp @@ -51,6 +51,7 @@ #include "proc.h" #include "reader.h" #include "screen.h" +#include "termsize.h" #include "wutil.h" // IWYU pragma: keep #define DEFAULT_TERM1 "ansi" @@ -231,8 +232,7 @@ static void handle_change_ambiguous_width(const env_stack_t &vars) { } static void handle_term_size_change(const env_stack_t &vars) { - UNUSED(vars); - invalidate_termsize(true); // force fish to update its idea of the terminal size plus vars + termsize_container_t::shared().handle_columns_lines_var_change(vars); } static void handle_fish_history_change(const env_stack_t &vars) { diff --git a/src/reader.cpp b/src/reader.cpp index 68127f60b..17e98c0b1 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -74,6 +74,7 @@ #include "sanity.h" #include "screen.h" #include "signal.h" +#include "termsize.h" #include "tnode.h" #include "tokenizer.h" #include "wutil.h" // IWYU pragma: keep @@ -621,6 +622,9 @@ class reader_data_t : public std::enable_shared_from_this { void update_command_line_from_history_search(); void set_buffer_maintaining_pager(const wcstring &b, size_t pos, bool transient = false); void delete_char(bool backward = true); + + /// Called to update the termsize, including $COLUMNS and $LINES, as necessary. + void update_termsize() { (void)termsize_container_t::shared().updating(parser()); } }; /// This variable is set to a signal by the signal handler when ^C is pressed. @@ -695,7 +699,7 @@ static void term_steal() { break; } - invalidate_termsize(); + termsize_container_t::shared().invalidate_tty(); } bool reader_exit_forced() { return s_exit_forced; } @@ -800,8 +804,7 @@ void reader_data_t::repaint() { size_t cursor_position = focused_on_pager ? pager.cursor_position() : cmd_line->position(); // Prepend the mode prompt to the left prompt. - int screen_width = common_get_width(); - s_write(&screen, screen_width, mode_prompt_buff + left_prompt_buff, right_prompt_buff, + s_write(&screen, termsize_last().width, mode_prompt_buff + left_prompt_buff, right_prompt_buff, full_line, cmd_line->size(), colors, indents, cursor_position, current_page_rendering, focused_on_pager); @@ -1053,9 +1056,9 @@ void reader_data_t::exec_prompt() { // Do not allow the exit status of the prompts to leak through. const bool apply_exit_status = false; - // HACK: Query winsize again because it might have changed. + // Update the termsize now. // This allows prompts to react to $COLUMNS. - (void)get_current_winsize(); + update_termsize(); // If we have any prompts, they must be run non-interactively. if (!left_prompt.empty() || !right_prompt.empty()) { @@ -1091,7 +1094,8 @@ void reader_data_t::exec_prompt() { } void reader_init() { - auto &vars = parser_t::principal_parser().vars(); + parser_t &parser = parser_t::principal_parser(); + auto &vars = parser.vars(); // Ensure this var is present even before an interactive command is run so that if it is used // in a function like `fish_prompt` or `fish_right_prompt` it is defined at the time the first @@ -1121,7 +1125,7 @@ void reader_init() { // We do this not because we actually need the window size but for its side-effect of correctly // setting the COLUMNS and LINES env vars. - get_current_winsize(); + termsize_container_t::shared().updating(parser); } /// Restore the term mode if we own the terminal. It's important we do this before @@ -1972,7 +1976,7 @@ static void reader_interactive_init(parser_t &parser) { } } - invalidate_termsize(); + termsize_container_t::shared().invalidate_tty(); // For compatibility with fish 2.0's $_, now replaced with `status current-command` parser.vars().set_one(L"_", ENV_GLOBAL, L"fish"); @@ -2329,7 +2333,7 @@ void reader_pop() { reader_interactive_destroy(); } else { s_end_current_loop = false; - s_reset_abandoning_line(&new_reader->screen, common_get_width()); + s_reset_abandoning_line(&new_reader->screen, termsize_last().width); } } @@ -2964,7 +2968,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat // already be printed, all we need to do is repaint. wcstring_list_t argv(1, el->text()); event_fire_generic(parser(), L"fish_posterror", &argv); - s_reset_abandoning_line(&screen, common_get_width()); + s_reset_abandoning_line(&screen, termsize_last().width); mark_repaint_needed(); } @@ -3485,7 +3489,7 @@ maybe_t reader_data_t::readline(int nchars_or_0) { history_search.reset(); - s_reset_abandoning_line(&screen, common_get_width()); + s_reset_abandoning_line(&screen, termsize_last().width); event_fire_generic(parser(), L"fish_prompt"); exec_prompt(); @@ -3510,6 +3514,9 @@ maybe_t reader_data_t::readline(int nchars_or_0) { } while (!rls.finished && !shell_is_exiting()) { + // Perhaps update the termsize. This is cheap if it has not changed. + update_termsize(); + if (rls.nchars <= command_line.size()) { // We've already hit the specified character limit. rls.finished = true; diff --git a/src/signal.cpp b/src/signal.cpp index 179d7d339..0d7148c7f 100644 --- a/src/signal.cpp +++ b/src/signal.cpp @@ -220,9 +220,8 @@ static void fish_signal_handler(int sig, siginfo_t *info, void *context) { switch (sig) { #ifdef SIGWINCH case SIGWINCH: - /// Respond to a winch signal by invalidating the terminal size. + /// Respond to a winch signal by telling the termsize container. termsize_container_t::handle_winch(); - common_handle_winch(sig); break; #endif diff --git a/src/termsize.cpp b/src/termsize.cpp index 22144a290..ee5d715bc 100644 --- a/src/termsize.cpp +++ b/src/termsize.cpp @@ -6,9 +6,8 @@ #include "parser.h" #include "wutil.h" -// A counter which is incremented every SIGWINCH. -// This is only updated from termsize_handle_winch(). -static volatile uint32_t s_sigwinch_gen_count{0}; +// A counter which is incremented every SIGWINCH, or when the tty is otherwise invalidated. +static volatile uint32_t s_tty_termsize_gen_count{0}; /// \return a termsize from ioctl, or none on error or if not supported. static maybe_t read_termsize_from_tty() { @@ -41,7 +40,7 @@ void termsize_container_t::data_t::mark_override_from_env(termsize_t ts) { // Here we pretend to have an up-to-date tty value so that we will prefer the environment value. this->last_from_env = ts; this->last_from_tty.reset(); - this->last_winch_gen_count = s_sigwinch_gen_count; + this->last_tty_gen_count = s_tty_termsize_gen_count; } termsize_t termsize_container_t::last() const { return this->data_.acquire()->current(); } @@ -58,11 +57,11 @@ termsize_t termsize_container_t::updating(parser_t &parser) { // Critical read of signal-owned variable. // This must happen before the TIOCGWINSZ ioctl. - const uint32_t sigwinch_gen_count = s_sigwinch_gen_count; - if (data->last_winch_gen_count != sigwinch_gen_count) { - // We have received a sigwinch (or we have not yet computed the value). + const uint32_t tty_gen_count = s_tty_termsize_gen_count; + if (data->last_tty_gen_count != tty_gen_count) { + // Our idea of the size of the terminal may be stale. // Apply any updates. - data->last_winch_gen_count = sigwinch_gen_count; + data->last_tty_gen_count = tty_gen_count; data->last_from_tty = this->tty_size_reader_(); } new_size = data->current(); @@ -103,7 +102,7 @@ termsize_t termsize_container_t::initialize(const environment_t &vars) { if (new_termsize.width > 0 && new_termsize.height > 0) { data->mark_override_from_env(new_termsize); } else { - data->last_winch_gen_count = s_sigwinch_gen_count; + data->last_tty_gen_count = s_tty_termsize_gen_count; data->last_from_tty = this->tty_size_reader_(); } return data->current(); @@ -124,4 +123,7 @@ void termsize_container_t::handle_columns_lines_var_change(const environment_t & } // static -void termsize_container_t::handle_winch() { s_sigwinch_gen_count += 1; } +void termsize_container_t::handle_winch() { s_tty_termsize_gen_count += 1; } + +// static +void termsize_container_t::invalidate_tty() { s_tty_termsize_gen_count += 1; } diff --git a/src/termsize.h b/src/termsize.h index e85257d6d..46416429b 100644 --- a/src/termsize.h +++ b/src/termsize.h @@ -62,6 +62,9 @@ struct termsize_container_t { /// Naturally this may be called from within a signal handler. static void handle_winch(); + /// Invalidate the tty in the sense that we need to re-fetch its termsize. + static void invalidate_tty(); + /// Note that COLUMNS and/or LINES global variables changed. void handle_columns_lines_var_change(const environment_t &vars); @@ -79,9 +82,9 @@ struct termsize_container_t { // The last termsize seen from the environment (COLUMNS/LINES), or none if none. maybe_t last_from_env{}; - // The last-seen winch generation count. + // The last-seen tty-invalidation generation count. // Set to a huge value so it's initially stale. - uint32_t last_winch_gen_count{UINT32_MAX}; + uint32_t last_tty_gen_count{UINT32_MAX}; /// \return the current termsize from this data. termsize_t current() const;