Don't conflate ticking with updating estimates #434
Conversation
f() will run for a user-determined amount of time, so the 'now' that was passed into the function isn't really valid after the point when f() returns.
src/state.rs
Outdated
| @@ -77,36 +77,37 @@ impl BarState { | |||
|
|
|||
| pub(crate) fn update(&mut self, now: Instant, f: impl FnOnce(&mut ProgressState)) { | |||
| f(&mut self.state); | |||
| self.tick(now); | |||
There was a problem hiding this comment.
This seems like a big change in behavior? Why do this?
There was a problem hiding this comment.
My thinking is that the whole reason the user is calling update is because they want to do something specific with ProgressState that isn't already covered by the existing API. I can't imagine they'd want to do something very custom + tick. Though I agree it is a change in behavior, so I could back out this change if you want.
There was a problem hiding this comment.
It doesn't seem like a big deal to me to tick in this particular case? It's not like ticking is expensive, we currently tick for most other kinds of updates IIRC.
There was a problem hiding this comment.
Sounds good, I'll back that change out after dinner tonight.
There was a problem hiding this comment.
So I was expecting that you'd change everything back to tick, like before, but you only backed this particular change out. I'm still not sure that makes sense?
There was a problem hiding this comment.
Sorry if I wasn't clear. Commit 0440689 is just something else I noticed while I was fixing the main issue, so it's not really related. Try running the yarnish example with and without my commit da9e432. You'll see that without my changes it ticks too fast. This is because even just setting the message/prefix increments the tick, which I doubt is what users are expecting.
There was a problem hiding this comment.
My thinking is that the old code was somewhat lazily calling tick even when in certain cases all it wanted to do was update_estimate_and_draw (e.g for message and prefix). It just so happened that the old condition if self.ticker.is_none() || self.state.tick == 0 mostly made it do the right thing. My changes here are intended to make the difference explicit.
chris-laplante
force-pushed
the
cpl/fix-ticker-inc
branch
from
60b6a05
to
62b0b0f
Compare
|
It would be nice to get a new published version with this fix. When is the next rc planned? |
|
We don't really plan rcs, we can just do one now. |
|
@fabricedesre out of curiosity, what are you using indicatif for? |
I use it in this command line tool: https://github.com/capyloon/nutria/tree/main/builder to display the download progress of prebuilt binaries. That can be slow when the ipfs gateway is not in great shape, so I'd like to use |
|
#435 has the version bump, will publish once that's merged. |
|
Lines 162 to 163 in 496a605 |
Good catch, thanks. I will submit a PR to update the documentation. |
Fixes #433