From 85d697f13de8d7017800b56ee2f9fcbc266c0f8e Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 3 Dec 2016 13:12:44 -0800 Subject: [PATCH 1/7] Add some pager layout test cases Helps ensure correct truncation logic --- src/fish_tests.cpp | 101 +++++++++++++++++++++++++++++++++++++++++++++ src/pager.h | 2 - src/screen.h | 8 +++- 3 files changed, 108 insertions(+), 3 deletions(-) diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 63b1a58a0..9bfb52efe 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -1627,6 +1627,106 @@ static void test_pager_navigation() { } } +struct pager_layout_testcase_t { + size_t 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); + page_rendering_t rendering = pager.render(); + const screen_data_t &sd = rendering.screen_data; + do_test(sd.line_count() == 1); + if (sd.line_count() > 0) { + wcstring expected = this->expected; + + // hack: handle the case where ellipsis is not L'\x2026' + if (ellipsis_char != L'\x2026') { + std::replace(expected.begin(), expected.end(), L'\x2026', ellipsis_char); + } + + wcstring text = sd.line(0).to_string(); + if (text != expected) { + fprintf(stderr, "width %lu got <%ls>, expected <%ls>\n", this->width, text.c_str(), expected.c_str()); + } + do_test(text == expected); + } + } +}; + +static void test_pager_layout() { + // These tests are woefully incomplete + // They only test the truncation logic for a single completion + say(L"Testing pager layout"); + pager_t pager; + + // These test cases have equal completions and descriptions + const completion_t c1(L"abcdefghij", L"1234567890"); + pager.set_completions(completion_list_t(1, c1)); + const pager_layout_testcase_t testcases1[] = { + {26, L"abcdefghij (1234567890)"}, + {25, L"abcdefghij (1234567890)"}, + {24, L"abcdefghij (1234567890)"}, + {23, L"abcdefghij (12345678…)"}, + {22, L"abcdefghij (1234567…)"}, + {21, L"abcdefghij (123456…)"}, + {20, L"abcdefghij (12345…)"}, + {19, L"abcdefghij (1234…)"}, + {18, L"abcdefgh… (1234…)"}, + {17, L"abcdefg… (1234…)"}, + {16, L"abcdefg… (123…)"}, + {0, NULL} // sentinel terminator + }; + for (size_t i=0; testcases1[i].expected != NULL; i++) { + testcases1[i].run(pager); + } + + // These test cases have heavyweight completions + const completion_t c2(L"abcdefghijklmnopqrs", L"1"); + pager.set_completions(completion_list_t(1, c2)); + const pager_layout_testcase_t testcases2[] = { + {26, L"abcdefghijklmnopqrs (1)"}, + {25, L"abcdefghijklmnopqrs (1)"}, + {24, L"abcdefghijklmnopqrs (1)"}, + {23, L"abcdefghijklmnopq… (1)"}, + {22, L"abcdefghijklmnop… (1)"}, + {21, L"abcdefghijklmno… (1)"}, + {20, L"abcdefghijklmn… (1)"}, + {19, L"abcdefghijklm… (1)"}, + {18, L"abcdefghijkl… (1)"}, + {17, L"abcdefghijk… (1)"}, + {16, L"abcdefghij… (1)"}, + {0, NULL} // sentinel terminator + }; + for (size_t i=0; testcases2[i].expected != NULL; i++) { + testcases2[i].run(pager); + } + + // These test cases have no descriptions + const completion_t c3(L"abcdefghijklmnopqrst", L""); + pager.set_completions(completion_list_t(1, c3)); + const pager_layout_testcase_t testcases3[] = { + {26, L"abcdefghijklmnopqrst"}, + {25, L"abcdefghijklmnopqrst"}, + {24, L"abcdefghijklmnopqrst"}, + {23, L"abcdefghijklmnopqrst"}, + {22, L"abcdefghijklmnopqrst"}, + {21, L"abcdefghijklmnopqrst"}, + {20, L"abcdefghijklmnopqrst"}, + {19, L"abcdefghijklmnopqr…"}, + {18, L"abcdefghijklmnopq…"}, + {17, L"abcdefghijklmnop…"}, + {16, L"abcdefghijklmno…"}, + {0, NULL} // sentinel terminator + }; + for (size_t i=0; testcases3[i].expected != NULL; i++) { + testcases3[i].run(pager); + } +} + + enum word_motion_t { word_motion_left, word_motion_right }; static void test_1_word_motion(word_motion_t motion, move_word_style_t style, const wcstring &test) { @@ -4123,6 +4223,7 @@ int main(int argc, char **argv) { if (should_test_function("test")) test_test(); if (should_test_function("path")) test_path(); if (should_test_function("pager_navigation")) test_pager_navigation(); + if (should_test_function("pager_layout")) test_pager_layout(); if (should_test_function("word_motion")) test_word_motion(); if (should_test_function("is_potential_path")) test_is_potential_path(); if (should_test_function("colors")) test_colors(); diff --git a/src/pager.h b/src/pager.h index 1376e51e7..dccfb74e9 100644 --- a/src/pager.h +++ b/src/pager.h @@ -43,8 +43,6 @@ class page_rendering_t { #define PAGER_UNDISCLOSED_MAX_ROWS 4 typedef std::vector completion_list_t; -page_rendering_t render_completions(const completion_list_t &raw_completions, - const wcstring &prefix); class pager_t { size_t available_term_width; diff --git a/src/screen.h b/src/screen.h index abe76df20..4dabeeb52 100644 --- a/src/screen.h +++ b/src/screen.h @@ -54,6 +54,10 @@ struct line_t { 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()); + } }; /// A class representing screen contents. @@ -89,7 +93,9 @@ class screen_data_t { line_t &line(size_t idx) { return line_datas.at(idx); } - size_t line_count(void) { return line_datas.size(); } + const line_t &line(size_t idx) const { return line_datas.at(idx); } + + size_t line_count() const { return line_datas.size(); } void append_lines(const screen_data_t &d) { this->line_datas.insert(this->line_datas.end(), d.line_datas.begin(), d.line_datas.end()); From d058d290be1dfe4c729a35dd5914ea8b6a8e282a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 2 Dec 2016 19:24:54 -0800 Subject: [PATCH 2/7] Factor pref_width into a function preferred_width() Beginnings of some pager cleanup --- src/pager.cpp | 9 +++------ src/pager.h | 16 +++++++++++++--- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/pager.cpp b/src/pager.cpp index 3c4adc7dd..deacdd960 100644 --- a/src/pager.cpp +++ b/src/pager.cpp @@ -88,10 +88,10 @@ line_t pager_t::completion_print_item(const wcstring &prefix, const comp_t *c, s UNUSED(column); UNUSED(row); UNUSED(rendering); - int comp_width, desc_width; + size_t comp_width, desc_width; line_t line_data; - if (c->pref_width <= width) { + if (c->preferred_width() <= width) { // The entry fits, we give it as much space as it wants. comp_width = c->comp_width; desc_width = c->desc_width; @@ -293,9 +293,6 @@ void pager_t::measure_completion_infos(comp_info_list_t *infos, const wcstring & // Compute desc_width. comp->desc_width = fish_wcswidth(comp->desc.c_str()); - - // Compute preferred width. - comp->pref_width = comp->comp_width + comp->desc_width + (comp->desc_width ? 4 : 0); } recalc_min_widths(infos); @@ -417,7 +414,7 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co if (lst.size() <= col * row_count + row) continue; c = &lst.at(col * row_count + row); - pref = c->pref_width; + pref = c->preferred_width(); min = c->min_width; if (col != cols - 1) { diff --git a/src/pager.h b/src/pager.h index dccfb74e9..7ab26c489 100644 --- a/src/pager.h +++ b/src/pager.h @@ -74,8 +74,6 @@ class pager_t { size_t comp_width; /// On-screen width of the description information. size_t desc_width; - /// Preferred total width. - size_t pref_width; /// Minimum acceptable width. size_t min_width; @@ -85,8 +83,20 @@ class pager_t { representative(L""), comp_width(0), desc_width(0), - pref_width(0), min_width(0) {} + + // Returns the width of the separator between the + // completion and description. If we have no description, + // we have no separator width + size_t separator_width() const { + return this->desc_width > 0 ? 4 : 0; + } + + // Returns the preferred width, containing the sum of the + // width of the completion, separator, and description + size_t preferred_width() const { + return this->comp_width + this->desc_width + this->separator_width(); + } }; private: From 2d5ce72cb44225b3fdc589e26bd16a8188556c00 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 2 Dec 2016 23:00:06 -0800 Subject: [PATCH 3/7] Remove the min_width parts of the pager min_width dates back to the original full-screen pager. After some careful inspection, the code path that uses min_width is never executed and so the min_width machinery is useless. Let's remove it! --- src/pager.cpp | 85 ++++++++++++++++++--------------------------------- src/pager.h | 8 ++--- 2 files changed, 34 insertions(+), 59 deletions(-) diff --git a/src/pager.cpp b/src/pager.cpp index deacdd960..6001fb4d4 100644 --- a/src/pager.cpp +++ b/src/pager.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include "common.h" @@ -43,17 +44,6 @@ static size_t divide_round_up(size_t numer, size_t denom) { return numer / denom + (has_rem ? 1 : 0); } -/// This function calculates the minimum width for each completion entry in the specified -/// array_list. This width depends on the terminal size, so this function should be called when the -/// terminal changes size. -void pager_t::recalc_min_widths(comp_info_list_t *lst) const { - for (size_t i = 0; i < lst->size(); i++) { - comp_t *c = &lst->at(i); - c->min_width = mini(c->desc_width, maxi((size_t)0, available_term_width / 3 - 2)) + - mini(c->desc_width, maxi((size_t)0, available_term_width / 5 - 4)) + 4; - } -} - /// Print the specified string, but use at most the specified amount of space. If the whole string /// can't be fitted, ellipsize it. /// @@ -62,7 +52,7 @@ void pager_t::recalc_min_widths(comp_info_list_t *lst) const { /// \param max the maximum space that may be used for printing /// \param has_more if this flag is true, this is not the entire string, and the string should be /// ellisiszed even if the string fits but takes up the whole space. -static int print_max(const wcstring &str, highlight_spec_t color, int max, bool has_more, +static int print_max(const wcstring &str, highlight_spec_t color, size_t max, bool has_more, line_t *line) { int written = 0; for (size_t i = 0; i < str.size(); i++) { @@ -155,7 +145,7 @@ line_t pager_t::completion_print_item(const wcstring &prefix, const comp_t *c, s /// \param row_stop the row after the last row to print /// \param prefix The string to print before each completion /// \param lst The list of completions to print -void pager_t::completion_print(size_t cols, int *width_per_column, size_t row_start, +void pager_t::completion_print(size_t cols, const int *width_per_column, size_t row_start, size_t row_stop, const wcstring &prefix, const comp_info_list_t &lst, page_rendering_t *rendering) const { // Teach the rendering about the rows it printed. @@ -294,8 +284,6 @@ void pager_t::measure_completion_infos(comp_info_list_t *infos, const wcstring & // Compute desc_width. comp->desc_width = fish_wcswidth(comp->desc.c_str()); } - - recalc_min_widths(infos); } // Indicates if the given completion info passes any filtering we have. @@ -356,29 +344,23 @@ void pager_t::set_term_size(int w, int h) { assert(h > 0); available_term_width = w; available_term_height = h; - recalc_min_widths(&completion_infos); } -/// Try to print the list of completions l with the prefix prefix using cols as the number of +/// Try to print the list of completions lst with the prefix prefix using cols as the number of /// columns. Return true if the completion list was printed, false if the terminal is too narrow for /// the specified number of columns. Always succeeds if cols is 1. bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const comp_info_list_t &lst, page_rendering_t *rendering, size_t suggested_start_row) const { // The calculated preferred width of each column. - int pref_width[PAGER_MAX_COLS] = {0}; - // The calculated minimum width of each column. - int min_width[PAGER_MAX_COLS] = {0}; - // If the list can be printed with this width, width will contain the width of each column. - int *width = pref_width; - + int width_by_column[PAGER_MAX_COLS] = {0}; + // Set to one if the list should be printed at this width. bool print = false; // Compute the effective term width and term height, accounting for disclosure. size_t term_width = this->available_term_width; - size_t term_height = - this->available_term_height - 1 - - (search_field_shown ? 1 : 0); // we always subtract 1 to make room for a comment row + // FIXME arithmetic + size_t term_height = this->available_term_height - 1 - (search_field_shown ? 1 : 0); // we always subtract 1 to make room for a comment row if (!this->fully_disclosed) { term_height = mini(term_height, (size_t)PAGER_UNDISCLOSED_MAX_ROWS); } @@ -395,49 +377,41 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co // If we have only one row remaining to disclose, then squelch the comment row. This prevents us // from consuming a line to show "...and 1 more row". - if (!this->fully_disclosed && rendering->remaining_to_disclose == 1) { + if (rendering->remaining_to_disclose == 1) { term_height += 1; rendering->remaining_to_disclose = 0; } - size_t pref_tot_width = 0; - size_t min_tot_width = 0; - // Skip completions on tiny terminals. if (term_width < PAGER_MIN_WIDTH) return true; // Calculate how wide the list would be. for (size_t col = 0; col < cols; col++) { for (size_t row = 0; row < row_count; row++) { - int pref, min; - const comp_t *c; - if (lst.size() <= col * row_count + row) continue; + const size_t comp_idx = col * row_count + row; + if (comp_idx >= lst.size()) continue; + const comp_t *c = &lst.at(comp_idx); - c = &lst.at(col * row_count + row); - pref = c->preferred_width(); - min = c->min_width; + // If we are not the last column, add two spaces after us + // to pad for the next column + const size_t column_padding = ((col+1 == cols) ? 0 : 2); + const size_t space_required = c->preferred_width() + column_padding; - if (col != cols - 1) { - pref += 2; - min += 2; - } - min_width[col] = maxi(min_width[col], min); - pref_width[col] = maxi(pref_width[col], pref); + width_by_column[col] = maxi(size_t(width_by_column[col]), space_required); } - min_tot_width += min_width[col]; - pref_tot_width += pref_width[col]; } + // Compute total preferred width + const size_t pref_tot_width = std::accumulate(width_by_column, width_by_column + cols, 0); + // Force fit if one column. if (cols == 1) { if (pref_tot_width > term_width) { - pref_width[0] = term_width; + width_by_column[0] = term_width; } - width = pref_width; print = true; } else if (pref_tot_width <= term_width) { // Terminal is wide enough. Print the list! - width = pref_width; print = true; } @@ -464,18 +438,17 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co assert(stop_row >= start_row); assert(stop_row <= row_count); assert(stop_row - start_row <= term_height); - completion_print(cols, width, start_row, stop_row, prefix, lst, rendering); + completion_print(cols, width_by_column, start_row, stop_row, prefix, lst, rendering); // Ellipsis helper string. Either empty or containing the ellipsis char. const wchar_t ellipsis_string[] = {ellipsis_char == L'\x2026' ? L'\x2026' : L'\0', L'\0'}; // Add the progress line. It's a "more to disclose" line if necessary, or a row listing if // it's scrollable; otherwise ignore it. + // We should never have one row remaining to disclose (else we would have just disclosed it) wcstring progress_text; - if (rendering->remaining_to_disclose == 1) { - // I don't expect this case to ever happen. - progress_text = format_string(_(L"%lsand 1 more row"), ellipsis_string); - } else if (rendering->remaining_to_disclose > 1) { + assert(rendering->remaining_to_disclose != 1); + if (rendering->remaining_to_disclose > 1) { progress_text = format_string(_(L"%lsand %lu more rows"), ellipsis_string, (unsigned long)rendering->remaining_to_disclose); } else if (start_row > 0 || stop_row < row_count) { @@ -491,9 +464,11 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co if (!progress_text.empty()) { line_t &line = rendering->screen_data.add_line(); - print_max(progress_text, highlight_spec_pager_progress | - highlight_make_background(highlight_spec_pager_progress), - term_width, true /* has_more */, &line); + highlight_spec_t spec = + highlight_spec_pager_progress | + highlight_make_background(highlight_spec_pager_progress); + print_max(progress_text, spec, term_width, + true /* has_more */, &line); } if (search_field_shown) { diff --git a/src/pager.h b/src/pager.h index 7ab26c489..406af3e40 100644 --- a/src/pager.h +++ b/src/pager.h @@ -75,15 +75,15 @@ class pager_t { /// On-screen width of the description information. size_t desc_width; /// Minimum acceptable width. - size_t min_width; + //size_t min_width; comp_t() : comp(), desc(), representative(L""), comp_width(0), - desc_width(0), - min_width(0) {} + desc_width(0) + {} // Returns the width of the separator between the // completion and description. If we have no description, @@ -118,7 +118,7 @@ class pager_t { bool completion_info_passes_filter(const comp_t &info) const; - void completion_print(size_t cols, int *width_per_column, size_t row_start, size_t row_stop, + void completion_print(size_t cols, const int *width_per_column, size_t row_start, size_t row_stop, const wcstring &prefix, const comp_info_list_t &lst, page_rendering_t *rendering) const; line_t completion_print_item(const wcstring &prefix, const comp_t *c, size_t row, size_t column, From 8041913e7a9efe6e85b9d3f5f3d81d0f2bbb7324 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 2 Dec 2016 23:29:14 -0800 Subject: [PATCH 4/7] Stop including spacer width in width_by_column in pager Additional refactoring to remove some sketchy-looking arithmetic --- src/pager.cpp | 45 +++++++++++++++++---------------------------- src/pager.h | 2 +- 2 files changed, 18 insertions(+), 29 deletions(-) diff --git a/src/pager.cpp b/src/pager.cpp index 6001fb4d4..a2b68bb50 100644 --- a/src/pager.cpp +++ b/src/pager.cpp @@ -140,12 +140,12 @@ line_t pager_t::completion_print_item(const wcstring &prefix, const comp_t *c, s /// style. /// /// \param cols number of columns to print in -/// \param width_per_column An array specifying the width of each column +/// \param width_by_column An array specifying the width of each column /// \param row_start The first row to print /// \param row_stop the row after the last row to print /// \param prefix The string to print before each completion /// \param lst The list of completions to print -void pager_t::completion_print(size_t cols, const int *width_per_column, size_t row_start, +void pager_t::completion_print(size_t cols, const size_t *width_by_column, size_t row_start, size_t row_stop, const wcstring &prefix, const comp_info_list_t &lst, page_rendering_t *rendering) const { // Teach the rendering about the rows it printed. @@ -154,14 +154,12 @@ void pager_t::completion_print(size_t cols, const int *width_per_column, size_t rendering->row_start = row_start; rendering->row_end = row_stop; - size_t rows = (lst.size() - 1) / cols + 1; + size_t rows = divide_round_up(lst.size(), cols); size_t effective_selected_idx = this->visual_selected_completion_index(rows, cols); for (size_t row = row_start; row < row_stop; row++) { for (size_t col = 0; col < cols; col++) { - int is_last = (col == (cols - 1)); - if (lst.size() <= col * rows + row) continue; size_t idx = col * rows + row; @@ -171,7 +169,7 @@ void pager_t::completion_print(size_t cols, const int *width_per_column, size_t // Print this completion on its own "line". line_t line = completion_print_item( prefix, el, row, col, - width_per_column[col] - (is_last ? 0 : PAGER_SPACER_STRING_WIDTH), row % 2, + width_by_column[col], row % 2, is_selected, rendering); // If there's more to come, append two spaces. @@ -351,12 +349,10 @@ void pager_t::set_term_size(int w, int h) { /// the specified number of columns. Always succeeds if cols is 1. bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const comp_info_list_t &lst, page_rendering_t *rendering, size_t suggested_start_row) const { + assert(cols > 0); // The calculated preferred width of each column. - int width_by_column[PAGER_MAX_COLS] = {0}; + size_t width_by_column[PAGER_MAX_COLS] = {0}; - // Set to one if the list should be printed at this width. - bool print = false; - // Compute the effective term width and term height, accounting for disclosure. size_t term_width = this->available_term_width; // FIXME arithmetic @@ -390,31 +386,24 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co for (size_t row = 0; row < row_count; row++) { const size_t comp_idx = col * row_count + row; if (comp_idx >= lst.size()) continue; - const comp_t *c = &lst.at(comp_idx); - - // If we are not the last column, add two spaces after us - // to pad for the next column - const size_t column_padding = ((col+1 == cols) ? 0 : 2); - const size_t space_required = c->preferred_width() + column_padding; - - width_by_column[col] = maxi(size_t(width_by_column[col]), space_required); + const comp_t &c = lst.at(comp_idx); + width_by_column[col] = std::max(width_by_column[col], c.preferred_width()); } } - // Compute total preferred width - const size_t pref_tot_width = std::accumulate(width_by_column, width_by_column + cols, 0); - + bool print; + assert(cols >= 1); // Force fit if one column. if (cols == 1) { - if (pref_tot_width > term_width) { - width_by_column[0] = term_width; - } - print = true; - } else if (pref_tot_width <= term_width) { - // Terminal is wide enough. Print the list! + width_by_column[0] = std::min(width_by_column[0], term_width); print = true; + } else { + // Compute total preferred width, plus spacing + assert(cols > 0); + size_t total_width_needed = std::accumulate(width_by_column, width_by_column + cols, 0); + total_width_needed += (cols - 1) * PAGER_SPACER_STRING_WIDTH; + print = (total_width_needed <= term_width); } - if (!print) { return false; // no need to continue } diff --git a/src/pager.h b/src/pager.h index 406af3e40..04f57a214 100644 --- a/src/pager.h +++ b/src/pager.h @@ -118,7 +118,7 @@ class pager_t { bool completion_info_passes_filter(const comp_t &info) const; - void completion_print(size_t cols, const int *width_per_column, size_t row_start, size_t row_stop, + void completion_print(size_t cols, const size_t *width_per_column, size_t row_start, size_t row_stop, const wcstring &prefix, const comp_info_list_t &lst, page_rendering_t *rendering) const; line_t completion_print_item(const wcstring &prefix, const comp_t *c, size_t row, size_t column, From 54dd4b7ed6919b0185b4336ec099379898582257 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 3 Dec 2016 02:40:07 -0800 Subject: [PATCH 5/7] Untangle some pager code and fix some warnings Fixes various warnings about implicit conversions --- src/pager.cpp | 112 +++++++++++++++++++++++++++++++------------------- src/pager.h | 17 ++++---- 2 files changed, 79 insertions(+), 50 deletions(-) diff --git a/src/pager.cpp b/src/pager.cpp index a2b68bb50..399d24566 100644 --- a/src/pager.cpp +++ b/src/pager.cpp @@ -51,29 +51,41 @@ static size_t divide_round_up(size_t numer, size_t denom) { /// \param color the color to apply to every printed character /// \param max the maximum space that may be used for printing /// \param has_more if this flag is true, this is not the entire string, and the string should be -/// ellisiszed even if the string fits but takes up the whole space. -static int print_max(const wcstring &str, highlight_spec_t color, size_t max, bool has_more, - line_t *line) { - int written = 0; +/// ellipsized even if the string fits but takes up the whole space. +static size_t print_max(const wcstring &str, highlight_spec_t color, size_t max, + bool has_more, line_t *line) { + size_t remaining = max; for (size_t i = 0; i < str.size(); i++) { wchar_t c = str.at(i); + int iwidth_c = fish_wcwidth(c); + if (iwidth_c < 0) { + // skip non-printable characters + continue; + } + size_t width_c = size_t(iwidth_c); - if (written + fish_wcwidth(c) > max) break; - if ((written + fish_wcwidth(c) == max) && (has_more || i + 1 < str.size())) { + if (width_c > remaining) break; + + if ((width_c == remaining) && (has_more || i + 1 < str.size())) { line->append(ellipsis_char, color); - written += fish_wcwidth(ellipsis_char); + int ellipsis_width = fish_wcwidth(ellipsis_char); + remaining -= std::min(remaining, size_t(ellipsis_width)); break; } line->append(c, color); - written += fish_wcwidth(c); + assert(remaining >= width_c); + remaining -= width_c; } - return written; + + // return how much we consumed + assert(remaining <= max); + return max - remaining; } /// Print the specified item using at the specified amount of space. line_t pager_t::completion_print_item(const wcstring &prefix, const comp_t *c, size_t row, - size_t column, int width, bool secondary, bool selected, + size_t column, size_t width, bool secondary, bool selected, page_rendering_t *rendering) const { UNUSED(column); UNUSED(row); @@ -87,11 +99,20 @@ line_t pager_t::completion_print_item(const wcstring &prefix, const comp_t *c, s desc_width = c->desc_width; } else { // The completion and description won't fit on the allocated space. Give a maximum of 2/3 of - // the space to the completion, and whatever is left to the description. - int desc_all = c->desc_width ? c->desc_width + 4 : 0; + // the space to the completion, and whatever is left to the description + // This expression is an overflow-safe way of calculating (width-4)*2/3 + size_t width_minus_spacer = width - std::min(width, size_t(4)); + size_t two_thirds_width = (width_minus_spacer/3)*2 + ((width_minus_spacer%3)*2)/3; + comp_width = std::min(c->comp_width, two_thirds_width); - comp_width = maxi(mini((int)c->comp_width, 2 * (width - 4) / 3), width - desc_all); - desc_width = c->desc_width ? width - 4 - comp_width : 0; + // If the description is short, give the completion the remaining space + size_t desc_punct_width = c->description_punctuated_width(); + if (width > desc_punct_width) { + comp_width = std::max(comp_width, width - desc_punct_width); + } + + // The description gets what's left + assert(comp_width <= width); } int bg_color = secondary ? highlight_spec_pager_secondary : highlight_spec_normal; @@ -99,38 +120,43 @@ line_t pager_t::completion_print_item(const wcstring &prefix, const comp_t *c, s bg_color = highlight_spec_search_match; } - int written = 0; + // Print the completion part + size_t comp_remaining = comp_width; for (size_t i = 0; i < c->comp.size(); i++) { const wcstring &comp = c->comp.at(i); + if (i > 0) { + comp_remaining -= print_max(PAGER_SPACER_STRING, highlight_spec_normal, comp_remaining, + true /* has_more */, &line_data); + } - if (i != 0) - written += print_max(PAGER_SPACER_STRING, highlight_spec_normal, comp_width - written, - true /* has_more */, &line_data); - - int packed_color = highlight_spec_pager_prefix | highlight_make_background(bg_color); - written += print_max(prefix, packed_color, comp_width - written, !comp.empty(), &line_data); + highlight_spec_t packed_color = highlight_spec_pager_prefix | highlight_make_background(bg_color); + comp_remaining -= print_max(prefix, packed_color, comp_remaining, !comp.empty(), &line_data); packed_color = highlight_spec_pager_completion | highlight_make_background(bg_color); - written += - print_max(comp, packed_color, comp_width - written, i + 1 < c->comp.size(), &line_data); + comp_remaining -= print_max(comp, packed_color, comp_remaining, i + 1 < c->comp.size(), &line_data); } - if (desc_width) { - int packed_color = highlight_spec_pager_description | highlight_make_background(bg_color); - while (written < (width - desc_width - 2)) // the 2 here refers to the parenthesis below - { - written += print_max(L" ", packed_color, 1, false, &line_data); + size_t desc_remaining = width - comp_width + comp_remaining; + if (c->desc_width > 0 && desc_remaining > 4) { + highlight_spec_t desc_color = highlight_spec_pager_description | highlight_make_background(bg_color); + highlight_spec_t punct_color = highlight_spec_pager_completion | highlight_make_background(bg_color); + + // always have at least two spaces to separate completion and description + desc_remaining -= print_max(L" ", punct_color, 2, false, &line_data); + + // right-justify the description by adding spaces + // the 2 here refers to the parenthesis below + while (desc_remaining > c->desc_width + 2) { + desc_remaining -= print_max(L" ", punct_color, 1, false, &line_data); } - // hack - this just works around the issue - print_max(L"(", highlight_spec_pager_completion | highlight_make_background(bg_color), 1, - false, &line_data); - print_max(c->desc, packed_color, desc_width, false, &line_data); - print_max(L")", highlight_spec_pager_completion | highlight_make_background(bg_color), 1, - false, &line_data); + + assert(desc_remaining >= 2); + desc_remaining -= print_max(L"(", punct_color, 1, false, &line_data); + desc_remaining -= print_max(c->desc, desc_color, desc_remaining - 1, false, &line_data); + desc_remaining -= print_max(L")", punct_color, 1, false, &line_data); } else { - while (written < width) { - written += print_max(L" ", 0, 1, false, &line_data); - } + // No description, or it won't fit. Just add spaces. + print_max(wcstring(desc_remaining, L' '), 0, desc_remaining, false, &line_data); } return line_data; @@ -352,7 +378,7 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co assert(cols > 0); // The calculated preferred width of each column. size_t width_by_column[PAGER_MAX_COLS] = {0}; - + // Compute the effective term width and term height, accounting for disclosure. size_t term_width = this->available_term_width; // FIXME arithmetic @@ -470,10 +496,12 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co line_t *search_field = &rendering->screen_data.insert_line_at_index(0); // We limit the width to term_width - 1. - int search_field_written = print_max(SEARCH_FIELD_PROMPT, highlight_spec_normal, - term_width - 1, false, search_field); - print_max(search_field_text, highlight_modifier_force_underline, - term_width - search_field_written - 1, false, search_field); + size_t search_field_remaining = term_width - 1; + search_field_remaining -= print_max(SEARCH_FIELD_PROMPT, highlight_spec_normal, + search_field_remaining, false, search_field); + + search_field_remaining -= print_max(search_field_text, highlight_modifier_force_underline, + search_field_remaining, false, search_field); } return true; diff --git a/src/pager.h b/src/pager.h index 04f57a214..0498b0803 100644 --- a/src/pager.h +++ b/src/pager.h @@ -85,17 +85,18 @@ class pager_t { desc_width(0) {} - // Returns the width of the separator between the - // completion and description. If we have no description, - // we have no separator width - size_t separator_width() const { - return this->desc_width > 0 ? 4 : 0; + // Our text looks like this: + // completion (description) + // Two spaces separating, plus parens, yields 4 total extra space + // but only if we have a description of course + size_t description_punctuated_width() const { + return this->desc_width + (this->desc_width ? 4 : 0); } // Returns the preferred width, containing the sum of the - // width of the completion, separator, and description + // width of the completion, separator, description size_t preferred_width() const { - return this->comp_width + this->desc_width + this->separator_width(); + return this->comp_width + this->description_punctuated_width(); } }; @@ -122,7 +123,7 @@ class pager_t { const wcstring &prefix, const comp_info_list_t &lst, page_rendering_t *rendering) const; line_t completion_print_item(const wcstring &prefix, const comp_t *c, size_t row, size_t column, - int width, bool secondary, bool selected, + size_t width, bool secondary, bool selected, page_rendering_t *rendering) const; public: From ffd4754cb2237718cec9d6b5697122bed6a3d7c8 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 3 Dec 2016 13:35:24 -0800 Subject: [PATCH 6/7] Don't show the pager on terminals of height less than 4 Prevents some potential overflow bugs and janky UI --- src/pager.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/pager.cpp b/src/pager.cpp index 399d24566..0dcf13b93 100644 --- a/src/pager.cpp +++ b/src/pager.cpp @@ -26,6 +26,9 @@ typedef std::vector comp_info_list_t; /// The minimum width (in characters) the terminal must to show completions at all. #define PAGER_MIN_WIDTH 16 +/// Minimum height to show completions +#define PAGER_MIN_HEIGHT 4 + /// The maximum number of columns of completion to attempt to fit onto the screen. #define PAGER_MAX_COLS 6 @@ -379,9 +382,11 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co // The calculated preferred width of each column. size_t width_by_column[PAGER_MAX_COLS] = {0}; + // Skip completions on tiny terminals. + if (this->available_term_width < PAGER_MIN_WIDTH || this->available_term_height < PAGER_MIN_HEIGHT) return true; + // Compute the effective term width and term height, accounting for disclosure. size_t term_width = this->available_term_width; - // FIXME arithmetic size_t term_height = this->available_term_height - 1 - (search_field_shown ? 1 : 0); // we always subtract 1 to make room for a comment row if (!this->fully_disclosed) { term_height = mini(term_height, (size_t)PAGER_UNDISCLOSED_MAX_ROWS); @@ -404,9 +409,6 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co rendering->remaining_to_disclose = 0; } - // Skip completions on tiny terminals. - if (term_width < PAGER_MIN_WIDTH) return true; - // Calculate how wide the list would be. for (size_t col = 0; col < cols; col++) { for (size_t row = 0; row < row_count; row++) { From ed85393611c2c63c3273eb696c8cb941d18e121f Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 3 Dec 2016 13:38:50 -0800 Subject: [PATCH 7/7] Restyle pager.cpp via make style --- src/pager.cpp | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/src/pager.cpp b/src/pager.cpp index 0dcf13b93..c4b9c7e2a 100644 --- a/src/pager.cpp +++ b/src/pager.cpp @@ -55,8 +55,8 @@ static size_t divide_round_up(size_t numer, size_t denom) { /// \param max the maximum space that may be used for printing /// \param has_more if this flag is true, this is not the entire string, and the string should be /// ellipsized even if the string fits but takes up the whole space. -static size_t print_max(const wcstring &str, highlight_spec_t color, size_t max, - bool has_more, line_t *line) { +static size_t print_max(const wcstring &str, highlight_spec_t color, size_t max, bool has_more, + line_t *line) { size_t remaining = max; for (size_t i = 0; i < str.size(); i++) { wchar_t c = str.at(i); @@ -105,7 +105,7 @@ line_t pager_t::completion_print_item(const wcstring &prefix, const comp_t *c, s // the space to the completion, and whatever is left to the description // This expression is an overflow-safe way of calculating (width-4)*2/3 size_t width_minus_spacer = width - std::min(width, size_t(4)); - size_t two_thirds_width = (width_minus_spacer/3)*2 + ((width_minus_spacer%3)*2)/3; + size_t two_thirds_width = (width_minus_spacer / 3) * 2 + ((width_minus_spacer % 3) * 2) / 3; comp_width = std::min(c->comp_width, two_thirds_width); // If the description is short, give the completion the remaining space @@ -129,20 +129,25 @@ line_t pager_t::completion_print_item(const wcstring &prefix, const comp_t *c, s const wcstring &comp = c->comp.at(i); if (i > 0) { comp_remaining -= print_max(PAGER_SPACER_STRING, highlight_spec_normal, comp_remaining, - true /* has_more */, &line_data); + true /* has_more */, &line_data); } - highlight_spec_t packed_color = highlight_spec_pager_prefix | highlight_make_background(bg_color); - comp_remaining -= print_max(prefix, packed_color, comp_remaining, !comp.empty(), &line_data); + highlight_spec_t packed_color = + highlight_spec_pager_prefix | highlight_make_background(bg_color); + comp_remaining -= + print_max(prefix, packed_color, comp_remaining, !comp.empty(), &line_data); packed_color = highlight_spec_pager_completion | highlight_make_background(bg_color); - comp_remaining -= print_max(comp, packed_color, comp_remaining, i + 1 < c->comp.size(), &line_data); + comp_remaining -= + print_max(comp, packed_color, comp_remaining, i + 1 < c->comp.size(), &line_data); } size_t desc_remaining = width - comp_width + comp_remaining; if (c->desc_width > 0 && desc_remaining > 4) { - highlight_spec_t desc_color = highlight_spec_pager_description | highlight_make_background(bg_color); - highlight_spec_t punct_color = highlight_spec_pager_completion | highlight_make_background(bg_color); + highlight_spec_t desc_color = + highlight_spec_pager_description | highlight_make_background(bg_color); + highlight_spec_t punct_color = + highlight_spec_pager_completion | highlight_make_background(bg_color); // always have at least two spaces to separate completion and description desc_remaining -= print_max(L" ", punct_color, 2, false, &line_data); @@ -196,10 +201,8 @@ void pager_t::completion_print(size_t cols, const size_t *width_by_column, size_ bool is_selected = (idx == effective_selected_idx); // Print this completion on its own "line". - line_t line = completion_print_item( - prefix, el, row, col, - width_by_column[col], row % 2, - is_selected, rendering); + line_t line = completion_print_item(prefix, el, row, col, width_by_column[col], row % 2, + is_selected, rendering); // If there's more to come, append two spaces. if (col + 1 < cols) { @@ -383,11 +386,15 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co size_t width_by_column[PAGER_MAX_COLS] = {0}; // Skip completions on tiny terminals. - if (this->available_term_width < PAGER_MIN_WIDTH || this->available_term_height < PAGER_MIN_HEIGHT) return true; + if (this->available_term_width < PAGER_MIN_WIDTH || + this->available_term_height < PAGER_MIN_HEIGHT) + return true; // Compute the effective term width and term height, accounting for disclosure. size_t term_width = this->available_term_width; - size_t term_height = this->available_term_height - 1 - (search_field_shown ? 1 : 0); // we always subtract 1 to make room for a comment row + size_t term_height = + this->available_term_height - 1 - + (search_field_shown ? 1 : 0); // we always subtract 1 to make room for a comment row if (!this->fully_disclosed) { term_height = mini(term_height, (size_t)PAGER_UNDISCLOSED_MAX_ROWS); } @@ -481,11 +488,9 @@ bool pager_t::completion_try_print(size_t cols, const wcstring &prefix, const co if (!progress_text.empty()) { line_t &line = rendering->screen_data.add_line(); - highlight_spec_t spec = - highlight_spec_pager_progress | - highlight_make_background(highlight_spec_pager_progress); - print_max(progress_text, spec, term_width, - true /* has_more */, &line); + highlight_spec_t spec = highlight_spec_pager_progress | + highlight_make_background(highlight_spec_pager_progress); + print_max(progress_text, spec, term_width, true /* has_more */, &line); } if (search_field_shown) {