Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fixes #40633: Progress bar is broken when progress bar character is unicode.#40635
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
grasmash commentedMar 29, 2021
| Q | A |
|---|---|
| Branch? | 5.2 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix#40633 |
| License | MIT |
Nyholm commentedMar 30, 2021
Thank you for this PR. Could you add a test to this bug? |
| $display =str_repeat($bar->getBarCharacter(),$completeBars); | ||
| if ($completeBars <$bar->getBarWidth()) { | ||
| $emptyBars =$bar->getBarWidth() -$completeBars - Helper::strlenWithoutDecoration($output->getFormatter(),$bar->getProgressCharacter()); | ||
| $display .=$bar->getProgressCharacter().str_repeat($bar->getEmptyBarCharacter(),$emptyBars); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO the issue is:$emptyBars should not be negative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree the issue is deeper, there is a confusion in the component between "width" and "length".Helper::strlen() deals with "width", and there is no helper to deal with "length", while it would be needed: a single simple-width character might be enough to replace a single double-width character. To be confirmed of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See this PR#40695
Am I correct?
grasmash commentedMar 31, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Maybe we should just revert the change that caused this? It seems like the issue extends beyond the scope of the progress bar. |
…, grasmash)This PR was merged into the 5.2 branch.Discussion----------[Console] Add Helper::width() and Helper::length()| Q | A| ------------- | ---| Branch? | 5.2| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Close#40697Fix#40634,fix#40635| License | MIT| Doc PR |This PR will add add a Helper::strwidth() and a Helper::strlength(). Same with with the Helper::strlenWithoutDecoration(). It does not deprecate anything. That is done in#40695With this PR we dont have to revert the emoji issue (ieclose#40697)FYI@grasmash, I used your tests from#40635Commits-------d9ea4c5 Add test.dc02ab3 [Console] Add Helper::strwidth() and Helper::strlength()