console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.23k stars 240 forks source link

Custom keys on ProgressStyle do not work together with `enable_steady_tick` #493

Closed shssoichiro closed 1 year ago

shssoichiro commented 1 year ago

It seems as though there are the following two interactions in indicatif 0.17.2, which were not present in 0.17.0-rc4:

djc commented 1 year ago

Hmm, I find both of these behaviors quite surprising. Would you be able to run a git bisect to find which commit between -rc.4 and .2 regressed this?

shssoichiro commented 1 year ago

It appears that 516c0dcea6eeb51407d8cf03fdcd96bf6f849220 is the commit that introduced the breakage. I suppose the possibility should also be considered that it may not be related to custom keys, but may be related to the per_sec and eta values. :thinking:

Also to give a clearer example, here are a couple of screenshots.

Expected behavior: Screenshot_20221114_122606

Buggy behavior: Screenshot_20221114_122654

djc commented 1 year ago

Pff, honestly not sure what's going on here. Would you be able to dig in a bit more, since you've already built up some context? Happy to answer further questions about how stuff is supposed to work.

shssoichiro commented 1 year ago

It seems I've narrowed it down further and it's related to ProgressState::per_sec not updating when steady tick is enabled. If I use the code from https://github.com/console-rs/indicatif/issues/394#issuecomment-1309971049 and build a per_sec or eta equivalent using .pos(), .len(), and .elapsed(), it works properly with steady tick.

aawsome commented 1 year ago

I observed the same issue, namely that {bytes_per_sec} always shows 0 when used with enable_steady_tick.

I think the issues is that the Ticker only calls state.draw() (progress_bar.rs, line 644) whereas inc() without a ticker calls state.tick() which calls state.update_estimate_and_draw() (state.rs, line 121).

So, the enabled Ticker never updates the Estimator nor any ProgressTracker.

I'm wondering, is there a reason for the Ticker not to simply also call state.update_estimate_and_draw()?

aawsome commented 1 year ago

495 worked for me and shows {bytes_per_sec} with a steady ticker enabled.

aawsome commented 1 year ago

@shssoichiro Would you mind testing if #495 is also able to solve your issue?

shssoichiro commented 1 year ago

Thanks, it works for me as well. Turns out the issue only affected my custom keys because my custom keys depended on per_sec and eta.

orhun commented 1 year ago

Having the same issue here.