CRT: Add direct color scheme support for TERM=*-direct - #2072
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. 📝 WalkthroughWalkthroughAdds direct-color support to CRT. The implementation detects terminal RGB support, exposes Poem
Merge Risk: 🟡 Moderate · up to The new Direct color scheme can leave the setup panel pointing at an invalid scheme entry, causing incorrect behavior or a crash. Merge should wait until the scheme lists are kept consistent or this risk is explicitly accepted. 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: a7ac5a30-8bc8-4218-997c-e1d74e6336d3
📒 Files selected for processing (2)
CRT.cCRT.h
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review.
| if (term_type_len >= suffix_len && strcmp(termType + term_type_len - suffix_len, suffix) == 0) { | ||
| CRT_hasDirectColors = true; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Please follow the project’s conditional style in both direct-color checks: use String_eq() instead of raw strcmp(...) == 0, and omit braces when the conditional body contains only the single assignment. Apply the brace change here and at the corresponding check near lines 1488–1490.
📍 Affects 1 file
CRT.c#L1350-L1352(this comment)CRT.c#L1488-L1490
Source: Coding guidelines
…getflag for true RGB support
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CRT.c (1)
961-1077: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the
COLORSCHEME_DIRECTentry toColorsPanel.
ColorSchemeNames[]has no entry forCOLORSCHEME_DIRECT. Therefore,ColorsPanel_new()fails its size assertion, and a non-assert build can callPanel_get(super, 8)without a matching item. Add the entry and restrict it to terminals with direct-color support, or keep the internal active scheme separate from the panel index.Source: MCP tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35192a0d-6cab-40d5-be93-434c983791e5
📒 Files selected for processing (1)
CRT.c
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review.
…tended API in preprocessor guards
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CRT.c (1)
1474-1499: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate direct-color detection on extended-color support.
When
NCURSES_EXT_COLORSis unavailable,tigetflag("RGB")can still selectCOLORSCHEME_DIRECT, while the fallback initializes indexed pairs. When onlyNCURSES_EXT_FUNCSis enabled,init_extended_pair()may also lack a library implementation. Require both macros for direct-color detection and pair initialization; otherwise select the indexed scheme.Source: MCP tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3afc6cb4-9514-4f2c-824f-bd6566489f1b
📒 Files selected for processing (2)
CRT.cColorsPanel.c
Included review availability: Your plan includes up to 8 reviews per rolling hour; 6 remain after this review.
|
Previous PR on the same topic: #1926 |
|
Thanks for pointing that out! I reviewed #1926. It looks like that PR closed, so I hope this new approach can help finally get this feature merged. In this implementation, I focused heavily on safety: it uses tigetflag("RGB") for dynamic detection, includes runtime fallbacks to COLORSCHEME_DEFAULT for non-RGB terminals, and adds compile-time guards for older ncurses versions to prevent build failure. |
|
Read and tested this on Linux (Alder Lake-P laptop, ncurses 6.4, Zorin/Ubuntu). Not a maintainer, Test harness, since it affects what you can trust below. This box has no
The one thing I would fix before this lands: Default becomes unreachable
if (colorScheme == COLORSCHEME_DEFAULT && CRT_hasDirectColors)
colorScheme = COLORSCHEME_DIRECT;so on any terminal whose terminfo has Those four triples are exactly The promotion is also the only asymmetric part. Nord under the same terminal is untouched The reverse direction is fine and worth keeping: The
|
- Make COLORSCHEME_DIRECT strictly opt-in by removing auto-promotion from Default.
- Remove unreachable '#else' fallback block, as the downgrade guard already handles non-RGB terminals.
- Fix accidental assignment ('=') to comparison ('==') in the downgrade guard.
- Fix indentation to 3 spaces and remove braces for the single-statement block per coding guidelines.
- Remove stray blank line in CRT_init.
|
Thank you for the incredibly detailed testing and feedback! I really appreciate the time you took to compile a local terminfo and verify the truecolor sequences. This was helpful. You brought up good points, and I have pushed a commit to address them:
Let me know if everything looks good to you now. |
|
Re-tested The promotion is gone, and Default is back to being DefaultEach run is
The counts differ run to run because the process list does; the set of triples is the part that matters. With Dropping the
|
|
Thanks for the detailed testing and validation. It's good to confirm that the fallback logic, preprocessor guards, and UI panel are behaving as expected across environments. Regarding the |
Description
This pull request adds true 24-bit direct color (RGB) support for modern terminals to
htop.-directsuffix could suffer from inconsistent styling, blocky background artifacts, or require forced fallback behavior to legacy 16-color palettes.COLORSCHEME_DIRECT) alongside safe detection and fallback mechanisms to fully utilize modern terminal capabilities without breaking older environments.Key Implementation Details
TrueColor Integration: Transitioned from the extended 256-color palette to exact 24-bit RGB hex values using the ncurses extended API (
init_extended_pair). This ensures crisp, vibrant colors (e.g., warm orange, neon cyan) with correct transparent backgrounds.Dynamic Capability Detection: Instead of strictly matching the
TERMsuffix, the implementation usestigetflag("RGB")to accurately query the terminal's actual direct-color capabilities.Robust Fallbacks & Safety Guards:
COLORSCHEME_DIRECTis loaded from a user'shtoprcconfiguration file, but they launchhtopin a non-RGB terminal (e.g., standardxtermorvt100), the application safely downgrades toCOLORSCHEME_DEFAULTto prevent rendering crashes or blank screens.#if defined(NCURSES_EXT_FUNCS) && defined(NCURSES_EXT_COLORS)) to ensurehtopcontinues to compile flawlessly on older systems with legacyncurseslibraries.UI Integration: Added the new Direct scheme to
ColorsPanel.c, allowing users to naturally select it from the Setup (F2) menu.Visual Comparison
TERM=xterm-direct htop:TERM=xterm-direct ./htop, the interface uses our enhanced palette layout with correct styling and transparency: