[PATCH 1/1] color: use "light" colors for dark background

[PATCH 1/1] color: use "light" colors for dark background

[Petr Vorel]

COLORFGBG environment variable is used to detect dark background.

Idea and a bit of code is borrowed from Vim, thanks.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Colors are nice, but the ones chosen aren't suitable for dark background.
COLORFGBG environment variable is used in some libraries and software (e.g.
ncurses, Vim). COLORFGBG is set by various terminal emulators (e.g. konsole,
rxvt and rxvt-unicode).

Chosen colors are questionable. Best solution would be also allow user to
redefine colors, like ls does with LS_COLORS or grep with GREP_COLORS. But that
is maybe overkill.
---
 include/color.h |  1 +
 lib/color.c     | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/color.h b/include/color.h
index c1c29831..43190db4 100644
--- a/include/color.h
+++ b/include/color.h
@@ -12,6 +12,7 @@ enum color_attr {
 };
 
 void enable_color(void);
+void set_background(void);
 int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...);
 enum color_attr ifa_family_color(__u8 ifa_family);
 enum color_attr oper_state_color(__u8 state);
diff --git a/lib/color.c b/lib/color.c
index 95596be2..69375b26 100644
--- a/lib/color.c
+++ b/lib/color.c
@@ -1,5 +1,7 @@
 #include <stdio.h>
 #include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
 #include <sys/socket.h>
 #include <sys/types.h>
 #include <linux/if.h>
@@ -14,6 +16,12 @@ enum color {
 	C_MAGENTA,
 	C_CYAN,
 	C_WHITE,
+	C_LIGHT_RED,
+	C_LIGHT_GREEN,
+	C_LIGHT_YELLOW,
+	C_LIGHT_BLUE,
+	C_LIGHT_MAGENTA,
+	C_LIGHT_CYAN,
 	C_CLEAR
 };
 
@@ -25,25 +33,58 @@ static const char * const color_codes[] = {
 	"\e[35m",
 	"\e[36m",
 	"\e[37m",
+	"\e[1;31m",
+	"\e[1;32m",
+	"\e[1;33m",
+	"\e[1;34m",
+	"\e[1;35m",
+	"\e[1;36m",
 	"\e[0m",
 	NULL,
 };
 
 static enum color attr_colors[] = {
+	/* light background */
 	C_CYAN,
 	C_YELLOW,
 	C_MAGENTA,
 	C_BLUE,
 	C_GREEN,
 	C_RED,
+	C_CLEAR,
+
+	/* dark background */
+	C_LIGHT_CYAN,
+	C_LIGHT_YELLOW,
+	C_LIGHT_MAGENTA,
+	C_LIGHT_BLUE,
+	C_LIGHT_GREEN,
+	C_LIGHT_RED,
 	C_CLEAR
 };
 
+static int is_dark_bg;
 static int color_is_enabled;
 
 void enable_color(void)
 {
 	color_is_enabled = 1;
+	set_background();
+}
+
+void set_background(void)
+{
+	char *p = getenv("COLORFGBG");
+
+	/*
+	 * COLORFGBG environment variable usually contains either two or three
+	 * values separated by semicolons; we want the last value in either case.
+	 * If this value is 0-6 or 8, background is dark.
+	 */
+	if (p && (p = (char *)strrchr(p, ';')) != NULL
+		&& ((p[1] >= '0' && p[1] <= '6') || p[1] == '8')
+		&& p[2] == '\0')
+		is_dark_bg = 1;
 }
 
 int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
@@ -58,7 +99,7 @@ int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
 		goto end;
 	}
 
-	ret += fprintf(fp, "%s", color_codes[attr_colors[attr]]);
+	ret += fprintf(fp, "%s", color_codes[attr_colors[is_dark_bg ? attr + 7 : attr]]);
 	ret += vfprintf(fp, fmt, args);
 	ret += fprintf(fp, "%s", color_codes[C_CLEAR]);
 
-- 
2.11.0

Re: [PATCH 1/1] color: use "light" colors for dark background

[David Miller]

From: Petr Vorel <pvorel@suse.cz>
Date: Fri, 24 Feb 2017 11:13:12 +0100

> COLORFGBG environment variable is used to detect dark background.
> 
> Idea and a bit of code is borrowed from Vim, thanks.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Colors are nice, but the ones chosen aren't suitable for dark background.
> COLORFGBG environment variable is used in some libraries and software (e.g.
> ncurses, Vim). COLORFGBG is set by various terminal emulators (e.g. konsole,
> rxvt and rxvt-unicode).
> 
> Chosen colors are questionable. Best solution would be also allow user to
> redefine colors, like ls does with LS_COLORS or grep with GREP_COLORS. But that
> is maybe overkill.

I think you may have posted this to the wrong mailing list.

Re: [PATCH 1/1] color: use "light" colors for dark background

[Johannes Berg]

> > Chosen colors are questionable. Best solution would be also allow
> user to
> > redefine colors, like ls does with LS_COLORS or grep with
> GREP_COLORS. But that
> > is maybe overkill.
> 
> I think you may have posted this to the wrong mailing list.

Actually, it seems to just be missing the "iproute" (or so) prefix?

johannes

Re: [PATCH 1/1] color: use "light" colors for dark background

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 24 Feb 2017 17:11:42 +0100

> 
>> > Chosen colors are questionable. Best solution would be also allow
>> user to
>> > redefine colors, like ls does with LS_COLORS or grep with
>> GREP_COLORS. But that
>> > is maybe overkill.
>> 
>> I think you may have posted this to the wrong mailing list.
> 
> Actually, it seems to just be missing the "iproute" (or so) prefix?

Ok that makes more sense :)

Re: [PATCH 1/1] color: use "light" colors for dark background

[Stephen Hemminger]

On Fri, 24 Feb 2017 11:22:13 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Fri, 24 Feb 2017 17:11:42 +0100
> 
> >   
> >> > Chosen colors are questionable. Best solution would be also allow  
> >> user to  
> >> > redefine colors, like ls does with LS_COLORS or grep with  
> >> GREP_COLORS. But that  
> >> > is maybe overkill.  
> >> 
> >> I think you may have posted this to the wrong mailing list.  
> > 
> > Actually, it seems to just be missing the "iproute" (or so) prefix?  
> 
> Ok that makes more sense :)

Yes. Iproute2 has  had color support for a while.
The CC list has the original developer

Re: [PATCH 1/1] color: use "light" colors for dark background

[Stephen Hemminger]

On Fri, 24 Feb 2017 11:13:12 +0100
Petr Vorel <pvorel@suse.cz> wrote:

> COLORFGBG environment variable is used to detect dark background.
> 
> Idea and a bit of code is borrowed from Vim, thanks.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Colors are nice, but the ones chosen aren't suitable for dark background.
> COLORFGBG environment variable is used in some libraries and software (e.g.
> ncurses, Vim). COLORFGBG is set by various terminal emulators (e.g. konsole,
> rxvt and rxvt-unicode).
> 
> Chosen colors are questionable. Best solution would be also allow user to
> redefine colors, like ls does with LS_COLORS or grep with GREP_COLORS. But that
> is maybe overkill.

This really needs to be standardized with some convention. So that ls, grep, git
all behave the same and done by some standard library.

The current method is hack that keeps on growing.

Re: [PATCH 1/1] color: use "light" colors for dark background

[Mathias Nyman]

On 2017-02-24 11:13+0100, Petr Vorel wrote:
>COLORFGBG environment variable is used to detect dark background.
>
>Idea and a bit of code is borrowed from Vim, thanks.
>
>Signed-off-by: Petr Vorel <pvorel@suse.cz>
>---
>Colors are nice, but the ones chosen aren't suitable for dark background.

Yea, I admit, the original color palette is kind of horrendous. I
wouldn't say the current colors are suitable for a light background
either.

I propose you just replace the current colors with their bold
variants, and leave the background hue guessing out all together. That
would be an all-round improvement for everyone.

I'll include some comments on this patch-set anyway, as I had a look
at it.

>COLORFGBG environment variable is used in some libraries and software (e.g.
>ncurses, Vim). COLORFGBG is set by various terminal emulators (e.g. konsole,
>rxvt and rxvt-unicode).
>
>Chosen colors are questionable. Best solution would be also allow user to
>redefine colors, like ls does with LS_COLORS or grep with GREP_COLORS. But that
>is maybe overkill.
>---
> include/color.h |  1 +
> lib/color.c     | 43 ++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
>diff --git a/include/color.h b/include/color.h
>index c1c29831..43190db4 100644
>--- a/include/color.h
>+++ b/include/color.h
>@@ -12,6 +12,7 @@ enum color_attr {
> };
>
> void enable_color(void);
>+void set_background(void);
> int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...);
> enum color_attr ifa_family_color(__u8 ifa_family);
> enum color_attr oper_state_color(__u8 state);
>diff --git a/lib/color.c b/lib/color.c
>index 95596be2..69375b26 100644
>--- a/lib/color.c
>+++ b/lib/color.c
>@@ -1,5 +1,7 @@
> #include <stdio.h>
> #include <stdarg.h>
>+#include <stdlib.h>
>+#include <string.h>
> #include <sys/socket.h>
> #include <sys/types.h>
> #include <linux/if.h>
>@@ -14,6 +16,12 @@ enum color {
> 	C_MAGENTA,
> 	C_CYAN,
> 	C_WHITE,
>+	C_LIGHT_RED,
>+	C_LIGHT_GREEN,
>+	C_LIGHT_YELLOW,
>+	C_LIGHT_BLUE,
>+	C_LIGHT_MAGENTA,
>+	C_LIGHT_CYAN,

You have missed to add C_LIGHT_WHITE here.

The naming is also confusing, because while you say "light", the
colors defined below are actually _bold_. They may have an lightening
effect, but this naming is just confusing in the code.

> 	C_CLEAR
> };
>
>@@ -25,25 +33,58 @@ static const char * const color_codes[] = {
> 	"\e[35m",
> 	"\e[36m",
> 	"\e[37m",
>+	"\e[1;31m",
>+	"\e[1;32m",
>+	"\e[1;33m",
>+	"\e[1;34m",
>+	"\e[1;35m",
>+	"\e[1;36m",

You have also missed to add the white color ("\e[1;37m",) here as
well.

> 	"\e[0m",
> 	NULL,
> };
>
> static enum color attr_colors[] = {
>+	/* light background */
> 	C_CYAN,
> 	C_YELLOW,
> 	C_MAGENTA,
> 	C_BLUE,
> 	C_GREEN,
> 	C_RED,
>+	C_CLEAR,
>+
>+	/* dark background */
>+	C_LIGHT_CYAN,
>+	C_LIGHT_YELLOW,
>+	C_LIGHT_MAGENTA,
>+	C_LIGHT_BLUE,
>+	C_LIGHT_GREEN,
>+	C_LIGHT_RED,
> 	C_CLEAR
> };
>
>+static int is_dark_bg;
> static int color_is_enabled;
>
> void enable_color(void)
> {
> 	color_is_enabled = 1;
>+	set_background();
>+}
>+
>+void set_background(void)

The function name is a bit misleading. Only after reading the whole
function did I understand that what you do here is choose the color
palette - not set the background.

>+{
>+	char *p = getenv("COLORFGBG");
>+
>+	/*
>+	 * COLORFGBG environment variable usually contains either two or three
>+	 * values separated by semicolons; we want the last value in either case.
>+	 * If this value is 0-6 or 8, background is dark.
>+	 */
>+	if (p && (p = (char *)strrchr(p, ';')) != NULL
>+		&& ((p[1] >= '0' && p[1] <= '6') || p[1] == '8')
>+		&& p[2] == '\0')
>+		is_dark_bg = 1;
> }
>
> int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
>@@ -58,7 +99,7 @@ int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
> 		goto end;
> 	}
>
>-	ret += fprintf(fp, "%s", color_codes[attr_colors[attr]]);
>+	ret += fprintf(fp, "%s", color_codes[attr_colors[is_dark_bg ? attr + 7 : attr]]);
> 	ret += vfprintf(fp, fmt, args);
> 	ret += fprintf(fp, "%s", color_codes[C_CLEAR]);
>
>-- 
>2.11.0
>

Re: [PATCH 1/1] color: use "light" colors for dark background

[Petr Vorel]

Hi,

> This really needs to be standardized with some convention. So that ls, grep, git
> all behave the same and done by some standard library.

> The current method is hack that keeps on growing.

It would be easier, if the code could be adapted by some broadly used library, than start
new project for it. But, as there is probably no suitable existing library, it would have
to be a new one.


Kind regards,
Petr

Re: [PATCH 1/1] color: use "light" colors for dark background

[Petr Vorel]

Hi,

> Yea, I admit, the original color palette is kind of horrendous. I
> wouldn't say the current colors are suitable for a light background
> either.

> I propose you just replace the current colors with their bold
> variants, and leave the background hue guessing out all together. That
> would be an all-round improvement for everyone.
Well, I agree that original colors aren't the best ones for a light background either, but
not all bold variants are suitable for light background - yellow, white and cyan are
problematic at least on some terminals with light background (e.g. konsole; white isn't
used). So I'd really keep separate sets of colors.

> I'll include some comments on this patch-set anyway, as I had a look
> at it.
Thanks for your review, I'll post fixed version with two sets of colors.


Kind regards,
Petr