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

fish doesn't understand the color white #3176

Closed
elias6 opened this Issue Jun 28, 2016 · 21 comments

Comments

Projects
None yet
6 participants
@elias6

elias6 commented Jun 28, 2016

I would like the current directory in my fish prompt to be light gray, as it was in bash before I started using fish. I tried running set -U fish_color_cwd white (since white is the closest color to light gray that is listed in the set_color documentation) and it didn't do anything. The text was green after, just like it was before.

I then tried set -U fish_color_cwd red to see what happened and the text turned red, like I expected. Then I tried set -U fish_color_cwd white --bold and it became bold, but green again. It seems like it understands all of the colors I tried, but not white.

I am using fish 2.3.0 on OS X 10.11.5 and the built-in Terminal. Here is a screenshot demonstrating what is happening:
white not working in fish shell

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jun 28, 2016

Member

You've probably customized the text color in your Terminal.app profile configuration. One of the whites or text/bold text colors in here is going to show that it's been customized to green (other than the standard ANSI green/bright green):
screenshot 2016-06-27 at 5 47 56 pm

Member

floam commented Jun 28, 2016

You've probably customized the text color in your Terminal.app profile configuration. One of the whites or text/bold text colors in here is going to show that it's been customized to green (other than the standard ANSI green/bright green):
screenshot 2016-06-27 at 5 47 56 pm

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jun 28, 2016

Member

(Or, you are using Apple's "Homebrew" profile which has green for the normal text already)

edit: Actually, pretty sure you need to have changed one of the ANSI whites to green to make it happen like this to you.

Member

floam commented Jun 28, 2016

(Or, you are using Apple's "Homebrew" profile which has green for the normal text already)

edit: Actually, pretty sure you need to have changed one of the ANSI whites to green to make it happen like this to you.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jun 28, 2016

Member

@mikez302: To test if floam is right, try echo (set_color white) banana.

Also, you can pass RGB values to set_color and the color variables and fish will do the right thing (or try to).

Member

faho commented Jun 28, 2016

@mikez302: To test if floam is right, try echo (set_color white) banana.

Also, you can pass RGB values to set_color and the color variables and fish will do the right thing (or try to).

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jun 28, 2016

Member

Except, of course the "Last login" output there is not colored with ASCII escapes. For that part to be green I suspect it'd need to be the Homebrew theme or have this same configuration:
screenshot 2016-06-27 at 6 16 21 pm

Still not sure why setting those variables to white also causes green. It doesn't do that here if I copy that configuration. For those, I need ANSI white remapped.

Member

floam commented Jun 28, 2016

Except, of course the "Last login" output there is not colored with ASCII escapes. For that part to be green I suspect it'd need to be the Homebrew theme or have this same configuration:
screenshot 2016-06-27 at 6 16 21 pm

Still not sure why setting those variables to white also causes green. It doesn't do that here if I copy that configuration. For those, I need ANSI white remapped.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 28, 2016

Contributor

The important thing to keep in mind is that color names like "white" are simply mapped to the corresponding ANSI X3.64 SGR sequence. Those sequences map each color name to a number. The terminal is free to map the number to whatever color it likes and different terminals use different default colors. However, in your case I strongly suspect that @floam is correct and you've reconfigured your terminal so that "white" appears as bright green.

On my terminal (iTerm2 with the default color palette) I get the expected result:

image

Contributor

krader1961 commented Jun 28, 2016

The important thing to keep in mind is that color names like "white" are simply mapped to the corresponding ANSI X3.64 SGR sequence. Those sequences map each color name to a number. The terminal is free to map the number to whatever color it likes and different terminals use different default colors. However, in your case I strongly suspect that @floam is correct and you've reconfigured your terminal so that "white" appears as bright green.

On my terminal (iTerm2 with the default color palette) I get the expected result:

image

@elias6

This comment has been minimized.

Show comment
Hide comment
@elias6

elias6 Jul 2, 2016

This screenshot shows what my Terminal color settings look like:
terminal color settings

I am using the Homebrew profile. The "Text" and "Bold Text" colors are green. Everything else looks mostly like it does in @floam's screenshot.

I tried running echo (set_color white) banana and saw the word banana in green.

What should I do about this? It seems strange to me that fish would be unable to show white or light gray text in my terminal, if bash can do it.

elias6 commented Jul 2, 2016

This screenshot shows what my Terminal color settings look like:
terminal color settings

I am using the Homebrew profile. The "Text" and "Bold Text" colors are green. Everything else looks mostly like it does in @floam's screenshot.

I tried running echo (set_color white) banana and saw the word banana in green.

What should I do about this? It seems strange to me that fish would be unable to show white or light gray text in my terminal, if bash can do it.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 2, 2016

Member

Is $fish_term24bit set to 1 somehow?
> echo $fish_term24bit

Member

floam commented Jul 2, 2016

Is $fish_term24bit set to 1 somehow?
> echo $fish_term24bit

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 2, 2016

Member

(Terminal.app, with green set for the text color in the preferences, when getting 24bit color codes thrown at it which it can't handle, will end up with a greenish color when set_color tries to do #FFFFFF. Longshot...)

screenshot 2016-07-02 at 11 57 14 am

Member

floam commented Jul 2, 2016

(Terminal.app, with green set for the text color in the preferences, when getting 24bit color codes thrown at it which it can't handle, will end up with a greenish color when set_color tries to do #FFFFFF. Longshot...)

screenshot 2016-07-02 at 11 57 14 am

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 2, 2016

Contributor

I can't reproduce with or without setting fish_term24bit. The following screen shot is after I moved my custom fish_prompt.fish script out of the way so that I'm using the default prompt. What does echo $TERM report? I'm betting it's xterm. Why? Because if I set TERM to that value I can reproduce your problem. Note that xterm only supports the first 8 named colors and white isn't in that set but grey is.

image

Contributor

krader1961 commented Jul 2, 2016

I can't reproduce with or without setting fish_term24bit. The following screen shot is after I moved my custom fish_prompt.fish script out of the way so that I'm using the default prompt. What does echo $TERM report? I'm betting it's xterm. Why? Because if I set TERM to that value I can reproduce your problem. Note that xterm only supports the first 8 named colors and white isn't in that set but grey is.

image

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 2, 2016

Contributor

BTW, you should configure the terminal to identify itself as xterm-256color.

Contributor

krader1961 commented Jul 2, 2016

BTW, you should configure the terminal to identify itself as xterm-256color.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 2, 2016

Member

TERM=xterm with Terminal.app I thought goes through the code path that gives it 256 colors anyhow.

Member

floam commented Jul 2, 2016

TERM=xterm with Terminal.app I thought goes through the code path that gives it 256 colors anyhow.

@floam floam self-assigned this Jul 2, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 2, 2016

Member

OK, I had that backwards. Everywhere except for TERM_PROGRAM=Apple_Terminal we do this. The comment in input.cpp says it is for Snow Leopard. I guess we can remove that someday (or check TERM_PROGRAM_VERSION - this was added in Lion.)

Member

floam commented Jul 2, 2016

OK, I had that backwards. Everywhere except for TERM_PROGRAM=Apple_Terminal we do this. The comment in input.cpp says it is for Snow Leopard. I guess we can remove that someday (or check TERM_PROGRAM_VERSION - this was added in Lion.)

@floam floam removed their assignment Jul 4, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 6, 2016

Member

Note that xterm only supports the first 8 named colors and white isn't in that set but grey is.

color table

If you have 8 colors, these are what they are:

  • 0 black
  • 1 red
  • 2 green
  • 3 yellow
  • 4 blue
  • 5 magenta
  • 6 cyan
  • 7 white

... and 9..15 render the same as 7 does.

If you have a 16 color xterm the first half above still applies and the next 8 are bright variants of the above.

See https://github.com/fish-shell/fish-shell/blob/master/src/color.cpp#L226-L239:

rgb_color_t rgb_color_t::white() { return rgb_color_t(type_named, 7); }

static unsigned char term8_color_for_rgb(const unsigned char rgb[3]) {
    const uint32_t kColors[] = {
        0x000000,  // Black
        0xFF0000,  // Red
        0x00FF00,  // Green
        0xFFFF00,  // Yellow
        0x0000FF,  // Blue
        0xFF00FF,  // Magenta
        0x00FFFF,  // Cyan
        0xFFFFFF,  // White
    };
    return convert_color(rgb, kColors, sizeof kColors / sizeof *kColors);
}

White is color 7. He's got enough colors for white.

However scroll down just a bit to static const named_color_t. This does not agree with rgb_color_t::white() or term8_color_for_rgb.

Broken by this commit: 0a0acc8

When adding bright color support you can see normal-brightness white is now called grey. We made it so that what should be called bright white and prepended with a br like we do the other brights is simply white, and instead of being color 7, white is color 15 now.

There are a handful of things we could have done to make this work if we were going to get cute with the color names like this:

For some reason we do not try to find the closest matching supported color for term8 or term16? And for term8 we are not willing to give them color 15 even though it will render as white. Fish doesn't appear to be taking advantage of the the RGB values we have along with each of those named colors in that struct on term256. If we wanted to we could ensure they get good colors. Instead they get the same exact thing for purple and magenta and for yellow and orange.

Member

floam commented Jul 6, 2016

Note that xterm only supports the first 8 named colors and white isn't in that set but grey is.

color table

If you have 8 colors, these are what they are:

  • 0 black
  • 1 red
  • 2 green
  • 3 yellow
  • 4 blue
  • 5 magenta
  • 6 cyan
  • 7 white

... and 9..15 render the same as 7 does.

If you have a 16 color xterm the first half above still applies and the next 8 are bright variants of the above.

See https://github.com/fish-shell/fish-shell/blob/master/src/color.cpp#L226-L239:

rgb_color_t rgb_color_t::white() { return rgb_color_t(type_named, 7); }

static unsigned char term8_color_for_rgb(const unsigned char rgb[3]) {
    const uint32_t kColors[] = {
        0x000000,  // Black
        0xFF0000,  // Red
        0x00FF00,  // Green
        0xFFFF00,  // Yellow
        0x0000FF,  // Blue
        0xFF00FF,  // Magenta
        0x00FFFF,  // Cyan
        0xFFFFFF,  // White
    };
    return convert_color(rgb, kColors, sizeof kColors / sizeof *kColors);
}

White is color 7. He's got enough colors for white.

However scroll down just a bit to static const named_color_t. This does not agree with rgb_color_t::white() or term8_color_for_rgb.

Broken by this commit: 0a0acc8

When adding bright color support you can see normal-brightness white is now called grey. We made it so that what should be called bright white and prepended with a br like we do the other brights is simply white, and instead of being color 7, white is color 15 now.

There are a handful of things we could have done to make this work if we were going to get cute with the color names like this:

For some reason we do not try to find the closest matching supported color for term8 or term16? And for term8 we are not willing to give them color 15 even though it will render as white. Fish doesn't appear to be taking advantage of the the RGB values we have along with each of those named colors in that struct on term256. If we wanted to we could ensure they get good colors. Instead they get the same exact thing for purple and magenta and for yellow and orange.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 6, 2016

Contributor

See also issue #2951. Also, it's debatable that this is a bug. It's working as documented and this is the first complaint in the seven months since that change was merged.

Contributor

krader1961 commented Jul 6, 2016

See also issue #2951. Also, it's debatable that this is a bug. It's working as documented and this is the first complaint in the seven months since that change was merged.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 21, 2016

Member

Actually, this is worse than what I assumed, the change didn't cause set_color white not to do anything. Actually, it's sending out bogus escapes by making tparm do something it shouldn't.

env TERM=xterm fish -c 'builtin set_color white | xxd'
00000000: 1b5b 3331 356d                           .[315m

this is the first complaint in the seven months since that change was merged.

We can't really use a lack of squeaky wheels as validation of what is or isn't a bug. Consider that this whole time we've also been locking out grey for Linux VTs in this situation, while the unblocked white option causes the output above. That's obviously a confounding of factors that represents a bug that should cause someone some kind of issue but we haven't heard a pip.

Member

floam commented Jul 21, 2016

Actually, this is worse than what I assumed, the change didn't cause set_color white not to do anything. Actually, it's sending out bogus escapes by making tparm do something it shouldn't.

env TERM=xterm fish -c 'builtin set_color white | xxd'
00000000: 1b5b 3331 356d                           .[315m

this is the first complaint in the seven months since that change was merged.

We can't really use a lack of squeaky wheels as validation of what is or isn't a bug. Consider that this whole time we've also been locking out grey for Linux VTs in this situation, while the unblocked white option causes the output above. That's obviously a confounding of factors that represents a bug that should cause someone some kind of issue but we haven't heard a pip.

@floam floam self-assigned this Jul 21, 2016

floam added a commit to floam/fish-shell that referenced this issue Jul 21, 2016

Improve compatibility with 16-color terminals.
Fish assumed that you could use tparm() as long as the color is under 16
without even checking terminfo.

Now, we will always let tparm take care of a color if it can. It will
never use tparm if the terminal indicates a lack of support. When it
cannot safely take care of this, we construct the escape ourselves
Here, we no longer assume that in this case it is OK to produce a
256-color output. Add an intermediate formatter that can handle colors
0-16.

Fixes issue when tparm was pushed beyond its limits:
env TERM='linux' ./fish -c 'set_color --background brred | xxd'

Restore harmony to ANSI color names and remove "grey".

Fixes #3176

floam added a commit to floam/fish-shell that referenced this issue Jul 21, 2016

Improve compatibility with 16-color terminals.
Fish assumed that you could use tparm setaf/bg as long
as the color was under 16 without even checking terminfo.

Now, we will always let tparm take care of a color if it
can. It will never use tparm if the terminal indicates a lack
of support. When tparm cannot safely take care of this, we
construct the escape ourselves. Now, fish no longer makes
such a assumption, either, that it can use a fancy 256-color
format for colors 0-16 when tparm can't do it natively.
Add an intermediate formatter that can handle colors 0-16.

That fixes the issue when tparm was pushed beyond its limits:

env TERM='linux' ./fish -c 'set_color --background brred | xxd'

... and producing bogus escape output.

Restores harmony to white, black ANSI color names and removes "grey".

Fixes #3176

floam added a commit to floam/fish-shell that referenced this issue Jul 21, 2016

floam added a commit to floam/fish-shell that referenced this issue Jul 23, 2016

Improve compatibility with 16-color terminals.
Fish assumed that you could use tparm setaf/bg as long
as the color was under 16 without even checking terminfo.

Now, we will always let tparm take care of a color if it
can. It will never use tparm if the terminal indicates a lack
of support. When tparm cannot safely take care of this, we
construct the escape ourselves. Now, fish no longer makes
such a assumption, either, that it can use a fancy 256-color
format for colors 0-16 when tparm can't do it natively.
Add an intermediate formatter that can handle colors 0-16.

That fixes the issue when tparm was pushed beyond its limits:

env TERM='linux' ./fish -c 'set_color --background brred | xxd'

... and producing bogus escape output.

Restores harmony to white, black ANSI color names and removes "grey".

Fixes #3176

Move comments from output.h to output.cpp

floam added a commit to floam/fish-shell that referenced this issue Jul 23, 2016

Improve compatibility with 16-color terminals.
Fish assumed that you could use tparm setaf/bg as long
as the color was under 16 without even checking terminfo.

Now, we will always let tparm take care of a color if it
can. It will never use tparm if the terminal indicates a lack
of support. When tparm cannot safely take care of this, we
construct the escape ourselves. Now, fish no longer makes
such a assumption, either, that it can use a fancy 256-color
format for colors 0-16 when tparm can't do it natively.
Add an intermediate formatter that can handle colors 0-16.

That fixes the issue when tparm was pushed beyond its limits:

env TERM='linux' ./fish -c 'set_color --background brred | xxd'

... and producing bogus escape output.

Restores harmony to white, black ANSI color names and removes "grey".

Fixes #3176

Move comments from output.h to output.cpp

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 16-color terminals.
Fish assumed that you could use tparm setaf/bg as long
as the color was under 16 without even checking terminfo.

Now, we will always let tparm take care of a color if it
can. It will never use tparm if the terminal indicates a lack
of support. When tparm cannot safely take care of this, we
construct the escape ourselves. Now, fish no longer makes
such a assumption, either, that it can use a fancy 256-color
format for colors 0-16 when tparm can't do it natively.
Add an intermediate formatter that can handle colors 0-16.

That fixes the issue when tparm was pushed beyond its limits:

env TERM='linux' ./fish -c 'set_color --background brred | xxd'

... and producing bogus escape output.

Restores harmony to white, black ANSI color names and removes "grey".

Fixes #3176

Move comments from output.h to output.cpp

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 16-color terminals.
Fish assumed that you could use tparm setaf/bg as long
as the color was under 16 without even checking terminfo.

Now, we will always let tparm take care of a color if it
can. It will never use tparm if the terminal indicates a lack
of support. When tparm cannot safely take care of this, we
construct the escape ourselves. Now, fish no longer makes
such a assumption, either, that it can use a fancy 256-color
format for colors 0-16 when tparm can't do it natively.
Add an intermediate formatter that can handle colors 0-16.

That fixes the issue when tparm was pushed beyond its limits:

env TERM='linux' ./fish -c 'set_color --background brred | xxd'

... and producing bogus escape output.

Restores harmony to white, black ANSI color names and removes "grey".

Fixes #3176

Move comments from output.h to output.cpp

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 16-color terminals.
Fish assumed that you could use tparm setaf/bg as long
as the color was under 16 without even checking terminfo.

Now, we will always let tparm take care of a color if it
can. It will never use tparm if the terminal indicates a lack
of support. When tparm cannot safely take care of this, we
construct the escape ourselves. Now, fish no longer makes
such a assumption, either, that it can use a fancy 256-color
format for colors 0-16 when tparm can't do it natively.
Add an intermediate formatter that can handle colors 0-16.

That fixes the issue when tparm was pushed beyond its limits:

env TERM='linux' ./fish -c 'set_color --background brred | xxd'

... and producing bogus escape output.

Restores harmony to white, brwhite, brblack, black color names
Fixes #3176

Move comments from output.h to output.cpp

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 16-color terminals.
Fish assumed that you could use tparm setaf/bg as long
as the color was under 16 without even checking terminfo.

Now, we will always let tparm take care of a color if it
can. It will never use tparm if the terminal indicates a lack
of support. When tparm cannot safely take care of this, we
construct the escape ourselves. Now, fish no longer makes
such a assumption, either, that it can use a fancy 256-color
format for colors 0-16 when tparm can't do it natively.
Add an intermediate formatter that can handle colors 0-16.

That fixes the issue when tparm was pushed beyond its limits:

env TERM='linux' ./fish -c 'set_color --background brred | xxd'
... and producing bogus escape output.

Restores harmony to white, brwhite, brblack, black color names

Fixes #3176

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs

I think this should be taken care of internally.

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 16-color terminals.
Fish assumed that it could use could use tparm to set colors
as long as the color was under 16 or max_colors from terminfo
is 256 (term256_support_is_native):

        if (idx < 16 || term256_support_is_native()) {
                // Use tparm to emit color escape
                writembs(tparm(todo, idx);

If a terminal has max_colors = 8, here is what happenened, essentially::

        > env TERM=xterm tput setaf 7 | xxd
        00000000: 1b5b 3337 6d                             .[37m
        > env TERM=xterm tput setaf 8 | xxd
        00000000: 1b5b 3338 6d                             .[38m

That second escape is not valid. Colors 8 through 16 ought to be \e[90m.

So we replace the term256_support_is_native(), which just checked if
max_colors is 256 or not, with a function that takes an argument and
checks terminfo for that to see if tparm can handle it:

        if (term_supports_color_natively(idx) {

And if that's not true, the "forced" colors no longer use this format,
as this is not going to be compatible with low color terminals:

        else {
                char buff[16] = "";
                snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);

Add an intermediate formatter that can handle colors 0-16:
        else {
                // We are attempting to bypass the term here. Generate the ANSI escape sequence ourself.
                char buff[16] = "";
                if (idx < 16) {
                        snprintf(buff, sizeof buff, "\x1b[%dm", ((idx > 7) ? 82 : 30) + idx + !is_fg * 10);
                } else {
                        snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);
                }

Restores harmony to white, brwhite, brblack, black color names.
We don't want "white to refer to color color number 16, but the
traditional color #8.

Fixes #3176

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs.

Sync up our various incomplete color lists with bright hex values
being given for colors 0-8. Perplexing!

Using this table:

http://www.calmar.ws/vim/256-xterm-24bit-rgb-color-chart.html

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 16-color terminals.
Fish assumed that it could use could use tparm to set colors
as long as the color was under 16 or max_colors from terminfo
is 256 (term256_support_is_native):

        if (idx < 16 || term256_support_is_native()) {
                // Use tparm to emit color escape
                writembs(tparm(todo, idx);

If a terminal has max_colors = 8, here is what happenened, except it was
inside fish:

        > env TERM=xterm tput setaf 7 | xxd
        00000000: 1b5b 3337 6d                             .[37m
        > env TERM=xterm tput setaf 8 | xxd
        00000000: 1b5b 3338 6d                             .[38m

That second escape is not valid. Colors 8 through 16 ought to be \e[90m.
This is what caused "white" not to work in #3176 in Terminal.app, and
obviously isn't good for real low-color terminals either.

So we replace the term256_support_is_native(), which just checked if
max_colors is 256 or not, with a function that takes an argument and
checks terminfo for that to see if tparm can handle it. We only use this
test, because otherwise, tparm should be expected to output garbage:

        ...
        /// Returns true if we think tparm can handle outputting a color index
        static bool term_supports_color_natively(unsigned int c) { return max_colors >= c; }
        ...

        if (term_supports_color_natively(idx) {

And if that's not true, the "forced" colors no longer use the fancy
format for colors under 16, as this is not going to be compatible with
the low color terminals:

        else {
                char buff[16] = "";
                snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);

I added an intermediate formatter that can handle colors 0-16:

        else {
                // We are attempting to bypass the term here. Generate the ANSI escape sequence ourself.
                char buff[16] = "";
                if (idx < 16) {
                        snprintf(buff, sizeof buff, "\x1b[%dm", ((idx > 7) ? 82 : 30) + idx + !is_fg * 10);
                } else {
                        snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);
                }

Restores harmony to white, brwhite, brblack, black color names.
We don't want "white" to refer to color color #16, but to the
standard color #8. #16 is "brwhite".

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs.

Sync up our various incomplete color lists with bright hex values
being given for colors 0-8. Perplexing!

Using this table:

http://www.calmar.ws/vim/256-xterm-24bit-rgb-color-chart.html

Fixes #3176

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 0-16 color terminals.
Fish assumed that it could use could use tparm to set colors
as long as the color was under 16 or max_colors from terminfo
is 256 (term256_support_is_native):

        if (idx < 16 || term256_support_is_native()) {
                // Use tparm to emit color escape
                writembs(tparm(todo, idx);

If a terminal has max_colors = 8, here is what happenened, except it was
inside fish:

        > env TERM=xterm tput setaf 7 | xxd
        00000000: 1b5b 3337 6d                             .[37m
        > env TERM=xterm tput setaf 8 | xxd
        00000000: 1b5b 3338 6d                             .[38m

That second escape is not valid. Colors 8 through 16 ought to be \e[90m.
This is what caused "white" not to work in #3176 in Terminal.app, and
obviously isn't good for real low-color terminals either.

So we replace the term256_support_is_native(), which just checked if
max_colors is 256 or not, with a function that takes an argument and
checks terminfo for that to see if tparm can handle it. We only use this
test, because otherwise, tparm should be expected to output garbage:

        ...
        /// Returns true if we think tparm can handle outputting a color index
        static bool term_supports_color_natively(unsigned int c) { return max_colors >= c; }
        ...

        if (term_supports_color_natively(idx) {

And if that's not true, the "forced" colors no longer use the fancy
format for colors under 16, as this is not going to be compatible with
the low color terminals:

        else {
                char buff[16] = "";
                snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);

I added an intermediate formatter that can handle colors 0-16:

        else {
                // We are attempting to bypass the term here. Generate the ANSI escape sequence ourself.
                char buff[16] = "";
                if (idx < 16) {
                        snprintf(buff, sizeof buff, "\x1b[%dm", ((idx > 7) ? 82 : 30) + idx + !is_fg * 10);
                } else {
                        snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);
                }

Restores harmony to white, brwhite, brblack, black color names.
We don't want "white" to refer to color color #16, but to the
standard color #8. #16 is "brwhite".

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs.

Sync up our various incomplete color lists with bright hex values
being given for colors 0-8. Perplexing!

Using this table:

http://www.calmar.ws/vim/256-xterm-24bit-rgb-color-chart.html

Fixes #3176

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 0-16 color terminals.
Fish assumed that it could use could use tparm to set colors
as long as the color was under 16 or max_colors from terminfo
is 256 (term256_support_is_native):

        if (idx < 16 || term256_support_is_native()) {
                // Use tparm to emit color escape
                writembs(tparm(todo, idx);

If a terminal has max_colors = 8, here is what happenened, except it was
inside fish:

        > env TERM=xterm tput setaf 7 | xxd
        00000000: 1b5b 3337 6d                             .[37m
        > env TERM=xterm tput setaf 9 | xxd
        00000000: 1b5b 3338 6d                             .[39m

The first escape is good, that second escape is not valid.
Bright colors should start at \e[90m:

        > env TERM=xterm-16color tput setaf 9 | xxd
        00000000: 1b5b 3931 6d                             .[91m

This is what caused "white" not to work in #3176 in Terminal.app, and
obviously isn't good for real low-color terminals either.

So we replace the term256_support_is_native(), which just checked if
max_colors is 256 or not, with a function that takes an argument and
checks terminfo for that to see if tparm can handle it. We only use this
test, because otherwise, tparm should be expected to output garbage:

        ...
        /// Returns true if we think tparm can handle outputting a color index
        static bool term_supports_color_natively(unsigned int c) { return max_colors >= c; }
        ...

        if (term_supports_color_natively(idx) {

And if that's not true, the "forced" colors no longer use the fancy
format for colors under 16, as this is not going to be compatible with
the low color terminals:

        else {
                char buff[16] = "";
                snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);

I added an intermediate formatter that can handle colors 0-16:

        else {
                // We are attempting to bypass the term here. Generate the ANSI escape sequence ourself.
                char buff[16] = "";
                if (idx < 16) {
                        snprintf(buff, sizeof buff, "\x1b[%dm", ((idx > 7) ? 82 : 30) + idx + !is_fg * 10);
                } else {
                        snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);
                }

Restores harmony to white, brwhite, brblack, black color names.
We don't want "white" to refer to color color #16, but to the
standard color #8. #16 is "brwhite".

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs.

Sync up our various incomplete color lists with bright hex values
being given for colors 0-8. Perplexing!

Using this table:

http://www.calmar.ws/vim/256-xterm-24bit-rgb-color-chart.html

Fixes #3176

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 0-16 color terminals.
Fish assumed that it could use could use tparm to set colors
as long as the color was under 16 or max_colors from terminfo
is 256 (term256_support_is_native):

 if (idx < 16 || term256_support_is_native()) {
    // Use tparm to emit color escape
    writembs(tparm(todo, idx);

If a terminal has max_colors = 8, here is what happenened, except it was
inside fish:

 > env TERM=xterm tput setaf 7 | xxd
   00000000: 1b5b 3337 6d                             .[37m
 > env TERM=xterm tput setaf 9 | xxd
   00000000: 1b5b 3338 6d                             .[39m

The first escape is good, that second escape is not valid.
Bright colors should start at \e[90m:

 > env TERM=xterm-16color tput setaf 9 | xxd
   00000000: 1b5b 3931 6d                             .[91m

This is what caused "white" not to work in #3176 in Terminal.app, and
obviously isn't good for real low-color terminals either.

So we replace the term256_support_is_native(), which just checked if
max_colors is 256 or not, with a function that takes an argument and
checks terminfo for that to see if tparm can handle it. We only use this
test, because otherwise, tparm should be expected to output garbage:

 /// Returns true if we think tparm can handle outputting a color index
 static bool term_supports_color_natively(unsigned int c) { return max_colors >= c; }
...

 if (term_supports_color_natively(idx) {

And if terminfo can't do it, the "forced" colors no longer use the fancy
format, when handling  colors under 16, as this is not going to be compatible with
the low color terminals. This looks like an escape for 256-aware
terminals.

 else {
     char buff[16] = "";
     snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);

I added an intermediate formatter that can handle colors 0-16:

 else {
     // We are attempting to bypass the term here. Generate the ANSI escape sequence ourself.
     char buff[16] = "";
     if (idx < 16) {
         snprintf(buff, sizeof buff, "\x1b[%dm", ((idx > 7) ? 82 : 30) + idx + !is_fg * 10);
     } else {
         snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);
     }

Restores harmony to white, brwhite, brblack, black color names.
We don't want "white" to refer to color color #16, but to the
standard color #8. #16 is "brwhite".

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs.

Sync up our various incomplete color lists and fix all color values.
Colors 0-8 are assumed to be brights - e.g. red was FF0000. Perplexing!

Using this table:
 <http://www.calmar.ws/vim/256-xterm-24bit-rgb-color-chart.html>

Fixes #3176

floam added a commit to floam/fish-shell that referenced this issue Jul 24, 2016

Improve compatibility with 0-16 color terminals.
Fish assumed that it could use tparm to emit escapes to set colors
as long as the color was under 16 or max_colors from terminfo was 256::

 if (idx < 16 || term256_support_is_native()) {
    // Use tparm to emit color escape
    writembs(tparm(todo, idx);

If a terminal has max_colors = 8, here is what happenened, except
inside fish:

 > env TERM=xterm tput setaf 7 | xxd
   00000000: 1b5b 3337 6d                             .[37m
 > env TERM=xterm tput setaf 9 | xxd
   00000000: 1b5b 3338 6d                             .[39m

The first escape is good, that second escape is not valid.
Bright colors should start at \e[90m:

 > env TERM=xterm-16color tput setaf 9 | xxd
   00000000: 1b5b 3931 6d                             .[91m

This is what caused "white" not to work in #3176 in Terminal.app, and
obviously isn't good for real low-color terminals either.

So we replace the term256_support_is_native(), which just checked if
max_colors is 256 or not, with a function that takes an argument and
checks terminfo for that to see if tparm can handle it. We only use this
test, because otherwise, tparm should be expected to output garbage:

 /// Returns true if we think tparm can handle outputting a color index
 static bool term_supports_color_natively(unsigned int c) { return max_colors >= c; }
...

 if (term_supports_color_natively(idx) {

And if terminfo can't do it, the "forced" escapes no longer use the fancy
format when handling colors under 16, as this is not going to be compatible with
low color terminals. The code before used:

 else {
     char buff[16] = "";
     snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);

I added an intermediate format for colors 0-15:

 else {
     // We are attempting to bypass the term here. Generate the ANSI escape sequence ourself.
     char buff[16] = "";
     if (idx < 16) {
         snprintf(buff, sizeof buff, "\x1b[%dm", ((idx > 7) ? 82 : 30) + idx + !is_fg * 10);
     } else {
         snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);
     }

Restores harmony to white, brwhite, brblack, black color names.
We don't want "white" to refer to color color #16, but to the
standard color #8. #16 is "brwhite".

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs.

Sync up our various incomplete color lists and fix all color values.
Colors 0-8 are assumed to be brights - e.g. red was FF0000. Perplexing!

Using this table:
 <http://www.calmar.ws/vim/256-xterm-24bit-rgb-color-chart.html>

Fixes #3176

floam added a commit to floam/fish-shell that referenced this issue Jul 25, 2016

Improve compatibility with 0-16 color terminals.
Fish assumed that it could use tparm to emit escapes to set colors
as long as the color was under 16 or max_colors from terminfo was 256::

 if (idx < 16 || term256_support_is_native()) {
    // Use tparm to emit color escape
    writembs(tparm(todo, idx);

If a terminal has max_colors = 8, here is what happenened, except
inside fish:

 > env TERM=xterm tput setaf 7 | xxd
   00000000: 1b5b 3337 6d                             .[37m
 > env TERM=xterm tput setaf 9 | xxd
   00000000: 1b5b 3338 6d                             .[39m

The first escape is good, that second escape is not valid.
Bright colors should start at \e[90m:

 > env TERM=xterm-16color tput setaf 9 | xxd
   00000000: 1b5b 3931 6d                             .[91m

This is what caused "white" not to work in #3176 in Terminal.app, and
obviously isn't good for real low-color terminals either.

So we replace the term256_support_is_native(), which just checked if
max_colors is 256 or not, with a function that takes an argument and
checks terminfo for that to see if tparm can handle it. We only use this
test, because otherwise, tparm should be expected to output garbage:

 /// Returns true if we think tparm can handle outputting a color index
 static bool term_supports_color_natively(unsigned int c) { return max_colors >= c; }
...

 if (term_supports_color_natively(idx) {

And if terminfo can't do it, the "forced" escapes no longer use the fancy
format when handling colors under 16, as this is not going to be compatible with
low color terminals. The code before used:

 else {
     char buff[16] = "";
     snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);

I added an intermediate format for colors 0-15:

 else {
     // We are attempting to bypass the term here. Generate the ANSI escape sequence ourself.
     char buff[16] = "";
     if (idx < 16) {
         snprintf(buff, sizeof buff, "\x1b[%dm", ((idx > 7) ? 82 : 30) + idx + !is_fg * 10);
     } else {
         snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);
     }

Restores harmony to white, brwhite, brblack, black color names.
We don't want "white" to refer to color color #16, but to the
standard color #8. #16 is "brwhite".

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs.

Sync up our various incomplete color lists and fix all color values.
Colors 0-8 are assumed to be brights - e.g. red was FF0000. Perplexing!

Using this table:
 <http://www.calmar.ws/vim/256-xterm-24bit-rgb-color-chart.html>

Fixes #3176

floam added a commit to floam/fish-shell that referenced this issue Jul 26, 2016

Improve compatibility with 0-16 color terminals.
Fish assumed that it could use tparm to emit escapes to set colors
as long as the color was under 16 or max_colors from terminfo was 256.

Now only uses tparm if max_colors indicates support. Otherwise, use
improved ability to generate color escapes ourselves.

This is what caused "white" not to work in TERM=xterm on Terminal.app, and
obviously isn't good for real low-color terminals either.

Fixes #3176

Restores harmony to color white. Adds brwhite, brblack.

Nukes the config.fish set_color hack for linux VTs.

Fixes #2951
@davinkevin

This comment has been minimized.

Show comment
Hide comment
@davinkevin

davinkevin Aug 1, 2016

I've just updated fish and I've got the same problem...

I've switch to grey, but for me it's a regression.

I've just updated fish and I've got the same problem...

I've switch to grey, but for me it's a regression.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 1, 2016

Member

Wow, sorry about those mentions above the prior comment spamming this thread. Moral of the story: don't add the "Fixes: " line until right before you push to master.

Member

floam commented Aug 1, 2016

Wow, sorry about those mentions above the prior comment spamming this thread. Moral of the story: don't add the "Fixes: " line until right before you push to master.

@zanchey zanchey added this to the fish-future milestone Aug 2, 2016

@elias6

This comment has been minimized.

Show comment
Hide comment
@elias6

elias6 Aug 7, 2016

I finally got a chance to try these things. This is what I have found:

  • I ran echo $fish_term24bit and got an empty string in response.
  • I ran echo (set_color grey) foo and it showed in light grey.
  • I ran echo (set_color white) foo and it showed in green.
  • I ran echo (set_color ffffff) foo and it showed in the same shade of light grey.
  • I ran echo $TERM and it said "xterm".

I then went into my advanced settings and changed the "Declare terminal as" setting to "xterm-256color". Now, echo (set_color white) foo shows me "foo" in white.

Out of curiosity, why isn't the terminal declared as "xterm-256color" by default? Will changing this setting cause any problems?

elias6 commented Aug 7, 2016

I finally got a chance to try these things. This is what I have found:

  • I ran echo $fish_term24bit and got an empty string in response.
  • I ran echo (set_color grey) foo and it showed in light grey.
  • I ran echo (set_color white) foo and it showed in green.
  • I ran echo (set_color ffffff) foo and it showed in the same shade of light grey.
  • I ran echo $TERM and it said "xterm".

I then went into my advanced settings and changed the "Declare terminal as" setting to "xterm-256color". Now, echo (set_color white) foo shows me "foo" in white.

Out of curiosity, why isn't the terminal declared as "xterm-256color" by default? Will changing this setting cause any problems?

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 7, 2016

Member

On OS X it has defaulted to xterm-256color ever since xterm-256color was added to the list AFAIK. I can't imagine any issues caused.

Is it possible you have migrated along with you some old settings over from a few versions of OS X ago, as you have upgraded? You could test by reverting one of the profiles/"themes" in Terminal.app to default with the button it has and see what setting it gets.

Member

floam commented Aug 7, 2016

On OS X it has defaulted to xterm-256color ever since xterm-256color was added to the list AFAIK. I can't imagine any issues caused.

Is it possible you have migrated along with you some old settings over from a few versions of OS X ago, as you have upgraded? You could test by reverting one of the profiles/"themes" in Terminal.app to default with the button it has and see what setting it gets.

@elias6

This comment has been minimized.

Show comment
Hide comment
@elias6

elias6 Aug 8, 2016

@floam, to be honest, I don't care enough to spend my time trying to figure out why that happened. Maybe you are right about the default. I changed the setting to xterm-256color and it is working fine now. Feel free to close this issue if you don't think it should be open any more.

elias6 commented Aug 8, 2016

@floam, to be honest, I don't care enough to spend my time trying to figure out why that happened. Maybe you are right about the default. I changed the setting to xterm-256color and it is working fine now. Feel free to close this issue if you don't think it should be open any more.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 8, 2016

Member

No problem, just a suggestion if you were curious 😄

This should be left open - the bug you encountered is a regression in fish that I'm working on fixing in #3260. Your changing that there should get you 256 colors and sidestep this bug - but this is a problem that shouldn't occur even only with 8 colors supported. set_color white should basically always work.

Member

floam commented Aug 8, 2016

No problem, just a suggestion if you were curious 😄

This should be left open - the bug you encountered is a regression in fish that I'm working on fixing in #3260. Your changing that there should get you 256 colors and sidestep this bug - but this is a problem that shouldn't occur even only with 8 colors supported. set_color white should basically always work.

@floam floam modified the milestones: next-2.x, fish-future Aug 8, 2016

floam added a commit to floam/fish-shell that referenced this issue Sep 11, 2016

Improve compatibility with 0-16 color terminals.
Fish assumed that it could use tparm to emit escapes to set colors
as long as the color was under 16 or max_colors from terminfo was 256::

 if (idx < 16 || term256_support_is_native()) {
    // Use tparm to emit color escape
    writembs(tparm(todo, idx);

If a terminal has max_colors = 8, here is what happenened, except
inside fish:

 > env TERM=xterm tput setaf 7 | xxd
   00000000: 1b5b 3337 6d                             .[37m
 > env TERM=xterm tput setaf 9 | xxd
   00000000: 1b5b 3338 6d                             .[39m

The first escape is good, that second escape is not valid.
Bright colors should start at \e[90m:

 > env TERM=xterm-16color tput setaf 9 | xxd
   00000000: 1b5b 3931 6d                             .[91m

This is what caused "white" not to work in #3176 in Terminal.app, and
obviously isn't good for real low-color terminals either.

So we replace the term256_support_is_native(), which just checked if
max_colors is 256 or not, with a function that takes an argument and
checks terminfo for that to see if tparm can handle it. We only use this
test, because otherwise, tparm should be expected to output garbage:

 /// Returns true if we think tparm can handle outputting a color index
 static bool term_supports_color_natively(unsigned int c) { return max_colors >= c; }
...

 if (term_supports_color_natively(idx) {

And if terminfo can't do it, the "forced" escapes no longer use the fancy
format when handling colors under 16, as this is not going to be compatible with
low color terminals. The code before used:

 else {
     char buff[16] = "";
     snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);

I added an intermediate format for colors 0-15:

 else {
     // We are attempting to bypass the term here. Generate the ANSI escape sequence ourself.
     char buff[16] = "";
     if (idx < 16) {
         snprintf(buff, sizeof buff, "\x1b[%dm", ((idx > 7) ? 82 : 30) + idx + !is_fg * 10);
     } else {
         snprintf(buff, sizeof buff, "\x1b[%d;5;%dm", is_fg ? 38 : 48, idx);
     }

Restores harmony to white, brwhite, brblack, black color names.
We don't want "white" to refer to color color #16, but to the
standard color #8. #16 is "brwhite".

Move comments from output.h to output.cpp

Nuke the config.fish set_color hack for linux VTs.

Sync up our various incomplete color lists and fix all color values.
Colors 0-8 are assumed to be brights - e.g. red was FF0000. Perplexing!

Using this table:
 <http://www.calmar.ws/vim/256-xterm-24bit-rgb-color-chart.html>

Fixes #3176

@floam floam closed this in #3260 Sep 11, 2016

@zanchey zanchey modified the milestones: fish 2.4.0, next-2.x Sep 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment