rustup icon indicating copy to clipboard operation
rustup copied to clipboard

Switch to indicatif for the progress bar

Open kirawi opened this issue 4 years ago • 6 comments

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.

kirawi avatar Jul 16 '21 22:07 kirawi

~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 avatar Jul 17 '21 02:07 kirawi

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 avatar Jul 17 '21 21:07 kirawi

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 avatar Jul 17 '21 22:07 kirawi

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.

kirawi avatar Jul 18 '21 00:07 kirawi

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.

kinnison avatar Oct 09 '21 09:10 kinnison

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.

kirawi avatar Oct 09 '21 13:10 kirawi

Closing due to staleness. I may come back to it some other day if it isn't tackled by someone else.

kirawi avatar Jul 11 '24 01:07 kirawi

Closing due to staleness. I may come back to it some other day if it isn't tackled by someone else.

@kirawi Thanks! Just to clarify, this is still on our roadmap, it's just that I've adjusted the roadmap to include the issue if possible, rather than directly pointing to the PR.

rami3l avatar Jul 11 '24 02:07 rami3l