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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to indicatif for the progress bar #2828

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kirawi
Copy link

@kirawi kirawi commented Jul 16, 2021

Resolves #1826 #1835

Output on Alacritty:

info: profile set to 'default'
info: default host triple is x86_64-pc-windows-msvc
info: syncing channel updates for 'stable-x86_64-pc-windows-msvc'
info: latest update on 2021-06-17, rust version 1.53.0 (53cb7b09b 2021-06-17)
info: downloading component 'cargo'
info: downloading component 'clippy'
info: downloading component 'rust-docs'
 16.13MiB / 16.13MiB (100%) 5.85MiB/s in 2s ETA: 0s
info: downloading component 'rust-std'
 22.78MiB / 22.78MiB (100%) 3.75MiB/s in 6s ETA: 0s
info: downloading component 'rustc'
 59.68MiB / 59.68MiB (100%) 4.08MiB/s in 14s ETA: 0s
info: downloading component 'rustfmt'
info: installing component 'cargo'
 3.59MiB / 3.59MiB (100%) 2.29MiB/s in 1s ETA: 0s
info: installing component 'clippy'
info: installing component 'rust-docs'
 16.13MiB / 16.13MiB (100%) 632.90KiB/s in 26s ETA: 0s
info: installing component 'rust-std'
 22.78MiB / 22.78MiB (100%) 1.79MiB/s in 12s ETA: 0s
info: installing component 'rustc'
 59.68MiB / 59.68MiB (100%) 3.20MiB/s in 18s ETA: 0s
info: installing component 'rustfmt'
info: default toolchain set to 'stable-x86_64-pc-windows-msvc'

  stable-x86_64-pc-windows-msvc installed - rustc 1.53.0 (53cb7b09b 2021-06-17)


Rust is installed now. Great!

Alternatively, I can also look into integrating crossterm instead.

26/8/2021: I'll get back to this when I can 馃槄, very busy right now.

@@ -1,11 +1,13 @@
use std::fmt::{self, Display};

#[allow(dead_code)]
Copy link
Author

@kirawi kirawi Jul 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure what to do here since I didn't know if it would be useful in the future, so I decided to ignore the warning. Thoughts?

@kirawi kirawi marked this pull request as ready for review July 17, 2021 01:50
@kirawi kirawi changed the title Switch to indicatif Switch to indicatif for the progress bar Jul 17, 2021
@kirawi
Copy link
Author

kirawi commented Jul 17, 2021

One issue: I can't seem to manually control rendering. There doesn't appear to be a way to make it render only when I call ProgressBar::tick(). Progress bars that elapsed <1 second will be cleared though, so otherwise it only causes some flickering during the process.

Edit: I've figured out a way around it.

@kirawi kirawi marked this pull request as draft July 17, 2021 04:14
@kirawi kirawi marked this pull request as ready for review July 17, 2021 04:17
@kirawi kirawi marked this pull request as draft July 17, 2021 04:21
@kirawi kirawi marked this pull request as ready for review July 17, 2021 04:23
@kirawi
Copy link
Author

kirawi commented Jul 17, 2021

I was confused as to why this problem was happening in the first place, since the logic seems sound at first glance. I semi-replicated it with this minimal example:

use std::{io::Write, time::Duration};

fn main() {
    let mut n = 0;
    for i in 0..=1000 {
        let string = format!("Progress: {}", i);
        print!("\r{}", " ".repeat(n));
        print!("\r");
        print!("{}", string);
        std::io::stdout().flush().unwrap();
        n = string.chars().count();
        std::thread::sleep(Duration::from_millis(50));
    }
}

And it worked correctly. I'll look into it more, hopefully there's a simpler solution than this PR.

@kirawi
Copy link
Author

kirawi commented Jul 17, 2021

Indeed, the problem lies with term.carriage_return(). Replacing it with a simple print!('\r') corrected the behaviour as well. I'll go ahead and switch to crossterm instead.

@kirawi kirawi closed this Jul 17, 2021
@kirawi kirawi reopened this Jul 18, 2021
@kirawi
Copy link
Author

kirawi commented Jul 18, 2021

Actually, I'll reopen this to see which direction you guys want. I could go ahead and adapt the current progress bar to use crossterm as well as refactor the code for DownloadTracker, or continue on with this PR.

@kinnison
Copy link
Contributor

kinnison commented Oct 9, 2021

Hi Kirawi -- thank you for putting the effort into this PR, and I'm really sorry that it has taken me so long to get to it. I have reasons, but they make poor excuses. If you are still interested in working on improving our UX then I'd love to discuss with you the merits of what you came up with here vs. what you think crossterm might offer.

@kirawi
Copy link
Author

kirawi commented Oct 9, 2021

Haha no worries, I myself have gotten a bit busy. I'm interested on continuing working on this, but it may have to wait another month or so for me to squeeze in the time to do it. However, I won't be offended if someone else decides to pick it up.

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 this pull request may close these issues.

Progress indicator broken on Alacritty with ConPTY
2 participants