feat(tui): add theme-aware diff backgrounds with capability-graded palettes#12581
Conversation
Add subtle RGB background tints on diff lines that adapt to terminal background lightness. Dark terminals get #212922 (green) for additions and #3C170F (red) for removals; light terminals get GitHub-style pastel tints with contrasting gutter backgrounds. Use Color::Rgb directly instead of best_color to avoid supports_color detection issues in the TUI alternate screen. Refactor diff styling into a DiffTheme enum with per-kind style helpers for line backgrounds, gutter, sign characters, and content.
There was a problem hiding this comment.
Pull request overview
Adds theme-aware diff rendering in the TUI by selecting a light/dark palette from the terminal background and applying full-width background tints plus improved gutter/sign styling for diff lines.
Changes:
- Introduces a
DiffTheme(Light/Dark) derived fromdefault_bg()andis_light(). - Adds RGB palette constants and style helpers to drive gutter/sign/content/background styling per
(theme, DiffLineType). - Applies line-level background styling via
RtLine::style()and adds unit tests for light/dark styling and wrapped lines.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add `preserve_side_content_bg` to `SelectionViewParams` and gate the side-panel background reset in `ListSelectionView` behind that flag. This keeps existing popup behavior unchanged by default. Enable the flag for `theme_picker` so preview rows rendered via `push_wrapped_diff_line*` keep their add/delete background fills, matching recent theme-aware diff rendering updates.
Use fixed add/delete palette pairs per terminal color capability in diff rendering and theme picker previews. The renderer now chooses explicit colors for truecolor, 256-color, and 16-color modes instead of nearest-color quantization for these backgrounds. This keeps insertion and deletion backgrounds visually distinct in 256-color terminals (including tmux/xterm-256color sessions) and keeps preview styling aligned with live diff rendering.
fcoury
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d05106691f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
etraut-openai
left a comment
etraut-openai
left a comment
There was a problem hiding this comment.
I like this solution.
The code lgtm. Codex left one code review comment that looks potentially legit. Please review and optionally fix that before merging.
I am on it. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Problem
Diff lines used only foreground colors (green/red) with no background tinting, making them hard to scan. The gutter (line numbers) also had no theme awareness — dimmed text was fine on dark terminals but unreadable on light ones.
Mental model
Each diff line now has four styled layers: gutter (line number), sign (
+/-), content (text), and line background (full terminal width). ADiffThemeenum (Dark/Light) is selected once per render by probing the terminal's queried background viadefault_bg(). A companionDiffColorLevelenum (TrueColor/Ansi256/Ansi16) is derived fromstdout_color_level()and gates which palette is used. All style helpers dispatch on(theme, DiffLineType, color_level)to pick the right colors.Non-goals
default_bg()already provides.Tradeoffs
best_colornearest-match. This is deliberate:supports_color::on_cached(Stream::Stdout)can misreport capabilities once crossterm enters the alternate screen, so hand-picked palette entries give better visual results than automatic quantization.Modifier::DIMto visually recede compared to insert lines. This trades some readability of deleted code for scan-ability of additions.preserve_side_content_bg: trueonListSelectionViewso diff background tints survive into the side panel. Other popups keep the default (false) to preserve their reset-background look.Architecture
constitems grouped by palette tier:DARK_TC_*/LIGHT_TC_*(truecolor RGB tuples),DARK_256_*/LIGHT_256_*(xterm indexed), with namedColorvariants used for the 16-color tier.DiffThemeis a private enum;diff_theme()probes the terminal anddiff_theme_for_bg()is the testable pure-function version.DiffColorLevelis a private enum derived fromStdoutColorLevelviadiff_color_level().add_line_bg,del_line_bg,light_gutter_fg,light_add_num_bg,light_del_num_bg) each take(DiffTheme, DiffColorLevel)or justDiffColorLeveland return aColor.style_line_bg_for,style_gutter_for,style_sign_add,style_sign_del,style_add,style_del) each take(DiffLineType, DiffTheme, DiffColorLevel)or(DiffTheme, DiffColorLevel)and return aStyle.push_wrapped_diff_line_inner_with_theme_and_color_levelis the innermost renderer, accepting both theme and color level so tests can exercise any combination without depending on the terminal.RtLine::from(...).style(line_bg)so the tint extends across the full terminal width, not just the text content.ListSelectionViewgained apreserve_side_content_bgflag. Whentrue, the side panel skipsforce_bg_to_terminal_bg, letting diff preview backgrounds render faithfully.Observability
No new logging. Theme selection is deterministic from
default_bg(), which is already queried and cached at TUI startup.Tests
DiffThemeis determined perrender_changecall — ifdefault_bg()changes mid-render (e.g.requery_default_colors()fires), different file chunks could render with different themes. Low risk in practice since re-query only happens on explicit user action.Colorvariants (Color::Green,Color::Red, etc.) which the terminal maps to its own palette. On unusual terminal themes these could clash with the background. Acceptable since 16-color terminals already have unpredictable color rendering.style_add/style_delset bg but no fg — on light terminals, non-syntax-highlighted content uses the terminal's default foreground against a pastel background. If the terminal's default fg happens to be very light, contrast could suffer. This is an edge case since light-terminal users typically have dark default fg.preserve_side_content_bgis a general-purpose flag but only used by the theme picker — if other popups start using side content with intentional backgrounds they'll need to opt in explicitly. Not a real risk today, just a note for future callers.