Add parameter to specify elapsed time precision

cheggaaa / pb

Console progress bar for Golang

BSD 3-Clause "New" or "Revised" License

3.64k stars 269 forks source link

Add parameter to specify elapsed time precision #181

Closed anishathalye closed 3 years ago

anishathalye commented 3 years ago

db60ccefe5cb616c2f8a1bde35ee059118dce62a (released in v3.0.7) increased the resolution shown on elapsed time from seconds to milliseconds. The behavior prior to v3.0.7 was to truncate the time to the nearest second. This patch allows the user to control the precision. The default is to round to the nearest second, like the pre-v3.0.7 behavior. This patch also switches from truncation to rounding.

This is an alternative to #180, as suggested in https://github.com/cheggaaa/pb/pull/180#issuecomment-813383854.

cheggaaa commented 3 years ago

Do we keep logic from #180? I think we can add the default is 1 sec to this parameter and it will be ok.

cheggaaa commented 3 years ago

@kevin-hanselman is it ok for you? to set desired resolution manually and with default as 1 sec

anishathalye commented 3 years ago

This patch as it is currently does not keep the logic from #180. If we did want that, would we want another tunable knob (at what threshold to start rounding)? That seems complex to me, to have two parameters to control rounding behavior.

A third option, similar to #180 but still different from #180 and #181 would be to have a single TimeRoundThreshold parameter, where if the elapsed time is less than the threshold, it's shown in milliseconds, and if it's greater, then it's shown in seconds. This would avoid the issue of the library having to decide what the threshold is (1 second? 10 seconds?)

anishathalye commented 3 years ago

An unrelated issue that I personally find annoying is the milliseconds formatting behavior from %s and a time.Duration. It is shown with the minimum number of decimal places, so 9.500 is displayed as 9.5s, 9.510 is displayed as 9.51s, and 9.512 is displayed as 9.512s. This makes the s rapidly shift around between columns as the progress bar is updated. (This is actually why I noticed this behavior and proposed #180, because I wanted to get back the pre-3.0.7 behavior of only showing seconds.)

See an example here:

https://user-images.githubusercontent.com/3526486/113727830-7fb09480-96c3-11eb-9ec4-54607c7ce9a4.mp4

cheggaaa commented 3 years ago

What we haven't thought about that rounding less than 0.1 seconds does not make sense because the bar refresh rate is 200 ms (by default)

cheggaaa commented 3 years ago

My proposal

Hardcode the default rounding to 100 ms up to 1 sec (or 10s) and 1s for above. It's like what did in #180. This will default behavior when TimeRound is empty
When TimeRound is non-empty we just round to TimeRound all time

kevin-hanselman commented 3 years ago

I like @cheggaaa's proposal above (the 10 second version, preferably). It uses a sensible default that compromises on precision for UX, and it allows for the user to override the behavior.

anishathalye commented 3 years ago

I like that idea, done in 49cf38c23beb46541c3e94031c8f8fadafb56651.

I also improved the printing behavior I didn't like (the 1s vs 1.1s shown in the video above). Here's what it looks like now:

https://user-images.githubusercontent.com/3526486/113958034-5e989280-97ee-11eb-9583-3725ad719972.mp4

I didn't want to re-implement the logic in Duration.String(), so this seemed like a reasonable way of doing it.

(apologies for any extra emails associated with a bunch of force-pushes; I kept fining more UI corner cases)

cheggaaa commented 3 years ago

Thank you, guys! I also fixed the TestElapsedTime version v3.0.8 released