ctongfei / progressbar

Terminal-based progress bar for Java / JVM
http://tongfei.me/progressbar/
MIT License
1.07k stars 102 forks source link

Allow ProgressBarBuilder.setETAFunction to be used externally #146

Closed deejgregor closed 1 year ago

deejgregor commented 1 year ago

ProgressState is package-private and setETAFunction's BiFunction includes ProgressState, preventing setETAFunction from being used outside of the package.

Reproducer:

import java.util.Optional;

import me.tongfei.progressbar.ProgressBarBuilder;

public class SetETAReproducer {
    public static void main(String[] argv) {
        ProgressBarBuilder pbb = new ProgressBarBuilder();
        pbb.setETAFunction((a, b) -> Optional.empty());
    }
}

Run this on the latest released version 0.9.4:

$ javac -cp $HOME/.m2/repository/me/tongfei/progressbar/0.9.4/progressbar-0.9.4.jar SetETAReproducer.java
error: ProgressState is not public in me.tongfei.progressbar; cannot be accessed from outside package
SetETAReproducer.java:8: error: ProgressState is not public in me.tongfei.progressbar; cannot be accessed from outside package
        pbb.setETAFunction((a, b) -> Optional.empty());
                           ^
2 errors
deejgregor commented 1 year ago

I noticed that ProgressBarRenderer#render also has ProgressState in its signature: https://github.com/ctongfei/progressbar/blob/main/src/main/java/me/tongfei/progressbar/ProgressBarRenderer.java#L17

But there isn't currently a way to set an alternate ProgressBarRenderer on the ProgressBarBuilder--only on two of the constructors on ProgressBar (both deprecated).

ctongfei commented 1 year ago

Thanks! ProgressState will have to be public in the next version. To guarantee safety, the ETA function will receive a cloned copy of the original ProgressState one has to use getters to access states in a ProgressState.

I think a better option is to change the type of the eta parameter to BiFunction<ProgressBar, Duration, Optional<Duration>>. In this way we can keep ProgressState package-private.

@deejgregor : what is the use case where you want to specify an alternate ProgressBarRenderer?

deejgregor commented 1 year ago

I think a better option is to change the type of the eta parameter to BiFunction<ProgressBar, Duration, Optional<Duration>>. In this way we can keep ProgressState package-private.

+1. That sounds good. I see that most of the important fields in ProgressState are exposed via ProgressBar.

FWIW, the only things that I see which aren't currently exposed via ProgressBar are:

deejgregor commented 1 year ago

@deejgregor : what is the use case where you want to specify an alternate ProgressBarRenderer?

I was going to try to do a few things to trim space, most notably something like this in place of Util.formatDuration() because for my application (daemon startup/shutdown progress), I pretty much just care about minutes and seconds:

String.format("%d:%02d", s / 60, s % 60)

I realize now, a simpler way to do that would be to allow specifying a custom duration formatter. Or even extend the existing formatter to exclude if s < 3600 (although that would change the format for existing users).

Here is what I'm working on BTW: https://github.com/OpenNMS/opennms/pull/5322