Good afternoon, due to the ongoing discussion about color-handling in st I decided to take a look at the code and figured out what the problem was.
What st basically does in xdraws() is to check if a given character is bold. If that's the case, certain colour-intervals are brightened up with offsets: 1) [0-7] -> offset 8 (normal system colors, full range) 2) [16-195] -> offset 36 (subset of the 256-color-range [16-231]) 3) [232-251] -> offset 4 (subset of the grayscale-range [232-255]) The behaviour in 1) is sane and expected (check config.h); 2) and 3) however suffer from a number of issues: a) The RGB-scale is non-linear[0]. ---------------------------------- Eric Pruitt <[email protected]> writes: > If it were _just_ brighter, I probably wouldn't care, but to me, the > difference in color is like night and day. This is not surprising, giving a fixed offset on a non-linear scale leads to color-shifts apart from the desired gamma-shift. This mistake has been made often enough and shouldn't be made in a suckless software. Either make it right or leave it out (cf. Unix Philosophy). b) Making bold text lighter doesn't make sense. ----------------------------------------------- Looking at the colour-impression, bold text should rather be darkened, as bigger areas of the same colour normally lead to a falsified lighter impression. Actually lighting it up even more makes color-shifts more obvious. c) The offsets are applied to subsets of the full ranges. --------------------------------------------------------- This is just an issue derived from how this was solved in a linear way. You might want to say that it's trivial, as the lightest colours don't need to be lit up, but just consider the gaps between 195,196 and 251,252. d) Consistency -------------- It's the client's job to set the 256-colour-representation. The portage-utils for instance, employing 256-colours, make their own adjustments to the colors of bold characters. Forcing our invalid color-modification on the user just messes with expected behaviour, and most importantly, with how the programs were designed visually. I wrote this patch to adress two things: First, fix xdraws() to make it more consistent and work by a valid colour-model (according to the points given above and Eric's patch). Second, simplify the loops in xloadcols and other minor simplifications. Cheers FRIGN [0]: http://www.4p8.com/eric.brasseur/gamma.html -- FRIGN <[email protected]>
>From f32c98d68f4bd0a66ac082c99a41187e44b8f423 Mon Sep 17 00:00:00 2001 From: FRIGN <[email protected]> Date: Thu, 22 May 2014 15:00:33 +0200 Subject: [PATCH] Fix colour-model and simplify xloadcols() --- st.c | 56 ++++++++++++++++++++------------------------------------ 1 file changed, 20 insertions(+), 36 deletions(-) diff --git a/st.c b/st.c index 78d8a01..a6b4ada 100644 --- a/st.c +++ b/st.c @@ -340,7 +340,7 @@ typedef struct { /* Drawing Context */ typedef struct { - Colour col[LEN(colorname) < 256 ? 256 : LEN(colorname)]; + Colour col[MAX(LEN(colorname), 256)]; Font font, bfont, ifont, ibfont; GC gc; } DC; @@ -2715,7 +2715,7 @@ sixd_to_16bit(int x) { void xloadcols(void) { - int i, r, g, b; + int i; XRenderColor color = { .alpha = 0xffff }; static bool loaded; Colour *cp; @@ -2725,7 +2725,7 @@ xloadcols(void) { XftColorFree(xw.dpy, xw.vis, xw.cmap, cp); } - /* load colors [0-15] colors and [256-LEN(colorname)[ (config.h) */ + /* load colours [0-15] and [256-LEN(colorname)] (config.h) */ for(i = 0; i < LEN(colorname); i++) { if(!colorname[i]) continue; @@ -2734,27 +2734,20 @@ xloadcols(void) { } } - /* load colors [16-255] ; same colors as xterm */ - for(i = 16, r = 0; r < 6; r++) { - for(g = 0; g < 6; g++) { - for(b = 0; b < 6; b++) { - color.red = sixd_to_16bit(r); - color.green = sixd_to_16bit(g); - color.blue = sixd_to_16bit(b); - if(!XftColorAllocValue(xw.dpy, xw.vis, xw.cmap, &color, &dc.col[i])) { - die("Could not allocate color %d\n", i); - } - i++; - } - } + /* load colours [16-231] ; same colours as xterm */ + for(i = 16; i < 6*6*6+16; i++) { + color.red = sixd_to_16bit( ((i-16)/36)%6 ); + color.green = sixd_to_16bit( ((i-16)/6) %6 ); + color.blue = sixd_to_16bit( ((i-16)/1) %6 ); + if(!XftColorAllocValue(xw.dpy, xw.vis, xw.cmap, &color, &dc.col[i])) + die("Could not allocate color %d\n", i); } - - for(r = 0; r < 24; r++, i++) { - color.red = color.green = color.blue = 0x0808 + 0x0a0a * r; - if(!XftColorAllocValue(xw.dpy, xw.vis, xw.cmap, &color, - &dc.col[i])) { + + /* load colours [232-255] ; grayscale */ + for(; i < 256; i++) { + color.red = color.green = color.blue = 0x0808 + 0x0a0a * (i-(6*6*6+16)); + if(!XftColorAllocValue(xw.dpy, xw.vis, xw.cmap, &color, &dc.col[i])) die("Could not allocate color %d\n", i); - } } loaded = true; } @@ -3149,22 +3142,13 @@ xdraws(char *s, Glyph base, int x, int y, int charlen, int bytelen) { } if(base.mode & ATTR_BOLD) { - if(BETWEEN(base.fg, 0, 7)) { - /* basic system colors */ - fg = &dc.col[base.fg + 8]; - } else if(BETWEEN(base.fg, 16, 195)) { - /* 256 colors */ - fg = &dc.col[base.fg + 36]; - } else if(BETWEEN(base.fg, 232, 251)) { - /* greyscale */ - fg = &dc.col[base.fg + 4]; - } /* - * Those ranges will not be brightened: - * 8 - 15 â bright system colors - * 196 - 231 â highest 256 color cube - * 252 - 255 â brightest colors in greyscale + * change basic system colours [0-7] + * to bright system colours [8-15] */ + if(BETWEEN(base.fg, 0, 7)) + fg = &dc.col[base.fg + 8]; + font = &dc.bfont; frcflags = FRC_BOLD; } -- 1.8.3.2
