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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix debug console to handle more than one graphics mode in a single escape sequence #38981

Merged
merged 2 commits into from Nov 27, 2017

Conversation

Projects
None yet
3 participants
@dyc3
Contributor

dyc3 commented Nov 22, 2017

According to ascii-table.com, the spec allows for more than one graphics mode to be specified at a time.

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Nov 22, 2017

CLA assistant check
All CLA requirements met.

msftclas commented Nov 22, 2017

CLA assistant check
All CLA requirements met.

@isidorn isidorn added this to the November 2017 milestone Nov 23, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 23, 2017

Contributor

@dyc3 thanks a lot for your contribution! This looks great to me.
However do you have an example where this contribution clearly helps?
Also does this maybe fix one of #21423 #6845

Contributor

isidorn commented Nov 23, 2017

@dyc3 thanks a lot for your contribution! This looks great to me.
However do you have an example where this contribution clearly helps?
Also does this maybe fix one of #21423 #6845

@dyc3

This comment has been minimized.

Show comment
Hide comment
@dyc3

dyc3 Nov 24, 2017

Contributor

Sure, @isidorn.
Here's before my fix:
Before
and after:
After

This would appear to fix #6845, but not #21423. I think that how the function is currently written does not make #21423 easy to fix, and it'll probably have to be rewritten for that.

Contributor

dyc3 commented Nov 24, 2017

Sure, @isidorn.
Here's before my fix:
Before
and after:
After

This would appear to fix #6845, but not #21423. I think that how the function is currently written does not make #21423 easy to fix, and it'll probably have to be rewritten for that.

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 24, 2017

Contributor

@dyc3 I checked and this indeed fixes #6845 - great!
In the after picture I do not see the actual difference since it is cutoff but does not really matter.
The only thing which makes me nervous is the while (chr !== 'm') loop which can spin infinetely. Can we rewrite this in such a way that we are sure it will always finishes?

Contributor

isidorn commented Nov 24, 2017

@dyc3 I checked and this indeed fixes #6845 - great!
In the after picture I do not see the actual difference since it is cutoff but does not really matter.
The only thing which makes me nervous is the while (chr !== 'm') loop which can spin infinetely. Can we rewrite this in such a way that we are sure it will always finishes?

@dyc3

This comment has been minimized.

Show comment
Hide comment
@dyc3

dyc3 Nov 24, 2017

Contributor

@isidorn Ah, I didn't see that before. I'll see what I can figure out.

Contributor

dyc3 commented Nov 24, 2017

@isidorn Ah, I didn't see that before. I'll see what I can figure out.

@dyc3

This comment has been minimized.

Show comment
Hide comment
@dyc3

dyc3 Nov 24, 2017

Contributor

Unless there is something I'm missing, there is a practical limit to the number of graphic modes specified at one time: bold, underscore, blink, reverse video, concealed, foreground color. and background color. So limiting codes to a length of 7 at most should do the trick.

Contributor

dyc3 commented Nov 24, 2017

Unless there is something I'm missing, there is a practical limit to the number of graphic modes specified at one time: bold, underscore, blink, reverse video, concealed, foreground color. and background color. So limiting codes to a length of 7 at most should do the trick.

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 27, 2017

Contributor

@dyc3 looks good, thanks a lot. Merging it in

Contributor

isidorn commented Nov 27, 2017

@dyc3 looks good, thanks a lot. Merging it in

@isidorn isidorn merged commit 0474a98 into Microsoft:master Nov 27, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment