Fix display of the "low ASCII" glyphs in PC code pages #1964
Conversation
… bit test rather than an equality comparison, otherwise it won't match anything.
…'t be reached now that the type check is working correctly.
|
Having done some more testing, I've found another issue related to this change. I think it's an existing bug in the conpty code rather than a problem with the PR itself (see the issue referenced above), but this "fix" clearly makes things worse, so it might be best to put it on hold for now. |
|
So my only big concern with this is how this behaves relative to the v1 console. If you have a test application that writes an ESC to the buffer, and then uses |
zadjii-msft
added
Product-Conhost
|
Just to be clear, if you write an ESC to the stdout, |
|
Excellent. At some point in the future (in a non-compatible mode) we may want to return |
DESIRE TO SHIP INTENSIFIES |
In the legacy console, it used to be possible to write out characters from the C0 range of a PC code page (e.g. CP437), and get the actual glyphs defined for those code points (at least those that weren't processed as control codes). In the v2 console this stopped working so you'd get an FFFD replacement glyph (�) for those characters instead. This PR fixes the issue so the correct glyphs are displayed again. There was already code in place to achieve this in the `WriteCharsLegacy` method. It used the `GetStringTypeW` method to determine the character type of the value being output, and if it was a `C1_CNTRL` character it performed the appropriate mapping. The problem was that the test of the character type flag was done as a direct comparision, when it should have been a bit test, so the condition was never met. With this condition fixed, the code also needed to be reordered slightly to handle the null character. That had a special-case mapping to space, which was previously performed after the control test, but since a null character now successfully matches `C1_CNTRL`, it no longer falls through to that special case. To address that, I've had to move the null check above the control test. I've tested this manually, by trying to output all the characters in the affected range (ASCII values 0 to 31, and 127, excluding the actual control codes 8,9,10 and 13). In all cases they now match the output that the legacy console produced. Note that this only applies to PC code pages that have glyphs defined for the C0 range, so it won't work with the UTF-8 code page, but that was to be expected - the legacy console behaved the same way. Also, note that this only works when the `ENABLE_PROCESSED_OUTPUT` console mode is set. That seems wrong to me (I'd expect the glyphs to work in both cases), but that's the way the legacy console behaved as well, so if that's a bug it's a separate issue. I haven't added any unit tests, because I expect the behaviour of some of these characters to change over time (as support is added for more control codes), which could then cause the tests to fail. But if that's not a concern, I could probably add something to the ScreenBufferTests (perhaps with a comment warning that the tests might be expected to fail in the future). Closes #166. (cherry picked from commit 9102c5d)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
This went out for conhost in insider build 19002! Thanks 😄 |
Summary of the Pull Request
In the legacy console, it used to be possible to write out characters from the C0 range of a PC code page (e.g. CP437), and get the actual glyphs defined for those code points (at least those that weren't processed as control codes). In the v2 console this stopped working so you'd get an FFFD replacement glyph (�) for those characters instead. This PR fixes the issue so the correct glyphs are displayed again.
PR Checklist
Detailed Description of the Pull Request / Additional comments
There was already code in place to achieve this in the
WriteCharsLegacymethod. It used theGetStringTypeWmethod to determine the character type of the value being output, and if it was aC1_CNTRLcharacter it performed the appropriate mapping. The problem was that the test of the character type flag was done as a direct comparision, when it should have been a bit test, so the condition was never met.With this condition fixed, the code also needed to be reordered slightly to handle the null character. That had a special-case mapping to space, which was previously performed after the control test, but since a null character now successfully matches
C1_CNTRL, it no longer falls through to that special case. To address that, I've had to move the null check above the control test.Validation Steps Performed
I've tested this manually, by trying to output all the characters in the affected range (ASCII values 0 to 31, and 127, excluding the actual control codes 8,9,10 and 13). In all cases they now match the output that the legacy console produced.
Note that this only applies to PC code pages that have glyphs defined for the C0 range, so it won't work with the UTF-8 code page, but that was to be expected - the legacy console behaved the same way.
Also, note that this only works when the
ENABLE_PROCESSED_OUTPUTconsole mode is set. That seems wrong to me (I'd expect the glyphs to work in both cases), but that's the way the legacy console behaved as well, so if that's a bug it's a separate issue.I haven't added any unit tests, because I expect the behaviour of some of these characters to change over time (as support is added for more control codes), which could then cause the tests to fail. But if that's not a concern, I could probably add something to the ScreenBufferTests (perhaps with a comment warning that the tests might be expected to fail in the future).