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

set_color updates for bright colors, fixes for low/no-color terminals #3260

Merged
merged 6 commits into from Sep 11, 2016

Conversation

@floam
Copy link
Member

@floam floam commented Jul 23, 2016

Description

In #3176 I noticed that the reason the addition of bright colors was also causing issues with TERM=xterm (on modern terminals), was that fish would generate invalid ANSI:
We assumed that it we could use tparm to emit color escapes 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);      

But if a terminal has max_colors (terminfo) of, say, 8, here is what will occur as it is asked for a number higher than that (except tparm did the same thing in output.cpp):

> 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. (On real 8-color terminals the bigger issue was that white should cause color 7, not color 15, addressed below)

So we replace the < 16 || term256_support_is_native(), with the latter function just checking if max_colors is 256 or not, with a function that takes an int and checks terminfo for the. colors supported by the terminal (really, we care that tparm can be expected not 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) {
    // Use tparm to emit color escape.
    writembs(tparm(todo, idx));

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);                                                                                             
     }

The PR also fixes the definition of color white, fixes color values for all colors. The non-brights were defined with the bright values and brights had something invented by fish. e.g. FF0000 for red (instead of 800000) and FF5555 for brred (instead of FF0000).

I got rid of the hack in config.fish. For #2951, one thing we should do is decide if we really want to send ANSI at terminals with max_colors <= 8, if no, solve it in C++?

Fixes #3176, #2951

TODOs:

  • Changes to fish usage are reflected in user documenation/manpages.
  • Tests have been added for regressions fixed
return str
str = str.toLowerCase();
if (str == 'black') return '000000';
if (str == 'red') return '800000';
Copy link
Member

@faho faho Jul 23, 2016

Wouldn't it be nice here to be consistent with other programs, particularly terminals?

From wikipedia, it seems X will use F00 for red, but 008000 for green and 00F for blue, like we currently do. Xterm is a bit weirder.

Copy link
Member Author

@floam floam Jul 23, 2016

There are good arguments for matching another terminal or some tangible standard. However if we might ever do something like let set_color red apply a 24bit color code instead of one of these 16 colors, I think there are better arguments for very carefully picking out a great set of preferred colors that work well on both dark and light terminals to make the color contrasts a bit easier on the eyes.

In the case above though, I was just going by this table, except for the colors which I left in for the sake of compatibility at their prior values that do not actually exist as named colors in ANSI (brown, purple, grey). I'm happy with using any values we can agree on.

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

Copy link
Member Author

@floam floam Jul 23, 2016

Huh, those few colors in the bright column for X are surprising (in a good way.) They're actually quite near what I end up using in my terminal configurations instead of full blast green and blue.

@faho
Copy link
Member

@faho faho commented Jul 23, 2016

The documentation changes are a definite ACK, and I think I'm okay with white->grey. I'd prefer it if we still accepted "grey" as an alternative name - set_color erroring out isn't a great experience.

I'll have to test this a bit, particularly in a VT.

@faho faho added this to the next-2.x milestone Jul 23, 2016
@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

ACK?

@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

prefer it if we still accepted "grey" as an alternative name - set_color erroring out isn't a great experience.

We do. What I decided to do was remove them from the documentation and hide them from the color list but make sure set_color does what the scripts using those values intend.

@faho
Copy link
Member

@faho faho commented Jul 23, 2016

ACK?

http://www.catb.org/jargon/html/A/ACK.html, meaning 4. You really need to grow your beard 😃.

Okay, in a VT this seems to "just work" - set_color maps the argument to the 8+8 normal/bright colors, and everytime it hits a bright one the VT just ignores it.

In a monochrome TERM=vt100, this hits #2951 in full force.

We do. What I decided to do was remove them from the documentation and color list but make sure set_color does what the scripts using those values intend.

Sorry, I should have tried before. I'm not sure if we shouldn't stick a note in the docs - "For compatibility, 'grey' is accepted as a synonym for white". Not having it in the list is probably right, though.

@@ -37,15 +37,20 @@ static int (*out)(char c) = writeb_internal;
/// Whether term256 and term24bit are supported.
static color_support_t color_support = 0;


/// Set the function used for writing in move_cursor, writespace and set_color and all other output
Copy link
Member

@faho faho Jul 23, 2016

What's up with the three "/" here? I see they've been here before, but why?

Copy link
Member Author

@floam floam Jul 23, 2016

These are HeaderDoc comments.

@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

What result does the monochrome terminal give for tput colors? I think if we document grey we should also document the other two aliases - brown and purple.

@faho
Copy link
Member

@faho faho commented Jul 23, 2016

What result does the monochrome terminal give for tput colors?

"-1".

In what way? The wrapper script is nuked from config.fish, so I know not the error quoted on that issue, what exactly is the terminal receiving for escapes and exhibiting?

It prints nothing, which is annoying in combination with the cartesian product - echo (set_color red)rose would not print anything. Maybe set_color should at least print a newline, or am I missing something?

(Also I've noticed that, when I give set_color an RGB value, $fish_term24bit (set via config.fish because I have $KONSOLE_PROFILE_NAME set) takes precedence and it'll print the RGB sequence)

@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

Hm, set_color probably shouldn't print a newline, any character that isn't whitespace? What would be a good one that would effectively be a NOP and not take a column or cause a linebreak on a vt100?

@faho
Copy link
Member

@faho faho commented Jul 23, 2016

Hm, set_color probably shouldn't print a newline.

Why not?

But I think any character that would effectively be a NOP and not take a column or cause a linebreak on a vt100 should suffice.

Which do you have in mind? I'm leaning towards a '\n' because that's the most-used character at the end of a programs output, so it seems to be the least likely to cause issues. Also it'd work (by being eliminated) in a command substitution, which is how set_color is used the most.

Compare:

echo (printf '\n')banana | od -t x1z
echo (printf '\r')banana | od -t x1z

And non-ASCII characters are right out here (haven't seen a vt100 with emoji support yet).

@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

Isn't that going to... add a spurious linebreak scripts were not expecting when there isn't the cartesian product thing happening?

@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

I was thinking something like an escape sequence that does nothing.

@faho
Copy link
Member

@faho faho commented Jul 23, 2016

Isn't that going to... add a spurious linebreak scripts were not expecting when there isn't the cartesian product thing happening?

You're right. I was under the impression we'd reset the terminal mode between commands, but we only do that for the prompt (which is why running set_color red interactively has no effect).

I was thinking something like an escape sequence that does nothing.

Maybe the "reset" sequence (\e[0m)? I mean in a command substitution that'd be captured, but that's okay since you were expecting something to be captured anyway (e.g. checking for string-equivalency on a variable that includes a color sequence is usually a dumb idea).

@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

I think that fixed it.

@@ -299,26 +299,6 @@ function __fish_config_interactive -d "Initializations that should be performed
set -g fish_pager_color_description yellow
set -g fish_pager_color_progress cyan

Copy link
Member

@ridiculousfish ridiculousfish Jul 23, 2016

Do I understand correctly that what this deleted function used to do (per commit f71e877 ) is now handled by the builtin set_color? That seems good if so.

Copy link
Member Author

@floam floam Jul 23, 2016

Not exactly. It does not mask off certain arguments for TERM=linux (or TERM=vt100), but the goal is it will work on a VT by not throwing escapes at it that are problematic. It silently might just do something other than try to set the color they asked for.

@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

OK, I think 6d12fbe should stop causing the empty output with echo (set_color red)foo - I hope there aren't side effects.

@floam
Copy link
Member Author

@floam floam commented Jul 23, 2016

(Also I've noticed that, when I give set_color an RGB value, $fish_term24bit (set via config.fish because I have $KONSOLE_PROFILE_NAME set) takes precedence and it'll print the RGB sequence)

Is that OK? (Because that's only set when you're actually in Konsole?)

@floam floam changed the title Bright colors: fix naming and bugs on terminals with 8-16 colors. set_color updates for bright colors, fixes for low/no-color terminals Jul 23, 2016
@floam floam force-pushed the colorfix branch 3 times, most recently from 0115a6a to ecb40a4 Jul 23, 2016
floam pushed a commit that referenced this issue Jul 24, 2016
.... should make the #3260 diff shorter.
@floam floam force-pushed the colorfix branch 3 times, most recently from 20e7f08 to 7276eff Jul 24, 2016
@floam floam added this to the fish 2.4.0 milestone Sep 11, 2016
@floam floam removed this from the next-2.x milestone Sep 11, 2016
@floam floam self-assigned this Sep 11, 2016
@floam floam force-pushed the colorfix branch 5 times, most recently from e360c0b to 464db16 Sep 11, 2016
@floam
Copy link
Member Author

@floam floam commented Sep 11, 2016

Ok, merging this. Hopefully we can shake out any bugs prior to release. The final TODO I'm hammering on to be committed later, brights for "8" color terminals.

What we want to do for max_colors == 8 terminals like linux is if 8 < idx < 16, we enter bold mode at > a higher level than where most of the work was so far, such that was_bold is correct (else we'll lose > track of being in bold mode), and then do a color escape for idx - 8.

@floam floam force-pushed the colorfix branch 9 times, most recently from 4d50178 to 63046c6 Sep 11, 2016
Adds a color reset thing, to ensure fish tries to use hard colors during
testing.

Also, work on a discrepancy (not introduced by my changes, afaik) when
with some combinations of color settings, and usage of --bold, caused super
flakey color paninting in the pager. Downwards movements that trigger
scrolling vs. upwards movement in the pager would only apply bold to
selections when moving upwards. The bold state of the command completions in
the pager was flipping flops on and off, depending on if there is a description
on the preceding line.

Implement a lame fix by reseting the color to normal and applying a
different style on the rightmost ')' which seems to be what was influencing it.

Makes fish use terminfo for coloring the newline glich char.
@floam floam merged commit f7c6426 into fish-shell:master Sep 11, 2016
1 check passed
EMayej
Copy link
Contributor

EMayej commented on 90e535f Sep 12, 2016

command substitution does not happen. Maybe you don't want surrounding quotes.

echo "$HOME/fish_previous_colors-(date).txt"
/Users/EMayej/fish_previous_colors-(date).txt

echo $HOME/fish_previous_colors-(date).txt
/Users/EMayej/fish_previous_colors-Mon Sep 12 13:14:07 CST 2016.txt

echo $HOME/fish_previous_colors-(date '+%Y%m%d%H%M%S').txt /Users/EMayej/fish_previous_colors-20160912131557.txt

faho
Copy link
Member

faho commented on 90e535f Sep 12, 2016

You're correct of course, though this doesn't really matter - this is just to set colors again so we can exercise the recent changes a bit (#3260).

It should be removed before a release.

floam
Copy link
Member

floam commented on 90e535f Sep 13, 2016

doh.

floam
Copy link
Member

floam commented on 90e535f Sep 13, 2016

And yeah, we won't be doing cat /dev/zero > $fish_color* on release. Still worth doing as intended until then.

I do want to force any settings using deprecated color-names to reset on release, however. I'm not sure it's necessary to be noisy/worry about backups. If we do pick some new defaults anywhere we really want for people to receive (perhaps for the pager, if we can fix the bolding bug), we can just quietly force those specific colors to reset.)

set -U fish_greeting $line1\n$line2
#
if not set -q __fish_init_2_39_8 # bump this to 2_4_0 when rolling release if anything changes after 9/10/2016
set -g colors_backup "$HOME/fish_previous_colors-(date).txt"

This does not properly insert the date. The file is literally named fish_previous_colors-\(date\).txt. I don't think this was indented, was it?

Copy link
Contributor

@krader1961 krader1961 Sep 13, 2016

Not only that but even if we supported command substitution inside quoted strings it is generally a bad idea to use the user locale for formatting the date in this type of context. It's almost always a better to use a fixed format such as ISO 8601. I'll take care of it.

Copy link
Member Author

@floam floam Sep 13, 2016

Why? it's just intended to be manually noticed by a developer with ls and not clobber old backups (hmm, I forgot noclobber and it bit me!) - not automatically restored or anything. Don't invest too much time in it.

Copy link
Member Author

@floam floam Sep 13, 2016

Could also just put (random) in there. The file birth date will keep the needed information.

Copy link
Member Author

@floam floam Sep 13, 2016

Thanks for fixing it.

@floam
Copy link
Member Author

@floam floam commented Sep 13, 2016

I do want to force any settings using deprecated color-names to reset on release, however.

What we should probably just do is keep an init routine that will run on upgrade to 2.4.0, but what it does instead of "normalizing" colors (sorry, was a joke) by deleting them is replace previous names it encounters with the correct names.

grey -> white
brgrey -> brblack
brown -> yellow
white -> brwhite
purple -> magenta

That should prevent us from "teaching" deprecated color names through our defaults, and fix it so that colors display like how they did before this patch.

@floam
Copy link
Member Author

@floam floam commented Sep 13, 2016

That should prevent us from "teaching" deprecated color names through our defaults, and fix it so that colors display like how they did before this patch.

Actually, I should check: did we adjust the defaults after doing our "white now refers to bright white/FF FF FF, normal white is called 'grey' now" change which this PR walked back? If no, then we could just leave those as is and they'll return to the behavior as designed finally.

edit: Looks like no

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants