RFC: Redesign how progress bars are added to MultiProgress

[chris-laplante]

I believe the current MultiProgress API is error-prone and too easy to "hold wrong", and would like to suggest a small-ish redesign.

MultiProgress exists primarily to coordinate draws of multiple ProgressBars.

With the current API, you create a ProgressBar and add it to the MultiProgress. These are two discrete steps:

let mp = MultiProgress::new();

// possibly other pbs added here too...

let pb = ProgressBar::new(100);
// <--- (1)  danger!
mp.add(pb);

The problem is that it is very easy to accidentally cause the ProgressBar to be drawn before it is added to the MultiProgress, i.e. at the spot marked (1). The most common offender is enable_steady_tick (for spinners). But lots of other methods (e.g. set_message) can cause a draw too.

If the MultiProgress has been drawn by the time we get to (1), then it is already game over: pb has likely corrupted the screen.

Off the top of my head, the most recent occurrences I've seen are listed below (I could have sworn there are more, but I can't find them):

• Second ProgressBar can't be cleared if set_position before added to MultiProgress #502
• Adding/removing progress bar spinners leaves newlines #645

The safe "fix" is to ensure ProgressBars are only mutated/customized after being added to MultiProgress. For example:

let mp = MultiProgress::new();

// possibly other pbs added here too...

let pb = mp.add(ProgressBar::new(100));
// no problem touching `pb` here

However, I think we can do better by ensuring the ProgressBars destined for a MultiProgress are never prematurely touched in the first place. We could replace MultiProgress::add with add_pb/add_progress and add_spinner methods to MultiProgress. (Similarly, insert_spinner_before, insert_pb_after, etc.)

let mp = MultiProgress::new();

// possibly other pbs added here too...

let pb: ProgressBar = mp.add_pb(100);

// no possibility of touching pb before we should

Obviously, this breaks backwards compatibility. But I think it will save users from a lot of troubleshooting and annoyance in the long term.

Another option would be to create some kind of proxy with a ProgressBar-like API, but which defers customizations/draws until added to a MultiProgress. Then change MultiProgress to only take those instead of ProgressBar. Example:

// other name options: DeferredProgressBar, ProgressBarBuilder, ???
let pbp: ProgressBarProxy = ProgressBarProxy::new_spinner();
pbp.enable_steady_tick(Duration::from_secs(5));   // <--- this call doesn't start the tick yet, just notes it down for later

let pb: ProgressBar = mp.add(pbp);  // <--- all customizations applied after internalizing `ProgressBar`

This might be a little nicer for users, at the cost of more implementation effort.

[chris-laplante]

@djc Interested in any thoughts you have on this, when you have time :).

[chris-laplante]

BTW in the past I have tried to explain my concerns about MultiProgress with a hand-wavy "MultiProgress can't know if the ProgressBar has already been drawn", ala #502 (comment). This RFC is my attempt to give more concrete reasons.

[djc]

Would be great to make this harder to break, thanks for writing this up!

I think we'd want to avoid combinatorial API explosion which add_progress() / add_spinner() / insert_spinner_before() / .. might trigger? If we're building a bar we should keep the API similar to bar creation.

I also think making things nicer for users at the cost of a little more code to maintain is probably worth it. I don't like the ProgressBarProxy name much, MultiBar seems like a decent concise way of expressing what it's for? The challenge will be avoiding duplicate stuff as much as possible.

I also think we should try to first expose this in a semver-compatible update, guiding users to the new API by way of #[deprecated] so we can see how things shake out, and then kill old/duplicate API in a future 0.18 release.

[chris-laplante]

Would be great to make this harder to break, thanks for writing this up!

I think we'd want to avoid combinatorial API explosion which add_progress() / add_spinner() / insert_spinner_before() / .. might trigger? If we're building a bar we should keep the API similar to bar creation.

I also think making things nicer for users at the cost of a little more code to maintain is probably worth it. I don't like the ProgressBarProxy name much, MultiBar seems like a decent concise way of expressing what it's for? The challenge will be avoiding duplicate stuff as much as possible.

I also think we should try to first expose this in a semver-compatible update, guiding users to the new API by way of #[deprecated] so we can see how things shake out, and then kill old/duplicate API in a future 0.18 release.

Sounds like a plan, and I like the name MultiBar.

Definitely agreed regarding the combinatorial explosion. As a first attempt, I think I can write some kind of macro to slap on ProgressBar to generate MultiBar automatically. I'll try to take a stab at it over the next few weeks during US holiday break.

[djc]

I don't love macros for this kind of thing but I guess it might be the best way to deduplicate.

[chris-laplante]

I don't love macros for this kind of thing but I guess it might be the best way to deduplicate.

I'll try with and without. I also don't love macros since they tend to break code completion in my IDE (RustRover).