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

Remove MultiProgress::join() based on #231 #284

Merged
merged 3 commits into from May 21, 2021

Conversation

aj-bagwell
Copy link
Contributor

This change moves drawing of MultiProgress from the join() thread to the child ProgressBar updates. fixes #125, #33 and lay groud work for work on #205

It is backwards-incompatible.

It is based on the work of @marienz in #231 more discussion and detail on that pull request.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! In order to make sure I can review this closely, I'm asking for some changes to the commit history presented in this PR. I usually use git's interactive rebase to do that, but if you're unfamiliar with that stuff I can also help split it up more. The goal is to have a few commits that each independently could build and pass the tests.

Also, I think the reference to #116 in the second commit's message is wrong, can you fix that up? It'd be nice if you could shorten the first line of that message to fit within 70-80 chars while you're in there, and a high level description of what problem the commit is trying to fix and the strategy for doing so.

src/progress_bar.rs Show resolved Hide resolved
src/progress_bar.rs Show resolved Hide resolved
src/state.rs Outdated Show resolved Hide resolved
src/state.rs Outdated Show resolved Hide resolved
src/state.rs Outdated Show resolved Hide resolved
src/state.rs Show resolved Hide resolved
src/progress_bar.rs Outdated Show resolved Hide resolved
@aj-bagwell aj-bagwell force-pushed the multiprogress-unjoined branch 2 times, most recently from 6ee4a8a to 120d652 Compare May 17, 2021 16:08
@boc-tothefuture
Copy link

boc-tothefuture commented May 17, 2021

I don't know enough about indicatif to know why it causes the defect, but the current PR does not respect when a progress bar is hidden.

        let mp = MultiProgress::new();
        let hidden_bar = ProgressBar::hidden();
        debug!("Bar Hidden?: {}", hidden_bar.is_hidden());
        let bar_after_add = mp.add(hidden_bar);
        debug!("Hidden after add?: {}", bar_after_add.is_hidden());

Corresponding output:

Bar Hidden?: true
Hidden after add?: false

@mibac138
Copy link
Contributor

@boc-tothefuture are you sure this PR actually changed this behavior? See the docs for set_draw_target which note this case explicitly in the current version.

@boc-tothefuture
Copy link

@boc-tothefuture are you sure this PR actually changed this behavior? See the docs for set_draw_target which note this case explicitly in the current version.

You are correct, the same behavior exists in the currently released version. I did not think I was experiencing that until I switched. My apologies.

@aj-bagwell aj-bagwell force-pushed the multiprogress-unjoined branch 2 times, most recently from a179348 to 47a7427 Compare May 18, 2021 09:22
src/state.rs Outdated Show resolved Hide resolved
src/progress_bar.rs Outdated Show resolved Hide resolved
src/state.rs Outdated Show resolved Hide resolved
@djc
Copy link
Collaborator

djc commented May 20, 2021

Except for the typos, this is looking pretty good! Thanks for working through my comments.

@aj-bagwell aj-bagwell force-pushed the multiprogress-unjoined branch 2 times, most recently from c031f70 to de129f1 Compare May 20, 2021 09:23
@aj-bagwell
Copy link
Contributor Author

Typos fixed also fixed the failing CI

@djc
Copy link
Collaborator

djc commented May 20, 2021

I just pushed another test that calls MultiProgress::join(), would you mind rebasing on main and pushing that once more? I can merge it after that.

@aj-bagwell aj-bagwell force-pushed the multiprogress-unjoined branch 2 times, most recently from 9ef6584 to de129f1 Compare May 20, 2021 10:07
Instead of drawing from a thread blocked on MultiProgress::join(), draw
directly from the threads updating the ProgressBars in the MultiProgress,
using write access to MultiProgress's state to synchronize terminal updates.
The leaky bucket makes sure that progress bars are drawn on average a
the desired rate
but allows it to burst faster if progress is not uniform.

It works by having a "bucket" of redraws that are added to every time a
draw is
requested, when the bucket is full the redraws are skipped, for every
tick a redraw is
removed from the bucket.

This is to fix the regression of console-rs#166 when join was removed.
@aj-bagwell
Copy link
Contributor Author

Rebased on the the latest main (ignore the moment of stupidity where I rebase it on master instead).

Thank for all your help an patience with this.

@djc djc merged commit bc360d1 into console-rs:main May 21, 2021
@aj-bagwell aj-bagwell deleted the multiprogress-unjoined branch May 21, 2021 12:38
@djc djc mentioned this pull request Feb 22, 2022
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.

MultiProgress should support tokio (async)
4 participants