From 061d1d3b5e55bcdf12865625fdde7f9757b6fa46 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Tue, 24 Feb 2026 16:55:01 -0300 Subject: [PATCH] feat(tui): add theme-aware diff backgrounds with capability-graded palettes (#12581) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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). A `DiffTheme` enum (`Dark` / `Light`) is selected once per render by probing the terminal's queried background via `default_bg()`. A companion `DiffColorLevel` enum (`TrueColor` / `Ansi256` / `Ansi16`) is derived from `stdout_color_level()` and gates which palette is used. All style helpers dispatch on `(theme, DiffLineType, color_level)` to pick the right colors. | Theme Picker Wide | Theme Picker Narrow | |---|---| | image | image | | Dark BG - 16 colors | Dark BG - 256 colors | Dark BG - True Colors | |---|---|---| | dark-16colors | dark-256colors | dark-truecolor | | Light BG - 16 colors | Light BG - 256 colors | Light BG - True Colors | |---|---|---| | light-16colors | light-256colors | light-truecolor | ## Non-goals - No runtime theme switching beyond what `default_bg()` already provides. - No change to syntax highlighting theme selection or the highlight module. ## Tradeoffs - Three fixed palettes (truecolor RGB, 256-color indexed, 16-color named) are maintained rather than using `best_color` nearest-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. - Delete lines in the syntax-highlighted path get `Modifier::DIM` to visually recede compared to insert lines. This trades some readability of deleted code for scan-ability of additions. - The theme picker's diff preview sets `preserve_side_content_bg: true` on `ListSelectionView` so diff background tints survive into the side panel. Other popups keep the default (`false`) to preserve their reset-background look. ## Architecture - **Color constants** are module-level `const` items grouped by palette tier: `DARK_TC_*` / `LIGHT_TC_*` (truecolor RGB tuples), `DARK_256_*` / `LIGHT_256_*` (xterm indexed), with named `Color` variants used for the 16-color tier. - **`DiffTheme`** is a private enum; `diff_theme()` probes the terminal and `diff_theme_for_bg()` is the testable pure-function version. - **`DiffColorLevel`** is a private enum derived from `StdoutColorLevel` via `diff_color_level()`. - **Palette helpers** (`add_line_bg`, `del_line_bg`, `light_gutter_fg`, `light_add_num_bg`, `light_del_num_bg`) each take `(DiffTheme, DiffColorLevel)` or just `DiffColorLevel` and return a `Color`. - **Style helpers** (`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 a `Style`. - **`push_wrapped_diff_line_inner_with_theme_and_color_level`** is the innermost renderer, accepting both theme and color level so tests can exercise any combination without depending on the terminal. - **Line-level background** is applied via `RtLine::from(...).style(line_bg)` so the tint extends across the full terminal width, not just the text content. - **Theme picker integration**: `ListSelectionView` gained a `preserve_side_content_bg` flag. When `true`, the side panel skips `force_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 1. **`DiffTheme` is determined per `render_change` call** — if `default_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. 2. **16-color tier uses named `Color` variants** (`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. 3. **Light-theme `style_add` / `style_del` set 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. 4. **`preserve_side_content_bg` is 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. --- .../src/bottom_pane/list_selection_view.rs | 101 ++- codex-rs/tui/src/diff_render.rs | 636 +++++++++++++++--- codex-rs/tui/src/terminal_palette.rs | 43 +- codex-rs/tui/src/theme_picker.rs | 1 + 4 files changed, 683 insertions(+), 98 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 2f566079a..9a50a0783 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -163,6 +163,10 @@ pub(crate) struct SelectionViewParams { /// When absent, `side_content` is reused. pub stacked_side_content: Option>, + /// Keep side-content background colors after rendering in side-by-side mode. + /// Disabled by default so existing popups preserve their reset-background look. + pub preserve_side_content_bg: bool, + /// Called when the highlighted item changes (navigation, filter, number-key). /// Receives the *actual* item index, not the filtered/visible index. pub on_selection_changed: OnSelectionChangedCallback, @@ -189,6 +193,7 @@ impl Default for SelectionViewParams { side_content_width: SideContentWidth::default(), side_content_min_width: 0, stacked_side_content: None, + preserve_side_content_bg: false, on_selection_changed: None, on_cancel: None, } @@ -220,6 +225,7 @@ pub(crate) struct ListSelectionView { side_content_width: SideContentWidth, side_content_min_width: u16, stacked_side_content: Option>, + preserve_side_content_bg: bool, /// Called when the highlighted item changes (navigation, filter, number-key). on_selection_changed: OnSelectionChangedCallback, @@ -271,6 +277,7 @@ impl ListSelectionView { side_content_width: params.side_content_width, side_content_min_width: params.side_content_min_width, stacked_side_content: params.stacked_side_content, + preserve_side_content_bg: params.preserve_side_content_bg, on_selection_changed: params.on_selection_changed, on_cancel: params.on_cancel, }; @@ -905,15 +912,17 @@ impl Renderable for ListSelectionView { ), ); self.side_content.render(side_area, buf); - Self::force_bg_to_terminal_bg( - buf, - Rect::new( - clear_x, - outer_content_area.y, - clear_w, - outer_content_area.height, - ), - ); + if !self.preserve_side_content_bg { + Self::force_bg_to_terminal_bg( + buf, + Rect::new( + clear_x, + outer_content_area.y, + clear_w, + outer_content_area.height, + ), + ); + } } else if stacked_side_area.height > 0 { // Stacked fallback: render below the list (same as old footer_content). let clear_height = (outer_content_area.y + outer_content_area.height) @@ -1004,6 +1013,28 @@ mod tests { } } + struct StyledMarkerRenderable { + marker: &'static str, + style: Style, + height: u16, + } + + impl Renderable for StyledMarkerRenderable { + fn render(&self, area: Rect, buf: &mut Buffer) { + for y in area.y..area.y.saturating_add(area.height) { + for x in area.x..area.x.saturating_add(area.width) { + if x < buf.area().width && y < buf.area().height { + buf[(x, y)].set_symbol(self.marker).set_style(self.style); + } + } + } + } + + fn desired_height(&self, _width: u16) -> u16 { + self.height + } + } + fn make_selection_view(subtitle: Option<&str>) -> ListSelectionView { let (tx_raw, _rx) = unbounded_channel::(); let tx = AppEventSender::new(tx_raw); @@ -1143,6 +1174,58 @@ mod tests { assert!(rendered.contains("Move up/down to live preview themes")); } + #[test] + fn theme_picker_enables_side_content_background_preservation() { + let params = crate::theme_picker::build_theme_picker_params(None, None, Some(120)); + assert!( + params.preserve_side_content_bg, + "theme picker should preserve side-content backgrounds to keep diff preview styling", + ); + } + + #[test] + fn preserve_side_content_bg_keeps_rendered_background_colors() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let view = ListSelectionView::new( + SelectionViewParams { + title: Some("Debug".to_string()), + items: vec![SelectionItem { + name: "Item 1".to_string(), + dismiss_on_select: true, + ..Default::default() + }], + side_content: Box::new(StyledMarkerRenderable { + marker: "+", + style: Style::default().bg(Color::Blue), + height: 1, + }), + side_content_width: SideContentWidth::Half, + side_content_min_width: 10, + preserve_side_content_bg: true, + ..Default::default() + }, + tx, + ); + let area = Rect::new(0, 0, 120, 35); + let mut buf = Buffer::empty(area); + + view.render(area, &mut buf); + + let plus_bg = (0..area.height) + .flat_map(|y| (0..area.width).map(move |x| (x, y))) + .find_map(|(x, y)| { + let cell = &buf[(x, y)]; + (cell.symbol() == "+").then(|| cell.style().bg) + }) + .expect("expected side content to render at least one '+' marker"); + assert_eq!( + plus_bg, + Some(Color::Blue), + "expected side-content marker to preserve custom background styling", + ); + } + #[test] fn snapshot_footer_note_wraps() { let (tx_raw, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index aa0481d04..32591945f 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -7,6 +7,13 @@ //! is present, the content text is syntax-highlighted using //! [`crate::render::highlight`]. //! +//! **Theme-aware styling:** diff backgrounds adapt to the terminal's +//! background lightness via [`DiffTheme`]. Dark terminals get muted tints +//! (`#212922` green, `#3C170F` red); light terminals get GitHub-style pastels +//! with distinct gutter backgrounds for contrast. The renderer uses fixed +//! palettes for truecolor / 256-color / 16-color terminals so add/delete lines +//! remain visually distinct even when quantizing to limited palettes. +//! //! **Highlighting strategy for `Update` diffs:** the renderer highlights each //! hunk as a single concatenated block rather than line-by-line. This //! preserves syntect's parser state across consecutive lines within a hunk @@ -37,6 +44,31 @@ use unicode_width::UnicodeWidthChar; /// Display width of a tab character in columns. const TAB_WIDTH: usize = 4; +// -- Diff background palette -------------------------------------------------- +// +// Dark-theme tints are subtle enough to avoid clashing with syntax colors. +// Light-theme values match GitHub's diff colors for familiarity. The gutter +// (line-number column) uses slightly more saturated variants on light +// backgrounds so the numbers remain readable against the pastel line background. +// Truecolor palette. +const DARK_TC_ADD_LINE_BG_RGB: (u8, u8, u8) = (33, 58, 43); // #213A2B +const DARK_TC_DEL_LINE_BG_RGB: (u8, u8, u8) = (74, 34, 29); // #4A221D +const LIGHT_TC_ADD_LINE_BG_RGB: (u8, u8, u8) = (218, 251, 225); // #dafbe1 +const LIGHT_TC_DEL_LINE_BG_RGB: (u8, u8, u8) = (255, 235, 233); // #ffebe9 +const LIGHT_TC_ADD_NUM_BG_RGB: (u8, u8, u8) = (172, 238, 187); // #aceebb +const LIGHT_TC_DEL_NUM_BG_RGB: (u8, u8, u8) = (255, 206, 203); // #ffcecb +const LIGHT_TC_GUTTER_FG_RGB: (u8, u8, u8) = (31, 35, 40); // #1f2328 + +// 256-color palette. +const DARK_256_ADD_LINE_BG_IDX: u8 = 22; +const DARK_256_DEL_LINE_BG_IDX: u8 = 52; +const LIGHT_256_ADD_LINE_BG_IDX: u8 = 194; +const LIGHT_256_DEL_LINE_BG_IDX: u8 = 224; +const LIGHT_256_ADD_NUM_BG_IDX: u8 = 157; +const LIGHT_256_DEL_NUM_BG_IDX: u8 = 217; +const LIGHT_256_GUTTER_FG_IDX: u8 = 236; + +use crate::color::is_light; use crate::exec_command::relativize_to_home; use crate::render::Insets; use crate::render::highlight::exceeds_highlight_limits; @@ -45,6 +77,11 @@ use crate::render::line_utils::prefix_lines; use crate::render::renderable::ColumnRenderable; use crate::render::renderable::InsetRenderable; use crate::render::renderable::Renderable; +use crate::terminal_palette::StdoutColorLevel; +use crate::terminal_palette::default_bg; +use crate::terminal_palette::indexed_color; +use crate::terminal_palette::rgb_color; +use crate::terminal_palette::stdout_color_level; use codex_core::git_info::get_git_repo_root; use codex_protocol::protocol::FileChange; @@ -60,6 +97,26 @@ pub(crate) enum DiffLineType { Context, } +/// Controls which color palette the diff renderer uses for backgrounds and +/// gutter styling. +/// +/// Determined once per `render_change` call via [`diff_theme`], which probes +/// the terminal's queried background color. When the background cannot be +/// determined (common in CI or piped output), `Dark` is used as the safe +/// default. +#[derive(Clone, Copy)] +enum DiffTheme { + Dark, + Light, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum DiffColorLevel { + TrueColor, + Ansi256, + Ansi16, +} + pub struct DiffSummary { changes: HashMap, cwd: PathBuf, @@ -243,6 +300,8 @@ fn render_change( width: usize, lang: Option<&str>, ) { + let theme = diff_theme(); + let color_level = diff_color_level(); match change { FileChange::Add { content } => { // Pre-highlight the entire file content as a whole. @@ -251,21 +310,26 @@ fn render_change( for (i, raw) in content.lines().enumerate() { let syn = syntax_lines.as_ref().and_then(|sl| sl.get(i)); if let Some(spans) = syn { - out.extend(push_wrapped_diff_line_with_syntax( + out.extend(push_wrapped_diff_line_inner_with_theme_and_color_level( i + 1, DiffLineType::Insert, raw, width, line_number_width, - spans, + Some(spans), + theme, + color_level, )); } else { - out.extend(push_wrapped_diff_line( + out.extend(push_wrapped_diff_line_inner_with_theme_and_color_level( i + 1, DiffLineType::Insert, raw, width, line_number_width, + None, + theme, + color_level, )); } } @@ -276,21 +340,26 @@ fn render_change( for (i, raw) in content.lines().enumerate() { let syn = syntax_lines.as_ref().and_then(|sl| sl.get(i)); if let Some(spans) = syn { - out.extend(push_wrapped_diff_line_with_syntax( + out.extend(push_wrapped_diff_line_inner_with_theme_and_color_level( i + 1, DiffLineType::Delete, raw, width, line_number_width, - spans, + Some(spans), + theme, + color_level, )); } else { - out.extend(push_wrapped_diff_line( + out.extend(push_wrapped_diff_line_inner_with_theme_and_color_level( i + 1, DiffLineType::Delete, raw, width, line_number_width, + None, + theme, + color_level, )); } } @@ -343,7 +412,10 @@ fn render_change( for h in patch.hunks() { if !is_first_hunk { let spacer = format!("{:width$} ", "", width = line_number_width.max(1)); - let spacer_span = RtSpan::styled(spacer, style_gutter()); + let spacer_span = RtSpan::styled( + spacer, + style_gutter_for(DiffLineType::Context, theme, color_level), + ); out.push(RtLine::from(vec![spacer_span, "⋮".dim()])); } is_first_hunk = false; @@ -374,66 +446,93 @@ fn render_change( diffy::Line::Insert(text) => { let s = text.trim_end_matches('\n'); if let Some(syn) = syntax_spans { - out.extend(push_wrapped_diff_line_with_syntax( - new_ln, - DiffLineType::Insert, - s, - width, - line_number_width, - syn, - )); + out.extend( + push_wrapped_diff_line_inner_with_theme_and_color_level( + new_ln, + DiffLineType::Insert, + s, + width, + line_number_width, + Some(syn), + theme, + color_level, + ), + ); } else { - out.extend(push_wrapped_diff_line( - new_ln, - DiffLineType::Insert, - s, - width, - line_number_width, - )); + out.extend( + push_wrapped_diff_line_inner_with_theme_and_color_level( + new_ln, + DiffLineType::Insert, + s, + width, + line_number_width, + None, + theme, + color_level, + ), + ); } new_ln += 1; } diffy::Line::Delete(text) => { let s = text.trim_end_matches('\n'); if let Some(syn) = syntax_spans { - out.extend(push_wrapped_diff_line_with_syntax( - old_ln, - DiffLineType::Delete, - s, - width, - line_number_width, - syn, - )); + out.extend( + push_wrapped_diff_line_inner_with_theme_and_color_level( + old_ln, + DiffLineType::Delete, + s, + width, + line_number_width, + Some(syn), + theme, + color_level, + ), + ); } else { - out.extend(push_wrapped_diff_line( - old_ln, - DiffLineType::Delete, - s, - width, - line_number_width, - )); + out.extend( + push_wrapped_diff_line_inner_with_theme_and_color_level( + old_ln, + DiffLineType::Delete, + s, + width, + line_number_width, + None, + theme, + color_level, + ), + ); } old_ln += 1; } diffy::Line::Context(text) => { let s = text.trim_end_matches('\n'); if let Some(syn) = syntax_spans { - out.extend(push_wrapped_diff_line_with_syntax( - new_ln, - DiffLineType::Context, - s, - width, - line_number_width, - syn, - )); + out.extend( + push_wrapped_diff_line_inner_with_theme_and_color_level( + new_ln, + DiffLineType::Context, + s, + width, + line_number_width, + Some(syn), + theme, + color_level, + ), + ); } else { - out.extend(push_wrapped_diff_line( - new_ln, - DiffLineType::Context, - s, - width, - line_number_width, - )); + out.extend( + push_wrapped_diff_line_inner_with_theme_and_color_level( + new_ln, + DiffLineType::Context, + s, + width, + line_number_width, + None, + theme, + color_level, + ), + ); } old_ln += 1; new_ln += 1; @@ -530,6 +629,56 @@ fn push_wrapped_diff_line_inner( width: usize, line_number_width: usize, syntax_spans: Option<&[RtSpan<'static>]>, +) -> Vec> { + push_wrapped_diff_line_inner_with_theme( + line_number, + kind, + text, + width, + line_number_width, + syntax_spans, + diff_theme(), + ) +} + +/// Core line renderer: builds one or more wrapped `RtLine`s for a single diff +/// line, applying gutter, sign, content, and full-width background styles +/// according to the given `theme`. +/// +/// Split out from `push_wrapped_diff_line_inner` so that tests can exercise +/// specific `DiffTheme` variants without depending on the terminal's queried +/// background. +fn push_wrapped_diff_line_inner_with_theme( + line_number: usize, + kind: DiffLineType, + text: &str, + width: usize, + line_number_width: usize, + syntax_spans: Option<&[RtSpan<'static>]>, + theme: DiffTheme, +) -> Vec> { + push_wrapped_diff_line_inner_with_theme_and_color_level( + line_number, + kind, + text, + width, + line_number_width, + syntax_spans, + theme, + diff_color_level(), + ) +} + +#[allow(clippy::too_many_arguments)] +fn push_wrapped_diff_line_inner_with_theme_and_color_level( + line_number: usize, + kind: DiffLineType, + text: &str, + width: usize, + line_number_width: usize, + syntax_spans: Option<&[RtSpan<'static>]>, + theme: DiffTheme, + color_level: DiffColorLevel, ) -> Vec> { let ln_str = line_number.to_string(); @@ -538,28 +687,37 @@ fn push_wrapped_diff_line_inner( let gutter_width = line_number_width.max(1); let prefix_cols = gutter_width + 1; - let (sign_char, line_style) = match kind { - DiffLineType::Insert => ('+', style_add()), - DiffLineType::Delete => ('-', style_del()), - DiffLineType::Context => (' ', style_context()), + let (sign_char, sign_style, content_style) = match kind { + DiffLineType::Insert => ( + '+', + style_sign_add(theme, color_level), + style_add(theme, color_level), + ), + DiffLineType::Delete => ( + '-', + style_sign_del(theme, color_level), + style_del(theme, color_level), + ), + DiffLineType::Context => (' ', style_context(), style_context()), }; + let line_bg = style_line_bg_for(kind, theme, color_level); + let gutter_style = style_gutter_for(kind, theme, color_level); + // When we have syntax spans, compose them with the diff style for a richer // view. The sign character keeps the diff color; content gets syntax colors // with an overlay modifier for delete lines (dim). if let Some(syn_spans) = syntax_spans { let gutter = format!("{ln_str:>gutter_width$} "); let sign = format!("{sign_char}"); - let dim_overlay = matches!(kind, DiffLineType::Delete); - - // Apply dim overlay to syntax spans if this is a delete line. let styled: Vec> = syn_spans .iter() .map(|sp| { - let mut style = sp.style; - if dim_overlay { - style.add_modifier |= Modifier::DIM; - } + let style = if matches!(kind, DiffLineType::Delete) { + sp.style.add_modifier(Modifier::DIM) + } else { + sp.style + }; RtSpan::styled(sp.content.clone().into_owned(), style) }) .collect(); @@ -576,22 +734,22 @@ fn push_wrapped_diff_line_inner( let mut row_spans: Vec> = Vec::new(); if i == 0 { // First line: gutter + sign + content - row_spans.push(RtSpan::styled(gutter.clone(), style_gutter())); - row_spans.push(RtSpan::styled(sign.clone(), line_style)); + row_spans.push(RtSpan::styled(gutter.clone(), gutter_style)); + row_spans.push(RtSpan::styled(sign.clone(), sign_style)); } else { // Continuation: empty gutter + two-space indent (matches // the plain-text wrapping continuation style). let cont_gutter = format!("{:gutter_width$} ", ""); - row_spans.push(RtSpan::styled(cont_gutter, style_gutter())); + row_spans.push(RtSpan::styled(cont_gutter, gutter_style)); } row_spans.extend(chunk); - lines.push(RtLine::from(row_spans)); + lines.push(RtLine::from(row_spans).style(line_bg)); } return lines; } let available_content_cols = width.saturating_sub(prefix_cols + 1).max(1); - let styled = vec![RtSpan::styled(text.to_string(), line_style)]; + let styled = vec![RtSpan::styled(text.to_string(), content_style)]; let wrapped_chunks = wrap_styled_spans(&styled, available_content_cols); let mut lines: Vec> = Vec::new(); @@ -600,14 +758,14 @@ fn push_wrapped_diff_line_inner( if i == 0 { let gutter = format!("{ln_str:>gutter_width$} "); let sign = format!("{sign_char}"); - row_spans.push(RtSpan::styled(gutter, style_gutter())); - row_spans.push(RtSpan::styled(sign, line_style)); + row_spans.push(RtSpan::styled(gutter, gutter_style)); + row_spans.push(RtSpan::styled(sign, sign_style)); } else { let cont_gutter = format!("{:gutter_width$} ", ""); - row_spans.push(RtSpan::styled(cont_gutter, style_gutter())); + row_spans.push(RtSpan::styled(cont_gutter, gutter_style)); } row_spans.extend(chunk); - lines.push(RtLine::from(row_spans)); + lines.push(RtLine::from(row_spans).style(line_bg)); } lines @@ -703,20 +861,171 @@ pub(crate) fn line_number_width(max_line_number: usize) -> usize { } } -fn style_gutter() -> Style { - Style::default().add_modifier(Modifier::DIM) +/// Testable helper: picks `DiffTheme` from an explicit background sample. +fn diff_theme_for_bg(bg: Option<(u8, u8, u8)>) -> DiffTheme { + if let Some(rgb) = bg + && is_light(rgb) + { + return DiffTheme::Light; + } + DiffTheme::Dark +} + +/// Probe the terminal's background and return the appropriate diff palette. +fn diff_theme() -> DiffTheme { + diff_theme_for_bg(default_bg()) +} + +fn diff_color_level() -> DiffColorLevel { + match stdout_color_level() { + StdoutColorLevel::TrueColor => DiffColorLevel::TrueColor, + StdoutColorLevel::Ansi256 => DiffColorLevel::Ansi256, + StdoutColorLevel::Ansi16 | StdoutColorLevel::Unknown => DiffColorLevel::Ansi16, + } +} + +// -- Style helpers ------------------------------------------------------------ +// +// Each diff line is composed of three visual regions, styled independently: +// +// ┌──────────┬──────┬──────────────────────────────────────────┐ +// │ gutter │ sign │ content │ +// │ (line #) │ +/- │ (plain or syntax-highlighted text) │ +// └──────────┴──────┴──────────────────────────────────────────┘ +// +// A fourth, full-width layer — `line_bg` — is applied via `RtLine::style()` +// so that the background tint extends from the leftmost column to the right +// edge of the terminal, including any padding beyond the content. +// +// On dark terminals, the sign and content share one style (colored fg + tinted +// bg), and the gutter is simply dimmed. On light terminals, sign and content +// are split: the sign gets only a colored foreground (no bg, so the line bg +// shows through), while content relies on the line bg alone; the gutter gets +// an opaque, more-saturated background so line numbers stay readable against +// the pastel line tint. + +/// Full-width background applied to the `RtLine` itself (not individual spans). +/// Context lines intentionally leave the background unset so the terminal +/// default shows through. +fn style_line_bg_for(kind: DiffLineType, theme: DiffTheme, color_level: DiffColorLevel) -> Style { + match kind { + DiffLineType::Insert => Style::default().bg(add_line_bg(theme, color_level)), + DiffLineType::Delete => Style::default().bg(del_line_bg(theme, color_level)), + DiffLineType::Context => Style::default(), + } } fn style_context() -> Style { Style::default() } -fn style_add() -> Style { - Style::default().fg(Color::Green) +fn add_line_bg(theme: DiffTheme, color_level: DiffColorLevel) -> Color { + match (theme, color_level) { + (DiffTheme::Dark, DiffColorLevel::TrueColor) => rgb_color(DARK_TC_ADD_LINE_BG_RGB), + (DiffTheme::Dark, DiffColorLevel::Ansi256) => indexed_color(DARK_256_ADD_LINE_BG_IDX), + (DiffTheme::Dark, DiffColorLevel::Ansi16) => Color::Green, + (DiffTheme::Light, DiffColorLevel::TrueColor) => rgb_color(LIGHT_TC_ADD_LINE_BG_RGB), + (DiffTheme::Light, DiffColorLevel::Ansi256) => indexed_color(LIGHT_256_ADD_LINE_BG_IDX), + (DiffTheme::Light, DiffColorLevel::Ansi16) => Color::LightGreen, + } } -fn style_del() -> Style { - Style::default().fg(Color::Red) +fn del_line_bg(theme: DiffTheme, color_level: DiffColorLevel) -> Color { + match (theme, color_level) { + (DiffTheme::Dark, DiffColorLevel::TrueColor) => rgb_color(DARK_TC_DEL_LINE_BG_RGB), + (DiffTheme::Dark, DiffColorLevel::Ansi256) => indexed_color(DARK_256_DEL_LINE_BG_IDX), + (DiffTheme::Dark, DiffColorLevel::Ansi16) => Color::Red, + (DiffTheme::Light, DiffColorLevel::TrueColor) => rgb_color(LIGHT_TC_DEL_LINE_BG_RGB), + (DiffTheme::Light, DiffColorLevel::Ansi256) => indexed_color(LIGHT_256_DEL_LINE_BG_IDX), + (DiffTheme::Light, DiffColorLevel::Ansi16) => Color::LightRed, + } +} + +fn light_gutter_fg(color_level: DiffColorLevel) -> Color { + match color_level { + DiffColorLevel::TrueColor => rgb_color(LIGHT_TC_GUTTER_FG_RGB), + DiffColorLevel::Ansi256 => indexed_color(LIGHT_256_GUTTER_FG_IDX), + DiffColorLevel::Ansi16 => Color::Black, + } +} + +fn light_add_num_bg(color_level: DiffColorLevel) -> Color { + match color_level { + DiffColorLevel::TrueColor => rgb_color(LIGHT_TC_ADD_NUM_BG_RGB), + DiffColorLevel::Ansi256 => indexed_color(LIGHT_256_ADD_NUM_BG_IDX), + DiffColorLevel::Ansi16 => Color::Green, + } +} + +fn light_del_num_bg(color_level: DiffColorLevel) -> Color { + match color_level { + DiffColorLevel::TrueColor => rgb_color(LIGHT_TC_DEL_NUM_BG_RGB), + DiffColorLevel::Ansi256 => indexed_color(LIGHT_256_DEL_NUM_BG_IDX), + DiffColorLevel::Ansi16 => Color::Red, + } +} + +/// Line-number gutter style. On light backgrounds the gutter has an opaque +/// tinted background so numbers contrast against the pastel line fill. On +/// dark backgrounds a simple `DIM` modifier is sufficient. +fn style_gutter_for(kind: DiffLineType, theme: DiffTheme, color_level: DiffColorLevel) -> Style { + match (theme, kind) { + (DiffTheme::Light, DiffLineType::Insert) => Style::default() + .fg(light_gutter_fg(color_level)) + .bg(light_add_num_bg(color_level)), + (DiffTheme::Light, DiffLineType::Delete) => Style::default() + .fg(light_gutter_fg(color_level)) + .bg(light_del_num_bg(color_level)), + _ => style_gutter_dim(), + } +} + +/// Sign character (`+`) for insert lines. On dark terminals it inherits the +/// full content style (green fg + tinted bg). On light terminals it uses only +/// a green foreground and lets the line-level bg show through. +fn style_sign_add(theme: DiffTheme, color_level: DiffColorLevel) -> Style { + match theme { + DiffTheme::Light => Style::default().fg(Color::Green), + DiffTheme::Dark => style_add(theme, color_level), + } +} + +/// Sign character (`-`) for delete lines. Mirror of [`style_sign_add`]. +fn style_sign_del(theme: DiffTheme, color_level: DiffColorLevel) -> Style { + match theme { + DiffTheme::Light => Style::default().fg(Color::Red), + DiffTheme::Dark => style_del(theme, color_level), + } +} + +/// Content style for insert lines (plain, non-syntax-highlighted text). +fn style_add(theme: DiffTheme, color_level: DiffColorLevel) -> Style { + match (theme, color_level) { + (DiffTheme::Dark, DiffColorLevel::Ansi16) => Style::default() + .fg(Color::Black) + .bg(add_line_bg(theme, color_level)), + (DiffTheme::Light, _) => Style::default().bg(add_line_bg(theme, color_level)), + (DiffTheme::Dark, _) => Style::default() + .fg(Color::Green) + .bg(add_line_bg(theme, color_level)), + } +} + +/// Content style for delete lines (plain, non-syntax-highlighted text). +fn style_del(theme: DiffTheme, color_level: DiffColorLevel) -> Style { + match (theme, color_level) { + (DiffTheme::Dark, DiffColorLevel::Ansi16) => Style::default() + .fg(Color::Black) + .bg(del_line_bg(theme, color_level)), + (DiffTheme::Light, _) => Style::default().bg(del_line_bg(theme, color_level)), + (DiffTheme::Dark, _) => Style::default() + .fg(Color::Red) + .bg(del_line_bg(theme, color_level)), + } +} + +fn style_gutter_dim() -> Style { + Style::default().add_modifier(Modifier::DIM) } #[cfg(test)] @@ -728,6 +1037,35 @@ mod tests { use ratatui::backend::TestBackend; use ratatui::text::Text; use ratatui::widgets::Paragraph; + + #[test] + fn dark_ansi16_add_style_has_contrast() { + let style = style_add(DiffTheme::Dark, DiffColorLevel::Ansi16); + assert_eq!(style.fg, Some(Color::Black)); + assert_eq!(style.bg, Some(Color::Green)); + assert_ne!(style.fg, style.bg); + } + + #[test] + fn dark_ansi16_del_style_has_contrast() { + let style = style_del(DiffTheme::Dark, DiffColorLevel::Ansi16); + assert_eq!(style.fg, Some(Color::Black)); + assert_eq!(style.bg, Some(Color::Red)); + assert_ne!(style.fg, style.bg); + } + + #[test] + fn dark_ansi16_sign_styles_have_contrast() { + let add_sign = style_sign_add(DiffTheme::Dark, DiffColorLevel::Ansi16); + assert_eq!(add_sign.fg, Some(Color::Black)); + assert_eq!(add_sign.bg, Some(Color::Green)); + assert_ne!(add_sign.fg, add_sign.bg); + + let del_sign = style_sign_del(DiffTheme::Dark, DiffColorLevel::Ansi16); + assert_eq!(del_sign.fg, Some(Color::Black)); + assert_eq!(del_sign.bg, Some(Color::Red)); + assert_ne!(del_sign.fg, del_sign.bg); + } use ratatui::widgets::WidgetRef; use ratatui::widgets::Wrap; fn diff_summary_for_tests(changes: &HashMap) -> Vec> { @@ -1128,6 +1466,148 @@ mod tests { snapshot_diff_gallery("diff_gallery_120x40", 120, 40); } + #[test] + fn truecolor_dark_theme_uses_configured_backgrounds() { + assert_eq!( + style_line_bg_for( + DiffLineType::Insert, + DiffTheme::Dark, + DiffColorLevel::TrueColor + ), + Style::default().bg(rgb_color(DARK_TC_ADD_LINE_BG_RGB)) + ); + assert_eq!( + style_line_bg_for( + DiffLineType::Delete, + DiffTheme::Dark, + DiffColorLevel::TrueColor + ), + Style::default().bg(rgb_color(DARK_TC_DEL_LINE_BG_RGB)) + ); + assert_eq!( + style_gutter_for( + DiffLineType::Insert, + DiffTheme::Dark, + DiffColorLevel::TrueColor + ), + style_gutter_dim() + ); + assert_eq!( + style_gutter_for( + DiffLineType::Delete, + DiffTheme::Dark, + DiffColorLevel::TrueColor + ), + style_gutter_dim() + ); + } + + #[test] + fn ansi256_dark_theme_uses_distinct_add_and_delete_backgrounds() { + assert_eq!( + style_line_bg_for( + DiffLineType::Insert, + DiffTheme::Dark, + DiffColorLevel::Ansi256 + ), + Style::default().bg(indexed_color(DARK_256_ADD_LINE_BG_IDX)) + ); + assert_eq!( + style_line_bg_for( + DiffLineType::Delete, + DiffTheme::Dark, + DiffColorLevel::Ansi256 + ), + Style::default().bg(indexed_color(DARK_256_DEL_LINE_BG_IDX)) + ); + assert_ne!( + style_line_bg_for( + DiffLineType::Insert, + DiffTheme::Dark, + DiffColorLevel::Ansi256 + ), + style_line_bg_for( + DiffLineType::Delete, + DiffTheme::Dark, + DiffColorLevel::Ansi256 + ), + "256-color mode should keep add/delete backgrounds distinct" + ); + } + + #[test] + fn light_truecolor_theme_uses_readable_gutter_and_line_backgrounds() { + assert_eq!( + style_line_bg_for( + DiffLineType::Insert, + DiffTheme::Light, + DiffColorLevel::TrueColor + ), + Style::default().bg(rgb_color(LIGHT_TC_ADD_LINE_BG_RGB)) + ); + assert_eq!( + style_line_bg_for( + DiffLineType::Delete, + DiffTheme::Light, + DiffColorLevel::TrueColor + ), + Style::default().bg(rgb_color(LIGHT_TC_DEL_LINE_BG_RGB)) + ); + assert_eq!( + style_gutter_for( + DiffLineType::Insert, + DiffTheme::Light, + DiffColorLevel::TrueColor + ), + Style::default() + .fg(rgb_color(LIGHT_TC_GUTTER_FG_RGB)) + .bg(rgb_color(LIGHT_TC_ADD_NUM_BG_RGB)) + ); + assert_eq!( + style_gutter_for( + DiffLineType::Delete, + DiffTheme::Light, + DiffColorLevel::TrueColor + ), + Style::default() + .fg(rgb_color(LIGHT_TC_GUTTER_FG_RGB)) + .bg(rgb_color(LIGHT_TC_DEL_NUM_BG_RGB)) + ); + } + + #[test] + fn light_theme_wrapped_lines_keep_number_gutter_contrast() { + let lines = push_wrapped_diff_line_inner_with_theme_and_color_level( + 12, + DiffLineType::Insert, + "abcdefghij", + 8, + line_number_width(12), + None, + DiffTheme::Light, + DiffColorLevel::TrueColor, + ); + + assert!( + lines.len() > 1, + "expected wrapped output for gutter style verification" + ); + assert_eq!( + lines[0].spans[0].style, + Style::default() + .fg(rgb_color(LIGHT_TC_GUTTER_FG_RGB)) + .bg(rgb_color(LIGHT_TC_ADD_NUM_BG_RGB)) + ); + assert_eq!( + lines[1].spans[0].style, + Style::default() + .fg(rgb_color(LIGHT_TC_GUTTER_FG_RGB)) + .bg(rgb_color(LIGHT_TC_ADD_NUM_BG_RGB)) + ); + assert_eq!(lines[0].style.bg, Some(rgb_color(LIGHT_TC_ADD_LINE_BG_RGB))); + assert_eq!(lines[1].style.bg, Some(rgb_color(LIGHT_TC_ADD_LINE_BG_RGB))); + } + #[test] fn add_diff_uses_path_extension_for_highlighting() { let mut changes: HashMap = HashMap::new(); diff --git a/codex-rs/tui/src/terminal_palette.rs b/codex-rs/tui/src/terminal_palette.rs index 6349c007e..304b8df08 100644 --- a/codex-rs/tui/src/terminal_palette.rs +++ b/codex-rs/tui/src/terminal_palette.rs @@ -9,26 +9,47 @@ fn bump_palette_version() { DEFAULT_PALETTE_VERSION.fetch_add(1, Ordering::Relaxed); } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum StdoutColorLevel { + TrueColor, + Ansi256, + Ansi16, + Unknown, +} + +pub fn stdout_color_level() -> StdoutColorLevel { + match supports_color::on_cached(supports_color::Stream::Stdout) { + Some(level) if level.has_16m => StdoutColorLevel::TrueColor, + Some(level) if level.has_256 => StdoutColorLevel::Ansi256, + Some(_) => StdoutColorLevel::Ansi16, + None => StdoutColorLevel::Unknown, + } +} + +#[allow(clippy::disallowed_methods)] +pub fn rgb_color((r, g, b): (u8, u8, u8)) -> Color { + Color::Rgb(r, g, b) +} + +#[allow(clippy::disallowed_methods)] +pub fn indexed_color(index: u8) -> Color { + Color::Indexed(index) +} + /// Returns the closest color to the target color that the terminal can display. pub fn best_color(target: (u8, u8, u8)) -> Color { - let Some(color_level) = supports_color::on_cached(supports_color::Stream::Stdout) else { - return Color::default(); - }; - if color_level.has_16m { - let (r, g, b) = target; - #[allow(clippy::disallowed_methods)] - Color::Rgb(r, g, b) - } else if color_level.has_256 + let color_level = stdout_color_level(); + if color_level == StdoutColorLevel::TrueColor { + rgb_color(target) + } else if color_level == StdoutColorLevel::Ansi256 && let Some((i, _)) = xterm_fixed_colors().min_by(|(_, a), (_, b)| { perceptual_distance(*a, target) .partial_cmp(&perceptual_distance(*b, target)) .unwrap_or(std::cmp::Ordering::Equal) }) { - #[allow(clippy::disallowed_methods)] - Color::Indexed(i as u8) + indexed_color(i as u8) } else { - #[allow(clippy::disallowed_methods)] Color::default() } } diff --git a/codex-rs/tui/src/theme_picker.rs b/codex-rs/tui/src/theme_picker.rs index 9205b5fb3..7a5871171 100644 --- a/codex-rs/tui/src/theme_picker.rs +++ b/codex-rs/tui/src/theme_picker.rs @@ -383,6 +383,7 @@ pub(crate) fn build_theme_picker_params( side_content_width: SideContentWidth::Half, side_content_min_width: WIDE_PREVIEW_MIN_WIDTH, stacked_side_content: Some(Box::new(ThemePreviewNarrowRenderable)), + preserve_side_content_bg: true, on_selection_changed, on_cancel, ..Default::default()