quay / quayctl

quayctl is a command-line client for Quay
Apache License 2.0
91 stars 17 forks source link

ProgressBar interface #47

Open jzelinskie opened 8 years ago

jzelinskie commented 8 years ago

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...

philips commented 8 years ago

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

cc @jonboulle

jzelinskie commented 8 years ago

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

jonboulle commented 8 years ago

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

On Wed, Apr 20, 2016 at 6:22 PM, Jimmy Zelinskie notifications@github.com 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

jzelinskie commented 8 years ago

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.

cgonyeo commented 8 years ago

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)
jzelinskie commented 8 years ago

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

cgonyeo commented 8 years ago

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 notifications@github.com 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

jzelinskie commented 8 years ago

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

cgonyeo commented 8 years ago

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 commented 8 years ago

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

jzelinskie commented 8 years ago

@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.

cgonyeo commented 8 years ago

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.

jzelinskie commented 8 years ago

That wrapper would be perfect for our use case.

cgonyeo commented 8 years ago

I'll implement that in a followup PR.

On Apr 20, 2016, at 17:54, Jimmy Zelinskie notifications@github.com 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