From 247809090f69e10b8bd7721cbde90f1f2ba8baff Mon Sep 17 00:00:00 2001 From: Martin Geisler Date: Sat, 28 Jan 2017 12:42:09 +0100 Subject: [PATCH] feature: use textwrap crate for wrapping help texts The textwrap crate uses a simpler linear-time algorithm for wrapping the text. The current algorithm in wrap_help uses several O(n) calls to String::insert and String::remove, which makes it potentially quadratic in complexity. Comparing the 05_ripgrep benchmark at commits textwrap~2 and textwrap gives this result on my machine: name before ns/iter after ns/iter diff ns/iter diff % build_app_long 22,101 21,099 -1,002 -4.53% build_app_short 22,138 21,205 -933 -4.21% build_help_long 514,265 284,467 -229,798 -44.68% build_help_short 85,720 85,693 -27 -0.03% parse_clean 23,471 22,859 -612 -2.61% parse_complex 29,535 28,919 -616 -2.09% parse_lots 422,815 414,577 -8,238 -1.95% As part of this commit, the wrapping_newline_chars test was updated. The old algorithm had a subtle bug where it would break lines too early. That is, it wrapped the text like ARGS: x, max, maximum 20 characters, contains symbols. l, long Copy-friendly, 14 characters, contains symbols. m, med, medium Copy-friendly, 8 characters, contains symbols."; when it should really have wrapped it like ARGS: x, max, maximum 20 characters, contains symbols. l, long Copy-friendly, 14 characters, contains symbols. m, med, medium Copy-friendly, 8 characters, contains symbols."; Notice how the word "14" was incorrectly moved to the next line. There is clearly room for the word on the line with the "l, long" option since there is room for "contains" just above it. I'm not sure why this is, but the algorithm in textwrap handles this case correctly. --- Cargo.toml | 1 + src/app/help.rs | 46 +++++++--------------------------------------- src/lib.rs | 1 + tests/help.rs | 4 ++-- 4 files changed, 11 insertions(+), 41 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5590a870f96..6b9d5f71f9b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,7 @@ bitflags = "0.8.0" vec_map = "0.8" unicode-width = "0.1.4" unicode-segmentation = "1.0.1" +textwrap = "0.6.0" strsim = { version = "0.6.0", optional = true } ansi_term = { version = "0.9.0", optional = true } term_size = { version = "0.3.0", optional = true } diff --git a/src/app/help.rs b/src/app/help.rs index f8ca33486cf..b2f749f547d 100644 --- a/src/app/help.rs +++ b/src/app/help.rs @@ -17,7 +17,7 @@ use app::usage; use unicode_width::UnicodeWidthStr; #[cfg(feature = "wrap_help")] use term_size; -use unicode_segmentation::UnicodeSegmentation; +use textwrap; use vec_map::VecMap; #[cfg(not(feature = "wrap_help"))] @@ -955,45 +955,13 @@ impl<'a> Help<'a> { } fn wrap_help(help: &mut String, longest_w: usize, avail_chars: usize) { - debugln!("Help::wrap_help: longest_w={}, avail_chars={}", - longest_w, - avail_chars); - debug!("Help::wrap_help: Enough space to wrap..."); + // Keep previous behavior of not wrapping at all if one of the + // words would overflow the line. if longest_w < avail_chars { - sdebugln!("Yes"); - let mut prev_space = 0; - let mut j = 0; - for (idx, g) in (&*help.clone()).grapheme_indices(true) { - debugln!("Help::wrap_help:iter: idx={}, g={}", idx, g); - if g == "\n" { - debugln!("Help::wrap_help:iter: Newline found..."); - debugln!("Help::wrap_help:iter: Still space...{:?}", - str_width(&help[j..idx]) < avail_chars); - if str_width(&help[j..idx]) < avail_chars { - j = idx; - continue; - } - } else if g != " " { - if idx != help.len() - 1 || str_width(&help[j..idx]) < avail_chars { - continue; - } - debugln!("Help::wrap_help:iter: Reached the end of the line and we're over..."); - } else if str_width(&help[j..idx]) <= avail_chars { - debugln!("Help::wrap_help:iter: Space found with room..."); - prev_space = idx; - continue; - } - debugln!("Help::wrap_help:iter: Adding Newline..."); - j = prev_space; - debugln!("Help::wrap_help:iter: prev_space={}, j={}", prev_space, j); - debugln!("Help::wrap_help:iter: Removing...{}", j); - debugln!("Help::wrap_help:iter: Char at {}: {:?}", j, &help[j..j + 1]); - help.remove(j); - help.insert(j, '\n'); - prev_space = idx; - } - } else { - sdebugln!("No"); + *help = help.lines() + .map(|line| textwrap::fill(line, avail_chars)) + .collect::>() + .join("\n"); } } diff --git a/src/lib.rs b/src/lib.rs index 7c3310ca1e7..2c212bf55b2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -541,6 +541,7 @@ extern crate bitflags; extern crate vec_map; #[cfg(feature = "wrap_help")] extern crate term_size; +extern crate textwrap; extern crate unicode_segmentation; #[cfg(feature = "color")] extern crate atty; diff --git a/tests/help.rs b/tests/help.rs index ff51311403e..b00ee048ac0 100644 --- a/tests/help.rs +++ b/tests/help.rs @@ -253,8 +253,8 @@ FLAGS: ARGS: x, max, maximum 20 characters, contains symbols. - l, long Copy-friendly, - 14 characters, contains symbols. + l, long Copy-friendly, 14 + characters, contains symbols. m, med, medium Copy-friendly, 8 characters, contains symbols.";