Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ProgressBar scrolls instead of redraws when character is present #497

Open
brooksprumo opened this issue Dec 9, 2022 · 11 comments · May be fixed by #647
Open

ProgressBar scrolls instead of redraws when character is present #497

brooksprumo opened this issue Dec 9, 2022 · 11 comments · May be fixed by #647

Comments

@brooksprumo
Copy link

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
Copy link
Member

djc commented Dec 9, 2022

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
Copy link
Author

brooksprumo commented Dec 9, 2022

git bisect pointed me to the following commit:

5038ba8019bad296b5a0314770db5d05c050a7dd is the first bad commit
commit 5038ba8019bad296b5a0314770db5d05c050a7dd
Author: Dirkjan Ochtman <[email protected]>
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
Copy link
Member

djc commented Dec 9, 2022

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
Copy link
Author

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
Copy link
Member

djc commented Dec 9, 2022

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
Copy link
Author

Would you be able to debug some more?

Yep, can do!

@brooksprumo
Copy link
Author

brooksprumo commented Dec 13, 2022

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:

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?

@djc
Copy link
Member

djc commented Dec 14, 2022

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
Copy link
Author

Yes, this sounds reasonable to me. Thanks!

@lgarron
Copy link

lgarron commented May 25, 2024

I think I might also be running into this with one of my projects. The two messages with an emoji cause the line to overflow by one char.

@djc djc linked a pull request May 25, 2024 that will close this issue
@djc
Copy link
Member

djc commented May 25, 2024

I tried to improve the situation with #647 but I can't quite get it to work (and/or my terminal isn't capable enough?). Happy if someone wants to help out (maybe submit a PR against my PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants