-
Notifications
You must be signed in to change notification settings - Fork 965
Refactor the DownloadTracker in favor of indicatif
#4426
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
base: master
Are you sure you want to change the base?
Refactor the DownloadTracker in favor of indicatif
#4426
Conversation
e172502
to
506536c
Compare
7ee064c
to
6eb8e1a
Compare
@FranciscoTGouveia I am thinking, the percentage has been replaced by the bar, but the speed meter might still be worth it (and I don't think the timer is that important)?
Also, I actually prefer the legacy progress bar style, something closer to
@djc @ChrisDenton What would you like (and don't like) to see in this interface that you might see every now and then? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit to not having any strong opinions here but the current style looks fine to me. The ETA can be useful on slower connections so it's clear if you should go and do something else for 20 minutes. Also when the CDN is having issues. Admittedly this can be discerned by looking at the download rate if you're thinking about it. |
cbbebb0
to
b8dd68e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the work, your patch works pretty well on my machine!
LGTM modulo some minor suggestions :)
Can you explain why? |
if self.progress_bar.is_hidden() && self.progress_bar.elapsed() >= Duration::from_secs(1) { | ||
self.multi_progress_bars.add(self.progress_bar.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djc This is to be coherent with the old rustup logic where if a progress is considered to have finished instantly, the corresponding progress bar will remain invisible all along.
Also, the .clone()
action is cheap because ProgressBar
has referential semantics.
Being a universal tool, we would expect rustup UI to work correctly even if our host doesn't come with proper support for Unicode (this doesn't mean non-ASCII chars are not displayed; but it could mean that they can be shown in a surprising way to be no longer suitable for UI use). Cargo, for example, has to detect this in case it needs to provide a fallback (thanks, @weihanglo!). The same is true for Python's As for the actual character set being used, |
483459c
to
1a5f8ca
Compare
Co-authored-by: 0xPoe <[email protected]>
1a5f8ca
to
63e867b
Compare
Closes #1835.
This PR refactors the current manual
DownloadTracker
progress reporting, replacing it with anindicatif
-based implementation, as previously discussed in #1835 and #3828.This change is a first step toward enabling concurrent component downloads, as the
MultiProgress
construct ofindicatif
will very helpful to concurrently report progress.In terms of performance, benchmarks using
hyperfine
showed no noticeable difference.Benchmarks were run 20 times (with 2 warmup runs) over a 50 Mbps connection, each downloading an entire toolchain.
Thanks to @0xPoe and his previous PR, which served as the basis for this change.
Prior art: #2610, #2828, #3349.