console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.22k stars 238 forks source link

Document when `with_key` callbacks get invoked, and in which order #578

Open joshtriplett opened 10 months ago

joshtriplett commented 10 months ago

With some types of callbacks, it may matter what order the callbacks get called in, and when in the formatting of the progress bar they're called. For instance, if a with_key callback calls ProgressBar::set_position, a subsequent reference to eta should take that update into account.

Would appreciate having this documented as something callers can rely on.

joshtriplett commented 10 months ago

The use case for this is a piece of code that keeps stats in its own Arc of an Atomic, and wanting to use that as the position of the progress bar. We didn't want to have to maintain a thread polling the atomic, so instead, we let steady-tick do that for us. That alone would be easy enough with with_key, but we want the ETA calculation, which means updating the position. And it appears to work to call set_position from within the with_key callback.

What approach would you suggest for this use case? Assume the code that keeps stats doesn't have any indicatif-specific interface.

djc commented 10 months ago

So here's what I think might make sense and wouldn't add too much complexity even though it's a bit involved:

pub trait ProgressInput {
    fn tick(&mut self, state: &mut ProgressState);
}

Then we'd add a method ProgressBar::enable_steady_tick_with_input() (mainly so this doesn't need a semver-breaking change) which, in addition to interval: Duration, also takes an input: Box<dyn ProgressInput>.

The input would stored in TickerControl and be invoked on &mut state.state before calling state.tick().

(Maybe not the best names, feel free to come up with better ones.)

How does that sound?

@chris-laplante any thoughts?

Byron commented 10 months ago

This sounds like ProgressInput would be able keep a reference or ownership over any kind of counter which then somehow makes it into the pos field? If so, that should definitely work.

djc commented 10 months ago

Would be great if someone working on the original issue (some Cargo/gitoxide thing?) could take a stab at implementing my proposal to see if it addresses the use case. Should be straightforward from my detailed comment before.

Byron commented 10 months ago

Thanks for suggesting the fix and I hope it will be implemented eventually to not be lost in this PR. For us it seems less time consuming to use the current API as intended and make the set_position() calls where the counting happens, instead of in with_key().

With that said, I think this PR can be closed as it had to be rejected. Thanks everyone.