mirror of
https://github.com/openai/codex.git
synced 2026-11-06 20:20:34 +00:00
feat(tui): harden theme fallback contracts and diff wrapping
This commit is contained in:
@@ -2825,10 +2825,7 @@ impl App {
|
||||
return;
|
||||
}
|
||||
|
||||
let auto_theme_name = match crate::terminal_palette::default_bg() {
|
||||
Some(bg) if crate::color::is_light(bg) => "catppuccin-latte",
|
||||
_ => "catppuccin-mocha",
|
||||
};
|
||||
let auto_theme_name = crate::render::highlight::adaptive_default_theme_name();
|
||||
if let Some(theme) = crate::render::highlight::resolve_theme_by_name(
|
||||
auto_theme_name,
|
||||
Some(&self.config.codex_home),
|
||||
|
||||
@@ -590,40 +590,26 @@ fn push_wrapped_diff_line_inner(
|
||||
return lines;
|
||||
}
|
||||
|
||||
// Fallback: no syntax spans — use the original wrapping behavior.
|
||||
let mut remaining_text: &str = text;
|
||||
let mut first = true;
|
||||
let available_content_cols = width.saturating_sub(prefix_cols + 1).max(1);
|
||||
let styled = vec![RtSpan::styled(text.to_string(), line_style)];
|
||||
let wrapped_chunks = wrap_styled_spans(&styled, available_content_cols);
|
||||
|
||||
let mut lines: Vec<RtLine<'static>> = Vec::new();
|
||||
|
||||
loop {
|
||||
let available_content_cols = width.saturating_sub(prefix_cols + 1).max(1);
|
||||
let split_at_byte_index = remaining_text
|
||||
.char_indices()
|
||||
.nth(available_content_cols)
|
||||
.map(|(i, _)| i)
|
||||
.unwrap_or_else(|| remaining_text.len());
|
||||
let (chunk, rest) = remaining_text.split_at(split_at_byte_index);
|
||||
remaining_text = rest;
|
||||
|
||||
if first {
|
||||
for (i, chunk) in wrapped_chunks.into_iter().enumerate() {
|
||||
let mut row_spans: Vec<RtSpan<'static>> = Vec::new();
|
||||
if i == 0 {
|
||||
let gutter = format!("{ln_str:>gutter_width$} ");
|
||||
let content = format!("{sign_char}{chunk}");
|
||||
lines.push(RtLine::from(vec![
|
||||
RtSpan::styled(gutter, style_gutter()),
|
||||
RtSpan::styled(content, line_style),
|
||||
]));
|
||||
first = false;
|
||||
let sign = format!("{sign_char}");
|
||||
row_spans.push(RtSpan::styled(gutter, style_gutter()));
|
||||
row_spans.push(RtSpan::styled(sign, line_style));
|
||||
} else {
|
||||
let gutter = format!("{:gutter_width$} ", "");
|
||||
lines.push(RtLine::from(vec![
|
||||
RtSpan::styled(gutter, style_gutter()),
|
||||
RtSpan::styled(chunk.to_string(), line_style),
|
||||
]));
|
||||
}
|
||||
if remaining_text.is_empty() {
|
||||
break;
|
||||
let cont_gutter = format!("{:gutter_width$} ", "");
|
||||
row_spans.push(RtSpan::styled(cont_gutter, style_gutter()));
|
||||
}
|
||||
row_spans.extend(chunk);
|
||||
lines.push(RtLine::from(row_spans));
|
||||
}
|
||||
|
||||
lines
|
||||
}
|
||||
|
||||
@@ -758,6 +744,19 @@ mod tests {
|
||||
assert_snapshot!(name, terminal.backend());
|
||||
}
|
||||
|
||||
fn display_width(text: &str) -> usize {
|
||||
text.chars()
|
||||
.map(|ch| ch.width().unwrap_or(if ch == '\t' { TAB_WIDTH } else { 0 }))
|
||||
.sum()
|
||||
}
|
||||
|
||||
fn line_display_width(line: &RtLine<'static>) -> usize {
|
||||
line.spans
|
||||
.iter()
|
||||
.map(|span| display_width(span.content.as_ref()))
|
||||
.sum()
|
||||
}
|
||||
|
||||
fn snapshot_lines_text(name: &str, lines: &[RtLine<'static>]) {
|
||||
// Convert Lines to plain text rows and trim trailing spaces so it's
|
||||
// easier to validate indentation visually in snapshots.
|
||||
@@ -1285,6 +1284,26 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fallback_wrapping_uses_display_width_for_tabs_and_wide_chars() {
|
||||
let width = 8;
|
||||
let lines = push_wrapped_diff_line(
|
||||
1,
|
||||
DiffLineType::Insert,
|
||||
"abcd\t界🙂",
|
||||
width,
|
||||
line_number_width(1),
|
||||
);
|
||||
|
||||
assert!(lines.len() >= 2, "expected wrapped output, got {lines:?}");
|
||||
for line in &lines {
|
||||
assert!(
|
||||
line_display_width(line) <= width,
|
||||
"fallback wrapped line exceeded width {width}: {line:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn large_update_diff_skips_highlighting() {
|
||||
// Build a patch large enough to exceed MAX_HIGHLIGHT_LINES (10_000).
|
||||
|
||||
@@ -51,8 +51,14 @@ fn syntax_set() -> &'static SyntaxSet {
|
||||
SYNTAX_SET.get_or_init(two_face::syntax::extra_newlines)
|
||||
}
|
||||
|
||||
/// Set the user-configured theme override before any highlighting occurs.
|
||||
/// Must be called at most once, before the first call to `theme()`.
|
||||
/// Set the user-configured syntax theme override and codex home path.
|
||||
///
|
||||
/// Call this with the **final resolved config** (after onboarding, resume, and
|
||||
/// fork reloads complete). The first call persists `name` and `codex_home` in
|
||||
/// `OnceLock`s used by startup/default theme resolution.
|
||||
///
|
||||
/// Subsequent calls cannot change the persisted `OnceLock` values, but they
|
||||
/// still update the runtime theme immediately for live preview flows.
|
||||
///
|
||||
/// Returns a warning message when the configured theme name cannot be
|
||||
/// resolved to a bundled theme or a custom `.tmTheme` file on disk.
|
||||
@@ -162,6 +168,25 @@ fn load_custom_theme(name: &str, codex_home: &Path) -> Option<Theme> {
|
||||
ThemeSet::get_theme(custom_theme_path(name, codex_home)).ok()
|
||||
}
|
||||
|
||||
fn adaptive_default_theme_selection() -> (EmbeddedThemeName, &'static str) {
|
||||
match crate::terminal_palette::default_bg() {
|
||||
Some(bg) if crate::color::is_light(bg) => {
|
||||
(EmbeddedThemeName::CatppuccinLatte, "catppuccin-latte")
|
||||
}
|
||||
_ => (EmbeddedThemeName::CatppuccinMocha, "catppuccin-mocha"),
|
||||
}
|
||||
}
|
||||
|
||||
fn adaptive_default_embedded_theme_name() -> EmbeddedThemeName {
|
||||
adaptive_default_theme_selection().0
|
||||
}
|
||||
|
||||
/// Return the kebab-case name of the adaptive default syntax theme selected
|
||||
/// from terminal background lightness.
|
||||
pub(crate) fn adaptive_default_theme_name() -> &'static str {
|
||||
adaptive_default_theme_selection().1
|
||||
}
|
||||
|
||||
/// Build the theme from current override/auto-detection settings.
|
||||
/// Extracted from the old `theme()` init closure so it can be reused.
|
||||
fn resolve_theme_with_override(name: Option<&str>, codex_home: Option<&Path>) -> Theme {
|
||||
@@ -182,12 +207,7 @@ fn resolve_theme_with_override(name: Option<&str>, codex_home: Option<&Path>) ->
|
||||
tracing::warn!("unknown syntax theme \"{name}\", falling back to auto-detection");
|
||||
}
|
||||
|
||||
// Adaptive default: light or dark based on terminal background.
|
||||
let name = match crate::terminal_palette::default_bg() {
|
||||
Some(bg) if crate::color::is_light(bg) => EmbeddedThemeName::CatppuccinLatte,
|
||||
_ => EmbeddedThemeName::CatppuccinMocha,
|
||||
};
|
||||
ts.get(name).clone()
|
||||
ts.get(adaptive_default_embedded_theme_name()).clone()
|
||||
}
|
||||
|
||||
/// Build the theme from current override/auto-detection settings.
|
||||
@@ -234,11 +254,7 @@ pub(crate) fn current_theme_name() -> String {
|
||||
return name.clone();
|
||||
}
|
||||
}
|
||||
// Adaptive default: light or dark based on terminal background.
|
||||
match crate::terminal_palette::default_bg() {
|
||||
Some(bg) if crate::color::is_light(bg) => "catppuccin-latte".to_string(),
|
||||
_ => "catppuccin-mocha".to_string(),
|
||||
}
|
||||
adaptive_default_theme_name().to_string()
|
||||
}
|
||||
|
||||
/// Resolve a theme name to a `Theme` (bundled or custom). Returns `None`
|
||||
|
||||
@@ -356,12 +356,13 @@ pub(crate) fn build_theme_picker_params(
|
||||
})
|
||||
.collect();
|
||||
|
||||
// Capture theme names from the stable entry list built above so the
|
||||
// preview closure indexes into the same ordering the picker uses.
|
||||
let preview_names: Vec<String> = entries.iter().map(|e| e.name.clone()).collect();
|
||||
// Derive preview targets from the final `items` list (not from `entries`)
|
||||
// so preview ordering stays aligned if item construction/sorting changes.
|
||||
let preview_theme_names: Vec<Option<String>> =
|
||||
items.iter().map(|item| item.search_value.clone()).collect();
|
||||
let preview_home = codex_home_owned.clone();
|
||||
let on_selection_changed = Some(Box::new(move |idx: usize, _tx: &_| {
|
||||
if let Some(name) = preview_names.get(idx)
|
||||
if let Some(Some(name)) = preview_theme_names.get(idx)
|
||||
&& let Some(theme) = highlight::resolve_theme_by_name(name, preview_home.as_deref())
|
||||
{
|
||||
highlight::set_syntax_theme(theme);
|
||||
@@ -471,6 +472,15 @@ mod tests {
|
||||
assert!(params.stacked_side_content.is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn theme_picker_items_include_search_values_for_preview_mapping() {
|
||||
let params = build_theme_picker_params(None, None, None);
|
||||
assert!(
|
||||
params.items.iter().all(|item| item.search_value.is_some()),
|
||||
"theme picker preview mapping relies on item search_value to stay aligned with final item order"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn wide_preview_renders_all_lines_with_vertical_center_and_left_inset() {
|
||||
let lines = render_lines(&ThemePreviewWideRenderable, 80, 20);
|
||||
|
||||
Reference in New Issue
Block a user