Add extended color support #452

tiehuis · 2017-04-18T06:44:30Z

Fixes #261.

Adds support for 256 and rgb color codes. Format is simply comma separated values.
Mixes fine with other styles such as bold/nobold. I did write a brief thing about this in the issue but doesn't seem necessary after writing the documentation.
Windows falls back to white in the presence of one of these new color codes.
This currently uses a fixed buffer for formatting the color codes instead of the format! macro. Its more complicated and I'm not entirely sure the benefit but it can always be changed back if the extra allocations are favored over the more complex code.

tiehuis · 2017-04-18T06:46:09Z

termcolor/src/lib.rs

@@ -954,6 +954,58 @@ impl<W: io::Write> Ansi<W> {
                }
            }
        }
+        macro_rules! write_var_ansi_code {


Some eyes on this would be good if its preferred over format!. I've added some test cases here but could have missed something.

Adds 256-color and 24-bit truecolor support to ripgrep. This only provides output support on ansi terminals. Windows will fallback to white for any argument given.

tiehuis · 2017-04-18T10:46:00Z

@scottchiefbaker just highlighted a problem with the color parsing (no surprises there). I've just squashed the fix into this commit and made a quick test script which can be used to check if the parsing code matches visually.

https://gist.github.com/tiehuis/15f6a85c1b8fe691008c10b4d325a7c3

scottchiefbaker · 2017-04-18T17:08:57Z

I can confirm that I've tested this branch and it does everything I want for this feature. Colors are accurate and simple to specify. This pull-request gets my 👍 for merge. Good work @tiehuis

scottchiefbaker · 2017-04-20T03:38:13Z

@tiehuis I don't know enough (any) rust to speak the quality of this code, but I think the implementation and output are great.

What are your thoughts on this PR?

tiehuis · 2017-04-20T10:40:57Z

I've added my notes about this in the initial post. It's up to BurntSushi whether it gets merged or not and if any changes need to be made.

I know he has a lot of other things to tend to as well so I usually don't want to be too overbearing. He does a great job at managing his various projects so just give him a bit of time and this'll get looked at eventually.

scottchiefbaker · 2017-05-01T16:22:34Z

@BurntSushi any thoughts on this PR? It's working pretty well for me.

leafgarland · 2017-05-11T23:21:55Z

I just wanted to add that the Windows 10 console has supported 256 ansi colours for a while and since Creators Update supports 24 bit RGB.
https://msdn.microsoft.com/en-us/library/windows/desktop/mt638032(v=vs.85).aspx

Other commonly used terminal emulators such as mintty and ConEmu also support 24 bit colours.

Since ripgrep already has a flag for ansi colours, please don't just disable by default on Windows. Windows users can opt in/out depending on what they are using. As an aside, ripgrep is roughly twice as fast on windows with --color=ansi.

BurntSushi · 2017-05-11T23:37:05Z

Since ripgrep already has a flag for ansi colours, please don't just disable by default on Windows.

@leafgarland Can you please elaborate? What does your comment have to do specifically with this PR?

BurntSushi · 2017-05-11T23:37:40Z

@tiehuis Sorry I've been dragging my feet looking at this! Thanks so much for doing it! At first glance, this looks pretty good but I'll want to do a more thorough review before merging.

leafgarland · 2017-05-11T23:40:16Z

@BurntSushi I was commenting because the description mentioned that this PR was defaulting to white on Windows.

tiehuis · 2017-05-11T23:44:23Z

Can't have a long conversation right now but just had a quick look and I think this should handle ansi codes correctly on windows if --color=ansi is specified.

The only thing that should be updated potentially is the should_ansi function in termcolor which determines if the AnsiWriter is used instead of the Windows one. Not sure if this does a thorough enough check for a Windows 10 terminal. I think the description in the initial message would be better described as "Fall back to white on windows where ANSI is not supported (i.e. cmd.exe)".

Haven't tested at all on Windows however, so wanted a minimal change incorporating the functionality first. It should be simple to add support for Windows later if needed with no real trouble.

Would be great if you could try out the branch to confirm though!

BurntSushi · 2017-05-11T23:44:32Z

@leafgarland That's only when printing using the console APIs. It looks like this PR doesn't do anything special with the ANSI codes when those are used on Windows (which seems correct to me).

leafgarland · 2017-05-11T23:45:18Z

Ok, thanks. Sorry, I should have looked at the code!

BurntSushi · 2017-05-11T23:45:36Z

The only thing that should be updated potentially is the should_ansi function in termcolor which determines if the AnsiWriter is used instead of the Windows one. Not sure if this does a thorough enough check for a Windows 10 terminal

Yeah, this is potentially future work. Def out of scope for this PR. :-) AFAIK, you actually need to make an API call to enable ANSI support in Windows 10 terminals, so it requires some Windows knowledge.

weiznich · 2017-06-15T14:57:05Z

What is the current state of this pull request? What needs to be done to get it merged?
(I would like to implement a feature based on this in cargo)

BurntSushi · 2017-06-15T15:23:15Z

@weiznich It's waiting on me giving it a more careful review. I'll try to look soon.

scottchiefbaker · 2017-06-15T15:26:10Z

@weiznich I looked at your PR for Rust, and I'm a little confused. What feature does that implement that's related to this? Or would it be a new feature for ripgrep you're waiting to implement?

weiznich · 2017-06-15T15:33:09Z

@alexcrichton moved the coloring in cargo to termcolor with this commit. After that it is only possible to use the 8 predefined color codes. This may result in a colors that are barley readable(See my old PR for an example).
This PR introduces two possibilities to define custom colors. Based on this it should be possible to allow configuring the colors through an environment variable as proposed in my PR to cargo.

scottchiefbaker · 2017-08-22T21:10:50Z

@BurntSushi have you had a chance to review this at all?

I'd really like to see this landed.

scottchiefbaker · 2018-01-08T17:39:24Z

@BurntSushi any movement on this? I'd really like to see better colors in ripgrep.

BurntSushi · 2018-01-29T23:27:07Z

Thanks @tiehuis for this! I'm going to close this PR in favor of #766, which is based on your work here. I wanted to just get this merged so I did a few fixups.

tiehuis commented Apr 18, 2017

View reviewed changes

tiehuis force-pushed the issue-261 branch from 730fb61 to 5e8df49 Compare April 18, 2017 07:01

Add extended color support

88cb148

Adds 256-color and 24-bit truecolor support to ripgrep. This only provides output support on ansi terminals. Windows will fallback to white for any argument given.

tiehuis force-pushed the issue-261 branch from 5e8df49 to 88cb148 Compare April 18, 2017 10:41

weiznich mentioned this pull request Jun 15, 2017

Allow to configure colors using an environment variable rust-lang/cargo#4136

Closed

okdana mentioned this pull request Jul 1, 2017

Improve zsh completion function #536

Merged

jmcomets mentioned this pull request Jul 22, 2017

Add windows compatibility sharkdp/fd#29

Closed

sharkdp mentioned this pull request Oct 4, 2017

Proper support for Windows sharkdp/fd#70

Closed

3 tasks

sharkdp mentioned this pull request Oct 18, 2017

Colored output does not work on cmd.exe (Windows) sharkdp/fd#129

Closed

okdana mentioned this pull request Jan 18, 2018

[termcolor] Add gray colors #754

Closed

BurntSushi mentioned this pull request Jan 29, 2018

termcolor: add extended color support #766

Merged

BurntSushi closed this Jan 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extended color support #452

Add extended color support #452

tiehuis commented Apr 18, 2017

tiehuis Apr 18, 2017

tiehuis commented Apr 18, 2017 •

edited

scottchiefbaker commented Apr 18, 2017 •

edited

scottchiefbaker commented Apr 20, 2017

tiehuis commented Apr 20, 2017

scottchiefbaker commented May 1, 2017

leafgarland commented May 11, 2017

BurntSushi commented May 11, 2017

BurntSushi commented May 11, 2017

leafgarland commented May 11, 2017

tiehuis commented May 11, 2017 •

edited

BurntSushi commented May 11, 2017 •

edited

leafgarland commented May 11, 2017

BurntSushi commented May 11, 2017 •

edited

weiznich commented Jun 15, 2017

BurntSushi commented Jun 15, 2017

scottchiefbaker commented Jun 15, 2017

weiznich commented Jun 15, 2017

scottchiefbaker commented Aug 22, 2017 •

edited

scottchiefbaker commented Jan 8, 2018

BurntSushi commented Jan 29, 2018

Add extended color support #452

Add extended color support #452

Conversation

tiehuis commented Apr 18, 2017

tiehuis Apr 18, 2017

Choose a reason for hiding this comment

tiehuis commented Apr 18, 2017 • edited

scottchiefbaker commented Apr 18, 2017 • edited

scottchiefbaker commented Apr 20, 2017

tiehuis commented Apr 20, 2017

scottchiefbaker commented May 1, 2017

leafgarland commented May 11, 2017

BurntSushi commented May 11, 2017

BurntSushi commented May 11, 2017

leafgarland commented May 11, 2017

tiehuis commented May 11, 2017 • edited

BurntSushi commented May 11, 2017 • edited

leafgarland commented May 11, 2017

BurntSushi commented May 11, 2017 • edited

weiznich commented Jun 15, 2017

BurntSushi commented Jun 15, 2017

scottchiefbaker commented Jun 15, 2017

weiznich commented Jun 15, 2017

scottchiefbaker commented Aug 22, 2017 • edited

scottchiefbaker commented Jan 8, 2018

BurntSushi commented Jan 29, 2018

tiehuis commented Apr 18, 2017 •

edited

scottchiefbaker commented Apr 18, 2017 •

edited

tiehuis commented May 11, 2017 •

edited

BurntSushi commented May 11, 2017 •

edited

BurntSushi commented May 11, 2017 •

edited

scottchiefbaker commented Aug 22, 2017 •

edited