quayctl icon indicating copy to clipboard operation
quayctl copied to clipboard

ProgressBar interface

Open jzelinskie opened this issue 9 years ago • 14 comments
trafficstars

In order to encapsulate the logic around ProgressBars, we should have a ProgressBar interface.

For example, in the main package we could detect the interactivity of the shell once and use a no-op ProgressBar implementation to avoid that logic bleeding throughout the program. A real implementation of this interface would also cover the default behaviors like setting the MaxWidth and what completed lines look like, etc...

jzelinskie avatar Apr 11 '16 19:04 jzelinskie

@jzelinskie can't we just use the progress bar from rkt?

cc @jonboulle

philips avatar Apr 20 '16 04:04 philips

@philips does that handle drawing multiple bars on the screen at once? I didn't think it did.

jzelinskie avatar Apr 20 '16 16:04 jzelinskie

@dgonyeo is working on that right now, looping him in

On Wed, Apr 20, 2016 at 6:22 PM, Jimmy Zelinskie [email protected] wrote:

@philips https://github.com/philips does that handle drawing multiple bars on the screen at once? I didn't think it did.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/coreos/quayctl/issues/47#issuecomment-212499669

jonboulle avatar Apr 20 '16 16:04 jonboulle

That's cool, either way I think the code can be much cleaner if we use an interface to minimize the ProgressBar logic that leaks into other functions.

jzelinskie avatar Apr 20 '16 16:04 jzelinskie

My redesign right now is actually aiming to do that, since (as I'm sure you've noticed) using ioprogress in its current state requires the user to write a good amount of logic.

The current API (feedback would be appreciated!):

type MultilineProgress struct {
    // DisplayWidth can be set to influence how large the progress bars are.
    // The bars will be scaled to attempt to produce lines of this number of
    // characters, but characters of different lengths may still be printed.
    // When this is set to 0 (aka unset), 80 character columns are assumed.
    DisplayWidth int

    // Has unexported fields.
}
    MultilineProgress will manage the copying of io.Readers to io.Writers while
    printing multiline progress bars about the progress of each copy.


func (mp *MultilineProgress) DisplayAndWait(displayTo io.Writer, drawInterval time.Duration) error
    DisplayAndWait will wait until all registered readers have finished copying
    (reached EOF), while drawing a progress bar for each reader to displayTo
    that updates every drawInterval. If displayTo is a terminal, draw codes will
    be used to draw over each previous progress bar. If it is not, the current
    copied amount and total will be printed for each reader.

func (mp *MultilineProgress) RegisterReader(reader io.Reader, name string, size int64, dest io.Writer)
    RegisterReader registeres an io.Reader with this MultilineProgress, and
    copies all data from reader into dest until EOF. name and size are used to
    display a progress bar. When size is zero, the size is assumed to be
    unknown.

Example usage would be something like this:

mp := &ioprogress.MultilineProgress{}
mp.RegisterReader(res1.Body, "ACI", num1, file1)
mp.RegisterReader(res2.Body, "Signature", num2, file2)
mp.DisplayAndWait(os.Stderr, time.Second)

cgonyeo avatar Apr 20 '16 16:04 cgonyeo

Our usage actually isn't based around Readers; has that been taken into consideration as well?

jzelinskie avatar Apr 20 '16 17:04 jzelinskie

Not at all, right now it's geared for just docker2acis use case. Would you just want to manually update the progress bars?

On Apr 20, 2016, at 13:06, Jimmy Zelinskie [email protected] wrote:

Our usage actually isn't based around Readers: has that been taken into consideration as well?

― You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

cgonyeo avatar Apr 20 '16 17:04 cgonyeo

Yeah, we manually update our progress bars: https://github.com/coreos/quayctl/blob/a75bf650cd08f91c036b92c92e6060c6d8be96e2/engine/torrent.go#L75-L160

jzelinskie avatar Apr 20 '16 17:04 jzelinskie

I can probably split out the progress bar logic and have the io.Reader copy logic just use that. I'll keep you posted.

cgonyeo avatar Apr 20 '16 17:04 cgonyeo

@jzelinskie Would this work for you giuys? https://github.com/coreos/ioprogress/pull/7

cgonyeo avatar Apr 20 '16 20:04 cgonyeo

@dgonyeo I think all we need to be able to specify prefixes and suffixes for each progress bar, set the total and the current state, and the frequency of redraws.

jzelinskie avatar Apr 20 '16 21:04 jzelinskie

The ProgressBarPrinter struct lets you specify prefixes and suffixes (and update them) for each progress bar and set the current state (from 0 to 1). For the frequency of redraws, you'll need to write your own logic for that. This should be an example of how to do that: https://github.com/dgonyeo/ioprogress/blob/master/iocopy.go

If a wrapper around ProgressBarPrinter that has the concept of current/total values and that has the logic for continually redrawing would be useful, it shouldn't be hard to implement.

cgonyeo avatar Apr 20 '16 21:04 cgonyeo

That wrapper would be perfect for our use case.

jzelinskie avatar Apr 20 '16 21:04 jzelinskie

I'll implement that in a followup PR.

On Apr 20, 2016, at 17:54, Jimmy Zelinskie [email protected] wrote:

That wrapper would be perfect for our use case.

― You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

cgonyeo avatar Apr 20 '16 22:04 cgonyeo