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

Solarized schemes shipped in Terminal Preview are incorrect #1509

Closed
TBBle opened this issue Jun 24, 2019 · 10 comments · Fixed by #1720
Closed

Solarized schemes shipped in Terminal Preview are incorrect #1509

TBBle opened this issue Jun 24, 2019 · 10 comments · Fixed by #1720
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Available It's available in an Insiders build or a release
Milestone

Comments

@TBBle
Copy link

TBBle commented Jun 24, 2019

Environment

Windows build number: Microsoft Windows [Version 10.0.18362.175]
Windows Terminal version: Windows Terminal (Preview) 0.2.1715.0

Steps to reproduce

Note: I already had PowerShell and CMD using Solarized Dark in ConHost, via cmd-colors-solarized.

  • Install Windows Terminal (Preview)
  • Hit Settings
  • In the JSON file, change the colorScheme for Powershell and CMD to Solarized Dark.

Expected behavior

The same colours as provided by cmd-colours-solarized, consistent with the main Solarized page screenshots

Actual behavior

The white is too bright. That stood out immediately, so I looked deeper at the pre-defined scheme.

Diagnosis

Comparing the default settings to upstream Solarized The Values and Usage & Development I noted the following differences.

Solarized: Role Tone HEX RGB Terminal: RGB HEX Tone
Dark Foreground base0 #839496 131_148_150 253_246_227 #FDF6E3 base3
Dark Background base03 #002b36 0 _43_54 7_54_66 #073642 base02
Light Foreground base00 #657b83 101_123_131 7_54_66 #073642 base02
Light Background base3 #fdf6e3 253_246_227 253_246_227 #FDF6E3 base3
Red red #dc322f 220_50_47 211_1_2 #D30102 Red (beta1)

The difference in Red is simply that the definition came from an older version of Solarized, and was changed in the 1.0.0beta2 release. This one shows up on the Internet a lot, if you search for #D30102.

The foreground/background differences are a little weird, my guess would be that a modified Light scheme was used (darker font text on the same background) and then some confusion or oversight caused Dark to be defined as 'Light with a swapped FG/BG', leading to an overall-higher brightness from the non-coloured text.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jun 24, 2019
@TBBle
Copy link
Author

TBBle commented Jun 24, 2019

A 👍 to the team for the "Live update on profile.json save" feature, that made this super-easy to diagnose/test.

@TBBle
Copy link
Author

TBBle commented Jun 24, 2019

There might be something else fundamentally-odd happening here. Using `Out-Colors.ps`` from cmd-colors-solarized in Terminal:
image
versus my existing PowerShell setup:
image
with the following profile:

        {
            "acrylicOpacity" : 0.5,
            "closeOnExit" : true,
            "colorScheme" : "Solarized Dark",
            "commandline" : "powershell.exe",
            "cursorColor" : "#839496",
            "cursorShape" : "bar",
            "fontFace" : "Fira Code Retina",
            "fontSize" : 12,
            "guid" : "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}",
            "historySize" : 9001,
            "icon" : "ms-appx:///ProfileIcons/{61c54bbd-c2c6-5271-96e7-009a87ff44bf}.png",
            "name" : "Windows PowerShell",
            "padding" : "0, 0, 0, 0",
            "snapOnInput" : true,
            "startingDirectory" : "%USERPROFILE%",
            "useAcrylic" : false
        },

and scheme

        {
            "background" : "#002B36",
            "black" : "#073642",
            "blue" : "#268BD2",
            "brightBlack" : "#002B36",
            "brightBlue" : "#839496",
            "brightCyan" : "#93A1A1",
            "brightGreen" : "#586E75",
            "brightPurple" : "#6C71C4",
            "brightRed" : "#CB4B16",
            "brightWhite" : "#FDF6E3",
            "brightYellow" : "#657B83",
            "cyan" : "#2AA198",
            "foreground" : "#839496",
            "green" : "#859900",
            "name" : "Solarized Dark",
            "purple" : "#D33682",
            "red" : "#DC322F",
            "white" : "#EEE8D5",
            "yellow" : "#B58900"
        },

It might be an issue in Out-Colors.ps1's name/value mapping, but I'm definitely seeing different PSReadLine colouring results and $HOST.UI.RawUI.ForegroundColor etc oddities too.

@DHowett-MSFT
Copy link
Contributor

There’s an outstanding design choice (or issue, depending on how you look at it) where terminal applications cannot emit colors 0 and 7 because they get converted into “background” and “foreground”. This is done for maximum compatibility with existing Windows console applications. It’ll probably be the cause of at least a couple solarized rendering bugs. #293

@TBBle
Copy link
Author

TBBle commented Jun 24, 2019

That would explain the difference in border for the output. The actual colour difference (dark/light mismatch mostly) is possibly due to the cmd-colours-solarized choice of mappings between ANSI/TERMCOL and PowerShell colours, which are clearly different than how Terminal maps its themes (defined in terms of TERMCOL).

@TBBle
Copy link
Author

TBBle commented Jun 24, 2019

Confirmed, with the following mapping (simply swapping bright and non-bright), the colours are the same as cmd-colours-solarized:

    {
      "background": "#002B36",
      "brightBlack": "#073642",
      "brightBlue": "#268BD2",
      "black": "#002B36",
      "blue": "#839496",
      "cyan": "#93A1A1",
      "green": "#586E75",
      "purple": "#6C71C4",
      "red": "#CB4B16",
      "white": "#FDF6E3",
      "yellow": "#657B83",
      "brightCyan": "#2AA198",
      "foreground": "#839496",
      "brightGreen": "#859900",
      "name": "Solarized Dark cmd-colors",
      "brightPurple": "#D33682",
      "brightRed": "#DC322F",
      "brightWhite": "#EEE8D5",
      "brightYellow": "#B58900"
    },

Confirmed with the following script, which also checks the ANSI codes.

# Check the mappings per https://github.com/neilpa/cmd-colors-solarized
# in the various ways they can be applied as foreground colours

$Pallet = [ordered]@{
	"base03" = [System.Tuple]::Create("0;30m", "Black")
	"base02" = [System.Tuple]::Create("1;30m", "DarkGray")
	"base01" = [System.Tuple]::Create("0;32m", "DarkGreen")
	"base00" = [System.Tuple]::Create("0;33m", "DarkYellow")
	"base0"  = [System.Tuple]::Create("0;34m", "DarkBlue")
	"base1"  = [System.Tuple]::Create("0;36m", "DarkCyan")
	"base2"  = [System.Tuple]::Create("0;37m", "Gray")
	"base3"  = [System.Tuple]::Create("1;37m", "White")
	"yellow" = [System.Tuple]::Create("1;33m", "Yellow")
	"orange" = [System.Tuple]::Create("0;31m", "DarkRed")
	"red"    = [System.Tuple]::Create("1;31m", "Red")
	"magenta"= [System.Tuple]::Create("1;35m", "Magenta")
	"violet" = [System.Tuple]::Create("0;35m", "DarkMagenta")
	"blue"   = [System.Tuple]::Create("1;34m", "Blue")
	"cyan"   = [System.Tuple]::Create("1;36m", "Cyan")
	"green"  = [System.Tuple]::Create("1;32m", "Green")
}

Write-Host ("{0,-15} | {1,-15} | {2,-15} |" -f "Color", "ANSI", "Write-Host")

$Pallet.keys | `
	ForEach-Object {
		$Color = $_
		$ANSI = $Pallette[$_].Item1
		$HostColour = $Pallette[$_].Item2

		Write-Host ("{0,-15}" -f $Color) -NoNewline
		Write-Host (" | ") -NoNewline
		Write-Host ("{0,-27}" -f "$([char]0x1b)[${ANSI}ESC[${ANSI}$([char]0x1b)[39m" ) -NoNewline
		Write-Host (" | ") -NoNewline
		Write-Host ("{0,-15}" -f $HostColour ) -ForegroundColor $HostColour -NoNewline
		Write-Host (" |")
	}

Write-Host

So really, the colour mismatch is probably a ticket to file against cmd-colors-solarized, to include the relevant pallet in its distribution. A quick look around, e.g. putty-colors-solarized, suggests that mappings for unix-hosted systems are probably consistent with the stock mappings in Terminal, not the cmd-colours-solarized mapping, at least for 8-colours-plus-bold setups.

So none of my comments here are really issues, just the original report's colour issues.

It's all very complex. Hopefully over time Terminal will help make this a bit simpler and more consistent.

@antoineco
Copy link
Contributor

antoineco commented Jun 25, 2019

I agree with the two main concerns expressed in the issue description:

  • red should be #DC322f
  • foreground should be #839496 (base0) for the Dark variant, #657b83 (base00) for the Light variant

as defined at https://github.com/altercation/solarized#the-values

Background colors are also off:

  • #002B36 is the correct Dark variant background (base03)
  • #FDF6E3 is the correct Light variant background (base3)

Not sure if that's related to #1509 (comment), but I don't think arbitrary foreground and background colors should be chosen.

The resulting scheme should be identical to this:

@DHowett-MSFT
Copy link
Contributor

We should fix this.

@DHowett-MSFT DHowett-MSFT added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 27, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 27, 2019
@DHowett-MSFT DHowett-MSFT added the Help Wanted We encourage anyone to jump in on these. label Jun 27, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Jun 27, 2019
@ghost ghost added the In-PR This issue has a related PR label Jun 29, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jul 2, 2019
@DHowett-MSFT DHowett-MSFT added Resolution-Fix-Available It's available in an Insiders build or a release and removed Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. labels Jul 3, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 3, 2019
@DHowett-MSFT
Copy link
Contributor

Thanks for the report! This was just submitted to the store with the v0.2.1831.0 servicing release. It may take some time for the store to process it.

@MorgusLethe
Copy link

Hi, thanks for all your work. Just to be clear, what exactly do I have to do to get Solarized Light to look good when I have Debian open? I can't seem to find a quick answer.

I expected that this will happen just by setting Solarized Light as the color scheme, but it does not look good to me, the folder names appear as badly readable lightblue on green background. Is this expected?
image

@antoineco
Copy link
Contributor

antoineco commented Jul 11, 2019

@MorgusLethe that's because the colored output of ls does not have Solarized-friendly colors by default. This can be changed using different "dircolors". Take a look at https://github.com/seebi/dircolors-solarized and pick the dircolors-ansi-light variant.

If you are new to Solarized, you will find out that a few extra terminal configurations like this one are sometimes necessary to display colors correctly (tmux, mutt, git, ...).

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 Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Available It's available in an Insiders build or a release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants