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

MultiProgress: should prune leading progress bars that have become DoneVisible or DoneHidden #426

Closed
chris-laplante opened this issue May 3, 2022 · 1 comment

Comments

@chris-laplante
Copy link
Collaborator

chris-laplante commented May 3, 2022

Consider this small example program:

use std::thread;
use std::time::Duration;

use indicatif::{MultiProgress, ProgressBar, ProgressFinish};

fn main() {
    let m = MultiProgress::new();
    let height = console::Term::stdout().size().0;

    for i in 0..(height + 1) {
        let p1 = m.add(
            ProgressBar::new_spinner()
                .with_message(format!("p{}", i))
                .with_finish(ProgressFinish::AndLeave),
        );
        p1.enable_steady_tick(Duration::from_millis(50));
        thread::sleep(Duration::from_millis(200));
    }
}

It creates one more spinner that there are lines in the terminal. Here is a video of it running:

Screencast.2022-05-03.13.29.03.mp4

Since we never call MultiProgress::remove on ProgressBars that are dropped/finished (see #419), bars are always redrawn even when they don't need to be. When the number of bars (n) becomes greater than terminal lines (m), on each draw MultiProgress will clear the last m lines but write n lines, leaving m - n lines in the scrollback history.

I propose the following algorithm. On each MultiProgress:draw, iterate over the bars in order (according to MultiState::ordering) and take_while(|bar| bar.is_finished()). This set of leading progress bars is then pruned by calling MultiProgress:remove on each one. This ensures we are not re-drawing bars that don't need to be redrawn and that we are some reclaiming memory.

The wrinkle is that MultiState cannot currently get at the ProgressState to check is_finished(), so we will need to restructure some code.

@djc
Copy link
Collaborator

djc commented May 10, 2022

This sounds reasonable. I don't have a deep understanding of MultiProgress paged in right now and the video doesn't want to play for me ("Video can't be played because the file is corrupt" -- Firefox on macOS), but I'd be happy to review a PR for this.

chris-laplante added a commit to chris-laplante/indicatif that referenced this issue May 26, 2022
This extra layer of abstraction will be necessary to fix console-rs#426
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue May 26, 2022
This extra layer of abstraction will be necessary to fix console-rs#426
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue May 27, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue May 27, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue May 27, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue May 27, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue May 27, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue May 27, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue Jun 10, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue Jun 10, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue Jun 13, 2022
chris-laplante added a commit to chris-laplante/indicatif that referenced this issue Jun 13, 2022
@djc djc closed this as completed in 0f33289 Jun 13, 2022
djc pushed a commit that referenced this issue Jun 13, 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

No branches or pull requests

2 participants