-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
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.
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? |
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. |
There was a problem hiding this 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.
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.
If there is no room to change the the Solarized themes I agree with @DHowett-MSFT sentiment about removing them. |
I'm mostly looking at the following table, which is taken from here on the solarized site. Clipped to only the relevant columns:
Which, if you re-order the rows to be in terminal color order, looks like the following:
This is also the order that the very excellent iTerm2-Color-Schemes repo uses |
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! |
Ah I see! I wasn't interrupting that table correctly. Now I understand the comments about not following the Solarized scheme. |
@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 |
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. |
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. |
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
Detailed Description of the Pull Request / Additional comments
Dark Theme
Light Theme
Palette
Validation Steps Performed