CRT: fix invisible gray color on direct-color terminals - #2079
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
Assessment against linked issues
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10aa91cb-2613-4b3a-93f9-6ea7726dc7d0
📒 Files selected for processing (3)
CRT.cCRT.hColorsPanel.c
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| "Black Night", | ||
| "Broken Gray", | ||
| "Nord", | ||
| "Direct", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the selected scheme consistent with the applied scheme.
On a terminal without RGB support, this new entry can be selected and persisted as COLORSCHEME_DIRECT. CRT_setColors() then applies COLORSCHEME_DEFAULT at CRT.c Line 1479. The panel check state and saved setting can therefore disagree with the active colors.
After CRT_setColors(mark), update the selected item and settings->colorScheme from CRT_colorScheme. Alternatively, disable this entry when CRT_hasDirectColors is false.
| #if defined(NCURSES_EXT_FUNCS) && defined(NCURSES_EXT_COLORS) | ||
| if (CRT_hasDirectColors) { | ||
| int grayBlackBg = (colorScheme != COLORSCHEME_BLACKNIGHT) ? -1 : 0; | ||
| init_extended_pair(ColorIndexGrayBlack, 0x949494, grayBlackBg); | ||
| } else | ||
| #endif | ||
| { | ||
| short int grayBlackFg = COLORS > 8 ? 8 : 0; | ||
| short int grayBlackBg = (colorScheme != COLORSCHEME_BLACKNIGHT) ? -1 : 0; | ||
| init_pair(ColorIndexGrayBlack, grayBlackFg, grayBlackBg); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the specified light-gray RGB value.
Line 1508 uses 0x949494. The PR objective requires 0xA0A0A0. This value controls the gray elements that this change fixes.
Proposed fix
- init_extended_pair(ColorIndexGrayBlack, 0x949494, grayBlackBg);
+ init_extended_pair(ColorIndexGrayBlack, 0xA0A0A0, grayBlackBg);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if defined(NCURSES_EXT_FUNCS) && defined(NCURSES_EXT_COLORS) | |
| if (CRT_hasDirectColors) { | |
| int grayBlackBg = (colorScheme != COLORSCHEME_BLACKNIGHT) ? -1 : 0; | |
| init_extended_pair(ColorIndexGrayBlack, 0x949494, grayBlackBg); | |
| } else | |
| #endif | |
| { | |
| short int grayBlackFg = COLORS > 8 ? 8 : 0; | |
| short int grayBlackBg = (colorScheme != COLORSCHEME_BLACKNIGHT) ? -1 : 0; | |
| init_pair(ColorIndexGrayBlack, grayBlackFg, grayBlackBg); | |
| } | |
| #if defined(NCURSES_EXT_FUNCS) && defined(NCURSES_EXT_COLORS) | |
| if (CRT_hasDirectColors) { | |
| int grayBlackBg = (colorScheme != COLORSCHEME_BLACKNIGHT) ? -1 : 0; | |
| init_extended_pair(ColorIndexGrayBlack, 0xA0A0A0, grayBlackBg); | |
| } else | |
| #endif | |
| { | |
| short int grayBlackFg = COLORS > 8 ? 8 : 0; | |
| short int grayBlackBg = (colorScheme != COLORSCHEME_BLACKNIGHT) ? -1 : 0; | |
| init_pair(ColorIndexGrayBlack, grayBlackFg, grayBlackBg); | |
| } |
2c42504 to
8719305
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 06318ec2-9756-46a5-9d56-e2d546a938f1
📒 Files selected for processing (1)
CRT.c
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
8719305 to
9123981
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0b96d63-4dad-4f4e-8ced-666155a38887
📒 Files selected for processing (2)
CRT.cconfigure.ac
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
9123981 to
0c649cf
Compare
|
Tested this on the same harness I offered in #2078: a locally compiled The fix works, on every affected schemeSame htoprc, same 40x160 screen, only
Totals move run to run because the process list does, so read the zeros and not the differences. The 256-colour path is untouched: Four things I checked because they were the ways this could go wrongThe bold attribute survives. The default background survives The scope is complete, not partial. Index 8 really was the only palette value htop ever asks for above 7: the pair loop at All four direct variants, not just the one I built for. Using ncurses 6.4's own terminfo database:
Worth knowing if you reproduce this: Two things to raise, neither of them blocking
The coordination point is #2072, not correctness. That PR adds
3.03:1 is below the 4.5:1 WCAG AA threshold for text. I want to be clear that this is arithmetic and not observation: I have no light-background direct-colour terminal here and did not see it rendered. If the value ends up scheme-dependent, or simply darker for Light Terminal and Black on White, the measurement above is the reason. If it is deliberate, a one-line comment naming the value would stop the next reader asking. Everything else looks right to me, and thank you for turning this around so quickly after #2078. |
0c649cf to
8cf4e50
Compare
|
Re-tested Per scheme,
|
| scheme | 0x5F5F5F |
0x949494 |
RGB(0,0,8) |
other triples |
|---|---|---|---|---|
| Default | 0 | 89 | 0 | none |
| Light Terminal | 96 | 0 | 0 | none |
| Midnight | 0 | 0 | 0 | none |
| Black Night | 0 | 16 | 0 | none |
| Nord | 0 | 85 | 0 | none |
| Monochrome, Black on White, Broken Gray | 0 | 0 | 0 | none |
So exactly one scheme moved, and it is the one that needed to. xterm-256color is still untouched on this head: ESC[90m and zero truecolor sequences on Default, Light Terminal and Nord. Clean build under htop's full warning set.
The counts above are what reaches the screen with my meter and column layout, not the number of elements in each scheme, which is why Midnight shows 0 despite having 2. Reading the scheme tables gives 9 / 0 / 0 / 8 / 2 / 5 / 0 / 18 references to ColorPairGrayBlack, matching the table in #2078 exactly.
The contrast, now
| foreground | on white | on black |
|---|---|---|
0x5F5F5F, light schemes |
6.39:1 | 3.29:1 |
0x949494, everything else |
3.03:1 | 6.92:1 |
Light Terminal goes from 3.03:1 to 6.39:1 against white, which clears AA with room. That was the ask and it is answered.
Worth saying out loud, since it is the other half of the same trade: a user running Light Terminal on a dark terminal now gets 3.29:1 where this morning's head gave them 6.92:1. I do not think that is worth changing. The scheme name is the only statement about the background that htop has, -1 leaves the actual background to the terminal, and someone picking Light Terminal on a dark terminal has already made a stranger choice than this. Recording the number so nobody discovers it as a surprise later.
Two small things
The COLORSCHEME_BLACKONWHITE arm is unreachable. That scheme's table has zero references to ColorPairGrayBlack, and it emitted zero sequences of either shade at runtime. So the arm costs nothing and does nothing. Fine as future-proofing if that is the intent, in which case a short comment would stop the next reader testing it the way I just did; otherwise it can go and the condition reads shorter.
Default on a light terminal is the case the heuristic still does not cover. It keeps 0x949494 at 3.03:1 against white. I would leave it: the answer for that user is to pick Light Terminal, which is what the scheme is for. Noting it only so the boundary of the fix is written down.
Nothing here blocks anything from my side, and the answer on the maintainer's actual question, whether to converge with #2072 on one direct-colour predicate and one capability check, has not changed.
8cf4e50 to
a471831
Compare
This PR fixes the issue where gray elements render as
RGB(0,0,8)(effectively black) on direct-color terminals, causing them to disappear against a black background.Changes
init_pairfallback forColorIndexGrayBlackwithinit_extended_pairwhenCOLORS >= 16777216. This makes the fix completely independent.0x949494(a highly visible 24-bit light gray) instead of the index8.ncursesbuilds.Visual Changes
RGB(0,0,8), making them virtually invisible.0x949494), restoring high contrast and readability for standard schemes on-directterminals.Fixes #2078