Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Get dimensions from stty on windows if possible#32763
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
Uh oh!
There was an error while loading.Please reload this page.
rtek commentedJul 26, 2019
Had to refactor Terminal into a singleton otherwise it couldn't be tested when mocking out the env. It was practically one already due to the static properties. |
fabpot commentedJul 27, 2019
@rtek Tests must not impact our code. I would highly suggest to remove the singleton and find a way to test the code in another way. |
Uh oh!
There was an error while loading.Please reload this page.
rtek commentedJul 27, 2019
@fabpot Understood - reverted the public API changes. Re |
| privatestatic$width; | ||
| privatestatic$height; | ||
| private$width; | ||
| private$height; |
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 think they were static as a way to cache the values.
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.
Agreed, but only in a non application context. Application sets envCOLUMNS/LINES from the calculated values inTerminal, which then provides caching sinceTerminal always checks the env first. Only those who instantiateTerminal outside of an application and did not reuse the instance would lose the cache. If this is not acceptable then I would have to use reflection to get the test coverage - is that OK?
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.
Looking at the code base, I can see that we are instantiating a Terminal instance several times (the instance is not shared).
rtek commentedAug 17, 2019
Reverted to maintain the static cache convention and used reflection to test. Test failures on unrelated component. |
chalasr commentedSep 13, 2019
PR target changed from 4.3 to 3.4 as the bug also exists there. |
fabpot commentedSep 13, 2019
Thank you@rtek. |
…(rtek)This PR was merged into the 3.4 branch.Discussion----------[Console] Get dimensions from stty on windows if possible| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |`Console\Terminal` is modified to extract width/height settings to from `stty` on windows if possible. Previously such values were extracted from `mode CON`. If another terminal is in use (eg, gitbash) then the incorrect values would be applied.Commits-------5265080 [Console] Get dimensions from stty on windows if possible
Console\Terminalis modified to extract width/height settings to fromsttyon windows if possible. Previously such values were extracted frommode CON. If another terminal is in use (eg, gitbash) then the incorrect values would be applied.