[2/3] color.c: Support bright aixterm colors
diff mbox series

Message ID 20200118145318.5177-2-shawarmakarma@gmail.com
State New
Headers show
Series
  • [1/3] color.c: Refactor color_output to use enums
Related show

Commit Message

Eyal Soha Jan. 18, 2020, 2:53 p.m. UTC
These colors are the bright variants of the 3-bit colors.

Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
---
 Documentation/config.txt |  4 +++-
 color.c                  | 34 +++++++++++++++++++++++++++-------
 t/t4026-color.sh         |  8 ++++++++
 3 files changed, 38 insertions(+), 8 deletions(-)

Comments

Junio C Hamano Jan. 18, 2020, 6:47 p.m. UTC | #1
Eyal Soha <shawarmakarma@gmail.com> writes:

> These colors are the bright variants of the 3-bit colors.

OK, so this round the design is to reuse the ANSI mode instead of
introducing a new AIX mode that sits next to ANSI, 256 and RGB?

For this to work, not just the 90-97 range for bright-ansi orders
the colors the same way as 30-37 range (only brighter), but also
the differences between corresponding fore- and background colors
must also be 10 just like the regular ANSI colors.

So perhaps an additional sentence or two deserve to be there, e.g.

	... of the 3-bit colors.  Instead of 30-37 range for the
	foreground and 40-47 range for the background, they live in
	90-97 and 100-107 range, respectively.

or something like that, perhaps?

>  The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`,
>  `blue`, `magenta`, `cyan` and `white`.  The first color given is the
> -foreground; the second is the background.
> +foreground; the second is the background.  All the basic colors except
> +`normal` have a bright variant that can be speficied by prefixing the
> +color with `bright`, like `brightred`.

Nicely and readably written.

I have to wonder if spelling "bright<color>", i.e. two words smashed
together without anything in between words, is in widespread use (in
other words, are we following an established practice, or are we
inventing our own), or if we need to prepare for synonyms?  HTML/CSS
folks seem to use words-smashed-without-anything-in-betwen so they
should be fine with this design; I no longer recall what X did ;-)

Looking good.  Thanks.
Eyal Soha Jan. 21, 2020, 4:52 p.m. UTC | #2
On Sat, Jan 18, 2020 at 10:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> OK, so this round the design is to reuse the ANSI mode instead of
> introducing a new AIX mode that sits next to ANSI, 256 and RGB?

Right.  Previously I had it with a new AIX enum parallel to ANSI, 256,
etc, but it just made the code longer for no good reason.

> For this to work, not just the 90-97 range for bright-ansi orders
> the colors the same way as 30-37 range (only brighter), but also
> the differences between corresponding fore- and background colors
> must also be 10 just like the regular ANSI colors.

Yes.  It's a happy coincidence that the background version is always
10 greater than the foreground version, for ANSI, for AIX, and even
for the 256-bit colors.   The code takes advantage of that.  If that
later proves to be not true, color_output needs to be modified.
However!, the modification would be just in color_output because the
input is now a boolean "background" instead of the previous char
"type".  I think that's a good improvement so that the caller of
color_output doesn't need to know that, ie, '3' is foreground and '4'
is background.

>
> So perhaps an additional sentence or two deserve to be there, e.g.
>
>         ... of the 3-bit colors.  Instead of 30-37 range for the
>         foreground and 40-47 range for the background, they live in
>         90-97 and 100-107 range, respectively.

Will do.

>
> or something like that, perhaps?
>
> >  The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`,
> >  `blue`, `magenta`, `cyan` and `white`.  The first color given is the
> > -foreground; the second is the background.
> > +foreground; the second is the background.  All the basic colors except
> > +`normal` have a bright variant that can be speficied by prefixing the
> > +color with `bright`, like `brightred`.
>
> Nicely and readably written.

Thanks.  I tried to keep the voice in line with the rest of the text.

>
> I have to wonder if spelling "bright<color>", i.e. two words smashed
> together without anything in between words, is in widespread use (in
> other words, are we following an established practice, or are we
> inventing our own), or if we need to prepare for synonyms?  HTML/CSS
> folks seem to use words-smashed-without-anything-in-betwen so they
> should be fine with this design; I no longer recall what X did ;-)

/usr/local/lib/X11/rgb.txt often uses smashed together:
https://github.com/vim/vim/blob/master/runtime/rgb.txt  Wikipedia
calls them "bright" consistently:
https://en.wikipedia.org/wiki/ANSI_escape_code#Colors .  So we've got
a vote for smashing them together and a vote for "bright".  Seems okay
by me!

Eyal
Junio C Hamano Jan. 21, 2020, 5:37 p.m. UTC | #3
Eyal Soha <shawarmakarma@gmail.com> writes:

>> I have to wonder if spelling "bright<color>", i.e. two words smashed
>> together without anything in between words, is in widespread use (in
>> other words, are we following an established practice, or are we
>> inventing our own), or if we need to prepare for synonyms?  HTML/CSS
>> folks seem to use words-smashed-without-anything-in-betwen so they
>> should be fine with this design; I no longer recall what X did ;-)
>
> /usr/local/lib/X11/rgb.txt often uses smashed together:
> https://github.com/vim/vim/blob/master/runtime/rgb.txt  Wikipedia
> calls them "bright" consistently:
> https://en.wikipedia.org/wiki/ANSI_escape_code#Colors .  So we've got
> a vote for smashing them together and a vote for "bright".  Seems okay
> by me!

OK.  By synonym, I did not mean any word other than "bright"; in the
context of my response, "brightred", "bright red" and "bright-red"
would have been synonyms, but I think it is sufficient to support
only the first one.

Of course, somebody may come up with a bright idea to treat the
"bright" adjective just like "underline" and "bold" and that might
improve the end user experience slightly better, but I dunno ;-)

Patch
diff mbox series

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 83e7bba872..08b13ba72b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -263,7 +263,9 @@  color::
 +
 The basic colors accepted are `normal`, `black`, `red`, `green`, `yellow`,
 `blue`, `magenta`, `cyan` and `white`.  The first color given is the
-foreground; the second is the background.
+foreground; the second is the background.  All the basic colors except
+`normal` have a bright variant that can be speficied by prefixing the
+color with `bright`, like `brightred`.
 +
 Colors may also be given as numbers between 0 and 255; these use ANSI
 256-color mode (but note that not all terminals may support this).  If
diff --git a/color.c b/color.c
index 3b734ccffd..66d32e1191 100644
--- a/color.c
+++ b/color.c
@@ -29,6 +29,7 @@  enum {
 	COLOR_FOREGROUND_ANSI = 30,
 	COLOR_FOREGROUND_RGB = 38,
 	COLOR_FOREGROUND_256 = 38,
+	COLOR_FOREGROUND_BRIGHT_ANSI = 90,
 };
 
 /* Ignore the RESET at the end when giving the size */
@@ -68,13 +69,36 @@  static int get_hex_color(const char *in, unsigned char *out)
 	return 0;
 }
 
-static int parse_color(struct color *out, const char *name, int len)
+/*
+ * If an ANSI color is recognized in "name", fill "out" and return 0.
+ * Otherwise, leave out unchanged and return -1.
+ */
+static int parse_ansi_color(struct color *out, const char *name, int len)
 {
 	/* Positions in array must match ANSI color codes */
 	static const char * const color_names[] = {
 		"black", "red", "green", "yellow",
 		"blue", "magenta", "cyan", "white"
 	};
+
+	int color_offset = COLOR_FOREGROUND_ANSI;
+	if (strncasecmp(name, "bright", 6) == 0) {
+		color_offset = COLOR_FOREGROUND_BRIGHT_ANSI;
+		name += 6;
+		len -= 6;
+	}
+	for (int i = 0; i < ARRAY_SIZE(color_names); i++) {
+		if (match_word(name, len, color_names[i])) {
+			out->type = COLOR_ANSI;
+			out->value = i + color_offset;
+			return 0;
+		}
+	}
+	return -1;
+}
+
+static int parse_color(struct color *out, const char *name, int len)
+{
 	char *end;
 	int i;
 	long val;
@@ -96,12 +120,8 @@  static int parse_color(struct color *out, const char *name, int len)
 	}
 
 	/* Then pick from our human-readable color names... */
-	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
-		if (match_word(name, len, color_names[i])) {
-			out->type = COLOR_ANSI;
-			out->value = i + COLOR_FOREGROUND_ANSI;
-			return 0;
-		}
+	if (parse_ansi_color(out, name, len) == 0) {
+		return 0;
 	}
 
 	/* And finally try a literal 256-color-mode number */
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 671e951ee5..78c69de90a 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -30,6 +30,14 @@  test_expect_success 'attribute before color name' '
 	color "bold red" "[1;31m"
 '
 
+test_expect_success 'aixterm bright fg color' '
+	color "brightred" "[91m"
+'
+
+test_expect_success 'aixterm bright bg color' '
+	color "green brightblue" "[32;104m"
+'
+
 test_expect_success 'color name before attribute' '
 	color "red bold" "[1;31m"
 '