Preserve colors/highlighting for highlighted lines

[BenBE]

While reviewing #240 it became quite visible, that when highlighting lines (e.g. with space, or with the newly added highlighting for new/dead processes) the markup of the line is dropped. With only a few lines affected with the search or manually selected processes this isn't a big issue, but when highlighting of lines becomes more visible, so becomes the dropped formatting for these lines.

Thus we should build some mechanism to add highlighting for a line AND keep it's formatting apart from the added highlighting.

[BenBE]

Idea for a patch:

diff --git a/Panel.c b/Panel.c
index fd9de2b..831452f 100644
--- a/Panel.c
+++ b/Panel.c
@@ -278,7 +278,7 @@ void Panel_draw(Panel* this, bool focus) {
          }
          if (item.highlightAttr) {
             attrset(item.highlightAttr);
-            RichString_setAttr(&item, item.highlightAttr);
+            RichString_setAttr_preserveBold(&item, item.highlightAttr);
             this->selectedLen = itemLen;
          }
          mvhline(y + line, x, ' ', this->w);
diff --git a/RichString.c b/RichString.c
index 904b44b..1a0d6ce 100644
--- a/RichString.c
+++ b/RichString.c
@@ -69,6 +69,15 @@ inline void RichString_setAttrn(RichString* this, int attrs, int start, int fini
    }
 }
 
+inline void RichString_setAttrn_preserveBold(RichString* this, int attrs, int start, int finish) {
+   cchar_t* ch = this->chptr + start;
+   finish = CLAMP(finish, 0, this->chlen - 1);
+   for (int i = start; i <= finish; i++) {
+      ch->attr = (ch->attr & A_BOLD) ? (attrs | A_BOLD) : attrs;
+      ch++;
+   }
+}
+
 int RichString_findChar(RichString* this, char c, int start) {
    wchar_t wc = btowc(c);
    cchar_t* ch = this->chptr + start;
@@ -100,6 +109,15 @@ void RichString_setAttrn(RichString* this, int attrs, int start, int finish) {
    }
 }
 
+void RichString_setAttrn_preserveBold(RichString* this, int attrs, int start, int finish) {
+   chtype* ch = this->chptr + start;
+   finish = CLAMP(finish, 0, this->chlen - 1);
+   for (int i = start; i <= finish; i++) {
+      *ch = (*ch & 0xff) | attrs | (*ch & A_BOLD);
+      ch++;
+   }
+}
+
 int RichString_findChar(RichString* this, char c, int start) {
    chtype* ch = this->chptr + start;
    for (int i = start; i < this->chlen; i++) {
@@ -123,6 +141,10 @@ void RichString_setAttr(RichString* this, int attrs) {
    RichString_setAttrn(this, attrs, 0, this->chlen - 1);
 }
 
+void RichString_setAttr_preserveBold(RichString* this, int attrs) {
+   RichString_setAttrn_preserveBold(this, attrs, 0, this->chlen - 1);
+}
+
 void RichString_append(RichString* this, int attrs, const char* data) {
    RichString_writeFrom(this, attrs, data, this->chlen, strlen(data));
 }
diff --git a/RichString.h b/RichString.h
index 12b0954..c690f8c 100644
--- a/RichString.h
+++ b/RichString.h
@@ -44,12 +44,16 @@ typedef struct RichString_ {
 
 void RichString_setAttrn(RichString* this, int attrs, int start, int finish);
 
+void RichString_setAttrn_preserveBold(RichString* this, int attrs, int start, int finish);
+
 int RichString_findChar(RichString* this, char c, int start);
 
 void RichString_prune(RichString* this);
 
 void RichString_setAttr(RichString* this, int attrs);
 
+void RichString_setAttr_preserveBold(RichString* this, int attrs);
+
 void RichString_append(RichString* this, int attrs, const char* data);
 
 void RichString_appendn(RichString* this, int attrs, const char* data, int len);

[cgzones]

On first look the patch looks reasonable; mind to create a pull request?

[BenBE]

The issue with this patch is some runtime behaviour that's kinda nasty IMO: A_BOLD automatically means light colors, which reduces contrast quite a bit. In particular with dark gray on cyan (for e.g. the basename) this makes reading things hard.

I don't really mind making a PR for this, but I think this caveat should be mentioned and discussed.

[C0rn3j]

Branch - https://github.com/C0rn3j/htop/tree/preserve

Here's the current state in Konsole:

Here's the PR(edited to use one function only so it can compile) in Konsole with the default theme, where it looks terrible and I presume why it got denied:

• Library:
  
• Binary:
  

Here's the PR in VSC Terminal, which has different colors and actually looks quite fine:

• Library:
  

• Binary:
  

I don't get why the difference exists, as both report $TERM=xterm-256color - background color coming through and mixing?

EDIT: Yup, background color is a big part of it I suppose back to poking in it nope I just had the wrong htop running for the first screenshot, sigh:

EDIT2: I am now remembering that this is going to be a text color issue with bold text in regular terminals, innit.

An idea would be to change bolded chars for underline, couple problems with that:

• It loses the color, but at least keeps the highlight
• Currently, htop colors spaces
• It's very visually distracting as it flickers on the changing properties, would only be applicable to the unchanging ones like the yellow/red on library/binary mismatch - but again, we can't tell WHICH it is since we only have underline yes/no -> maybe that could be worked around by full underline for binary: ______ and dashed for library: _ _ _ _ _

EDIT3: Wait... why can't it be just bolded red without NOT being bolded red when the bg color is in effect....

[C0rn3j]

So after having a fun ncurses dig, here's a slightly less confused writeup for someone who wants to take a stab at it but wants to save some time from not knowing ncurses.

Ncurses requires all Foreground+Background combinations to have a registered, indexed ColorPair.

I believe the color index for selection highlight in question here is PANEL_SELECTION_FOCUS - its background color, for each theme where it is unique, needs to be taken and paired for each other element's FG.
There are at least two more if highlight_changes=1 is enabled, red and green by default.

The original PR simply ends up adding bold against whatever the PANEL_SELECTION_FOCUS FG value is, in which case it was black for the default theme, so it ends up looking really washed out against cyan on some terminal setups where it makes the text brighter.

Crt.c needs to be edited to support this(current code):

void CRT_setColors(int colorScheme) {
   CRT_colorScheme = colorScheme;

   for (short int i = 0; i < 8; i++) {
      for (short int j = 0; j < 8; j++) {
         if (ColorIndex(i, j) != ColorIndexGrayBlack && ColorIndex(i, j) != ColorIndexWhiteDefault) {
            short int bg = (colorScheme != COLORSCHEME_BLACKNIGHT) && (j == 0) ? -1 : j;
            init_pair(ColorIndex(i, j), i, bg);
         }
      }
   }

   short int grayBlackFg = COLORS > 8 ? 8 : 0;
   short int grayBlackBg = (colorScheme != COLORSCHEME_BLACKNIGHT) ? -1 : 0;
   init_pair(ColorIndexGrayBlack, grayBlackFg, grayBlackBg);

   init_pair(ColorIndexWhiteDefault, White, -1);

   CRT_colors = CRT_colorSchemes[colorScheme];
}

i.e. default theme is Black, Cyan, so we need to create every <Color>, Cyan combination for all elements in some predetermined fashion so we can refer to it later.

The idea from the original PR then needs to be taken and implemented in a way where everything except BG color(coming from the highlight) is preserved in the original.

After those code issues are solved, the remaining problem might be that some FG colors will look bad against the various PANEL_SELECTION_FOCUS+dead/alive BGs, they might need slight changes.
Or maybe some designer who understand color theory + terminal rendering might chime in with a deterministic solution where the colors won't end up looking poorly.

This is not helped by the fact that htop relies on legacy methods in ncurses color functions, making it a bit annoying to work with this: #1541, so ideally that gets fixed first.

---

Now, if we want to MODIFY the text attributes, as was done in the original PR if we detect bold or something, we can do that without new definitions as changing attributes does not require registering anything, but that is far from an optimal solution.

STANDOUT + making sure text that is spaces does not have set params like "bold" seems like the best candidate if this sub-par option is desired as a workaround for now:

Slightly less subpar option that would not require any large changes AND would definitely look correct would be to just not apply background to special text(note this is a quick demo hack that does not work on non-bolded colored text):

If the left-out chars are underlined, it even looks quite decent as is:

Which just made me notice that bold text overflows the bg of the next char on VSC terminal somehow, which I presume is a VSC bug:

On Konsole it works fine (underline is intended):

#define WA_ATTRIBUTES	A_ATTRIBUTES
#define WA_NORMAL	A_NORMAL
#define WA_STANDOUT	A_STANDOUT
#define WA_UNDERLINE	A_UNDERLINE
#define WA_REVERSE	A_REVERSE
#define WA_BLINK	A_BLINK
#define WA_DIM		A_DIM
#define WA_BOLD		A_BOLD
#define WA_ALTCHARSET	A_ALTCHARSET
#define WA_INVIS	A_INVIS
#define WA_PROTECT	A_PROTECT
#define WA_HORIZONTAL	A_HORIZONTAL
#define WA_LEFT		A_LEFT
#define WA_LOW		A_LOW
#define WA_RIGHT	A_RIGHT
#define WA_TOP		A_TOP
#define WA_VERTICAL	A_VERTICAL
#if 1
#define WA_ITALIC	A_ITALIC	/* ncurses extension */
#endif

EDIT:

Actually, throwing away the FG+BG change and setting STANDOUT seems quite decent on my terminals! *

* still does not apply for colors without bold in this demo

When text color matches highlight, it does not work as well:

[C0rn3j]

https://github.com/C0rn3j/htop/tree/preserve-test

So, I have another testing branch, very messy, where I got the colors to preserve, properly this time, without attribute hacks.

It is a pain, because I have half-hacked the codebase to work with the newer way to handle colors.

It looks bad at the moment, as expected.

We can at least bold the text that has the same color as bg to make it slightly less terrible, but it won't really help, the colors need to change.

Now, the good news is that htop assumes we have 16 colors to work with, just like in ye olden times. (2x8, one bold|intensified version, which is apparently why some terminals highlight and make it bright, and some bold instead).

We should actually have 256 at worst (someone please tell me that BSD is not stuck with 16 colors for some reason), and if I understand correctly, we should actually have 16777216 on modern terminals which support True Color/Direct Color for 24-bit color support -> that's good old 8-bit RGB.

We are still limited by ncurses at 65k~ colors on the screen at one time, but I don't suppose that's an issue for theming htop.

There are many ways to skin this particular cat - we can start changing text colors, or we can start changing the highlight colors(of which I understand there are five), or both.

TL;DR POC code is there, but someone now needs to redesign how htop uses colors and change all the themes.

Honestly, it could be as easy as redefining the colors for the 5 highlight bars to use something acceptable from the 256 color range instead of 16.

One can easily test that by adding to the K parameter in my testing branch -> init_extended_pair(1000 + i + (j*10) + (k*100), i, k+12);

EDIT: It should be possible to just use truecolor/directcolor, as terminals will downgrade to the best available option.

https://gist.github.com/kurahaupo/6ce0eaefe5e730841f03cb82b061daa2

tty:

Konsole: