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

ProgressBar => make setMaxSteps public #24100

Closed
dominikzogg opened this issue Sep 5, 2017 · 13 comments
Closed

ProgressBar => make setMaxSteps public #24100

dominikzogg opened this issue Sep 5, 2017 · 13 comments

Comments

@dominikzogg
Copy link

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version 3.x

At the moment its not possible to change the max steps. In our company we have a few importers which run over a long time which means the max to read data can get higher while execution. Would be cool if we could update the progressbar without the need using reflection on the private method (which we've done, cause it worked fine).

@dominikzogg dominikzogg changed the title ProgressBar => max setMaxSteps public ProgressBar => make setMaxSteps public Sep 5, 2017
@linaori
Copy link
Contributor

linaori commented Sep 5, 2017

Instead of changing the visibility of this particular method, what about calling it adjustMaxSteps or updateMaxSteps? This would imply the max steps gets adjusted or updated instead of merely setting it, could in theory even add some hooks in there if needed.

@javiereguiluz
Copy link
Member

Instead of doing this change, I think this problem could be solved in two different ways as explained in https://symfony.com/doc/current/components/console/helpers/progressbar.html:

  1. Don't use a progress bar with a defined number of steps:
If you don't know the number of steps in advance, just omit the
steps argument when creating the ProgressBar instance:

$progress = new ProgressBar($output);
  1. Regress the progress bar when more work is found, passing a negative number to advance():
$progress->advance(-2);

@dominikzogg
Copy link
Author

@javiereguiluz what i know (last tried) you have to put the number at __construct or at start. And if the Progress does not know the max amount at the current time, its very hard to know the aprox. time left.

@javiereguiluz
Copy link
Member

@dominikzogg I'm not sure I follow you. The number of steps in an optional argument in the ProgressBar constructor: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Console/Helper/ProgressBar.php#L57

@dominikzogg
Copy link
Author

@javiereguiluz yes, also on https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Console/Helper/ProgressBar.php#L339, but if its missing on both you get an Exception on a advance, at least in my tries. But even if it would work, you explanation would not solve the issue that i want a max, but a changeable onces.

@chalasr
Copy link
Member

chalasr commented Sep 6, 2017

Agree with @javiereguiluz here. Calling advance() while omitting the $maxSteps argument in both constructor and start() call should work. Please retry and, if it doesn't work for you, consider providing enough code to reproduce the issue.

@dominikzogg
Copy link
Author

@chalasr even if it does i would have no calculation, or wrong if set initial, if there is no max set

@javiereguiluz
Copy link
Member

I'm not completely sure, but in some Symfony apps I've seen those kind of progress bars "overflow" and start over from the beginning. This is the same behavior as most operating systems when displaying what they call "Indeterminate Progress Indicators".

We should check if this is indeed working like that.

@Adwitiya012
Copy link

Outside a class we can not access any private function.
But we can change the access specifier in the following way.

$class = new \ReflectionClass('\Symfony\Component\Console\Helper\ProgressBar');
$method = $class->getMethod('setMaxSteps');
$method->setAccessible(true);
$progress = new ProgressBar($output, 50);
$method->invokeArgs($progress, array(100));

Please let me know if it helps you or not.

@javiereguiluz
Copy link
Member

Yes, but the main question here I think is whether we want to support this feature or not. I'm 👎 because it's uncommon/unused in other software applications, such as operating systems. Usually you only have two kinds of progress bars: 1) you know the total in advance; 2) you don't know the total.

In the 2) case, the progress bar simply cycles or shows an undetermined animation. But this proposal asks for a third case: "I know the total in advance, but it can change, so I want to update it".

@Adwitiya012
Copy link

Adwitiya012 commented Sep 8, 2017

@javiereguiluz Yes you can update the max steps when it is needed to be changed due to any situation. This can be done by change of accessibility outside the class.
Based on this logic we can write test cases of private methods.

@mvrhov
Copy link

mvrhov commented Sep 11, 2017

I also have a case for that. I'm downloading and repackaging some data. I know the upper bound beforehand, however the real max is known after some status files are downloaded and compared against the locally cached ones. Having a totally borked ETA and max count doesn't look nice.

@ostrolucky
Copy link
Contributor

Please see #26449

Issue with overflowing/starting over of a bar is moot when you realize you can set custom format for ProgressBar. In my case I don't show %bar% at all, but other information, like percentage, time left, etc

fabpot added a commit that referenced this issue Mar 19, 2018
This PR was merged into the 4.1-dev branch.

Discussion
----------

Make ProgressBar::setMaxSteps public

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24100
| License       | MIT
| Doc PR        | -

This is useful in cases when target of tracking changes its size during progress advancement. My exact use case is showing progress of file upload for file which is still being downloading.

Commits
-------

2b3c37a Make ProgressBar::setMaxSteps public
@fabpot fabpot closed this as completed Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants