ProgressBar scrolls instead of redraws when ◎ character is present

[brooksprumo]

I am experiencing my ProgressBar scrolling instead of truncating/redrawing when I shrink my terminal's window.

I am running on a remote Ubuntu server. Terminal is Mac's iTerm2, with and without tmux. The ProgressBar's template uses {wide_msg}, so I expect the progress bar to truncate text as the window sizes decreases.

This only manifests when I shrink my past the ◎ character. If this character is not present in the message, the progress bar works as expected regardless of how small I shrink my terminal's window.

Also, version v0.16.2 does not exhibit this behavior; it works as expected.

I've tried v0.17.1 and v0.17.2. With v0.17.2 I also tried using the "improved_unicode" feature. None of these resulted in correct behavior.

Here's more information in an issue I have against my repo where I debugged this:
solana-labs/solana#29158

If it's helpful, I can get a screen capture and/or a set of steps to reproduce the issue (may be slightly involved though). Thanks in advance!

[djc]

Sorry for regressing this!

Since you already have a working version and a failing version, would you be able to git bisect this? Pinpointing which change broke this would probably make it a bunch easier to diagnose.

[brooksprumo]

git bisect pointed me to the following commit:

5038ba8019bad296b5a0314770db5d05c050a7dd is the first bad commit
commit 5038ba8019bad296b5a0314770db5d05c050a7dd
Author: Dirkjan Ochtman <dirkjan@ochtman.nl>
Date:   Wed Mar 16 21:02:17 2022 -0700

    Account for alignment when truncating

 src/style.rs | 48 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 4 deletions(-)

Edit: Adding links to make it easier to jump around:
commit: 5038ba8
associated pull request: #402

[djc]

Hmm, are you specifically setting alignment in your templates? It seems like we don't correctly measure the text width in the face of non-ASCII characters when we try to align.

[brooksprumo]

Here's the code for our ProgressBar:

    let progress_bar = indicatif::ProgressBar::new(42);
    progress_bar.set_draw_target(ProgressDrawTarget::stdout());
    progress_bar.set_style(
        ProgressStyle::default_spinner()
            .template("{spinner:.green} {wide_msg}")
            .expect("ProgresStyle::template direct input to be correct"),
    );
    progress_bar.enable_steady_tick(Duration::from_millis(100));

And .set_message() is here:

                        progress_bar.set_message(format!(
                            "{}{}| \
                                    Processed Slot: {} | Confirmed Slot: {} | Finalized Slot: {} | \
                                    Full Snapshot Slot: {} | Incremental Snapshot Slot: {} | \
                                    Transactions: {} | {}",

And the whole output looks like this:

⠒ 00:00:10 | Processed Slot: 2467 | Confirmed Slot: 2467 | Finalized Slot: 2434 | Full Snapshot Slot: 2408 | Incremental Snapshot Slot: 2428 | Transactions: 2446 | ◎499.993885000

[djc]

So that doesn't look like any alignment is involved. Would you be able to debug some more? Now that we know the regressing commit it shouldn't be too hard to put some debug stuff in there to figure out what's up.

[brooksprumo]

Would you be able to debug some more?

Yep, can do!

[brooksprumo]

This will exhibit the issue:

diff --git a/src/style.rs b/src/style.rs
index 98b9c05..6e3b202 100644
--- a/src/style.rs
+++ b/src/style.rs
@@ -894,13 +894,13 @@ mod tests {
         let mut buf = Vec::new();

         let style = ProgressStyle::with_template("{wide_msg}").unwrap();
-        state.message = TabExpandedString::NoTabs("abcdefghijklmnopqrst".into());
+        state.message = TabExpandedString::NoTabs("abcdefghij◎klmnopqrst".into());
         style.format_state(&state, &mut buf, WIDTH);
         assert_eq!(&buf[0], "abcdefghij");

         buf.clear();
         let style = ProgressStyle::with_template("{wide_msg:>}").unwrap();
-        state.message = TabExpandedString::NoTabs("abcdefghijklmnopqrst".into());
+        state.message = TabExpandedString::NoTabs("abcdefghij◎klmnopqrst".into());
         style.format_state(&state, &mut buf, WIDTH);
         assert_eq!(&buf[0], "klmnopqrst");

Looks like calling get() on the str is returning None:

indicatif/src/style.rs

Line 701 in d54fb0e

return f.write_str(self.str.get(start..end).unwrap_or(self.str));

The docs for get() mention that it will return None if the indices are not on a UTF-8 sequence boundary.

If you call is_char_boundary() on the self.str's start and end indices, then those will return false, thus get() returning None, and no truncation occurring.

So the unicode width calculation by str_width() is likely incorrect (or used here in an incorrect context).
https://github.com/console-rs/console/blob/8c6b487574274b9666da4ec9e6da6a5f912d9330/src/utils.rs#L687-L697

It looks like str::get() needs its indices in bytes (not characters), based on looking at the code for str::is_utf8_char_boundary().

By using str.len() to get the bytes of the message, this correctly truncates a message with a single unicode character, but the character counting needed when truncating and aligning in the center will be incorrect (resulting string will be shorter than expected).

Maybe these crates can be of use?

• https://crates.io/crates/unicode-segmentation
• https://crates.io/crates/unicode-width

[djc]

Right, so the code that trims the output if it's about to overflow the terminal width is wrong. So if unicode-width is enabled, the outputs from the match self.align { .. } should be a bit more complicated, for example, for Alignment::Left we could do something like this:

let end = self.str.len() - excess;
if !self.str.is_char_boundary(end) {
    // copy the code for the nightly-only `ceil_char_boundary()`
}
let mut diff = (self.str[..end].width() as i16) - self.width as i16;
while diff != 0 {
    match diff > 0 {
        // trim an additional character off the end of `self.str`
        true => ..
        // increase `end` by one character's worth
        false => ..
    }
}

Does that make sense?

Should also add a test to make sure we don't regress this.

[brooksprumo]

Yes, this sounds reasonable to me. Thanks!