From 28605b8a2c3ef2b6ac4ab8f772f3257380c42954 Mon Sep 17 00:00:00 2001 From: laoye_tchs <95684065+laoye2020@users.noreply.github.com> Date: Tue, 12 May 2026 22:50:18 +0800 Subject: [PATCH] Address review feedback: clamp picker size, dedupe IO, preserve bg override - theme_picker: drop .max() floors that could produce dimensions larger than the available area on small terminals; return early on a zero area instead of underflowing the centering arithmetic. - theme_picker: reject the '0' digit shortcut (previously `saturating_sub('1')` quietly remapped it to row 0). - theme_picker: cache the System UiTheme at construction so the per-row swatch loop and chrome lookup don't re-invoke `UiTheme::detect()` (which reads `COLORFGBG`) on every keystroke. - color_compat: backend now carries the resolved `UiTheme` alongside `ThemeId`, and `adapt_{fg,bg}_for_theme` read from that instead of re-resolving from `theme_id.ui_theme()`. Fixes the case where `theme = "tokyo-night"` + `background_color = "#000000"` was being silently overwritten back to tokyo-night's surface_bg by the remap. Added a test pinning the override. - ui: the `ConfigUpdated` handler now only appends the confirmation `System` cell when `persist == true`, so live-preview navigation in the picker stops spamming "theme = ... (session only)" rows into the transcript per arrow keystroke. - ui: `AppAction::OpenThemePicker` reads the active name from `app.theme_id` instead of reloading `settings.toml` from disk. - settings: collapse the double `ThemeId::from_name` pass in load() into a single map+unwrap_or. Added tests for digit '0' rejection and tiny/zero-sized render paths. --- crates/tui/src/palette.rs | 17 +++++-- crates/tui/src/settings.rs | 20 +++----- crates/tui/src/tui/color_compat.rs | 53 ++++++++++++++++++-- crates/tui/src/tui/theme_picker.rs | 79 ++++++++++++++++++++++++++---- crates/tui/src/tui/ui.rs | 21 ++++---- 5 files changed, 148 insertions(+), 42 deletions(-) diff --git a/crates/tui/src/palette.rs b/crates/tui/src/palette.rs index 9d566093..f9231aee 100644 --- a/crates/tui/src/palette.rs +++ b/crates/tui/src/palette.rs @@ -678,12 +678,19 @@ pub const fn theme_remap_active(theme: ThemeId) -> bool { /// Remap a foreground color for a community theme preset. Mirrors the /// structure of [`adapt_fg_for_palette_mode`] — same source set, different /// destinations sourced from the preset's [`UiTheme`]. +/// +/// The `ui` argument is the *active* UiTheme as carried on `App` — +/// `ThemeId.ui_theme()` with the user's `background_color` override +/// already applied. Passing it through (rather than re-resolving from +/// `theme` inside this function) preserves that override; otherwise a +/// user combining `background_color = "#..."` with a community theme +/// would see their override silently overwritten by the preset's +/// surface_bg on every cell remap. #[must_use] -pub fn adapt_fg_for_theme(color: Color, theme: ThemeId) -> Color { +pub fn adapt_fg_for_theme(color: Color, theme: ThemeId, ui: &UiTheme) -> Color { if !theme_remap_active(theme) { return color; } - let ui = theme.ui_theme(); if color == TEXT_BODY || color == SELECTION_TEXT || color == Color::White { ui.text_body @@ -715,13 +722,13 @@ pub fn adapt_fg_for_theme(color: Color, theme: ThemeId) -> Color { } } -/// Remap a background color for a community theme preset. +/// Remap a background color for a community theme preset. See the +/// `ui` note on [`adapt_fg_for_theme`] — same contract here. #[must_use] -pub fn adapt_bg_for_theme(color: Color, theme: ThemeId) -> Color { +pub fn adapt_bg_for_theme(color: Color, theme: ThemeId, ui: &UiTheme) -> Color { if !theme_remap_active(theme) { return color; } - let ui = theme.ui_theme(); if color == DEEPSEEK_INK || color == BACKGROUND_DARK { ui.surface_bg diff --git a/crates/tui/src/settings.rs b/crates/tui/src/settings.rs index 98174090..d61268c8 100644 --- a/crates/tui/src/settings.rs +++ b/crates/tui/src/settings.rs @@ -360,19 +360,13 @@ impl Settings { .unwrap_or("en") .to_string(); s.background_color = normalize_optional_background_color(s.background_color.as_deref()); - // Drop unknown theme names so a stale settings file with a typo - // doesn't pin the app to whatever the default happens to be. - // `from_name` is intentionally permissive about case + aliases. - if crate::palette::ThemeId::from_name(&s.theme).is_none() { - s.theme = "system".to_string(); - } else { - s.theme = s.theme.trim().to_ascii_lowercase(); - // Canonicalize aliases (e.g. "whale" -> "dark") so the saved - // form matches the picker's stored value. - if let Some(id) = crate::palette::ThemeId::from_name(&s.theme) { - s.theme = id.name().to_string(); - } - } + // Canonicalize the theme value: unknown / typo → "system"; an + // alias (e.g. "whale") collapses to its canonical name. Single + // `from_name` pass handles both — the previous version called + // it twice. + s.theme = crate::palette::ThemeId::from_name(&s.theme) + .map(|id| id.name().to_string()) + .unwrap_or_else(|| "system".to_string()); s.default_model = s.default_model.as_deref().and_then(normalize_default_model); s }; diff --git a/crates/tui/src/tui/color_compat.rs b/crates/tui/src/tui/color_compat.rs index 95df7fc6..82e3b38e 100644 --- a/crates/tui/src/tui/color_compat.rs +++ b/crates/tui/src/tui/color_compat.rs @@ -14,7 +14,7 @@ use ratatui::{ layout::{Position, Size}, }; -use crate::palette::{self, ColorDepth, PaletteMode, ThemeId}; +use crate::palette::{self, ColorDepth, PaletteMode, ThemeId, UiTheme}; #[derive(Debug)] pub(crate) struct ColorCompatBackend { @@ -26,6 +26,13 @@ pub(crate) struct ColorCompatBackend { /// community presets (Catppuccin, Tokyo Night, Dracula, Gruvbox) trigger /// a per-cell rewrite of dark-palette constants → preset slots. theme_id: ThemeId, + /// Resolved active `UiTheme`, *including* any user `background_color` + /// override (`UiTheme::with_background_color`). The cell remap reads + /// target slots from this struct, not from `theme_id.ui_theme()`, so + /// `theme = "tokyo-night"` + `background_color = "#000000"` lands as a + /// pure-black surface instead of being overwritten back to + /// tokyo-night's `#16161e` by the remap. + active_ui_theme: UiTheme, /// During a resize event the terminal emulator may report stale dimensions /// for a brief window (observed on macOS Terminal.app and Windows ConHost). /// Forcing the expected size prevents ratatui's internal `autoresize` from @@ -40,6 +47,11 @@ impl ColorCompatBackend { depth, palette_mode, theme_id: ThemeId::System, + // Default to whatever System resolves to right now — it stays a + // no-op for the remap since `theme_id` is also System, so this + // initial value only matters once `set_theme` flips both fields + // to a community preset. + active_ui_theme: UiTheme::detect(), forced_size: None, } } @@ -56,8 +68,9 @@ impl ColorCompatBackend { self.palette_mode = palette_mode; } - pub(crate) fn set_theme(&mut self, theme_id: ThemeId) { + pub(crate) fn set_theme(&mut self, theme_id: ThemeId, ui_theme: UiTheme) { self.theme_id = theme_id; + self.active_ui_theme = ui_theme; } } @@ -81,7 +94,13 @@ impl Backend for ColorCompatBackend { let adapted = content .map(|(x, y, cell)| { let mut cell = cell.clone(); - adapt_cell_colors(&mut cell, self.depth, self.palette_mode, self.theme_id); + adapt_cell_colors( + &mut cell, + self.depth, + self.palette_mode, + self.theme_id, + &self.active_ui_theme, + ); (x, y, cell) }) .collect::>(); @@ -138,14 +157,15 @@ fn adapt_cell_colors( depth: ColorDepth, palette_mode: PaletteMode, theme_id: ThemeId, + ui_theme: &UiTheme, ) { // Stage 1: community-theme remap (dark palette → preset slots). No-op // for System / Whale / WhaleLight so legacy dark/light flows are // untouched. Runs *before* the palette-mode remap so a light terminal // running e.g. Catppuccin still routes the preset colors through the // light adaptation below (rare combo, but the sequencing is the same). - cell.fg = palette::adapt_fg_for_theme(cell.fg, theme_id); - cell.bg = palette::adapt_bg_for_theme(cell.bg, theme_id); + cell.fg = palette::adapt_fg_for_theme(cell.fg, theme_id, ui_theme); + cell.bg = palette::adapt_bg_for_theme(cell.bg, theme_id, ui_theme); // Stage 2: legacy dark↔light remap. let original_bg = cell.bg; cell.fg = palette::adapt_fg_for_palette_mode(cell.fg, original_bg, palette_mode); @@ -189,6 +209,7 @@ mod tests { ColorDepth::Ansi256, PaletteMode::Dark, ThemeId::System, + &palette::UI_THEME, ); assert!(matches!(cell.fg, Color::Indexed(_))); @@ -206,6 +227,7 @@ mod tests { ColorDepth::TrueColor, PaletteMode::Dark, ThemeId::System, + &palette::UI_THEME, ); assert_eq!(cell.fg, Color::Rgb(53, 120, 229)); @@ -240,12 +262,33 @@ mod tests { ColorDepth::TrueColor, PaletteMode::Light, ThemeId::WhaleLight, + &palette::LIGHT_UI_THEME, ); assert_eq!(cell.fg, palette::LIGHT_TEXT_BODY); assert_eq!(cell.bg, palette::LIGHT_SURFACE); } + #[test] + fn community_theme_remap_honors_background_color_override() { + // Tokyo Night + a custom black surface: the remap must rewrite + // `palette::DEEPSEEK_INK` to the *active* UiTheme's overridden + // surface, not to tokyo-night's default surface. + let active = palette::TOKYO_NIGHT_UI_THEME.with_background_color(Color::Rgb(0, 0, 0)); + let mut cell = Cell::default(); + cell.set_bg(palette::DEEPSEEK_INK); + + adapt_cell_colors( + &mut cell, + ColorDepth::TrueColor, + PaletteMode::Dark, + ThemeId::TokyoNight, + &active, + ); + + assert_eq!(cell.bg, Color::Rgb(0, 0, 0)); + } + #[test] fn backend_palette_mode_can_follow_runtime_theme_changes() { let writer = SharedWriter::default(); diff --git a/crates/tui/src/tui/theme_picker.rs b/crates/tui/src/tui/theme_picker.rs index be7f788b..e6b1b336 100644 --- a/crates/tui/src/tui/theme_picker.rs +++ b/crates/tui/src/tui/theme_picker.rs @@ -18,7 +18,7 @@ use ratatui::{ widgets::{Block, Borders, Clear, Padding, Paragraph, Widget}, }; -use crate::palette::{SELECTABLE_THEMES, ThemeId}; +use crate::palette::{SELECTABLE_THEMES, ThemeId, UiTheme}; use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent}; pub struct ThemePickerView { @@ -26,6 +26,10 @@ pub struct ThemePickerView { /// Settings name of the theme that was active when the picker opened. /// Used to revert on Esc. original_name: String, + /// Cached UiTheme for `ThemeId::System`, captured once at construction + /// so the per-frame render doesn't re-invoke `UiTheme::detect()` (which + /// reads `COLORFGBG`) on every keystroke. + system_ui_theme: UiTheme, } impl ThemePickerView { @@ -40,6 +44,7 @@ impl ThemePickerView { Self { selected, original_name, + system_ui_theme: UiTheme::detect(), } } @@ -50,6 +55,16 @@ impl ThemePickerView { .unwrap_or(ThemeId::System) } + /// Resolve a theme to a `UiTheme`, returning the cached `System` + /// resolution to avoid repeated env-var reads inside `render`. + fn ui_theme_for(&self, id: ThemeId) -> UiTheme { + if matches!(id, ThemeId::System) { + self.system_ui_theme + } else { + id.ui_theme() + } + } + fn preview_event(&self) -> ViewAction { ViewAction::Emit(ViewEvent::ConfigUpdated { key: "theme".to_string(), @@ -117,13 +132,15 @@ impl ModalView for ThemePickerView { self.selected = SELECTABLE_THEMES.len().saturating_sub(1); self.preview_event() } - // Number shortcuts: 1..=9 → jump to that row (1-indexed). + // Number shortcuts: '1'..='9' jump to that row (1-indexed). + // '0' is rejected explicitly — saturating_sub would otherwise + // collapse it onto row 0, which is unintuitive. KeyCode::Char(c) - if c.is_ascii_digit() + if matches!(c, '1'..='9') && !key.modifiers.contains(KeyModifiers::CONTROL) && !key.modifiers.contains(KeyModifiers::ALT) => { - let idx = (c as usize).saturating_sub('1' as usize); + let idx = (c as usize) - ('1' as usize); if idx < SELECTABLE_THEMES.len() { self.selected = idx; self.preview_event() @@ -136,10 +153,21 @@ impl ModalView for ThemePickerView { } fn render(&self, area: Rect, buf: &mut Buffer) { - let popup_width = 78.min(area.width.saturating_sub(4)).max(52); - // 1 title row + 1 spacer + N options + 2 spacer + 1 swatch row + // Modal must always fit inside `area`. The old `.max(52) / .max(10)` + // floors could produce dimensions larger than the available area on + // very small terminals (or split-pane setups), which then made the + // centering arithmetic underflow and ratatui assert. Take a + // soft-preferred size and clamp it strictly to `area`. + let popup_width = 78u16.min(area.width.saturating_sub(4)); + // 1 title + 1 spacer + N rows + spacer + bottom hint let needed_height = (SELECTABLE_THEMES.len() as u16).saturating_add(9); - let popup_height = needed_height.min(area.height.saturating_sub(4)).max(10); + let popup_height = needed_height.min(area.height.saturating_sub(4)); + + if popup_width == 0 || popup_height == 0 { + // Nothing sensible to draw — the host's caller has already + // cleared the area, so we just return. + return; + } let popup_area = Rect { x: area.x + (area.width.saturating_sub(popup_width)) / 2, @@ -153,7 +181,7 @@ impl ModalView for ThemePickerView { // skin the modal chrome. That way the popup itself shifts color as // the cursor moves, matching what the background will look like // after Enter. - let live = self.current().ui_theme(); + let live = self.ui_theme_for(self.current()); Clear.render(popup_area, buf); @@ -215,8 +243,9 @@ impl ModalView for ThemePickerView { // 3-cell color swatch per row using the candidate theme's own // accent + panel + border colors so the picker doubles as a - // legend. - let row_theme = id.ui_theme(); + // legend. Use the cached resolver so `System` doesn't repeat + // `UiTheme::detect()`. + let row_theme = self.ui_theme_for(id); let swatch = vec![ Span::styled(" ", Style::default().bg(row_theme.surface_bg)), Span::styled(" ", Style::default().bg(row_theme.panel_bg)), @@ -335,4 +364,34 @@ mod tests { Some(ThemeId::CatppuccinMocha.name()) ); } + + #[test] + fn digit_zero_is_rejected_not_remapped_to_row_zero() { + let mut v = ThemePickerView::new("dracula".to_string()); + let before = v.selected; + let action = v.handle_key(key(KeyCode::Char('0'))); + assert!(matches!(action, ViewAction::None)); + assert_eq!(v.selected, before, "'0' should not move the cursor"); + } + + #[test] + fn render_does_not_panic_on_zero_sized_area() { + // The picker historically panicked here via .max(W).max(H) floors + // that produced dimensions larger than the available area, then + // underflowed the centering arithmetic. + let v = ThemePickerView::new("system".to_string()); + let outer = ratatui::layout::Rect::new(0, 0, 10, 10); + let area = ratatui::layout::Rect::new(0, 0, 0, 0); + let mut buf = ratatui::buffer::Buffer::empty(outer); + v.render(area, &mut buf); + } + + #[test] + fn render_does_not_panic_on_tiny_area() { + // 20×6 is smaller than every soft floor the picker prefers. + let v = ThemePickerView::new("system".to_string()); + let area = ratatui::layout::Rect::new(0, 0, 20, 6); + let mut buf = ratatui::buffer::Buffer::empty(area); + v.render(area, &mut buf); + } } diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index dac36151..69825a07 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -5272,13 +5272,11 @@ async fn apply_command_result( } AppAction::OpenThemePicker => { if app.view_stack.top_kind() != Some(ModalKind::ThemePicker) { - // Capture the persisted theme name so Esc can revert - // through the same ConfigUpdated channel. Falling back - // to "system" when Settings::load fails matches the - // app-startup default. - let original = crate::settings::Settings::load() - .map(|s| s.theme) - .unwrap_or_else(|_| "system".to_string()); + // Capture the active theme name straight from `app` so + // Esc can revert through the same ConfigUpdated channel. + // Avoids re-reading settings.toml from disk on every + // `/theme` invocation. + let original = app.theme_id.name().to_string(); app.view_stack .push(crate::tui::theme_picker::ThemePickerView::new(original)); } @@ -6180,7 +6178,7 @@ fn draw_app_frame_inner( full_repaint: bool, ) -> Result<()> { terminal.backend_mut().set_palette_mode(app.ui_theme.mode); - terminal.backend_mut().set_theme(app.theme_id); + terminal.backend_mut().set_theme(app.theme_id, app.ui_theme); // DEC 2026 wrapping is on by default but can be turned off for // terminals that mishandle it (Ptyxis 50.x + VTE 0.84.x flashes the // whole viewport on every wrapped frame instead of deferring as the @@ -6464,7 +6462,12 @@ async fn handle_view_events( persist, } => { let result = commands::set_config_value(app, &key, &value, persist); - if let Some(msg) = result.message { + // Only surface the "key = value" confirmation when the + // change is being persisted. Live-preview events + // (`persist: false`, e.g. arrow keys in the theme picker) + // fire on every navigation tick and would otherwise spam + // a `System` cell into the transcript per row visited. + if persist && let Some(msg) = result.message { app.add_message(HistoryCell::System { content: msg }); }