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

Correct the default 6x6x6 palette entries #5999

Merged
3 commits merged into from
May 19, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented May 19, 2020

There is a range of 216 colors in the default 256-color table that is
meant to be initialized with a 6x6x6 color cube, with each color
component iterating over the values 00, 5F, 87, AF, D7, and
FF. A few of the entries incorrectly had the red component has DF,
when it should have been D7. This PR corrects those entries. It also
removes a bit of unnecessary whitespace in the first 100 entries.

Validation Steps Performed

I have a visual test script that renders the full 256-color palette,
using both the indexed color sequence (SGR 38;5) and the equivalent
rgb representation (SGR 38;2) side by side. Although the difference
was subtle when it was incorrect, I can now see that it has been fixed.

Closes #5994

@j4james
Copy link
Collaborator Author

j4james commented May 19, 2020

At some point we may want to generate this entire palette programmatically, but for now I just wanted to get a quick fix in place, because the discrepancy was bugging me.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Huh, good eye. Thanks!

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

thanks!

@DHowett
Copy link
Member

DHowett commented May 19, 2020

@msftbot merge this in 1 minute

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 19, 2020
@ghost
Copy link

ghost commented May 19, 2020

Hello @DHowett!

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 won't merge this pull request until after the UTC date Tue, 19 May 2020 20:02:17 GMT, which is in 1 minute

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".

@ghost ghost merged commit 5d6fdf3 into microsoft:master May 19, 2020
@j4james j4james deleted the fix-default-palette branch May 24, 2020 17:23
jelster pushed a commit to jelster/terminal that referenced this pull request May 28, 2020
There is a range of 216 colors in the default 256-color table that is
meant to be initialized with a 6x6x6 color cube, with each color
component iterating over the values `00`, `5F`, `87`, `AF`, `D7`, and
`FF`. A few of the entries incorrectly had the _red_ component has `DF`,
when it should have been `D7`.  This PR corrects those entries. It also
removes a bit of unnecessary whitespace in the first 100 entries.

## Validation Steps Performed

I have a visual test script that renders the full 256-color palette,
using both the indexed color sequence (`SGR 38;5`) and the equivalent
rgb representation (`SGR 38;2`) side by side. Although the difference
was subtle when it was incorrect, I can now see that it has been fixed.

Closes microsoft#5994
@ghost
Copy link

ghost commented Jun 18, 2020

🎉Windows Terminal Preview v1.1.1671.0 has been released which incorporates this pull request.:tada:

Handy links:

DHowett pushed a commit that referenced this pull request Jun 24, 2020
There is a range of 216 colors in the default 256-color table that is
meant to be initialized with a 6x6x6 color cube, with each color
component iterating over the values `00`, `5F`, `87`, `AF`, `D7`, and
`FF`. A few of the entries incorrectly had the _red_ component has `DF`,
when it should have been `D7`.  This PR corrects those entries. It also
removes a bit of unnecessary whitespace in the first 100 entries.

## Validation Steps Performed

I have a visual test script that renders the full 256-color palette,
using both the indexed color sequence (`SGR 38;5`) and the equivalent
rgb representation (`SGR 38;2`) side by side. Although the difference
was subtle when it was incorrect, I can now see that it has been fixed.

Closes #5994

(cherry picked from commit 5d6fdf3)
DHowett pushed a commit that referenced this pull request Jun 24, 2020
There is a range of 216 colors in the default 256-color table that is
meant to be initialized with a 6x6x6 color cube, with each color
component iterating over the values `00`, `5F`, `87`, `AF`, `D7`, and
`FF`. A few of the entries incorrectly had the _red_ component has `DF`,
when it should have been `D7`.  This PR corrects those entries. It also
removes a bit of unnecessary whitespace in the first 100 entries.

## Validation Steps Performed

I have a visual test script that renders the full 256-color palette,
using both the indexed color sequence (`SGR 38;5`) and the equivalent
rgb representation (`SGR 38;2`) side by side. Although the difference
was subtle when it was incorrect, I can now see that it has been fixed.

Closes #5994

(cherry picked from commit 5d6fdf3)
@ghost
Copy link

ghost commented Jun 30, 2020

🎉Windows Terminal v1.0.1811.0 has been released which incorporates this pull request.:tada:

Handy links:

@DHowett
Copy link
Member

DHowett commented Jul 2, 2020

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 20161.

This pull request was closed.
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 of the 6x6x6 palette entries are incorrect
3 participants