Skip to content

Commit

Permalink
feat: use textwrap crate for wrapping help texts
Browse files Browse the repository at this point in the history
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:
        <mode>    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:
        <mode>    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.
  • Loading branch information
mgeisler authored and kbknapp committed May 29, 2017
1 parent 1365304 commit b93870c
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 41 deletions.
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ bitflags = "0.8.0"
vec_map = "0.8"
unicode-width = "0.1.4"
unicode-segmentation = "~1.1.0" # 1.2.0 requires Rust 1.13.0
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 }
Expand Down
46 changes: 7 additions & 39 deletions src/app/help.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down Expand Up @@ -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::<Vec<String>>()
.join("\n");
}
}

Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions tests/help.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ FLAGS:
ARGS:
<mode> 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.";

Expand Down

0 comments on commit b93870c

Please sign in to comment.