Correctly clear lines for multi-line progress bar messages #40460
Conversation
|
Thanks for this. Should it be applied to 4.4 as well? |
|
Yes, I’m working on a test and also trying to diagnose why the current tests are failing. But, I find that running tests while Xdebug is enableed is extremely slow with Symfony. Any tips on speeding that up? |
|
So, the test is failing because it expects: But the actual is: I don't really understand the difference between the |
|
Ok, so I did so research. This is a great article: https://notes.burke.libbey.me/ansi-escape-codes/. The 2A indicates that 2 lines should be cleared rather than 1. That seems right looking at the 3 line output in the example. The rather unfun part of testing this is that you can't actually see the diff for the failing tests on the command line because they're rendered ANSI escape codes and not actually visible. Apart from using xDebug to inspect the variables (which is terribly slow) I'm not sure how one is supposed to know what's being printed. |
| $message_lines = explode("\n", $message); | ||
| $line_count = \count($message_lines) - $this->formatLineCount + 1; | ||
| foreach ($message_lines as $message_line) { | ||
| if (Helper::strlen($message_line) / $this->terminal->getWidth() > 1) { |
There was a problem hiding this comment.
This is a second bug that's being fixed. If the Terminal width exactly matches the line length, we should not count an extra line. We should only count an extra line if it exceeds the line length.
|
I don't believe new tests are needed. The existing test fixtures have been updated to assert that lines are emitted correctly. Admittedly just looking at the test fixtures doesn't grant much confidence. I performed a number of successful manual tests that used combinations of short content, long content, and content with new line characters. They all displayed as expected. However, I'd like a functional review by someone else who is able to more readily identify issues. |
|
@chalasr I believe this is ready to be merged. The failing tests seem unrelated to the changes made here. |
|
Thank you for the detailed explanations, it makes everyone more comfortable with reviewing/merging such a patch (low-level + changes in existing tests). Still, I'd like to checkout your branch to ensure we don't have any regression on the modified test cases (as it happened a lot in the past). I will look into this by the end of the week. |
Completely agree. 👍 |
|
Separately, it looks like I should merge my approach with this proposed change: https://github.com/symfony/symfony/pull/40450/files Done :) |
|
Thanks, I was about to ask you if you could have a look at #40450 :) |
|
I can rebase it, but I would prefer to have that in a separate pull request. Would that be OK? |
|
Need to do some more work here to correctly integrate changes from #40450, seems to have caused a problem. |
|
@chalasr Ok. Lot's of changes, but much better now:
|
|
I think that all test failures are unrelated to the changes. You can see the relevant tests passing here: |
|
As the owner of #40450, I think this approach makes sense. |
|
I guess I should say, I'd prefer for #40450 to be merged first since it addresses a separate bug and I'd love to get credit for the fix 😄 . But as long the bugs get fixed (this PR now fixes both), that's what matters most. |
|
@danepowell I'm fine with that! Though I should say, I intentionally cherry picked your commit so you'd get commit credit if this PR is merged as is. |
|
I merged #40450 to ease with squashing this PR. |
|
Rebased. |
|
@grasmash Can you squash your 7 commits into 1 so that we can merge? |
|
@chalasr Done! |
|
Thank you @grasmash. |
Measure the length of individual lines when calculating progress bar message length.
When a multiline message is added to a progress bar, the progress bar measures the length of all lines combined in a single string without considering whether that string contains new line characters. It is not accurately measuring the number of lines that are displayed on the terminal.
On the terminal, the actual number of new lines is dictated by a combination of new line characters and text extending beyond the terminal width. Due to this bug, the progress bar sometimes clears too many lines when the progress bar is advanced.
The progress bar should instead consider the overlap of $this->formatLineCount and the actual number of lines in the message and then add any remainder.