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

Fix swapped black/brightBlack in Solarized Dark #6618

Closed
wants to merge 1 commit into from

Conversation

M-Pixel
Copy link

@M-Pixel M-Pixel commented Jun 20, 2020

Summary of the Pull Request

Console applications assume that backgrounds are black, and that lightBlack/DarkGrey are lighter than black/Black. This assumption is accounted for by all color schemes in defaults.json, except for the Solarized themes.

The Solarized Dark theme, in particular, makes -Parameters invisible against the background in PowerShell, which is obviously an unacceptable usability flaw.

This change makes black and background to the same (which is common throughout the color schemes), and makes brightBlack (DarkGray in .NET) lighter than black (which is obviously more correct given the meanings of those words).

References

#4047

PR Checklist

Validation Steps Performed

  1. Applied these changes to the default settings in my installation
  2. Observed that -Flags in PowerShell are now visible.

Console applications assume that backgrounds are black, and that `lightBlack`/`DarkGrey` are lighter than `black`/`Black`.  This assumption is accounted for by all color schemes in `defaults.json`, except for the Solarized themes.  This change affects the Solarized Dark color scheme, causing `black` and `background` to be the same (which is common throughout the color schemes), and causing `brightBlack` (`DarkGray` in .NET) to be lighter than black (which is obviously more correct given the meanings of those words).
@ghost ghost added Issue-Question For questions or discussion Product-Terminal The new Windows Terminal. labels Jun 20, 2020
@M-Pixel
Copy link
Author

M-Pixel commented Jun 20, 2020

Personally, I don't like the lack of contrast that #073642 has against #002B36, and I have chosen different colors (as well as different symbol-color assignments on the PowerShell side) in my own environment, but poor contrast is certainly better than no contrast whatsoever. I believe that this PR will significantly improve the default behavior of Terminal for its users.

@j4james
Copy link
Collaborator

j4james commented Jun 21, 2020

If you think the Solarized colors need changing you should really submit a PR with the official Solarized repository (https://github.com/altercation/solarized). We're just using the colors defined there.

Otherwise just use a different color scheme. Solarized is clearly not a good choice for use with PowerShell, but not everyone uses the same applications as you, and some people actually like Solarized exactly as it is.

@M-Pixel
Copy link
Author

M-Pixel commented Jul 6, 2020

Otherwise just use a different color scheme.
- @j4james

I am using my own implementation that doesn't have these problems. I'm making this PR and arguing in its favor on behalf of the community. I care about this software, this color scheme, and their adoption. I believe strongly that my efforts are aligned with the goals both of Windows Terminal and Solarized.

If you think the Solarized colors need changing you should really submit a PR with the official Solarized repository
- @j4james

I see now that Solarized officially associates #002b36 with brblack. This is certainly a problem on their end, as it's widely documented that brblack should be ligher (closer to grey) than black.

This issue has been in discussion on the Solarized repo since 2012. It's a very slow-moving repo that rarely makes commits - most new commits constitute the addition of configurations for new software to use the already-established scheme. The readme itself hasn't been updated since 2016. It's unreasonable to think that this will change anytime soon. Why would Ethan bother with something like that when it's easy enough for people to fix it in their own implementations? As many, in fact, do.

Meanwhile, a fresh install of Windows Terminal becomes temporarily unusable with a single innocuous setting change. Is that really acceptable? If a third party library has a bug, and leaves your patch in limbo for years, it's very common to fork so that you can continue to use it without leaving undesirable bugs in your own software. I'm sure Microsoft does this all the time. This is the same principle.

Regardless of the Solarized PR feasibility, the table that associates brblack with #002b36 isn't law, and isn't necessarily even what Solarized is. From the Solarized repo itself:

Solarized is a sixteen color palette (eight monotones, eight accent colors) designed for use with terminal and gui applications. It has several unique properties. I designed this colorscheme with both precise CIELAB lightness relationships and a refined set of hues based on fixed color wheel relationships. It has been tested extensively in real world use on color calibrated displays (as well as uncalibrated/intentionally miscalibrated displays) and in a variety of lighting conditions.
- Ethan Schoonover

This says nothing about brblack or specific terminals. It talks about the color choices, in their innate properties, and as they relate to each other. As long as I'm picking only from the provided hex values, in this regard, it's still in line with what it means to be Solarized Dark.

I often switch between dark and light modes when editing text and code. Solarized retains the same selective contrast relationships and overall feel when switching between the light and dark background modes... switching from dark to light mode retains the same perceived contrast in brightness between each value
- Ethan Schoonover

The Windows Terminal implementation of Solarized is arguably in violation of this. My flags have zero contrast in Dark, some contrast in Light. My numbers have zero contrast in Light, some contrast in Dark. This PR improves Windows Terminal's conformance to this aspect of Solarized.

Solarized defines some particular choices for which text colors are appropriate on which backgrounds. The readme defines a dark background as #002b36, and the next brightest color (meant for highlights) as #073642. In terminals, black is the darkest color, and brblack is the next lightest. It stands to reason that black should be #002b36, and brblack should be #073642. It stands to reason that defining #002b36 as a color used by text violates Solarized's rules (as does using #073642, but it is a less severe violation, and is the best that we can do given other constraints). While this PR may conflict with the brblack association in the readme, the current implementation is in conflict with the dark/light background/text color diagrams. The diagrams and table contradict each other, but the diagrams have much more explanation & justification than the table, so they should be given greater consideration.

Overall, the recurring theme of Solarized's readme is readability, readability, readability. Placing #002b36 text on a #002b36 background clearly violates that principle.

Given all of this, I hope you find it within reason that this PR remains true to the definition of "Solarized Dark". That cleared, this becomes a wonderfully easy opportunity to improve the usability of your software, which is surely more important than staying true to some particular terminal color association suggested once (in a contradictory manner) by a 3rd party.

...and some people actually like Solarized exactly as it is.
- @j4james

I like Solarized exactly as-is, too. But I don't like the Windows Terminal mappings for Solarized Dark. Changing these doesn't mean changing Solarized, but does mean fixing a usability issue. Among the many related issue reports that I found, I count at least 10 Solarized users voicing support for a fix, one maintainer and one user declaring that they don't use solarized at all for unrelated reasons, maintainer @zadjii-msft suggesting at one point to make a PR that does exactly what mine does, and your own explanations of why it is the way it is, but I see 0 users disagreeing with the call for change and no maintainer arguing that the colors are better the way that they currently are.

Given how frequently this arises as a duplicate issue, you may also consider that accepting this PR will save the maintainers time.

@DHowett
Copy link
Member

DHowett commented Jul 17, 2020

I'm going to try to set my opinions on Solarized aside for a moment. Okay. Here goes.

Out of the box, we ship a pretty shitty behavior.

If I look at all of the existing shipped color schemes--and that includes things like Tango and One Half--we are universally following a background == black rule.

If I consult gnome-terminal or xterm, they do the same thing; Xterm by default, gnome-terminal for solarized. The background generally matches color index 0 across all their dark schemes.
Konsole and lxterminal disagree and map background to 0 intense for Solarized.

I want to put our Solarized schemes on a deprecation path, but unfortunately we still need to ship something for users who we're going to strand on them.

I'm going to have to swallow my bitter and say that yes, we should probably just change the index mapping and go with something that works right out of the box while we figure out how to do perceptual color nudging and eventually remove bad defaults (like Campbell and Solarized D).

@M-Pixel would you mind making the same change for Solarized Light, such that black/brightBlack match Solarized Dark?

(with apologies to @j4james. James, I regret shipping this scheme. I'd like to make it as unsucky as possible for the month or two while users are transitioning off it onto their own schemes.)

@j4james
Copy link
Collaborator

j4james commented Jul 17, 2020

I really don't care what we set the colors too, as long as it reduces the bug reports, and I'm not sure this helps that much. My main concern is that we're going to be swapping one set of complaints for another. And while I don't mind telling people that don't like Solarized to just not use it, I don't know what to tell people that do actually like Solarized when they ask why we've deliberately broken their color scheme.

That said, if the real Solarized users are a small minority, then maybe this is the least worst option. Your metrics should be able to answer that question.

DHowett added a commit that referenced this pull request Jul 20, 2020
Original notes from @M-Pixel:

> Console applications assume that backgrounds are black, and that
> `lightBlack`/`DarkGrey` are lighter than `black`/`Black`.  This
> assumption is accounted for by all color schemes in `defaults.json`,
> except for the Solarized themes.
> 
> The Solarized Dark theme, in particular, makes `-Parameters` invisible
> against the background in PowerShell, which is obviously an unacceptable
> usability flaw.
> 
> This change makes `black` and `background` to the same (which is common
> throughout the color schemes), and makes `brightBlack` (`DarkGray` in
> .NET) lighter than black (which is obviously more correct given the
> meanings of those words).

Out of the box, we ship a pretty bad behavior.

If I look at all of the existing shipped color schemes--and that
includes things like Tango and One Half--we are universally following a
`background` == `black` rule.

If I consult gnome-terminal or xterm, they do the same thing; Xterm by
default, gnome-terminal for solarized. The background generally matches
color index `0` across all their **dark** schemes. Konsole and
lxterminal disagree and map background to `0 intense` for Solarized.

I want to put our Solarized schemes on a deprecation path, but
unfortunately we still need to ship _something_ for users who we're
going to strand on them.

I'm going to have to swallow my bitter and say that yes, we should
probably just change the index mapping and go with something that works
right out of the box while we figure out how to do perceptual color
nudging and eventually remove bad defaults (like Solarized).

From #6618.

Fixes #4047.
Closes #6618.
DHowett added a commit that referenced this pull request Jul 20, 2020
Original notes from @M-Pixel:

> Console applications assume that backgrounds are black, and that
> `lightBlack`/`DarkGrey` are lighter than `black`/`Black`.  This
> assumption is accounted for by all color schemes in `defaults.json`,
> except for the Solarized themes.
>
> The Solarized Dark theme, in particular, makes `-Parameters` invisible
> against the background in PowerShell, which is obviously an unacceptable
> usability flaw.
>
> This change makes `black` and `background` to the same (which is common
> throughout the color schemes), and makes `brightBlack` (`DarkGray` in
> .NET) lighter than black (which is obviously more correct given the
> meanings of those words).

Out of the box, we ship a pretty bad behavior.

If I look at all of the existing shipped color schemes--and that
includes things like Tango and One Half--we are universally following a
`background` == `black` rule.

If I consult gnome-terminal or xterm, they do the same thing; Xterm by
default, gnome-terminal for solarized. The background generally matches
color index `0` across all their **dark** schemes. Konsole and
lxterminal disagree and map background to `0 intense` for Solarized.

I want to put our Solarized schemes on a deprecation path, but
unfortunately we still need to ship _something_ for users who we're
going to strand on them.

I'm going to have to swallow my bitter and say that yes, we should
probably just change the index mapping and go with something that works
right out of the box while we figure out how to do perceptual color
nudging and eventually remove bad defaults (like Solarized).

From #6618.

Fixes #4047.
Closes #6618.

(cherry picked from commit 04f5ee7)
DHowett added a commit that referenced this pull request Jul 20, 2020
Original notes from @M-Pixel:

> Console applications assume that backgrounds are black, and that
> `lightBlack`/`DarkGrey` are lighter than `black`/`Black`.  This
> assumption is accounted for by all color schemes in `defaults.json`,
> except for the Solarized themes.
>
> The Solarized Dark theme, in particular, makes `-Parameters` invisible
> against the background in PowerShell, which is obviously an unacceptable
> usability flaw.
>
> This change makes `black` and `background` to the same (which is common
> throughout the color schemes), and makes `brightBlack` (`DarkGray` in
> .NET) lighter than black (which is obviously more correct given the
> meanings of those words).

Out of the box, we ship a pretty bad behavior.

If I look at all of the existing shipped color schemes--and that
includes things like Tango and One Half--we are universally following a
`background` == `black` rule.

If I consult gnome-terminal or xterm, they do the same thing; Xterm by
default, gnome-terminal for solarized. The background generally matches
color index `0` across all their **dark** schemes. Konsole and
lxterminal disagree and map background to `0 intense` for Solarized.

I want to put our Solarized schemes on a deprecation path, but
unfortunately we still need to ship _something_ for users who we're
going to strand on them.

I'm going to have to swallow my bitter and say that yes, we should
probably just change the index mapping and go with something that works
right out of the box while we figure out how to do perceptual color
nudging and eventually remove bad defaults (like Solarized).

From #6618.

Fixes #4047.
Closes #6618.

(cherry picked from commit 04f5ee7)
@ghost
Copy link

ghost commented Jul 21, 2020

🎉This issue was addressed in #6985, which has now been successfully released as Windows Terminal v1.1.2021.0.:tada:

Handy links:

1 similar comment
@ghost
Copy link

ghost commented Jul 22, 2020

🎉This issue was addressed in #6985, which has now been successfully released as Windows Terminal v1.1.2021.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 22, 2020

🎉This issue was addressed in #6985, which has now been successfully released as Windows Terminal Preview v1.2.2022.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Question For questions or discussion Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OperatorColor and ParameterColor are same color as background on Solarized Dark
3 participants