mirror of
https://github.com/openai/codex.git
synced 2026-05-05 05:42:33 +03:00
fix(tui): theme-aware diff backgrounds with fallback behavior (#13037)
## Problem The TUI diff renderer uses hardcoded background palettes for insert/delete lines that don't respect the user's chosen syntax theme. When a theme defines `markup.inserted` / `markup.deleted` scope backgrounds (the convention used by GitHub, Solarized, Monokai, and most VS Code themes), those colors are ignored — the diff always renders with the same green/red tints regardless of theme selection. Separately, ANSI-16 terminals (and Windows Terminal sessions misreported as ANSI-16) rendered diff backgrounds as full-saturation blocks that obliterated syntax token colors, making highlighted diffs unreadable. ## Mental model Diff backgrounds are resolved in three layers: 1. **Color level detection** — `diff_color_level_for_terminal()` maps the raw `supports-color` probe + Windows Terminal heuristics to a `DiffColorLevel` (TrueColor / Ansi256 / Ansi16). Windows Terminal gets promoted from Ansi16 to TrueColor when `WT_SESSION` is present. 2. **Background resolution** — `resolve_diff_backgrounds()` queries the active syntax theme for `markup.inserted`/`markup.deleted` (falling back to `diff.inserted`/`diff.deleted`), then overlays those on top of the hardcoded palette. For ANSI-256, theme RGB values are quantized to the nearest xterm-256 index. For ANSI-16, backgrounds are `None` (foreground-only). 3. **Style composition** — The resolved `ResolvedDiffBackgrounds` is threaded through every call to `style_add`, `style_del`, `style_sign_*`, and `style_line_bg_for`, which decide how to compose foreground+background for each line kind and theme variant. A new `RichDiffColorLevel` type (a subset of `DiffColorLevel` without Ansi16) encodes the invariant "we have enough depth for tinted backgrounds" at the type level, so background-producing functions have exhaustive matches without unreachable arms. ## Non-goals - No change to gutter (line number column) styling — gutter backgrounds still use the hardcoded palette. - No per-token scope background resolution — this is line-level background only; syntax token colors come from the existing `highlight_code_to_styled_spans` path. - No dark/light theme auto-switching from scope backgrounds — `DiffTheme` is still determined by querying the terminal's background color. ## Tradeoffs - **Theme trust vs. visual safety:** When a theme defines scope backgrounds, we trust them unconditionally for rich color levels. A badly authored theme could produce illegible combinations. The fallback for `None` backgrounds (foreground-only) is intentionally conservative. - **Quantization quality:** ANSI-256 quantization uses perceptual distance across indices 16–255, skipping system colors. The result is approximate — a subtle theme tint may land on a noticeably different xterm index. - **Single-query caching:** `resolve_diff_backgrounds` is called once per `render_change` invocation (i.e., once per file in a diff). If the theme changes mid-render (live preview), the next file picks up the new backgrounds. ## Architecture Files changed: | File | Role | |---|---| | `tui/src/render/highlight.rs` | New: `DiffScopeBackgroundRgbs`, `diff_scope_background_rgbs()`, scope extraction helpers | | `tui/src/diff_render.rs` | New: `RichDiffColorLevel`, `ResolvedDiffBackgrounds`, `resolve_diff_backgrounds*`, `quantize_rgb_to_ansi256`, Windows Terminal promotion; modified: all style helpers to accept/thread `ResolvedDiffBackgrounds` | The scope-extraction code lives in `highlight.rs` because it uses `syntect::highlighting::Highlighter` and the theme singleton. The resolution and quantization logic lives in `diff_render.rs` because it depends on diff-specific types (`DiffTheme`, `DiffColorLevel`, ratatui `Color`). ## Observability No runtime logging was added. The most useful debugging aid is the `diff_color_level_for_terminal` function, which is pure and fully unit-tested — to diagnose a color-depth mismatch, log its four inputs (`StdoutColorLevel`, `TerminalName`, `WT_SESSION` presence, `FORCE_COLOR` presence). Scope resolution can be tested by loading a custom `.tmTheme` with known `markup.inserted` / `markup.deleted` backgrounds and checking the diff output in a truecolor terminal. ## Tests - **Windows Terminal promotion:** 7 unit tests cover every branch of `diff_color_level_for_terminal` (ANSI-16 promotion, `WT_SESSION` unconditional promotion, `FORCE_COLOR` suppression, conservative `Unknown` level). - **ANSI-16 foreground-only:** Tests verify that `style_add`, `style_del`, `style_sign_*`, `style_line_bg_for`, and `style_gutter_for` all return `None` backgrounds on ANSI-16. - **Scope resolution:** Tests verify `markup.*` preference over `diff.*`, `None` when no scope matches, bundled theme resolution, and custom `.tmTheme` round-trip. - **Quantization:** Test verifies ANSI-256 quantization of a known RGB triple. - **Insta snapshots:** 2 new snapshot tests (`ansi16_insert_delete_no_background`, `theme_scope_background_resolution`) lock visual output.
This commit is contained in:
@@ -32,9 +32,11 @@ use std::sync::OnceLock;
|
||||
use std::sync::RwLock;
|
||||
use syntect::easy::HighlightLines;
|
||||
use syntect::highlighting::FontStyle;
|
||||
use syntect::highlighting::Highlighter;
|
||||
use syntect::highlighting::Style as SyntectStyle;
|
||||
use syntect::highlighting::Theme;
|
||||
use syntect::highlighting::ThemeSet;
|
||||
use syntect::parsing::Scope;
|
||||
use syntect::parsing::SyntaxReference;
|
||||
use syntect::parsing::SyntaxSet;
|
||||
use syntect::util::LinesWithEndings;
|
||||
@@ -241,6 +243,50 @@ pub(crate) fn current_syntax_theme() -> Theme {
|
||||
}
|
||||
}
|
||||
|
||||
/// Raw RGB background colors extracted from syntax theme diff/markup scopes.
|
||||
///
|
||||
/// These are theme-provided colors, not yet adapted for any particular color
|
||||
/// depth. [`diff_render`](crate::diff_render) converts them to ratatui
|
||||
/// `Color` values via `color_from_rgb_for_level` after deciding whether to
|
||||
/// emit truecolor or quantized ANSI-256.
|
||||
///
|
||||
/// Both fields are `None` when the active theme defines no relevant scope
|
||||
/// backgrounds, in which case the diff renderer falls back to its hardcoded
|
||||
/// palette.
|
||||
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
|
||||
pub(crate) struct DiffScopeBackgroundRgbs {
|
||||
pub inserted: Option<(u8, u8, u8)>,
|
||||
pub deleted: Option<(u8, u8, u8)>,
|
||||
}
|
||||
|
||||
/// Query the active syntax theme for diff-scope background colors.
|
||||
///
|
||||
/// Prefers `markup.inserted` / `markup.deleted` (the TextMate convention used
|
||||
/// by most VS Code themes) and falls back to `diff.inserted` / `diff.deleted`
|
||||
/// (used by some older `.tmTheme` files).
|
||||
pub(crate) fn diff_scope_background_rgbs() -> DiffScopeBackgroundRgbs {
|
||||
let theme = current_syntax_theme();
|
||||
diff_scope_background_rgbs_for_theme(&theme)
|
||||
}
|
||||
|
||||
/// Pure extraction helper, separated from the global theme singleton so tests
|
||||
/// can pass arbitrary themes.
|
||||
fn diff_scope_background_rgbs_for_theme(theme: &Theme) -> DiffScopeBackgroundRgbs {
|
||||
let highlighter = Highlighter::new(theme);
|
||||
let inserted = scope_background_rgb(&highlighter, "markup.inserted")
|
||||
.or_else(|| scope_background_rgb(&highlighter, "diff.inserted"));
|
||||
let deleted = scope_background_rgb(&highlighter, "markup.deleted")
|
||||
.or_else(|| scope_background_rgb(&highlighter, "diff.deleted"));
|
||||
DiffScopeBackgroundRgbs { inserted, deleted }
|
||||
}
|
||||
|
||||
/// Extract the background color for a single TextMate scope, if defined.
|
||||
fn scope_background_rgb(highlighter: &Highlighter<'_>, scope_name: &str) -> Option<(u8, u8, u8)> {
|
||||
let scope = Scope::new(scope_name).ok()?;
|
||||
let bg = highlighter.style_mod_for_stack(&[scope]).background?;
|
||||
Some((bg.r, bg.g, bg.b))
|
||||
}
|
||||
|
||||
/// Return the configured kebab-case theme name when it resolves; otherwise
|
||||
/// return the adaptive auto-detected default theme name.
|
||||
///
|
||||
@@ -551,6 +597,12 @@ pub(crate) fn highlight_code_to_styled_spans(
|
||||
mod tests {
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::str::FromStr;
|
||||
use syntect::highlighting::Color as SyntectColor;
|
||||
use syntect::highlighting::ScopeSelectors;
|
||||
use syntect::highlighting::StyleModifier;
|
||||
use syntect::highlighting::ThemeItem;
|
||||
use syntect::highlighting::ThemeSettings;
|
||||
|
||||
fn write_minimal_tmtheme(path: &Path) {
|
||||
// Minimal valid .tmTheme plist (enough for syntect to parse).
|
||||
@@ -570,6 +622,43 @@ mod tests {
|
||||
.unwrap();
|
||||
}
|
||||
|
||||
fn write_tmtheme_with_diff_backgrounds(
|
||||
path: &Path,
|
||||
inserted_scope: &str,
|
||||
inserted_background: &str,
|
||||
deleted_scope: &str,
|
||||
deleted_background: &str,
|
||||
) {
|
||||
let contents = format!(
|
||||
r#"<?xml version="1.0" encoding="UTF-8"?>
|
||||
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
|
||||
<plist version="1.0"><dict>
|
||||
<key>name</key><string>Custom Diff Theme</string>
|
||||
<key>settings</key><array>
|
||||
<dict>
|
||||
<key>settings</key><dict>
|
||||
<key>foreground</key><string>#FFFFFF</string>
|
||||
<key>background</key><string>#000000</string>
|
||||
</dict>
|
||||
</dict>
|
||||
<dict>
|
||||
<key>scope</key><string>{inserted_scope}</string>
|
||||
<key>settings</key><dict>
|
||||
<key>background</key><string>{inserted_background}</string>
|
||||
</dict>
|
||||
</dict>
|
||||
<dict>
|
||||
<key>scope</key><string>{deleted_scope}</string>
|
||||
<key>settings</key><dict>
|
||||
<key>background</key><string>{deleted_background}</string>
|
||||
</dict>
|
||||
</dict>
|
||||
</array>
|
||||
</dict></plist>"#
|
||||
);
|
||||
std::fs::write(path, contents).unwrap();
|
||||
}
|
||||
|
||||
/// Reconstruct plain text from highlighted Lines.
|
||||
fn reconstructed(lines: &[Line<'static>]) -> String {
|
||||
lines
|
||||
@@ -584,6 +673,16 @@ mod tests {
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
fn theme_item(scope: &str, background: Option<(u8, u8, u8)>) -> ThemeItem {
|
||||
ThemeItem {
|
||||
scope: ScopeSelectors::from_str(scope).expect("scope selector should parse"),
|
||||
style: StyleModifier {
|
||||
background: background.map(|(r, g, b)| SyntectColor { r, g, b, a: 255 }),
|
||||
..StyleModifier::default()
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn highlight_rust_has_keyword_style() {
|
||||
let code = "fn main() {}";
|
||||
@@ -848,6 +947,79 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn diff_scope_backgrounds_prefer_markup_scope_then_diff_fallback() {
|
||||
let theme = Theme {
|
||||
settings: ThemeSettings::default(),
|
||||
scopes: vec![
|
||||
theme_item("markup.inserted", Some((10, 20, 30))),
|
||||
theme_item("diff.deleted", Some((40, 50, 60))),
|
||||
],
|
||||
..Theme::default()
|
||||
};
|
||||
let rgbs = diff_scope_background_rgbs_for_theme(&theme);
|
||||
assert_eq!(
|
||||
rgbs,
|
||||
DiffScopeBackgroundRgbs {
|
||||
inserted: Some((10, 20, 30)),
|
||||
deleted: Some((40, 50, 60)),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn diff_scope_backgrounds_return_none_when_no_background_scope_matches() {
|
||||
let theme = Theme {
|
||||
settings: ThemeSettings::default(),
|
||||
scopes: vec![theme_item("constant.numeric", Some((1, 2, 3)))],
|
||||
..Theme::default()
|
||||
};
|
||||
let rgbs = diff_scope_background_rgbs_for_theme(&theme);
|
||||
assert_eq!(
|
||||
rgbs,
|
||||
DiffScopeBackgroundRgbs {
|
||||
inserted: None,
|
||||
deleted: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bundled_theme_can_provide_diff_scope_backgrounds() {
|
||||
let theme =
|
||||
resolve_theme_by_name("github", None).expect("expected built-in GitHub theme to load");
|
||||
let rgbs = diff_scope_background_rgbs_for_theme(&theme);
|
||||
assert!(
|
||||
rgbs.inserted.is_some() && rgbs.deleted.is_some(),
|
||||
"expected built-in theme to provide insert/delete backgrounds, got {rgbs:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn custom_tmtheme_diff_scope_backgrounds_are_resolved() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let themes_dir = dir.path().join("themes");
|
||||
std::fs::create_dir(&themes_dir).unwrap();
|
||||
write_tmtheme_with_diff_backgrounds(
|
||||
&themes_dir.join("custom-diff.tmTheme"),
|
||||
"diff.inserted",
|
||||
"#102030",
|
||||
"markup.deleted",
|
||||
"#405060",
|
||||
);
|
||||
|
||||
let theme = resolve_theme_by_name("custom-diff", Some(dir.path()))
|
||||
.expect("expected custom theme to resolve");
|
||||
let rgbs = diff_scope_background_rgbs_for_theme(&theme);
|
||||
assert_eq!(
|
||||
rgbs,
|
||||
DiffScopeBackgroundRgbs {
|
||||
inserted: Some((16, 32, 48)),
|
||||
deleted: Some((64, 80, 96)),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_theme_name_covers_all_variants() {
|
||||
let known = [
|
||||
|
||||
Reference in New Issue
Block a user