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

Modify solarized themes for windows terminal to make all text visible #4480

Closed

Conversation

soul4soul
Copy link

Summary of the Pull Request

I have been wanting to use the solarized theme provided in windows terminal since release. I have been unable to use it since the terminal becomes unreadable with the solarized themes supplied. The Solarized theme doesn't specify colors in a manner which complies with the options required by windows terminal. For example there is no concept of "bright" colors in the Solarized spec. This change treats the normal colors the same as the bright colors.

All of the text is at least visible now. The the colors (white/black/green/blue/etc..) no longer match the backgrounds. Black is still hard to read on the dark theme. White is still hard to read on the light theme.

PR Checklist

  • Closes #xxx
  • [X ] CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Dark Theme

Old New
dark_old dark_new

Light Theme

Old New
light_old light_new

Palette

Old New
old_palette new_palette

Validation Steps Performed

I have been wanting to use the solarized theme provided in windows terminal since release. I have been unable to use it since the terminal becomes unreadable with the solarized themes supplied. The Solarized theme doesn't specify colors in a manner which complies with the options required by windows terminal. For example there is no concept of "bright" colors in the Solarized spec.
@DHowett-MSFT
Copy link
Contributor

So, regarding there being no "bright" colors in the solarized spec: I don't believe we are using them as bright colors. Just like Solarized, we use them to gain an additional 9 color palette slots. These themes are specialized for things that know about Solarized, and perhaps they were not appropriate to ship for general purpose use as Terminal themes.

/cc @zadjii-msft thoughts?

@j4james
Copy link
Collaborator

j4james commented Feb 6, 2020

I'm not going to try and defend the Solarized color schemes, because I think they're awful. But if you don't like those colors, then you really ought to come up with your own name for whatever alternatives you're proposing, and not just replace the official Solarized values. People that actually like the Solarized schemes assumedly expect the colors exactly the way they are.

And if it comes to proposing new schemes to be included in the app by default, I'd like to think we could do better than a scheme that can't distinguish bright from dark. At the very least open an issue first and give people a chance to vote on it.

Just my 2c.

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.

I think I agree with @j4james on this. As much as I definitely believe these changes make these schemes infinitely better, they're not technically Solarized anymore.

I'd accept this PR if it was adding 2 new color schemes named something like "Solarized Dark (Patched)", as opposed to modifying the existing Solarized ones.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 11, 2020
@soul4soul
Copy link
Author

Can you point me to the official Solarized documentation for its use with conhost/windows terminal? I don't see it listed on the website or repo. I see official use of the color palettes for other terminals but not conhost/wt.

To try and get a point of reference I remembered VS Code has a Solarized theme and a built in terminal. Below is a comparison of the wt theme and VS code terminal both using Solarized dark. One of several differences is that all text in VS Code is visible which is critically important.

wt VS Code
wt image

If there is no room to change the the Solarized themes I agree with @DHowett-MSFT sentiment about removing them.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 12, 2020
@zadjii-msft
Copy link
Member

I'm mostly looking at the following table, which is taken from here on the solarized site. Clipped to only the relevant columns:

SOLARIZED HEX 16/8 TERMCOL M/HEX RGB
base03 #002b36 8/4 brblack #1c1c1c 0 43 54
base02 #073642 0/4 black #262626 7 54 66
base01 #586e75 10/7 brgreen #585858 88 110 117
base00 #657b83 11/7 bryellow #626262 101 123 131
base0 #839496 12/6 brblue #808080 131 148 150
base1 #93a1a1 14/4 brcyan #8a8a8a 147 161 161
base2 #eee8d5 7/7 white #e4e4e4 238 232 213
base3 #fdf6e3 15/7 brwhite #ffffd7 253 246 227
yellow #b58900 3/3 yellow #af8700 181 137 0
orange #cb4b16 9/3 brred #d75f00 203 75 22
red #dc322f 1/1 red #d70000 220 50 47
magenta #d33682 5/5 magenta #af005f 211 54 130
violet #6c71c4 13/5 brmagenta #5f5faf 108 113 196
blue #268bd2 4/4 blue #0087ff 38 139 210
cyan #2aa198 6/6 cyan #00afaf 42 161 152
green #859900 2/2 green #5f8700 133 153 0

Which, if you re-order the rows to be in terminal color order, looks like the following:

SOLARIZED HEX 16/8 TERMCOL M/HEX RGB
base02 #073642 0/4 black #262626 7 54 66
red #dc322f 1/1 red #d70000 220 50 47
green #859900 2/2 green #5f8700 133 153 0
yellow #b58900 3/3 yellow #af8700 181 137 0
blue #268bd2 4/4 blue #0087ff 38 139 210
magenta #d33682 5/5 magenta #af005f 211 54 130
cyan #2aa198 6/6 cyan #00afaf 42 161 152
base2 #eee8d5 7/7 white #e4e4e4 238 232 213
base03 #002b36 8/4 brblack #1c1c1c 0 43 54
orange #cb4b16 9/3 brred #d75f00 203 75 22
base01 #586e75 10/7 brgreen #585858 88 110 117
base00 #657b83 11/7 bryellow #626262 101 123 131
base0 #839496 12/6 brblue #808080 131 148 150
violet #6c71c4 13/5 brmagenta #5f5faf 108 113 196
base1 #93a1a1 14/4 brcyan #8a8a8a 147 161 161
base3 #fdf6e3 15/7 brwhite #ffffd7 253 246 227

This is also the order that the very excellent iTerm2-Color-Schemes repo uses

@DHowett-MSFT
Copy link
Contributor

Until we can get to discussion consensus, I'm gonna close this one. Feel free to continue the chat - this just isn't actionable right now!

@soul4soul
Copy link
Author

I'm mostly looking at the following table, which is taken from here on the solarized site. Clipped to only the relevant columns:

Ah I see! I wasn't interrupting that table correctly. Now I understand the comments about not following the Solarized scheme.

@soul4soul soul4soul deleted the solarized_theme_updates branch February 14, 2020 17:14
@zadjii-msft
Copy link
Member

@DHowett-MSFT I know we don't love the default Solarized themes, and it's too late to remove them, but what are your thoughts on shipping these proposed changes as "Solarized [Light|Dark] (Patched)"?

Actually lets get some more peeps in this discussion: @carlos-zamora @leonMSFT @miniksa @cinnamon-msft @craigloewen-msft @the-entire-console-team-and-everyone-we-know-why-not

@cinnamon-msft
Copy link
Contributor

I'm okay with shipping these modified schemes. Also, if we're going to ship these, I feel like we could add more in? Not sure how everyone feels about shipping a ton of themes in-box.

@j4james
Copy link
Collaborator

j4james commented Mar 6, 2020

In case anyone has missed this point, let me reiterate that these patched Solarized schemes are still very much broken. By making the light and dark colors the same, they are guaranteed to produce invisible text in some applications.

If we are going to include these, I'd urge you to try and find a bunch of good color schemes to include as well. Otherwise we're going to be left with 4 of the 9 bundled schemes being some variant of Solarized, all of which have problems of some sort. That's not a great user experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants