| Message ID | E1s9rpA-00000006Jy7-18Q5@ws2.gedalya.net (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [resend] color: default to dark background | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, 23 May 2024 02:43:57 +0800
Gedalya Nie <gedalya@gedalya.net> wrote:
> Signed-off-by: Gedalya Nie <gedalya@gedalya.net>
Why? What other utilities do the same thin?
On 5/23/24 4:57 AM, Stephen Hemminger wrote:
> Why? What other utilities do the same thin?
I'm truly sorry, I don't understand the question
On 2024-05-22 23:01, Gedalya wrote: > On 5/23/24 4:57 AM, Stephen Hemminger wrote: >> Why? What other utilities do the same thin? > > I'm truly sorry, I don't understand the question Basically, you need to provide the patch description.
On Wed, 22 May 2024 23:02:37 +0200 Dragan Simic <dsimic@manjaro.org> wrote: > On 2024-05-22 23:01, Gedalya wrote: > > On 5/23/24 4:57 AM, Stephen Hemminger wrote: > >> Why? What other utilities do the same thin? > > > > I'm truly sorry, I don't understand the question > > Basically, you need to provide the patch description. The color handling of iproute2 was inherited from other utilities such as vim. There doesn't appear to be any library or standardization, all this ad-hoc. In current vim: char_u * term_bg_default(void) { #if defined(MSWIN) // DOS console is nearly always black return (char_u *)"dark"; #else char_u *p; if (STRCMP(T_NAME, "linux") == 0 || STRCMP(T_NAME, "screen.linux") == 0 || STRNCMP(T_NAME, "cygwin", 6) == 0 || STRNCMP(T_NAME, "putty", 5) == 0 || ((p = mch_getenv((char_u *)"COLORFGBG")) != NULL && (p = vim_strrchr(p, ';')) != NULL && ((p[1] >= '0' && p[1] <= '6') || p[1] == '8') && p[2] == NUL)) return (char_u *)"dark"; return (char_u *)"light"; #endif }
On 5/23/24 5:33 AM, Stephen Hemminger wrote: > The color handling of iproute2 was inherited from other utilities such as vim. > There doesn't appear to be any library or standardization, all this ad-hoc. Looking at the vim code, and playing around with the program, I have a few observations. The snippet you quoted isn't doing anything brilliant. It just assumes that certain types of terminals are dark, regardless of the implementation and configuration. All you can really say is that terminals are often dark which is what I was saying here in the first place. I'm not seeing any justification for assuming dark in certain cases and light otherwise. The code just happens to be that way. More importantly, vim does happen to actually work. So far I was only able to get it to show dark blue on a black background by setting TERM=ansi. The results are what is important. Vim has its own various color palettes and it's a serious full-screen app, uses the terminfo library, its support for terminals is much more complex than just two palettes. One way or another, we need to fix this, probably not by linking against ncurses, and "assuming terminal backgrounds are light" isn't the nugget of wisdom vim has to offer.
On Thu, 23 May 2024 06:11:56 +0800 Gedalya <gedalya@gedalya.net> wrote: > On 5/23/24 5:33 AM, Stephen Hemminger wrote: > > The color handling of iproute2 was inherited from other utilities such as vim. > > There doesn't appear to be any library or standardization, all this ad-hoc. > > Looking at the vim code, and playing around with the program, I have a few observations. > > The snippet you quoted isn't doing anything brilliant. It just assumes that certain types of terminals are dark, regardless of the implementation and configuration. All you can really say is that terminals are often dark which is what I was saying here in the first place. > > I'm not seeing any justification for assuming dark in certain cases and light otherwise. The code just happens to be that way. > > More importantly, vim does happen to actually work. So far I was only able to get it to show dark blue on a black background by setting TERM=ansi. > > The results are what is important. Vim has its own various color palettes and it's a curses app, its support for terminals is much more complex than just two palettes. One way or another, we need to fix this, probably not by linking against ncurses, and "assuming terminal backgrounds are light" isn't the nugget of wisdom vim has to offer. > Overall, I am concerned that changing this will upset existing users. Not that it is impossible, just need more consensus and testing.
On 5/23/24 6:52 AM, Stephen Hemminger wrote: > Overall, I am concerned that changing this will upset existing users. > Not that it is impossible, just need more consensus and testing. Makes sense. It seems that possible negative impact would be when the following are all true: 1. Users actually using color. For debian and derivatives, up until now this would have been people enabling color themselves. Debian's recent enabling color by default was the impetus for this issue. 2. Using light background. 3. Not using COLORFGBG, or setting it incorrectly.
On 5/23/24 6:52 AM, Stephen Hemminger wrote: > Overall, I am concerned that changing this will upset existing users. > Not that it is impossible, just need more consensus and testing. Turns out people were complaining about it years ago, not realizing that the color scheme they were looking at was actually not meant for dark backgrounds: https://www.reddit.com/r/linux/comments/kae9qa/comment/gfgew0x/
On 2024-05-23 01:41, Gedalya wrote: > On 5/23/24 6:52 AM, Stephen Hemminger wrote: > >> Overall, I am concerned that changing this will upset existing users. >> Not that it is impossible, just need more consensus and testing. > > Turns out people were complaining about it years ago, not realizing > that > the color scheme they were looking at was actually not meant for dark > backgrounds: > > https://www.reddit.com/r/linux/comments/kae9qa/comment/gfgew0x/ That's what I also wondered about after enabling the color support for the first time, which wasn't very unusable with dark background. After a bit of reading, I got it all configured properly.
On 2024-05-23 02:12, Dragan Simic wrote: > On 2024-05-23 01:41, Gedalya wrote: >> On 5/23/24 6:52 AM, Stephen Hemminger wrote: >> >>> Overall, I am concerned that changing this will upset existing users. >>> Not that it is impossible, just need more consensus and testing. >> >> Turns out people were complaining about it years ago, not realizing >> that >> the color scheme they were looking at was actually not meant for dark >> backgrounds: >> >> https://www.reddit.com/r/linux/comments/kae9qa/comment/gfgew0x/ > > That's what I also wondered about after enabling the color support > for the first time, which wasn't very unusable with dark background. > After a bit of reading, I got it all configured properly. Oops... s/very unusable/very usable/ Sorry for the noise.
diff --git a/lib/color.c b/lib/color.c index cd0f9f75..6692f9c1 100644 --- a/lib/color.c +++ b/lib/color.c @@ -72,7 +72,7 @@ static enum color attr_colors_dark[] = { C_CLEAR }; -static int is_dark_bg; +static int is_dark_bg = 1; static int color_is_enabled; static void enable_color(void) @@ -127,11 +127,11 @@ static void set_color_palette(void) * 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 it is 7, 9 or greater, background is light. */ if (p && (p = strrchr(p, ';')) != NULL - && ((p[1] >= '0' && p[1] <= '6') || p[1] == '8') - && p[2] == '\0') - is_dark_bg = 1; + && (p[1] == '7' || p[1] == '9' || p[2] != '\0')) + is_dark_bg = 0; } __attribute__((format(printf, 3, 4)))
Signed-off-by: Gedalya Nie <gedalya@gedalya.net> --- lib/color.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)