clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-04-24 14:45:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(tui): address syntax-theme review feedback

Browse Source
This commit is contained in:
Felipe Coury
2026-02-11 15:08:04 -03:00
parent 486f5850ca
commit 55c961040c
4 changed files with 65 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

54
codex-rs/tui/src/bottom_pane/list_selection_view.rs
View File

@@ -42,6 +42,9 @@ const MIN_LIST_WIDTH_FOR_SIDE: u16 = 40;
/// panel when side-by-side layout is active.
const SIDE_CONTENT_GAP: u16 = 2;
/// Shared menu-surface horizontal inset (2 cells per side) used by selection popups.
const MENU_SURFACE_HORIZONTAL_INSET: u16 = 4;
/// Controls how the side content panel is sized relative to the popup width.
///
/// When the computed side width falls below `side_content_min_width` or the
@@ -61,17 +64,44 @@ impl Default for SideContentWidth {
}
}
/// Returns the popup content width after subtracting the shared menu-surface
/// horizontal inset (2 columns on each side).
pub(crate) fn popup_content_width(total_width: u16) -> u16 {
total_width.saturating_sub(MENU_SURFACE_HORIZONTAL_INSET)
}
/// Returns side-by-side layout widths as `(list_width, side_width)` when the
/// layout can fit. Returns `None` when the side panel is disabled/too narrow or
/// when the remaining list width would become unusably small.
pub(crate) fn side_by_side_layout_widths(
content_width: u16,
side_content_width: SideContentWidth,
side_content_min_width: u16,
) -> Option<(u16, u16)> {
let side_width = match side_content_width {
SideContentWidth::Fixed(0) => return None,
SideContentWidth::Fixed(width) => width,
SideContentWidth::Half => content_width.saturating_sub(SIDE_CONTENT_GAP) / 2,
};
if side_width < side_content_min_width {
return None;
}
let list_width = content_width.saturating_sub(SIDE_CONTENT_GAP + side_width);
(list_width >= MIN_LIST_WIDTH_FOR_SIDE).then_some((list_width, side_width))
}
/// One selectable item in the generic selection list.
pub(crate) type SelectionAction = Box<dyn Fn(&AppEventSender) + Send + Sync>;
/// Callback invoked whenever the highlighted item changes (arrow keys, search
/// filter, number-key jump). Receives the *actual* index into the unfiltered
/// `items` list and the event sender. Used by the theme picker for live preview.
pub type OnSelectionChangedCallback = Option<Box<dyn Fn(usize, &AppEventSender) + Send + Sync>>;
pub(crate) type OnSelectionChangedCallback =
Option<Box<dyn Fn(usize, &AppEventSender) + Send + Sync>>;
/// Callback invoked when the picker is dismissed without accepting (Esc or
/// Ctrl+C). Used by the theme picker to restore the pre-open theme.
pub type OnCancelCallback = Option<Box<dyn Fn(&AppEventSender) + Send + Sync>>;
pub(crate) type OnCancelCallback = Option<Box<dyn Fn(&AppEventSender) + Send + Sync>>;
/// One row in a [`ListSelectionView`] selection list.
///
@@ -488,16 +518,12 @@ impl ListSelectionView {
/// Returns `Some(side_width)` when the content area is wide enough for a
/// side-by-side layout (list + gap + side panel), `None` otherwise.
fn side_layout_width(&self, content_width: u16) -> Option<u16> {
let side_width = match self.side_content_width {
SideContentWidth::Fixed(0) => return None,
SideContentWidth::Fixed(width) => width,
SideContentWidth::Half => content_width.saturating_sub(SIDE_CONTENT_GAP) / 2,
};
if side_width < self.side_content_min_width {
return None;
}
let list_width = content_width.saturating_sub(SIDE_CONTENT_GAP + side_width);
(list_width >= MIN_LIST_WIDTH_FOR_SIDE).then_some(side_width)
side_by_side_layout_widths(
content_width,
self.side_content_width,
self.side_content_min_width,
)
.map(|(_, side_width)| side_width)
}
fn skip_disabled_down(&mut self) {
@@ -652,7 +678,7 @@ impl BottomPaneView for ListSelectionView {
impl Renderable for ListSelectionView {
fn desired_height(&self, width: u16) -> u16 {
// Inner content width after menu surface horizontal insets (2 per side).
let inner_width = width.saturating_sub(4);
let inner_width = popup_content_width(width);
// When side-by-side is active, measure the list at the reduced width
// that accounts for the gap and side panel.
@@ -734,7 +760,7 @@ impl Renderable for ListSelectionView {
// Paint the shared menu surface and then layout inside the returned inset.
let content_area = render_menu_surface(outer_content_area, buf);
let inner_width = outer_content_area.width.saturating_sub(4);
let inner_width = popup_content_width(outer_content_area.width);
let side_w = self.side_layout_width(inner_width);
// When side-by-side is active, shrink the list to make room.

2
codex-rs/tui/src/bottom_pane/mod.rs
View File

@@ -79,6 +79,8 @@ pub(crate) use footer::CollaborationModeIndicator;
pub(crate) use list_selection_view::ColumnWidthMode;
pub(crate) use list_selection_view::SelectionViewParams;
pub(crate) use list_selection_view::SideContentWidth;
pub(crate) use list_selection_view::popup_content_width;
pub(crate) use list_selection_view::side_by_side_layout_widths;
mod feedback_view;
pub(crate) use feedback_view::FeedbackAudience;
pub(crate) use feedback_view::feedback_disabled_params;

10
codex-rs/tui/src/render/highlight.rs
View File

@@ -241,10 +241,12 @@ pub(crate) fn current_syntax_theme() -> Theme {
}
}
/// Return the kebab-case name of the currently active theme.
/// This accounts for the user override, custom .tmTheme files, and the
/// adaptive auto-detection fallback.
pub(crate) fn current_theme_name() -> String {
/// Return the configured kebab-case theme name when it resolves; otherwise
/// return the adaptive auto-detected default theme name.
///
/// This intentionally reflects persisted configuration/default selection, not
/// transient runtime swaps applied via `set_syntax_theme`.
pub(crate) fn configured_theme_name() -> String {
// Explicit user override?
if let Some(Some(name)) = THEME_OVERRIDE.get() {
if parse_theme_name(name).is_some() {

41
codex-rs/tui/src/theme_picker.rs
View File

@@ -26,6 +26,8 @@ use crate::bottom_pane::SelectionItem;
use crate::bottom_pane::SelectionViewParams;
use crate::bottom_pane::SideContentWidth;
use crate::bottom_pane::popup_consts::standard_popup_hint_line;
use crate::bottom_pane::popup_content_width;
use crate::bottom_pane::side_by_side_layout_widths;
use crate::diff_render::DiffLineType;
use crate::diff_render::line_number_width;
use crate::diff_render::push_wrapped_diff_line;
@@ -134,15 +136,6 @@ const PREVIEW_FRAME_PADDING: u16 = 1;
const PREVIEW_FALLBACK_SUBTITLE: &str = "Move up/down to live preview themes";
/// Shared menu-surface horizontal inset (2 cells per side) used by selection popups.
const MENU_SURFACE_HORIZONTAL_INSET: u16 = 4;
/// Horizontal gap between list and side panel when side-by-side layout is active.
const SIDE_CONTENT_GAP: u16 = 2;
/// Minimum list width required for side-by-side mode in the selection popup.
const MIN_LIST_WIDTH_FOR_SIDE: u16 = 40;
/// Side-by-side preview: syntax-highlighted Rust diff snippet, vertically
/// centered with a 2-column left inset. Fills the entire side panel height.
struct ThemePreviewWideRenderable;
@@ -261,12 +254,12 @@ impl Renderable for ThemePreviewNarrowRenderable {
fn subtitle_available_width(terminal_width: Option<u16>) -> usize {
let width = terminal_width.unwrap_or(80);
let content_width = width.saturating_sub(MENU_SURFACE_HORIZONTAL_INSET);
let side_width = content_width.saturating_sub(SIDE_CONTENT_GAP) / 2;
let list_width = content_width.saturating_sub(SIDE_CONTENT_GAP + side_width);
let side_by_side =
side_width >= WIDE_PREVIEW_MIN_WIDTH && list_width >= MIN_LIST_WIDTH_FOR_SIDE;
if side_by_side {
let content_width = popup_content_width(width);
if let Some((list_width, _side_width)) = side_by_side_layout_widths(
content_width,
SideContentWidth::Half,
WIDE_PREVIEW_MIN_WIDTH,
) {
list_width as usize
} else {
content_width as usize
@@ -299,9 +292,9 @@ fn theme_picker_subtitle(codex_home: Option<&Path>, terminal_width: Option<u16>)
///
/// `current_name` should be the value of `Config::tui_theme` (the persisted
/// preference). When it names a theme that is currently available the picker
/// pre-selects it; otherwise the picker falls back to the active runtime theme
/// (from adaptive auto-detection) so opening the picker without a persisted
/// preference still highlights the correct entry.
/// pre-selects it; otherwise the picker falls back to the configured name (or
/// adaptive default) so opening the picker without a persisted preference still
/// highlights the most likely intended entry.
pub(crate) fn build_theme_picker_params(
current_name: Option<&str>,
codex_home: Option<&Path>,
@@ -314,14 +307,14 @@ pub(crate) fn build_theme_picker_params(
let codex_home_owned = codex_home.map(Path::to_path_buf);
// Resolve the effective theme name: honor explicit config only when it is
// currently available; otherwise fall back to the active runtime theme so
// opening `/theme` does not auto-preview an unrelated first entry.
// currently available; otherwise fall back to configured/default selection
// so opening `/theme` does not auto-preview an unrelated first entry.
let effective_name = if let Some(name) = current_name
&& entries.iter().any(|entry| entry.name == name)
{
name.to_string()
} else {
highlight::current_theme_name()
highlight::configured_theme_name()
};
// Track the index of the current theme so we can pre-select it.
@@ -611,8 +604,8 @@ mod tests {
}
#[test]
fn unavailable_configured_theme_falls_back_to_active_theme_selection() {
let active_theme = highlight::current_theme_name();
fn unavailable_configured_theme_falls_back_to_configured_or_default_selection() {
let configured_or_default_theme = highlight::configured_theme_name();
let params = build_theme_picker_params(Some("not-a-real-theme"), None, Some(120));
let selected_idx = params
.initial_selected_idx
@@ -622,6 +615,6 @@ mod tests {
.as_deref()
.expect("expected search value to contain canonical theme name");
assert_eq!(selected_name, active_theme);
assert_eq!(selected_name, configured_or_default_theme);
}
}