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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
};
|
||||
|
||||
@@ -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<W: Write> {
|
||||
@@ -26,6 +26,13 @@ pub(crate) struct ColorCompatBackend<W: Write> {
|
||||
/// 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<W: Write> ColorCompatBackend<W> {
|
||||
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<W: Write> ColorCompatBackend<W> {
|
||||
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<W: Write> Backend for ColorCompatBackend<W> {
|
||||
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::<Vec<_>>();
|
||||
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user