From 5cf2b6615a7aa3d5607139605b6a78bd9b16320f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 26 Dec 2022 23:23:19 -0800 Subject: [PATCH] formatter: use `crossterm` for colors Let's use `crossterm` to make `ColorFormatter` a little more readable, and maybe also more portable. This uses the `SetForegroundColor()` function, which uses the escapes for 256-color support (code 38) instead of the 8-color escapes (codes 30-37) combined with bold/bright (code 1) we were using before. IIUC, most terminals support the 16 base colors when using the 256-color escape even if they don't support all the 256 colors. It seems like an improvement to use actual color codes for the bright colors too, instead of assuming that terminals render bold as bright (even though most terminals do). Before this commit, we relied on ANSI escape 1 - which is specified to make the font bold - to make the color brighter. That's why we call the colors "bright blue" etc. When we switch from using code 30-37 to using 38 to let our color config just control the color (not using escape1), we therefore lose the bold font on many terminals (at least in iTerm2 and in the terminal application on my Debian work computer). As a workaround, I made us still use escape 1 when the bright colors are used. I'll make boldness a separately configurable attribute soon. Then we'll be able to remove this hack. With the switch to `crossterm`, we also reset just the foreground color (code 39) instead of resetting all attributes (code 0). That also seems like an improvement, probably making it easier for us to later support different background colors, underlining, etc. --- src/formatter.rs | 130 ++++++++++++++++++++-------------- tests/test_alias.rs | 2 +- tests/test_commit_template.rs | 24 +++---- tests/test_global_opts.rs | 12 ++-- tests/test_log_command.rs | 12 ++-- tests/test_resolve_command.rs | 8 +-- 6 files changed, 106 insertions(+), 82 deletions(-) diff --git a/src/formatter.rs b/src/formatter.rs index d6e35c8d3..abe5a6771 100644 --- a/src/formatter.rs +++ b/src/formatter.rs @@ -18,6 +18,9 @@ use std::io::{Error, Write}; use std::sync::Arc; use std::{fmt, io}; +use crossterm::queue; +use crossterm::style::{Attribute, Color, SetAttribute, SetForegroundColor}; + // Lets the caller label strings and translates the labels to colors pub trait Formatter: Write { fn write_bytes(&mut self, data: &[u8]) -> io::Result<()> { @@ -150,8 +153,8 @@ pub struct ColorFormatter { output: W, colors: Arc>, labels: Vec, - cached_colors: HashMap, Vec>, - current_color: Vec, + cached_colors: HashMap, Color>, + current_color: Color, } fn config_colors(config: &config::Config) -> HashMap { @@ -171,13 +174,13 @@ impl ColorFormatter { colors, labels: vec![], cached_colors: HashMap::new(), - current_color: b"\x1b[0m".to_vec(), + current_color: Color::Reset, } } - fn current_color(&mut self) -> Vec { + fn current_color(&mut self) -> Color { if let Some(cached) = self.cached_colors.get(&self.labels) { - cached.clone() + *cached } else { let mut best_match = (-1, ""); for (key, value) in self.colors.as_ref() { @@ -209,8 +212,7 @@ impl ColorFormatter { } let color = color_for_name(best_match.1); - self.cached_colors - .insert(self.labels.clone(), color.clone()); + self.cached_colors.insert(self.labels.clone(), color); color } } @@ -218,32 +220,55 @@ impl ColorFormatter { fn write_new_color(&mut self) -> io::Result<()> { let new_color = self.current_color(); if new_color != self.current_color { - self.output.write_all(&new_color)?; + // For now, make bright colors imply bold font. That better matches our + // behavior from when we used ANSI codes 30-37 plus an optional 1 for + // bold/bright (we now use code 38 for setting foreground color). + // TODO: Make boldness configurable separately from color + if !is_bright(&self.current_color) && is_bright(&new_color) { + queue!(self.output, SetAttribute(Attribute::Bold))?; + } else if !is_bright(&new_color) && is_bright(&self.current_color) { + queue!(self.output, SetAttribute(Attribute::Reset))?; + } + queue!(self.output, SetForegroundColor(new_color))?; self.current_color = new_color; } Ok(()) } } -fn color_for_name(color_name: &str) -> Vec { +fn is_bright(color: &Color) -> bool { + matches!( + color, + Color::DarkGrey + | Color::Red + | Color::Green + | Color::Yellow + | Color::Blue + | Color::Magenta + | Color::Cyan + | Color::White + ) +} + +fn color_for_name(color_name: &str) -> Color { match color_name { - "black" => b"\x1b[30m".to_vec(), - "red" => b"\x1b[31m".to_vec(), - "green" => b"\x1b[32m".to_vec(), - "yellow" => b"\x1b[33m".to_vec(), - "blue" => b"\x1b[34m".to_vec(), - "magenta" => b"\x1b[35m".to_vec(), - "cyan" => b"\x1b[36m".to_vec(), - "white" => b"\x1b[37m".to_vec(), - "bright black" => b"\x1b[1;30m".to_vec(), - "bright red" => b"\x1b[1;31m".to_vec(), - "bright green" => b"\x1b[1;32m".to_vec(), - "bright yellow" => b"\x1b[1;33m".to_vec(), - "bright blue" => b"\x1b[1;34m".to_vec(), - "bright magenta" => b"\x1b[1;35m".to_vec(), - "bright cyan" => b"\x1b[1;36m".to_vec(), - "bright white" => b"\x1b[1;37m".to_vec(), - _ => b"\x1b[0m".to_vec(), + "black" => Color::Black, + "red" => Color::DarkRed, + "green" => Color::DarkGreen, + "yellow" => Color::DarkYellow, + "blue" => Color::DarkBlue, + "magenta" => Color::DarkMagenta, + "cyan" => Color::DarkCyan, + "white" => Color::Grey, + "bright black" => Color::DarkGrey, + "bright red" => Color::Red, + "bright green" => Color::Green, + "bright yellow" => Color::Yellow, + "bright blue" => Color::Blue, + "bright magenta" => Color::Magenta, + "bright cyan" => Color::Cyan, + "bright white" => Color::White, + _ => Color::Reset, } } @@ -321,22 +346,22 @@ mod tests { formatter.write_str("\n").unwrap(); } insta::assert_snapshot!(String::from_utf8(output).unwrap(), @r###" -  black  -  red  -  green  -  yellow  -  blue  -  magenta  -  cyan  -  white  -  bright black  -  bright red  -  bright green  -  bright yellow  -  bright blue  -  bright magenta  -  bright cyan  -  bright white  +  black  +  red  +  green  +  yellow  +  blue  +  magenta  +  cyan  +  white  +  bright black  +  bright red  +  bright green  +  bright yellow  +  bright blue  +  bright magenta  +  bright cyan  +  bright white  "###); } @@ -356,7 +381,7 @@ mod tests { formatter.write_str(" inside ").unwrap(); formatter.remove_label().unwrap(); formatter.write_str(" after ").unwrap(); - insta::assert_snapshot!(String::from_utf8(output).unwrap(), @" before  inside  after "); + insta::assert_snapshot!(String::from_utf8(output).unwrap(), @" before  inside  after "); } #[test] @@ -378,7 +403,7 @@ mod tests { formatter.write_str("second").unwrap(); formatter.remove_label().unwrap(); formatter.write_str("after").unwrap(); - insta::assert_snapshot!(String::from_utf8(output).unwrap(), @"beforefirstsecondafter"); + insta::assert_snapshot!(String::from_utf8(output).unwrap(), @"beforefirstsecondafter"); } #[test] @@ -397,7 +422,7 @@ mod tests { .unwrap(); formatter.remove_label().unwrap(); // TODO: Replace the ANSI escape (\x1b) by something else (🌈?) - insta::assert_snapshot!(String::from_utf8(output).unwrap(), @"not actually bold"); + insta::assert_snapshot!(String::from_utf8(output).unwrap(), @"not actually bold"); } #[test] @@ -424,7 +449,7 @@ mod tests { formatter.remove_label().unwrap(); formatter.write_str(" after outer ").unwrap(); insta::assert_snapshot!(String::from_utf8(output).unwrap(), - @" before outer  before inner  inside inner  after inner  after outer "); + @" before outer  before inner  inside inner  after inner  after outer "); } #[test] @@ -445,7 +470,7 @@ mod tests { formatter.write_str(" not colored ").unwrap(); formatter.remove_label().unwrap(); insta::assert_snapshot!(String::from_utf8(output).unwrap(), - @" not colored  colored  not colored "); + @" not colored  colored  not colored "); } #[test] @@ -468,7 +493,7 @@ mod tests { formatter.remove_label().unwrap(); // TODO: Make this not reset the color inside insta::assert_snapshot!(String::from_utf8(output).unwrap(), - @" red before  still red inside  also red afterwards "); + @" red before  still red inside  also red afterwards "); } #[test] @@ -488,7 +513,7 @@ mod tests { formatter.remove_label().unwrap(); formatter.remove_label().unwrap(); insta::assert_snapshot!(String::from_utf8(output).unwrap(), - @" hello "); + @" hello "); } #[test] @@ -506,8 +531,7 @@ mod tests { formatter.write_str(" hello ").unwrap(); formatter.remove_label().unwrap(); formatter.remove_label().unwrap(); - insta::assert_snapshot!(String::from_utf8(output).unwrap(), - @" hello "); + insta::assert_snapshot!(String::from_utf8(output).unwrap(), @" hello "); } #[test] @@ -538,7 +562,7 @@ mod tests { formatter.write_str(" a2 ").unwrap(); formatter.remove_label().unwrap(); insta::assert_snapshot!(String::from_utf8(output).unwrap(), - @" a1  b1 c1  d  c2 b2  a2 "); + @" a1  b1 c1  d  c2 b2  a2 "); } #[test] @@ -567,6 +591,6 @@ mod tests { formatter.remove_label().unwrap(); // TODO: This is currently not deterministic. // insta::assert_snapshot!(String::from_utf8(output).unwrap(), - // @" a1  b1  c  b2  a2 "); + // @" a1  b1  c  b2  a2 "); } } diff --git a/tests/test_alias.rs b/tests/test_alias.rs index d8453391b..514203840 100644 --- a/tests/test_alias.rs +++ b/tests/test_alias.rs @@ -214,7 +214,7 @@ fn test_alias_global_args_in_definition() { // The global argument in the alias is respected let stdout = test_env.jj_cmd_success(&repo_path, &["l"]); insta::assert_snapshot!(stdout, @r###" - o 0000000000000000000000000000000000000000 + o 0000000000000000000000000000000000000000 "###); } diff --git a/tests/test_commit_template.rs b/tests/test_commit_template.rs index da248c965..509585ea1 100644 --- a/tests/test_commit_template.rs +++ b/tests/test_commit_template.rs @@ -75,22 +75,22 @@ fn test_log_default() { // Color let stdout = test_env.jj_cmd_success(&repo_path, &["log", "--color=always"]); insta::assert_snapshot!(stdout, @r###" - @ ffdaa62087a2 test.user@example.com 2001-02-03 04:05:09.000 +07:00 my-branch 9de54178d59d - | description 1 - o 9a45c67d3e96 test.user@example.com 2001-02-03 04:05:08.000 +07:00 4291e264ae97 + @ ffdaa62087a2 test.user@example.com 2001-02-03 04:05:09.000 +07:00 my-branch 9de54178d59d + | description 1 + o 9a45c67d3e96 test.user@example.com 2001-02-03 04:05:08.000 +07:00 4291e264ae97 | add a file - o 000000000000  1970-01-01 00:00:00.000 +00:00 000000000000 + o 000000000000  1970-01-01 00:00:00.000 +00:00 000000000000 (no description set) "###); // Color without graph let stdout = test_env.jj_cmd_success(&repo_path, &["log", "--color=always", "--no-graph"]); insta::assert_snapshot!(stdout, @r###" - ffdaa62087a2 test.user@example.com 2001-02-03 04:05:09.000 +07:00 my-branch 9de54178d59d - description 1 - 9a45c67d3e96 test.user@example.com 2001-02-03 04:05:08.000 +07:00 4291e264ae97 + ffdaa62087a2 test.user@example.com 2001-02-03 04:05:09.000 +07:00 my-branch 9de54178d59d + description 1 + 9a45c67d3e96 test.user@example.com 2001-02-03 04:05:08.000 +07:00 4291e264ae97 add a file - 000000000000  1970-01-01 00:00:00.000 +00:00 000000000000 + 000000000000  1970-01-01 00:00:00.000 +00:00 000000000000 (no description set) "###); } @@ -131,11 +131,11 @@ fn test_log_default_divergence() { // Color let stdout = test_env.jj_cmd_success(&repo_path, &["log", "--color=always"]); insta::assert_snapshot!(stdout, @r###" - o 9a45c67d3e96?? test.user@example.com 2001-02-03 04:05:10.000 +07:00 8979953d4c67 + o 9a45c67d3e96?? test.user@example.com 2001-02-03 04:05:10.000 +07:00 8979953d4c67 | description 2 - | @ 9a45c67d3e96?? test.user@example.com 2001-02-03 04:05:08.000 +07:00 7a17d52e633c - |/ description 1 - o 000000000000  1970-01-01 00:00:00.000 +00:00 000000000000 + | @ 9a45c67d3e96?? test.user@example.com 2001-02-03 04:05:08.000 +07:00 7a17d52e633c + |/ description 1 + o 000000000000  1970-01-01 00:00:00.000 +00:00 000000000000 (no description set) "###); } diff --git a/tests/test_global_opts.rs b/tests/test_global_opts.rs index 95da4e100..58755bde0 100644 --- a/tests/test_global_opts.rs +++ b/tests/test_global_opts.rs @@ -182,8 +182,8 @@ fn test_color_config() { // Test that --color=always is respected. let stdout = test_env.jj_cmd_success(&repo_path, &["--color=always", "log", "-T", "commit_id"]); insta::assert_snapshot!(stdout, @r###" - @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22 - o 0000000000000000000000000000000000000000 + @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22 + o 0000000000000000000000000000000000000000 "###); // Test that color is used if it's requested in the config file @@ -193,8 +193,8 @@ color="always""#, ); let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-T", "commit_id"]); insta::assert_snapshot!(stdout, @r###" - @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22 - o 0000000000000000000000000000000000000000 + @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22 + o 0000000000000000000000000000000000000000 "###); // Test that --color=never overrides the config. @@ -249,8 +249,8 @@ color="always""#, test_env.add_env_var("NO_COLOR", ""); let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-T", "commit_id"]); insta::assert_snapshot!(stdout, @r###" - @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22 - o 0000000000000000000000000000000000000000 + @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22 + o 0000000000000000000000000000000000000000 "###); // Test that per-repo config overrides the user config. diff --git a/tests/test_log_command.rs b/tests/test_log_command.rs index 4248f78c6..b6d33ca27 100644 --- a/tests/test_log_command.rs +++ b/tests/test_log_command.rs @@ -573,13 +573,13 @@ fn test_graph_template_color() { // TODO: The color codes shouldn't span the graph lines, and we shouldn't get an // extra line at the end insta::assert_snapshot!(stdout, @r###" - @ single line - |  - o first line + @ single line + |  + o first line | second line | third line - |  - o (no description set) -  + |  + o (no description set) +  "###); } diff --git a/tests/test_resolve_command.rs b/tests/test_resolve_command.rs index 2f9e81dea..8420db282 100644 --- a/tests/test_resolve_command.rs +++ b/tests/test_resolve_command.rs @@ -386,7 +386,7 @@ fn test_too_many_parents() { // Test warning color insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list", "--color=always"]), @r###" - file 3-sided conflict + file 3-sided conflict "###); let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]); @@ -518,7 +518,7 @@ fn test_description_with_dir_and_deletion() { // Test warning color. The deletion is fine, so it's not highlighted insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list", "--color=always"]), @r###" - file 3-sided conflict including 1 deletion and a directory + file 3-sided conflict including 1 deletion and a directory "###); let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]); insta::assert_snapshot!(error, @r###" @@ -618,8 +618,8 @@ fn test_multiple_conflicts() { // Test colors insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list", "--color=always"]), @r###" - another_file 2-sided conflict - this_file_has_a_very_long_name_to_test_padding 2-sided conflict + another_file 2-sided conflict + this_file_has_a_very_long_name_to_test_padding 2-sided conflict "###); let editor_script = test_env.set_up_fake_editor();