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

Accept color name "magenta" instead of "purple" #13261

Merged
2 commits merged into from
Jun 10, 2022

Conversation

matthewd673
Copy link
Contributor

@matthewd673 matthewd673 commented Jun 10, 2022

When loading color schemes from Json, check if GetValueForKey failed. If
it did, and we were searching for the colors "purple"/"brightPurple",
swap out the color name with "magenta"/"brightMagenta" and try again.
This was tested manually by creating a new color scheme using the colors
"magenta"/"brightMagenta" and loading it, then printing colored text in
the terminal to assure that the colors were correctly assigned as
purple.

This changes the color loader to use an index pair table to add support
for alternate color names.

Validation Steps Performed

  • Manually edit settings.json, creating a new color scheme with colors
    "magenta" and "brightMagenta"
  • View the color scheme in settings - the colors will appear as "purple"
    and "brightPurple" in the UI
  • Run a program that prints colored text, purple and brightPurple text
    will appear in the colors defined as magenta and brightMagenta
  • Modify the color scheme in the settings UI and save it, the colors
    will be saved as "purple" and "brightPurple" (I think this is the
    right behavior, since calling them "magenta" is technically a
    mistake)
  • Repeat the steps above with a normal color scheme - colors named
    "purple" and "brightPurple" still load properly

Closes #11456

@ghost ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Jun 10, 2022
@lhecker
Copy link
Member

lhecker commented Jun 10, 2022

If most people edit their schemes in the settings UI, is there a benefit of changing the label in the settings.json only, without also changing it in the settings UI? How did the "magenta" get into the settings.json in the first place?

In other words, if #11456 is about "purple" vs. "magenta" in general (because ECMA-48 calls it "magenta"), should we prefer changing the naming throughout the entire application to be consistent? Otherwise I'm worried whether only changing the settings.json parser will have any positive effect on the issue.

@zadjii-msft
Copy link
Member

How did the "magenta" get into the settings.json in the first place?

There are other tools that generate terminal color schemes for terminal emulators, and we're a little different than the others in regard to that key. The original report of this (#2641 (comment)) greatly predates the SUI, when this would have been more of an issue.

I don't think it's necessarily super valuable to change the whole app over to Magenta instead of purple (that would of course cause a rewrite of everyone's settings). I do think it's valuable to gracefully accept magenta though.

@matthewd673
Copy link
Contributor Author

Thank you for the feedback.
In terms of the usefulness of this fix, besides the original complaint #11456 (comment), in the case that "purple" is renamed to "magenta" across the entire program, this code could support legacy configurations that still say "purple".

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.

❤️

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 10, 2022
@ghost
Copy link

ghost commented Jun 10, 2022

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 1b630ab into microsoft:main Jun 10, 2022
@ghost
Copy link

ghost commented Jul 6, 2022

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

Handy links:

@ghost ghost mentioned this pull request Jul 6, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Magenta" is called "Purple" in WT
3 participants