10000 [Console] ProgressBar clears too many lines on update by danepowell · Pull Request #40450 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console] ProgressBar clears too many lines on update #40450

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 16, 2021
Merged

[Console] ProgressBar clears too many lines on update #40450

merged 1 commit into from
Mar 16, 2021

Conversation

danepowell
Copy link
Contributor
Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

The ProgressBar incorrectly calculates line lengths when updating, including non-displayable characters such as ANSI colors. This causes it to clear too many lines if the terminal width is greater than the displayed line length but less than the line length including non-displayed characters. An example of this bug in action is https://github.com/acquia/cli/issues/467

@danepowell danepowell requested a review from chalasr as a code owner March 11, 2021 20:18
@carsonbot carsonbot added this to the 4.4 milestone Mar 11, 2021
@danepowell
Copy link
Contributor Author

I don't see how the test failures are related to this PR, let me know if anything else is needed

@fabpot
Copy link
Member
fabpot commented Mar 12, 2021

Can you add a test to cover this bug?

@grasmash
Copy link
Contributor

I can reproduce the bug with a small terminal window. I can confirm via a manual test that the upstream change does resolve the issue.

@danepowell danepowell force-pushed the progressbar-clears-lines branch from f17a75b to 159bd27 Compare March 12, 2021 19:02
@danepowell
Copy link
Contributor Author

I added a unit test. You can see it failing in the test-only commit and then passing after the fix.

(both commits fail the full test suite due to some unrelated issue on the 4.4 branch)

@danepowell danepowell mentioned this pull request Mar 12, 2021
Merged
2 tasks
@chalasr chalasr mentioned this pull request Mar 14, 2021
Merged
@nicolas-grekas nicolas-grekas changed the title ProgressBar clears too many lines on update [Console] ProgressBar clears too many lines on update Mar 16, 2021
nicolas-grekas
nicolas-grekas approved these changes Mar 16, 2021
View reviewed changes
@nicolas-grekas nicolas-grekas force-pushed the progressbar-clears-lines branch from 22a4b09 to 2aa3df0 Compare March 16, 2021 08:54
@nicolas-grekas
Copy link
Member

Thank you @danepowell.

@nicolas-grekas nicolas-grekas merged commit a78fb18 into symfony:4.4 Mar 16, 2021
This was referenced Mar 29, 2021
Merged
Merged
@glensc glensc mentioned this pull request Apr 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@danepowell @fabpot @grasmash @nicolas-grekas @chalasr @carsonbot
0