From d7eb134692a072c2a2f3fd77189a763f770716f2 Mon Sep 17 00:00:00 2001 From: David Wiesen Date: Thu, 6 Nov 2025 12:40:12 -0800 Subject: [PATCH] more review comment fixes. --- codex-rs/tui/src/exec_cell/render.rs | 3 +- codex-rs/tui/src/terminal_palette.rs | 62 +++++++++++++++++----------- 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/codex-rs/tui/src/exec_cell/render.rs b/codex-rs/tui/src/exec_cell/render.rs index 85648c5121..e7845b5038 100644 --- a/codex-rs/tui/src/exec_cell/render.rs +++ b/codex-rs/tui/src/exec_cell/render.rs @@ -140,8 +140,7 @@ pub(crate) fn output_lines( pub(crate) fn spinner(start_time: Option) -> Span<'static> { let elapsed = start_time.map(|st| st.elapsed()).unwrap_or_default(); - if crate::terminal_palette::stdout_supports_truecolor() - { + if crate::terminal_palette::stdout_supports_truecolor() { shimmer_spans("•")[0].clone() } else { let blink_on = (elapsed.as_millis() / 600).is_multiple_of(2); diff --git a/codex-rs/tui/src/terminal_palette.rs b/codex-rs/tui/src/terminal_palette.rs index 9a9c17c53c..bd715c412e 100644 --- a/codex-rs/tui/src/terminal_palette.rs +++ b/codex-rs/tui/src/terminal_palette.rs @@ -1,6 +1,7 @@ use crate::color::perceptual_distance; use ratatui::style::Color; -use std::sync::OnceLock; +use std::sync::LazyLock; +use std::sync::Mutex; /// Returns the closest color to the target color that the terminal can display. pub fn best_color(target: (u8, u8, u8)) -> Color { @@ -75,12 +76,15 @@ fn enable_vt_stdout() -> std::io::Result<()> { std::io::ErrorKind::Other, "ANSI support unavailable", )), - Err(e) => Err(std::io::Error::new(std::io::ErrorKind::Other, format!("{e}"))), + Err(e) => Err(std::io::Error::new( + std::io::ErrorKind::Other, + format!("{e}"), + )), } } pub fn requery_default_colors() { - imp::requery_default_colors(); + imp::refresh_cached_default_colors(); } #[derive(Clone, Copy)] @@ -90,7 +94,7 @@ pub struct DefaultColors { } pub fn default_colors() -> Option { - imp::default_colors() + imp::cached_default_colors() } pub fn default_fg() -> Option<(u8, u8, u8)> { @@ -143,36 +147,41 @@ impl Cache { } } -fn default_colors_cache() -> &'static std::sync::Mutex> { - static CACHE: OnceLock>> = OnceLock::new(); - CACHE.get_or_init(|| std::sync::Mutex::new(Cache::default())) +fn default_terminal_colors_cache() -> &'static Mutex> { + // LazyLock creates the single cache instance; the Mutex guards concurrent refreshes/reads. + static CACHE: LazyLock>> = + LazyLock::new(|| Mutex::new(Cache::default())); + &CACHE } #[cfg(all(unix, not(test)))] mod imp { - use super::default_colors_cache; use super::DefaultColors; + use super::default_terminal_colors_cache; use crossterm::style::Color as CrosstermColor; use crossterm::style::query_background_color; use crossterm::style::query_foreground_color; - pub(super) fn default_colors() -> Option { - let cache = default_colors_cache(); + // Returns cached terminal defaults, probing once on first use. + pub(super) fn cached_default_colors() -> Option { + let cache = default_terminal_colors_cache(); let mut cache = cache.lock().ok()?; - cache.get_or_init_with(|| query_default_colors().unwrap_or_default()) + cache.get_or_init_with(|| probe_default_terminal_colors().unwrap_or_default()) } - pub(super) fn requery_default_colors() { - if let Ok(mut cache) = default_colors_cache().lock() { + // Refreshes cached defaults unless we already know probing fails. + pub(super) fn refresh_cached_default_colors() { + if let Ok(mut cache) = default_terminal_colors_cache().lock() { // Don't try to refresh if the cache is already attempted and failed. if cache.attempted && cache.value.is_none() { return; } - cache.refresh_with(|| query_default_colors().unwrap_or_default()); + cache.refresh_with(|| probe_default_terminal_colors().unwrap_or_default()); } } - fn query_default_colors() -> std::io::Result> { + // Probes the terminal for default colors; returns None when unsupported/unavailable. + fn probe_default_terminal_colors() -> std::io::Result> { let fg = query_foreground_color()?.and_then(color_to_tuple); let bg = query_background_color()?.and_then(color_to_tuple); Ok(fg.zip(bg).map(|(fg, bg)| DefaultColors { fg, bg })) @@ -188,26 +197,29 @@ mod imp { #[cfg(all(windows, not(test)))] mod imp { - use super::default_colors_cache; use super::DefaultColors; + use super::default_terminal_colors_cache; - pub(super) fn default_colors() -> Option { - let cache = default_colors_cache(); + // Returns cached terminal defaults, probing once on first use. + pub(super) fn cached_default_colors() -> Option { + let cache = default_terminal_colors_cache(); let mut cache = cache.lock().ok()?; - cache.get_or_init_with(|| query_default_colors().unwrap_or_default()) + cache.get_or_init_with(|| probe_default_terminal_colors().unwrap_or_default()) } - pub(super) fn requery_default_colors() { - if let Ok(mut cache) = default_colors_cache().lock() { + // Refreshes cached defaults unless we already know probing fails. + pub(super) fn refresh_cached_default_colors() { + if let Ok(mut cache) = default_terminal_colors_cache().lock() { // Don't try to refresh if the cache is already attempted and failed. if cache.attempted && cache.value.is_none() { return; } - cache.refresh_with(|| query_default_colors().unwrap_or_default()); + cache.refresh_with(|| probe_default_terminal_colors().unwrap_or_default()); } } - fn query_default_colors() -> std::io::Result> { + // Probes the terminal for default colors; returns None when unsupported/unavailable. + fn probe_default_terminal_colors() -> std::io::Result> { match terminal_colorsaurus::color_palette(terminal_colorsaurus::QueryOptions::default()) { Ok(p) => { let (fr, fg, fb) = p.foreground.scale_to_8bit(); @@ -225,11 +237,11 @@ mod imp { #[cfg(not(any(all(unix, not(test)), all(windows, not(test)))))] mod imp { use super::DefaultColors; - pub(super) fn default_colors() -> Option { + pub(super) fn cached_default_colors() -> Option { None } - pub(super) fn requery_default_colors() {} + pub(super) fn refresh_cached_default_colors() {} } /// The subset of Xterm colors that are usually consistent across terminals.