Off by one when wrapping InputStream about progressbar

[mordechaim]

Looking at the code, Util.getInputStreamSize() returns -1 for non-file streams (which I use) and that is used in the string constructor, but passing my own builder and leaving out the max initial it somehow detects it correctly.

While the second constructor has a check for -1, the first will set the initial max regardless. This means that the first will have -1 while the second will have 0.

This all is only about the apparent inconsistency of the different behaviour in the different constructors. This still doesn't explain the off by one.

[mordechaim]

So it is still a bug, right?

Yes, this is a bug. Here you do it correctly.

[ctongfei]

Just to confirm -- you mean that you want to override the default size (as computed by the underlying stream) to a customized size using ProgressBarBuilder?

Why do you want to do this? I assume that the underlying actual file size is the size of the progress bar you want?

[mordechaim]

you want to override the default size (as computed by the underlying stream)

Wrapping the stream without setting the max initial ended up showing question marks (unknown file size) and a messed up progress bar:

So I set the max initial to the file size (correctly shown at the right side of the /) but the total bytes downloaded (at the left of /) is off by one even though the actual file is downloaded in full and my OS shows the file size correctly.

[mordechaim]

Just checked, when using the ProgressBar.wrap(InputStream, String) constructor, it will show question marks, but using ProgressBar.wrap(InputStream, ProgressBarBuilder) even if I don't set the max initial it will show exactly like the first snip, i.e. correct file size but still off by one in progress.

[mordechaim]

OK! I got it. When reading the stream, I read until OEF by:

while (in.read(buffer, 0, buffer.length) > -1) {
    // do something.
}

This causes stepBy to step back by one on EOF.

[ctongfei]

So it is still a bug, right?
In https://github.com/ctongfei/progressbar/blob/master/src/main/java/me/tongfei/progressbar/wrapped/ProgressBarWrappedInputStream.java#L45, if stream.read() == -1, the progress bar should not stepBy -1.

[ctongfei]

Looking at the code, Util.getInputStreamSize() returns -1 for non-file streams (which I use) and that is used in the string constructor, but passing my own builder and leaving out the max initial it somehow detects it correctly.

This surprises me -- if it is not a FileInputStream the max initial should not be detected?

[mordechaim]

This surprises me -- if it is not a FileInputStream the max initial should not be detected?

Ok, so downloading with a small delay reveals why this is happening. Not that it detects the file size, it just downloaded so fast that I couldn't see it.

This line of code shows us that the max increases as the download proceeds but of-course won't backtrack when the stepBy is -1, so it ended up showing the correct file size on the right size of the slash. The progress bar itself always showed 100% only jumping back to 99% on EOF.

Additionally, it was not detected as indefinite when building my own as this only checks for -1, not 0. So this was never executed. But using the string constructor correctly set it to -1 thus triggering the indefinite code.

[ctongfei]

Thanks @mordechaim ! These will be fixed in the next release.

[mordechaim]

I think I know your whole codebase by now 😉, it took me some half-hour or so.

[ctongfei]

A pull request would be welcome!

[mordechaim]

Do you want to handle 0 max as indefinite? Also, should changing the max affect the indefinite state even after start?

[mordechaim]

Do you want to handle 0 max as indefinite?

Or perhaps use -1 as the default value of the initial max in the builder.

[ctongfei]

-1 should be the default max of the initial max. 0 means an empty sequence.