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.

Sign up for GitHub

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

Jump to bottom

Use indicatif for progress bars #8

Open
wants to merge 2 commits into
base: refactor
Choose a base branch
from
Open

Use indicatif for progress bars #8

wants to merge 2 commits into from

Conversation

fabianfreyer
Copy link

builds on the refactor branch.

indicatif has some nicer, stylable, progress bars:

asciicast

@Arcterus
Copy link
Owner

I'm not 100% sure I want to use indicatif due to this. On the other hand, I wouldn't be surprised if pbr has the same issue, and it only shows up in rare cases (still not sure what the exact trigger is though). The flickering in the example above also bothers me, so it would be cool if you could find a way to get rid of it.

@fabianfreyer
Copy link
Author

I think the flickering is an artifact of asciinema tbh - it looks smooth on my screen (TM). Might be an idea to include both UIs then, and hide them behind feature flags?

@Arcterus
Copy link
Owner

Ah okay, I was wondering if it was due to asciinema. Including them both behind feature flags is fine with me as long as it's clean. BTW this should also be using rustfmt with 3 spaces instead of 4.

@fabianfreyer
rustfmt: run latest nightly
8ed3d42 
also add rustfmt.toml
@fabianfreyer fabianfreyer force-pushed the indicatif branch 2 times, most recently from 6020b41 to 66067ff Compare June 19, 2018 14:26
@fabianfreyer
Copy link
Author

Like that?

@fabianfreyer
ui/multibar: use indicatif
49d927f 
Keep the old pbr UI behind a feature flag.
Arcterus
Arcterus reviewed Jun 20, 2018
View reviewed changes
src/main.rs Outdated
downloader.download::<rget::ui::multibar::MultibarUi, _>(input, matches.value_of("OUTPUT"))

if let Err(f) = downloader
.download::<rget::ui::multibar_indicatif::MultibarUi, _>(input, matches.value_of("OUTPUT"))
Copy link
Owner

Choose a reason for hiding this comment

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

This should pick the progress bar setup based on available features rather than being hard-coded to indicatif.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arcterus Arcterus Arcterus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fabianfreyer @Arcterus