[Console] progress bar fix #16490
[Console] progress bar fix #16490
Conversation
| private function adjustBarWidthToWindowWidth($line) | ||
| { | ||
| $lineLength = Helper::strlenWithoutDecoration($this->output->getFormatter(), $line); | ||
| $windowWidth = exec('tput cols'); |
There was a problem hiding this comment.
This is not enough... what about windows? Look at https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Console/Application.php#L734
|
Thank you for working on this! This is an annoying issue. Would be great if you could add some tests too. Status: Needs Work |
|
Thanks for the feedback and code references. It's really helpful. I'll look on it tonight. |
|
@1ed Do you have any suggestion how to test this? |
|
I've extracted Still need help with way of testing |
|
Sorry for the late answer. With this new interface adding a test is easy: public function testResizeItself()
{
$terminal = $this->getMock('Symfony\Component\Console\Terminal\TerminalDimensionsProviderInterface');
$terminal
->expects($this->at(0))
->method('getTerminalWidth')
->will($this->returnValue(36));
$terminal
->expects($this->at(1))
->method('getTerminalWidth')
->will($this->returnValue(35));
$terminal
->expects($this->at(2))
->method('getTerminalWidth')
->will($this->returnValue(20));
$bar = new ProgressBar($output = $this->getOutputStream(), 0, $terminal);
$bar->start();
$bar->advance();
$bar->advance();
rewind($output->getStream());
$this->assertEquals(
$this->generateOutput(' 0 [>---------------------------]').
$this->generateOutput(' 1 [->-------------------------] ').
$this->generateOutput(' 2 [-->---------] '),
stream_get_contents($output->getStream())
);
}But it will not pass with your implementation which has flaws. I'm not sure if it should be the progress bar responsibility to resize itself and I think it should not happen automatically but be controlled by the user. It would be cool after the bar reaches its minimum width the whole format can be changed if needed and progressively reduced to only a percentage counter or something. But let's see what others say. |
| * @var int[] | ||
| */ | ||
| private $terminalDimensions = array(); | ||
|
|
There was a problem hiding this comment.
You should use 4 spaces not tabs for indentation.
There was a problem hiding this comment.
Sorry, wrong IDE code style.
|
@1ed I'm rather looking for test for the issue. See #13019. How to test that? Your code tests the output is printed, that's working fine. I'm looking for test, that check output it properly overwritten. Since buffer/stream collects all the output and not only the final one, I find it impossible to test ATM. |
|
@1ed Just added better naming and decoupled |
|
|
||
| namespace Symfony\Component\Console\Terminal; | ||
|
|
||
| interface TerminalDimensionsProviderInterface |
|
@TomasVotruba This one looks good to me. Several changes are needed before the merge though:
If you don't have time, just tell me and I will finish it for you. Thanks. |
|
@fabpot Thanks for feedback. I have time and I will finish this during the weekend. |
|
Tests are failing for Twig, otherwise passing. @fabpot I've made points 1 and 2. As for point 3: In 2.3 there is ProgressHelper instead of ProgressBar, which has different architecture and it would take me long time to investigate differences, as I never used it. Not sure if this is bug in 2.3 either. Could I ask you to do this 2.3 merge, please? Thank you |
| use Symfony\Component\Console\Terminal\TerminalDimensionsProvider; | ||
| use Symfony\Component\Console\Terminal\TerminalDimensionsProviderInterface; | ||
|
|
||
| class TerminalDimensionsProviderTest extends PHPUnit_Framework_TestCase |
There was a problem hiding this comment.
The PHPUnit_Framework_TestCase is not imported in other Symfony tests - so it may be unified.
|
Ping @fabpot |
| $dimensions = $this->getTerminalDimensions(); | ||
|
|
||
| return $dimensions[1]; | ||
| return $this->terminalDimensionsProvider->getTerminalWidth(); |
|
I'm made some changes to this PR here #19012 |
|
Thank you @fabpot for taking care of this |
This PR was merged into the 3.2-dev branch. Discussion ---------- [Console] progress bar fix | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13019 | License | MIT | Doc PR | - This is #16490 where I've simplified the code as much as possible and added a test for the bug we're trying to fix. The main change is the renaming of the `TerminalDimensionsProvider` to just `Terminal`. The new class can probably be useful to add more about the terminal. Commits ------- 2f81247 switched to use COLUMNS and LINES env vars to change terminal dimensions bf7a5c5 fixed logic a589635 deprecated some Console Application methods 8f206c8 fixed CS, simplified code b030c24 [Console] ProgressBar - adjust to the window width (static)
This is just a simple proposal for the issue.
I don't understand
overwrite()deep enough method yet. I'd like to hear some feedback to the solution and to improve it further.It solves it partially, by limiting too long static output to shorter progress bar width. Window resizing or dynamic line size breaks.
Important changes so far
TerminalDimensionProvider, which takes care of detecting width and height of terminal - before it was responsibility ofApplicationclassProgresBar- check for positive number added tosetMaxWidth()(1 is set, when not available)ProgresBar- overwrite method now makes sure the output is not longer then the terminal width (the issue [Console] ProgressBar behavior (question) #13019)