mirror of
https://github.com/openai/codex.git
synced 2026-05-05 22:01:37 +03:00
feat(tui): add theme-aware diff backgrounds with capability-graded palettes (#12581)
## 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 | |---|---| | <img width="1552" height="1012" alt="image" src="https://github.com/user-attachments/assets/231b21b7-32d4-4727-80ed-7d01924954be" /> | <img width="795" height="1012" alt="image" src="https://github.com/user-attachments/assets/549cacdf-daec-43c9-ad64-2a28d16d140e" /> | | Dark BG - 16 colors | Dark BG - 256 colors | Dark BG - True Colors | |---|---|---| | <img width="1552" height="1012" alt="dark-16colors" src="https://github.com/user-attachments/assets/fba36de3-c101-47d4-9e63-88cdd00410d0" /> | <img width="1552" height="1012" alt="dark-256colors" src="https://github.com/user-attachments/assets/f39e4307-c6b0-49c4-b4fe-bd26d3d8e41c" /> | <img width="1552" height="1012" alt="dark-truecolor" src="https://github.com/user-attachments/assets/1af4ec57-04bf-4dfb-8a44-0ab5e5aaaf18" /> | | Light BG - 16 colors | Light BG - 256 colors | Light BG - True Colors | |---|---|---| | <img width="1552" height="1012" alt="light-16colors" src="https://github.com/user-attachments/assets/2b5423d1-74b4-4b1e-8123-7c2488ff436b" /> | <img width="1552" height="1012" alt="light-256colors" src="https://github.com/user-attachments/assets/c94cff9a-8d3e-42c9-bbe7-079da39953a8" /> | <img width="1552" height="1012" alt="light-truecolor" src="https://github.com/user-attachments/assets/f73da626-725f-4452-99ee-69ef706df2c6" /> | ## 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.
This commit is contained in:
@@ -163,6 +163,10 @@ pub(crate) struct SelectionViewParams {
|
||||
/// When absent, `side_content` is reused.
|
||||
pub stacked_side_content: Option<Box<dyn Renderable>>,
|
||||
|
||||
/// 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<Box<dyn Renderable>>,
|
||||
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::<AppEvent>();
|
||||
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::<AppEvent>();
|
||||
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::<AppEvent>();
|
||||
|
||||
Reference in New Issue
Block a user