Skip to content
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 a couple of the DEC Special Graphics characters #2081

Merged
merged 4 commits into from
Jul 25, 2019

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jul 24, 2019

Summary of the Pull Request

Corrects the 0x5f code point in the DEC Special Graphics character set, which is meant to map to a blank glyph rather than an underscore. and updates the 0x60 code point to map to a "black diamond suite" glyph, rather than the "black diamond" glyph, since the latter currently renders as a double width character.

PR Checklist

Detailed Description of the Pull Request / Additional comments

The was mostly a matter of updating the s_rgDECSpecialGraphicsTranslations table in the TerminalOutput class, but also required a change in the TranslateKey method, to make sure the 0x5f code point was included in the range of translatable characters.

I've also made a few cosmetic changes to the source, fixing the comments on the translation table to match the DEC documentation (a couple of them were not quite right), and tidying up the case of the hex values in the table, so they're all consistently lowercase now.

The latter changes have been committed separately, so they're easy enough to revert if there are any objections to including them in this PR.

Validation Steps Performed

I've tested manually with Vttest's Test of character sets, making sure the two code points are mapped as expected, and the diamond glyph doesn't break the layout by being too wide.

…ode "black diamond suite", rather than the "black diamond", since the latter is currently rendered as a double width glyph.
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it. Thanks!

@DHowett-MSFT
Copy link
Contributor

@msftbot make sure @zadjii-msft signs off

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 25, 2019
@ghost
Copy link

ghost commented Jul 25, 2019

Hello @DHowett-MSFT!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @zadjii-msft

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@DHowett-MSFT
Copy link
Contributor

Actually, nevermind.

@DHowett-MSFT
Copy link
Contributor

@msftbot forget everything I just told you

@ghost
Copy link

ghost commented Jul 25, 2019

Hello @DHowett-MSFT!

Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request.

@DHowett-MSFT DHowett-MSFT merged commit 2febe1f into microsoft:master Jul 25, 2019
@ghost
Copy link

ghost commented Aug 3, 2019

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

@j4james j4james deleted the fix-special-graphics branch August 3, 2019 12:19
@DHowett-MSFT
Copy link
Contributor

This went out for conhost in insider build 19002! Thanks 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some DEC Special Graphics characters aren't quite right
3 participants