Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ProgressBarBuilder.setETAFunction to be used externally #146

Merged

Conversation

deejgregor
Copy link
Contributor

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

ProgressState was package-private and setETAFunction's BiFunction
includes ProgressState, preventing setETAFunction from being used
outside of the package.
@deejgregor
Copy link
Contributor Author

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
Copy link
Owner

ctongfei commented Oct 4, 2022

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
Copy link
Contributor Author

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:

  • start
  • startInstant
  • elapsedBeforeStart
  • paused
  • alive

@deejgregor
Copy link
Contributor Author

deejgregor commented Oct 4, 2022

@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: OpenNMS/opennms#5322

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants