From 09b57f590dba092a7e547d95f021714765038fb7 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Thu, 6 Nov 2025 09:00:34 -0800 Subject: [PATCH] address review feedback --- codex-rs/tui/src/exec_cell/render.rs | 4 +- codex-rs/tui/src/shimmer.rs | 4 +- codex-rs/tui/src/terminal_palette.rs | 172 +++++++++++++-------------- 3 files changed, 84 insertions(+), 96 deletions(-) diff --git a/codex-rs/tui/src/exec_cell/render.rs b/codex-rs/tui/src/exec_cell/render.rs index 3ccc0527e9..85648c5121 100644 --- a/codex-rs/tui/src/exec_cell/render.rs +++ b/codex-rs/tui/src/exec_cell/render.rs @@ -140,9 +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 supports_color::on_cached(supports_color::Stream::Stdout) - .map(|level| level.has_16m) - .unwrap_or(false) + if crate::terminal_palette::stdout_supports_truecolor() { shimmer_spans("•")[0].clone() } else { diff --git a/codex-rs/tui/src/shimmer.rs b/codex-rs/tui/src/shimmer.rs index ba081e7c49..3d8289d9c3 100644 --- a/codex-rs/tui/src/shimmer.rs +++ b/codex-rs/tui/src/shimmer.rs @@ -30,9 +30,7 @@ pub(crate) fn shimmer_spans(text: &str) -> Vec> { let pos_f = (elapsed_since_start().as_secs_f32() % sweep_seconds) / sweep_seconds * (period as f32); let pos = pos_f as usize; - let has_true_color = supports_color::on_cached(supports_color::Stream::Stdout) - .map(|level| level.has_16m) - .unwrap_or(false); + let has_true_color = crate::terminal_palette::stdout_supports_truecolor(); let band_half_width = 5.0; let mut spans: Vec> = Vec::with_capacity(chars.len()); diff --git a/codex-rs/tui/src/terminal_palette.rs b/codex-rs/tui/src/terminal_palette.rs index ca01265ed3..f8bb4e334f 100644 --- a/codex-rs/tui/src/terminal_palette.rs +++ b/codex-rs/tui/src/terminal_palette.rs @@ -1,12 +1,13 @@ use crate::color::perceptual_distance; use ratatui::style::Color; +use std::sync::OnceLock; /// Returns the closest color to the target color that the terminal can display. pub fn best_color(target: (u8, u8, u8)) -> Color { let Some(color_level) = supports_color::on_cached(supports_color::Stream::Stdout) else { return Color::default(); }; - if stdout_has_truecolor(&color_level) { + if stdout_supports_truecolor() { let (r, g, b) = target; #[allow(clippy::disallowed_methods)] Color::Rgb(r, g, b) @@ -25,31 +26,47 @@ pub fn best_color(target: (u8, u8, u8)) -> Color { } } -#[cfg(not(windows))] -fn stdout_has_truecolor(level: &supports_color::ColorLevel) -> bool { - level.has_16m -} - -#[cfg(windows)] -fn stdout_has_truecolor(level: &supports_color::ColorLevel) -> bool { - if level.has_16m { - return true; +/// Returns true if stdout supports truecolor (24-bit), using a single shared decision. +/// +/// - On non-Windows, this is simply `supports_color`'s 16m capability. +/// - On Windows, we upgrade to truecolor on Windows Terminal if Virtual Terminal +/// Processing can be enabled successfully. This probing happens once and is cached. +pub fn stdout_supports_truecolor() -> bool { + #[cfg(windows)] + { + static TRUECOLOR: OnceLock = OnceLock::new(); + *TRUECOLOR.get_or_init(|| { + let has = supports_color::on_cached(supports_color::Stream::Stdout) + .map(|level| level.has_16m) + .unwrap_or(false); + if has { + return true; + } + // Upgrade to truecolor on Windows Terminal when VT processing is available. + // We only attempt to enable VT once per process to avoid per-frame overhead. + std::env::var_os("WT_SESSION").is_some() && enable_vt_stdout().is_ok() + }) + } + #[cfg(not(windows))] + { + supports_color::on_cached(supports_color::Stream::Stdout) + .map(|level| level.has_16m) + .unwrap_or(false) } - // Upgrade to truecolor on Windows Terminal when VT processing is available. - std::env::var_os("WT_SESSION").is_some() && enable_vt_stdout().is_ok() } /// Enables Virtual Terminal Processing (ANSI escape sequence handling) on Windows for stdout. /// -/// This ensures that ANSI SGR sequences (including 24‑bit truecolor) are interpreted by the +/// This ensures that ANSI SGR sequences (including 24-bit truecolor) are interpreted by the /// Windows console pipeline. We use this as a lightweight capability gate before upgrading to /// truecolor on Windows Terminal: /// -/// - If enabling VT processing succeeds, we can safely emit 24‑bit color. -/// - If it fails (e.g., legacy consoles or non‑TTY), we do not upgrade and fall back to the +/// - If enabling VT processing succeeds, we can safely emit 24-bit color. +/// - If it fails (e.g., legacy consoles or non-TTY), we do not upgrade and fall back to the /// base `supports_color` decision to avoid printing raw escape sequences. /// -/// Calling this repeatedly is idempotent (it simply sets the corresponding console mode flag). +/// Calling this repeatedly is idempotent (it simply sets the corresponding console mode flag), +/// and we cache the result in `stdout_supports_truecolor` so we only attempt it once. #[cfg(windows)] fn enable_vt_stdout() -> std::io::Result<()> { use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; @@ -98,49 +115,60 @@ pub fn default_bg() -> Option<(u8, u8, u8)> { default_colors().map(|c| c.bg) } +// ------------------------------------------------------------------------------------------------- +// Shared cache utility for default terminal colors +// ------------------------------------------------------------------------------------------------- + +/// Small helper cache that remembers whether initialization was attempted and stores a value. +/// +/// Used to memoize the terminal's default foreground/background colors. We expose both +/// `get_or_init_with` for the first successful query and `refresh_with` for a manual re-query. +/// If a first query fails, we remember the failure to avoid repeated failed probes, unless +/// a subsequent explicit refresh is performed. +struct Cache { + attempted: bool, + value: Option, +} + +impl Default for Cache { + fn default() -> Self { + Self { + attempted: false, + value: None, + } + } +} + +impl Cache { + /// Returns the cached value if available; otherwise runs `init` once and caches its result. + fn get_or_init_with(&mut self, mut init: impl FnMut() -> Option) -> Option { + if !self.attempted { + self.value = init(); + self.attempted = true; + } + self.value + } + + /// Re-runs `init` and replaces the cached value. Marks the cache as attempted. + fn refresh_with(&mut self, mut init: impl FnMut() -> Option) -> Option { + self.value = init(); + self.attempted = true; + self.value + } +} + +fn default_colors_cache() -> &'static std::sync::Mutex> { + static CACHE: OnceLock>> = OnceLock::new(); + CACHE.get_or_init(|| std::sync::Mutex::new(Cache::default())) +} + #[cfg(all(unix, not(test)))] mod imp { + use super::default_colors_cache; use super::DefaultColors; use crossterm::style::Color as CrosstermColor; use crossterm::style::query_background_color; use crossterm::style::query_foreground_color; - use std::sync::Mutex; - use std::sync::OnceLock; - - struct Cache { - attempted: bool, - value: Option, - } - - impl Default for Cache { - fn default() -> Self { - Self { - attempted: false, - value: None, - } - } - } - - impl Cache { - fn get_or_init_with(&mut self, mut init: impl FnMut() -> Option) -> Option { - if !self.attempted { - self.value = init(); - self.attempted = true; - } - self.value - } - - fn refresh_with(&mut self, mut init: impl FnMut() -> Option) -> Option { - self.value = init(); - self.attempted = true; - self.value - } - } - - fn default_colors_cache() -> &'static Mutex> { - static CACHE: OnceLock>> = OnceLock::new(); - CACHE.get_or_init(|| Mutex::new(Cache::default())) - } pub(super) fn default_colors() -> Option { let cache = default_colors_cache(); @@ -174,44 +202,8 @@ mod imp { #[cfg(all(windows, not(test)))] mod imp { + use super::default_colors_cache; use super::DefaultColors; - use std::sync::Mutex; - use std::sync::OnceLock; - - struct Cache { - attempted: bool, - value: Option, - } - - impl Default for Cache { - fn default() -> Self { - Self { - attempted: false, - value: None, - } - } - } - - impl Cache { - fn get_or_init_with(&mut self, mut init: impl FnMut() -> Option) -> Option { - if !self.attempted { - self.value = init(); - self.attempted = true; - } - self.value - } - - fn refresh_with(&mut self, mut init: impl FnMut() -> Option) -> Option { - self.value = init(); - self.attempted = true; - self.value - } - } - - fn default_colors_cache() -> &'static Mutex> { - static CACHE: OnceLock>> = OnceLock::new(); - CACHE.get_or_init(|| Mutex::new(Cache::default())) - } pub(super) fn default_colors() -> Option { let cache = default_colors_cache();