| Message ID | 20240429164849.78509-4-dev+git@drbeat.li (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | color: add support for 12-bit RGB colors | expand |
"Beat Bolli" <bb@drbeat.li> writes: > +static int get_hex_color(const char **inp, int width, unsigned char *out) > { > + const char *in = *inp; > unsigned int val; > - val = (hexval(in[0]) << 4) | hexval(in[1]); > + > + assert(width == 1 || width == 2); > + val = (hexval(in[0]) << 4) | hexval(in[width - 1]); So this makes #111111 out of #111 and #ffffff out of #fff. Nice. > diff --git a/color.h b/color.h > index bb28343be210..7ed259a35bb4 100644 > --- a/color.h > +++ b/color.h > @@ -112,7 +112,8 @@ int want_color_fd(int fd, int var); > * Translate a Git color from 'value' into a string that the terminal can > * interpret and store it into 'dst'. The Git color values are of the form > * "foreground [background] [attr]" where fore- and background can be a color > - * name ("red"), a RGB code (#0xFF0000) or a 256-color-mode from the terminal. > + * name ("red"), a RGB code (#FF0000 or #F00) or a 256-color-mode from the > + * terminal. > */ Good. Hopefully we do not have such extra 0x in our end-user facing documentation? > diff --git a/t/t4026-color.sh b/t/t4026-color.sh > index 0c39bd74a613..9a6f8a4bc5bf 100755 > --- a/t/t4026-color.sh > +++ b/t/t4026-color.sh > @@ -96,8 +96,8 @@ test_expect_success '256 colors' ' > color "254 bold 255" "[1;38;5;254;48;5;255m" > ' > > -test_expect_success '24-bit colors' ' > - color "#ff00ff black" "[38;2;255;0;255;40m" > +test_expect_success 'RGB colors' ' > + color "#ff00ff #0f0" "[38;2;255;0;255;48;2;0;255;0m" > ' > > test_expect_success '"default" foreground' ' > @@ -146,7 +146,10 @@ test_expect_success 'non-hex character in RGB color' ' > invalid_color "#12x456" && > invalid_color "#123x56" && > invalid_color "#1234x6" && > - invalid_color "#12345x" > + invalid_color "#12345x" && > + invalid_color "#x23" && > + invalid_color "#1x3" && > + invalid_color "#12x" > ' OK, looking good. Thanks.
On 29.04.2024 19:23, Junio C Hamano wrote: > "Beat Bolli" <bb@drbeat.li> writes: > >> diff --git a/color.h b/color.h >> index bb28343be210..7ed259a35bb4 100644 >> --- a/color.h >> +++ b/color.h >> @@ -112,7 +112,8 @@ int want_color_fd(int fd, int var); >> * Translate a Git color from 'value' into a string that the terminal can >> * interpret and store it into 'dst'. The Git color values are of the form >> * "foreground [background] [attr]" where fore- and background can be a color >> - * name ("red"), a RGB code (#0xFF0000) or a 256-color-mode from the terminal. >> + * name ("red"), a RGB code (#FF0000 or #F00) or a 256-color-mode from the >> + * terminal. >> */ > > Good. Hopefully we do not have such extra 0x in our end-user facing > documentation? No, this was the only '#0x' I found. config.txt is fine as per the first hunk.
On Mon, Apr 29, 2024 at 06:48:49PM +0200, Beat Bolli wrote: > -test_expect_success '24-bit colors' ' > - color "#ff00ff black" "[38;2;255;0;255;40m" > +test_expect_success 'RGB colors' ' > + color "#ff00ff #0f0" "[38;2;255;0;255;48;2;0;255;0m" > ' Heh, I would still think of it as a shorthand for 24-bit color, but I guess you could argue it is now 12-bit color. :) (Only observing, I think the new name is fine). > test_expect_success '"default" foreground' ' > @@ -146,7 +146,10 @@ test_expect_success 'non-hex character in RGB color' ' > invalid_color "#12x456" && > invalid_color "#123x56" && > invalid_color "#1234x6" && > - invalid_color "#12345x" > + invalid_color "#12345x" && > + invalid_color "#x23" && > + invalid_color "#1x3" && > + invalid_color "#12x" > ' This made me wonder what we'd do with "#1", "#12", "#1234", etc. Looking at the code change, I think we'd continue to reject them. I wonder if it is worth covering here. -Peff
Jeff King <peff@peff.net> writes: > On Mon, Apr 29, 2024 at 06:48:49PM +0200, Beat Bolli wrote: > >> -test_expect_success '24-bit colors' ' >> - color "#ff00ff black" "[38;2;255;0;255;40m" >> +test_expect_success 'RGB colors' ' >> + color "#ff00ff #0f0" "[38;2;255;0;255;48;2;0;255;0m" >> ' > > Heh, I would still think of it as a shorthand for 24-bit color, but I > guess you could argue it is now 12-bit color. :) > > (Only observing, I think the new name is fine). > >> test_expect_success '"default" foreground' ' >> @@ -146,7 +146,10 @@ test_expect_success 'non-hex character in RGB color' ' >> invalid_color "#12x456" && >> invalid_color "#123x56" && >> invalid_color "#1234x6" && >> - invalid_color "#12345x" >> + invalid_color "#12345x" && >> + invalid_color "#x23" && >> + invalid_color "#1x3" && >> + invalid_color "#12x" >> ' > > This made me wonder what we'd do with "#1", "#12", "#1234", etc. Looking > at the code change, I think we'd continue to reject them. I wonder if it > is worth covering here. Worth covering in this test, yes, but I am perfectly OK with leaving it outside the series as a #leftoverbit clean-up.
Junio C Hamano <gitster@pobox.com> writes: >>> @@ -146,7 +146,10 @@ test_expect_success 'non-hex character in RGB color' ' >>> invalid_color "#12x456" && >>> invalid_color "#123x56" && >>> invalid_color "#1234x6" && >>> - invalid_color "#12345x" >>> + invalid_color "#12345x" && >>> + invalid_color "#x23" && >>> + invalid_color "#1x3" && >>> + invalid_color "#12x" >>> ' >> >> This made me wonder what we'd do with "#1", "#12", "#1234", etc. Looking >> at the code change, I think we'd continue to reject them. I wonder if it >> is worth covering here. > > Worth covering in this test, yes, but I am perfectly OK with leaving > it outside the series as a #leftoverbit clean-up. Ah, I take it back. The preimage was added by [2/3] so it is fair to say that that step would be the right place to do that from the get-go.
Junio C Hamano <gitster@pobox.com> writes: > Ah, I take it back. The preimage was added by [2/3] so it is fair > to say that that step would be the right place to do that from the > get-go. Perhaps something like this can be squashed in. Subject: [PATCH] fixup! t/t4026-color: add test coverage for invalid RGB colors --- t/t4026-color.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t4026-color.sh b/t/t4026-color.sh index 9a6f8a4bc5..e60aa588c2 100755 --- a/t/t4026-color.sh +++ b/t/t4026-color.sh @@ -140,6 +140,14 @@ test_expect_success 'extra character after attribute' ' invalid_color "dimX" ' +test_expect_success 'wrong number of letters in RGB color' ' + invalid_color "#1" && + invalid_color "#23" && + invalid_color "#4567" && + invalid_color "#89abc" && + invalid_color "#def0123" +' + test_expect_success 'non-hex character in RGB color' ' invalid_color "#x23456" && invalid_color "#1x3456" &&
On Tue, Apr 30, 2024 at 12:01:54PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Ah, I take it back. The preimage was added by [2/3] so it is fair > > to say that that step would be the right place to do that from the > > get-go. > > Perhaps something like this can be squashed in. > > Subject: [PATCH] fixup! t/t4026-color: add test coverage for invalid RGB colors Yup, that looks good to me. -Peff
diff --git a/Documentation/config.txt b/Documentation/config.txt index 70b448b13262..6f649c997c0f 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -316,7 +316,8 @@ terminals, this is usually not the same as setting to "white black". 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 your terminal supports it, you may also specify 24-bit RGB values as -hex, like `#ff0ab3`. +hex, like `#ff0ab3`, or 12-bit RGB values like `#f1b`, which is +equivalent to the 24-bit color `#ff11bb`. + The accepted attributes are `bold`, `dim`, `ul`, `blink`, `reverse`, `italic`, and `strike` (for crossed-out or "strikethrough" letters). diff --git a/color.c b/color.c index f663c06ac4ed..227a5ab2f42e 100644 --- a/color.c +++ b/color.c @@ -64,12 +64,16 @@ static int match_word(const char *word, int len, const char *match) return !strncasecmp(word, match, len) && !match[len]; } -static int get_hex_color(const char *in, unsigned char *out) +static int get_hex_color(const char **inp, int width, unsigned char *out) { + const char *in = *inp; unsigned int val; - val = (hexval(in[0]) << 4) | hexval(in[1]); + + assert(width == 1 || width == 2); + val = (hexval(in[0]) << 4) | hexval(in[width - 1]); if (val & ~0xff) return -1; + *inp += width; *out = val; return 0; } @@ -135,11 +139,14 @@ static int parse_color(struct color *out, const char *name, int len) return 0; } - /* Try a 24-bit RGB value */ - if (len == 7 && name[0] == '#') { - if (!get_hex_color(name + 1, &out->red) && - !get_hex_color(name + 3, &out->green) && - !get_hex_color(name + 5, &out->blue)) { + /* Try a 24- or 12-bit RGB value prefixed with '#' */ + if ((len == 7 || len == 4) && name[0] == '#') { + int width_per_color = (len == 7) ? 2 : 1; + const char *color = name + 1; + + if (!get_hex_color(&color, width_per_color, &out->red) && + !get_hex_color(&color, width_per_color, &out->green) && + !get_hex_color(&color, width_per_color, &out->blue)) { out->type = COLOR_RGB; return 0; } diff --git a/color.h b/color.h index bb28343be210..7ed259a35bb4 100644 --- a/color.h +++ b/color.h @@ -112,7 +112,8 @@ int want_color_fd(int fd, int var); * Translate a Git color from 'value' into a string that the terminal can * interpret and store it into 'dst'. The Git color values are of the form * "foreground [background] [attr]" where fore- and background can be a color - * name ("red"), a RGB code (#0xFF0000) or a 256-color-mode from the terminal. + * name ("red"), a RGB code (#FF0000 or #F00) or a 256-color-mode from the + * terminal. */ int color_parse(const char *value, char *dst); int color_parse_mem(const char *value, int len, char *dst); diff --git a/t/t4026-color.sh b/t/t4026-color.sh index 0c39bd74a613..9a6f8a4bc5bf 100755 --- a/t/t4026-color.sh +++ b/t/t4026-color.sh @@ -96,8 +96,8 @@ test_expect_success '256 colors' ' color "254 bold 255" "[1;38;5;254;48;5;255m" ' -test_expect_success '24-bit colors' ' - color "#ff00ff black" "[38;2;255;0;255;40m" +test_expect_success 'RGB colors' ' + color "#ff00ff #0f0" "[38;2;255;0;255;48;2;0;255;0m" ' test_expect_success '"default" foreground' ' @@ -146,7 +146,10 @@ test_expect_success 'non-hex character in RGB color' ' invalid_color "#12x456" && invalid_color "#123x56" && invalid_color "#1234x6" && - invalid_color "#12345x" + invalid_color "#12345x" && + invalid_color "#x23" && + invalid_color "#1x3" && + invalid_color "#12x" ' test_expect_success 'unknown color slots are ignored (diff)' '
RGB color parsing currently supports 24-bit values in the form #RRGGBB. As in Cascading Style Sheets (CSS [1]), also allow to specify an RGB color using only three digits with #RGB. In this shortened form, each of the digits is – again, as in CSS – duplicated to convert the color to 24 bits, e.g. #f1b specifies the same color as #ff11bb. In color.h, remove the '0x' prefix in the example to match the actual syntax. [1] https://developer.mozilla.org/en-US/docs/Web/CSS/hex-color Signed-off-by: Beat Bolli <dev+git@drbeat.li> --- Documentation/config.txt | 3 ++- color.c | 21 ++++++++++++++------- color.h | 3 ++- t/t4026-color.sh | 9 ++++++--- 4 files changed, 24 insertions(+), 12 deletions(-)