-
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
Deprecate and eventually replace Solarized #6997
Comments
This seems like the easiest solution |
I just went through and corrected a couple problems in the Solarized Dark and Light themes based on the published standard. When you are saying deprecate and replace are you talking about when it was already in the defaults.json before the Settings UI? |
@DHowett and @zadjii-msft, any guidance about what deprecating means in this context? I think this color scheme will continue to be supported and this issue is describing the behaviour of the Settings UI, where it adds the color scheme to the settings.json when it is selected in the interface. |
The problem is that the real Solarized color schemes are a bit rubbish, and they were a constant source of bug reports because they make certain common color combinations invisible (one example in particular being the parameter colors in PowerShell). At one point we were planning to deprecate them, so they wouldn't be one of the default themes available to new users. But in the end it was decided to just tweak the colors so that they didn't break PowerShell and hope that kept everyone happy. I was convinced we'd still get a bunch of complaints from hardcore Solarized fans insisting we use the official colors, but that doesn't appear to have happened. So I suspect we aren't actually planning the deprecation anymore. The one thing I am fairly certain of: we won't be changing the colors back to the published standard. That's a nightmare that nobody here wants to relive. Edit: Just found the PR where we applied the Solarized "fix". See PR #6985 for more details. |
@j4james, thank you, that explains a lot. I have contributed to cmd-colors-solarized in the past and have worked with Paul to make Solarized color schemes work with cmd and PS before Windows Terminal, and I was investigating a bug someone submitted there. One of my "fixes" I was considering for submission here was that there is a difference in base03 and base02 and how it is defined in the official spec. There's something else which doesn't seem to align in the base* range, but I haven't worked out where that issue exists. And lastly, I think Solarized Light is incorrect... While it technically matches the way the spec names the base* range, the color scheme has an expectation that the application would know that the dark text foreground should be base0 and when switched to light it should be base00. This means that the shell or an application would need to be aware of the context it is running in and this is how it is encoded as a color scheme in Windows Terminal. The additional thing I have prepared is that the base03-base3 entries are inverted. This makes it so that the output from applications don't need to care and they will do the right thing. This would provide this result, using the current definition of Solarized Dark: With my proposed inversion applied to Solarized Light, perhaps as an alternate Solarized Light variant: Which would resolve this current mess: For the purpose of usability, with the Solarized Light color scheme applied, the colors intended for grayscale use should be flipped while preserving the actual colors. Black might not be black, but as it comes to how the output is rendered, this inversion seems more appropriate. |
That's what PR #6985 was all about. We deliberately made that change. |
That's why what I was showing in the photos is just inverting the base* for Solarized Light and doesn't have the correction which also flips the equivalent of Black and Dark Gray, as per the issue referenced. The advantage of this proposed change is that it makes switching from the Dark scheme to the Light scheme as more than just switching the Foreground and Background colors, but it keeps the relative intensities the same and applies the scheme more closely with how it is intended. Foreground and Background colors seem better supported now, so I think there is still value in matching the termcolors values, but I can detail that in an issue specific to the changes and we can shift discussion there. |
@j4james, and now going back through all of this I'm reminded about why this is so frustrating to work with. Yellow warnings are gray and red errors are orange... which is why with cmd-colors-solarized we inverted the bright/dark relationship so that red was always red and yellow was always yellow. See @TBBle's comment on issue #6985. I think I can provide a profile fix (or maybe that's already part of Paul's Solarized canon wiki link), and we'd be able to fix the output for errors and warnings, but Gray/White aka Color Table 7, seems to be still locked to a specific color, so not a perfect fix, but I think this will improve readability when switching between dark and light variants. TBD. Thanks for getting me caught up on the history and the reason for this issue. |
I probably should go check that I haven't made any meaningful local changes to my "canon solarized setup" and not committed them, in case others actually look at it. I do roughly agree with the WT maintainers that canon Solarized in the default profile breaks a bunch of expectations, but I say that coming from the position of several years of Solarized use, and so I have the expectation already that if I want to use Solarized in a terminal, I have to plumb the config a long way through. Generally these days, I plumb it through to RGB-supporting programs, rather than relying on the 16-colour terminal palette. I think PowerShell and PS-Readline are the only things still generating 16-colour codes, but I'm not using Linux very much, so wouldn't have noticed if dircolors (for example) was mismapping over an SSH session. That's why I suggested perhaps replacing Solarized in the default config with, e.g. Selenized, which is basically tuned Solarized, but with the major advantage that it tries to map to the 16-colour ANSI palette sensibly, so should do the right thing as a default colour theme, where Solarized cannot without breaking all the existing users. The "if you say colour 7, Terminal turns that into BG" thing was a workaround for old PowerShell (PS-Readline technically) in #6810, and that will eventually be dropped from Terminal. The PS-Readline fix merged in July 2020, and PS-Readline 2.1.0 with the fix was GA in November 2020, so perhaps it's time to revert #6810 and close #6807? Or is there some other delivery pipeline that needs to pump first, e.g., is inbox PS-Readline a concern? |
hot take, with the auto-adjustment in #11095, we should close this. We can leave Solarized in now, because it will be less painful. (not not painful, just, less). Thoughts? |
Agreed. Perceptual color nudging is the "boots on the ground" solution to color schemes that are not properly perceptible. 😄 |
Good enough for me. |
I'll have to go look at what #11095 changes, but it sounds like it will fix the problems I was trying to address with tweaking the color scheme. If so, then I am probably prefectly fine with this. I think the effect of inverting should have the actual effect of inverting the colors in a way which preserves hue and adjusts things so that readability is maintained, but the essence of the solarized color scheme is there. |
Ideas for deprecation include shipping it built in but not in defaults.json and patching it into the user's settings when they're using it.
The text was updated successfully, but these errors were encountered: