From 118e6598481ee89d3e68682ef72f5eece53b79e3 Mon Sep 17 00:00:00 2001 From: Daniel Edrisian Date: Tue, 16 Sep 2025 19:48:38 -0700 Subject: [PATCH] Revert "fix permissions alignment" This reverts commit 45bccd36b038a28b23663189cc6f7557e49e06d0. --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 11 +- codex-rs/tui/src/bottom_pane/command_popup.rs | 33 +-- .../src/bottom_pane/list_selection_view.rs | 34 +-- .../src/bottom_pane/selection_popup_common.rs | 280 +++++------------- ..._chat_composer__tests__slash_popup_mo.snap | 2 +- ..._list_selection_spacing_with_subtitle.snap | 2 +- ...st_selection_spacing_without_subtitle.snap | 2 +- 7 files changed, 85 insertions(+), 279 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index bf11ff571d..2138d54098 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -142,16 +142,14 @@ impl ChatComposer { .desired_height(width.saturating_sub(LIVE_PREFIX_COLS)) + match &self.active_popup { ActivePopup::None => FOOTER_HEIGHT_WITH_HINT, - ActivePopup::Command(c) => c.calculate_required_height(width), + ActivePopup::Command(c) => c.calculate_required_height(), ActivePopup::File(c) => c.calculate_required_height(), } } pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { let popup_constraint = match &self.active_popup { - ActivePopup::Command(popup) => { - Constraint::Max(popup.calculate_required_height(area.width)) - } + ActivePopup::Command(popup) => Constraint::Max(popup.calculate_required_height()), ActivePopup::File(popup) => Constraint::Max(popup.calculate_required_height()), ActivePopup::None => Constraint::Max(FOOTER_HEIGHT_WITH_HINT), }; @@ -1234,10 +1232,7 @@ impl ChatComposer { impl WidgetRef for ChatComposer { fn render_ref(&self, area: Rect, buf: &mut Buffer) { let (popup_constraint, hint_spacing) = match &self.active_popup { - ActivePopup::Command(popup) => ( - Constraint::Max(popup.calculate_required_height(area.width)), - 0, - ), + ActivePopup::Command(popup) => (Constraint::Max(popup.calculate_required_height()), 0), ActivePopup::File(popup) => (Constraint::Max(popup.calculate_required_height()), 0), ActivePopup::None => ( Constraint::Length(FOOTER_HEIGHT_WITH_HINT), diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index 8de266f819..a5961e3188 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -92,35 +92,10 @@ impl CommandPopup { .ensure_visible(matches_len, MAX_POPUP_ROWS.min(matches_len)); } - /// Determine the preferred height of the popup for a given width. - /// Accounts for wrapped descriptions so that long tooltips don't overflow. - pub(crate) fn calculate_required_height(&self, width: u16) -> u16 { - use super::selection_popup_common::GenericDisplayRow; - use super::selection_popup_common::measure_rows_height; - let matches = self.filtered(); - let rows_all: Vec = if matches.is_empty() { - Vec::new() - } else { - matches - .into_iter() - .map(|(item, indices, _)| match item { - CommandItem::Builtin(cmd) => GenericDisplayRow { - name: format!("/{}", cmd.command()), - match_indices: indices.map(|v| v.into_iter().map(|i| i + 1).collect()), - is_current: false, - description: Some(cmd.description().to_string()), - }, - CommandItem::UserPrompt(i) => GenericDisplayRow { - name: format!("/{}", self.prompts[i].name), - match_indices: indices.map(|v| v.into_iter().map(|i| i + 1).collect()), - is_current: false, - description: Some("send saved prompt".to_string()), - }, - }) - .collect() - }; - - measure_rows_height(&rows_all, &self.state, MAX_POPUP_ROWS, width) + /// Determine the preferred height of the popup. This is the number of + /// rows required to show at most MAX_POPUP_ROWS commands. + pub(crate) fn calculate_required_height(&self) -> u16 { + self.filtered_items().len().clamp(1, MAX_POPUP_ROWS) as u16 } /// Compute fuzzy-filtered matches over built-in commands and user prompts, diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 5d5dbf0f33..7be91db202 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -17,7 +17,6 @@ use super::bottom_pane_view::BottomPaneView; use super::popup_consts::MAX_POPUP_ROWS; use super::scroll_state::ScrollState; use super::selection_popup_common::GenericDisplayRow; -use super::selection_popup_common::measure_rows_height; use super::selection_popup_common::render_rows; /// One selectable item in the generic selection list. @@ -136,36 +135,11 @@ impl BottomPaneView for ListSelectionView { CancellationEvent::Handled } - fn desired_height(&self, width: u16) -> u16 { - // Measure wrapped height for up to MAX_POPUP_ROWS items at the given width. - // Build the same display rows used by the renderer so wrapping math matches. - let rows: Vec = self - .items - .iter() - .enumerate() - .map(|(i, it)| { - let is_selected = self.state.selected_idx == Some(i); - let prefix = if is_selected { '>' } else { ' ' }; - let name_with_marker = if it.is_current { - format!("{} (current)", it.name) - } else { - it.name.clone() - }; - let display_name = format!("{} {}. {}", prefix, i + 1, name_with_marker); - GenericDisplayRow { - name: display_name, - match_indices: None, - is_current: it.is_current, - description: it.description.clone(), - } - }) - .collect(); - - let rows_height = measure_rows_height(&rows, &self.state, MAX_POPUP_ROWS, width); - + fn desired_height(&self, _width: u16) -> u16 { + let rows = (self.items.len()).clamp(1, MAX_POPUP_ROWS); // +1 for the title row, +1 for a spacer line beneath the header, - // +1 for optional subtitle, +1 for optional footer (2 lines incl. spacing) - let mut height = rows_height + 2; + // +1 for optional subtitle, +1 for optional footer + let mut height = rows as u16 + 2; if self.subtitle.is_some() { // +1 for subtitle (the spacer is accounted for above) height = height.saturating_add(1); diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index 61a26f9341..3852d10bc1 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -1,7 +1,6 @@ use ratatui::buffer::Buffer; use ratatui::layout::Rect; -// Note: Table-based layout previously used Constraint; the manual renderer -// below no longer requires it. +use ratatui::prelude::Constraint; use ratatui::style::Color; use ratatui::style::Modifier; use ratatui::style::Style; @@ -11,7 +10,9 @@ use ratatui::text::Span; use ratatui::widgets::Block; use ratatui::widgets::BorderType; use ratatui::widgets::Borders; -use ratatui::widgets::Paragraph; +use ratatui::widgets::Cell; +use ratatui::widgets::Row; +use ratatui::widgets::Table; use ratatui::widgets::Widget; use super::scroll_state::ScrollState; @@ -26,61 +27,6 @@ pub(crate) struct GenericDisplayRow { impl GenericDisplayRow {} -/// Compute a shared description-column start based on the widest visible name -/// plus two spaces of padding. Ensures at least one column is left for the -/// description. -fn compute_desc_col( - rows_all: &[GenericDisplayRow], - start_idx: usize, - visible_items: usize, - content_width: u16, -) -> usize { - let visible_range = start_idx..(start_idx + visible_items); - let max_name_width = rows_all - .iter() - .enumerate() - .filter(|(i, _)| visible_range.contains(i)) - .map(|(_, r)| Line::from(r.name.clone()).width()) - .max() - .unwrap_or(0); - let mut desc_col = max_name_width.saturating_add(2); - if (desc_col as u16) >= content_width { - desc_col = content_width.saturating_sub(1) as usize; - } - desc_col -} - -/// Build the full display line for a row with the description padded to start -/// at `desc_col`. Applies fuzzy-match bolding when indices are present and -/// dims the description. -fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { - let mut name_spans: Vec = Vec::with_capacity(row.name.len()); - if let Some(idxs) = row.match_indices.as_ref() { - let mut idx_iter = idxs.iter().peekable(); - for (char_idx, ch) in row.name.chars().enumerate() { - if idx_iter.peek().is_some_and(|next| **next == char_idx) { - idx_iter.next(); - name_spans.push(ch.to_string().bold()); - } else { - name_spans.push(ch.to_string().into()); - } - } - } else { - name_spans.push(row.name.clone().into()); - } - - let this_name_width = Line::from(name_spans.clone()).width(); - let mut full_spans: Vec = name_spans; - if let Some(desc) = row.description.as_ref() { - let gap = desc_col.saturating_sub(this_name_width); - if gap > 0 { - full_spans.push(" ".repeat(gap).into()); - } - full_spans.push(desc.clone().dim()); - } - Line::from(full_spans) -} - /// Render a list of rows using the provided ScrollState, with shared styling /// and behavior for selection popups. pub(crate) fn render_rows( @@ -92,168 +38,84 @@ pub(crate) fn render_rows( _dim_non_selected: bool, empty_message: &str, ) { - // Always draw a dim left border to match other popups. - let block = Block::default() - .borders(Borders::LEFT) - .border_type(BorderType::QuadrantOutside) - .border_style(Style::default().add_modifier(Modifier::DIM)); - block.render(area, buf); - - // Content renders to the right of the border. - let content_area = Rect { - x: area.x.saturating_add(1), - y: area.y, - width: area.width.saturating_sub(1), - height: area.height, - }; - + let mut rows: Vec = Vec::new(); if rows_all.is_empty() { - if content_area.height > 0 { - let para = Paragraph::new(Line::from(empty_message.dim().italic())); - para.render( - Rect { - x: content_area.x, - y: content_area.y, - width: content_area.width, - height: 1, - }, - buf, - ); - } - return; - } + rows.push(Row::new(vec![Cell::from(Line::from( + empty_message.dim().italic(), + ))])); + } else { + let max_rows_from_area = area.height as usize; + let visible_rows = max_results + .min(rows_all.len()) + .min(max_rows_from_area.max(1)); - // Determine which logical rows (items) are visible given the selection and - // the max_results clamp. Scrolling is still item-based for simplicity. - let max_rows_from_area = content_area.height as usize; - let visible_items = max_results - .min(rows_all.len()) - .min(max_rows_from_area.max(1)); - - let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); - if let Some(sel) = state.selected_idx { - if sel < start_idx { - start_idx = sel; - } else if visible_items > 0 { - let bottom = start_idx + visible_items - 1; - if sel > bottom { - start_idx = sel + 1 - visible_items; + // Compute starting index based on scroll state and selection. + let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); + if let Some(sel) = state.selected_idx { + if sel < start_idx { + start_idx = sel; + } else if visible_rows > 0 { + let bottom = start_idx + visible_rows - 1; + if sel > bottom { + start_idx = sel + 1 - visible_rows; + } } } - } - let desc_col = compute_desc_col(rows_all, start_idx, visible_items, content_area.width); + for (i, row) in rows_all + .iter() + .enumerate() + .skip(start_idx) + .take(visible_rows) + { + let GenericDisplayRow { + name, + match_indices, + is_current: _is_current, + description, + } = row; - // Render items, wrapping descriptions and aligning wrapped lines under the - // shared description column. Stop when we run out of vertical space. - let mut cur_y = content_area.y; - for (i, row) in rows_all - .iter() - .enumerate() - .skip(start_idx) - .take(visible_items) - { - if cur_y >= content_area.y + content_area.height { - break; - } - - let GenericDisplayRow { - name, - match_indices, - is_current: _is_current, - description, - } = row; - - let full_line = build_full_line( - &GenericDisplayRow { - name: name.clone(), - match_indices: match_indices.clone(), - is_current: *_is_current, - description: description.clone(), - }, - desc_col, - ); - - // Wrap with subsequent indent aligned to the description column. - use crate::wrapping::RtOptions; - use crate::wrapping::word_wrap_line; - let options = RtOptions::new(content_area.width as usize) - .initial_indent(Line::from("")) - .subsequent_indent(Line::from(" ".repeat(desc_col))); - let wrapped = word_wrap_line(&full_line, options); - - // Render the wrapped lines. - for mut line in wrapped { - if cur_y >= content_area.y + content_area.height { - break; + // Highlight fuzzy indices when present. + let mut spans: Vec = Vec::with_capacity(name.len()); + if let Some(idxs) = match_indices.as_ref() { + let mut idx_iter = idxs.iter().peekable(); + for (char_idx, ch) in name.chars().enumerate() { + if idx_iter.peek().is_some_and(|next| **next == char_idx) { + idx_iter.next(); + spans.push(ch.to_string().bold()); + } else { + spans.push(ch.to_string().into()); + } + } + } else { + spans.push(name.clone().into()); } + + if let Some(desc) = description.as_ref() { + spans.push(" ".into()); + spans.push(desc.clone().dim()); + } + + let mut cell = Cell::from(Line::from(spans)); if Some(i) == state.selected_idx { - // Match previous behavior: cyan + bold for the selected row. - line.style = Style::default() - .fg(Color::Cyan) - .add_modifier(Modifier::BOLD); - } - let para = Paragraph::new(line); - para.render( - Rect { - x: content_area.x, - y: cur_y, - width: content_area.width, - height: 1, - }, - buf, - ); - cur_y = cur_y.saturating_add(1); - } - } -} - -/// Compute the number of terminal rows required to render up to `max_results` -/// items from `rows_all` given the current scroll/selection state and the -/// available `width`. Accounts for description wrapping and alignment so the -/// caller can allocate sufficient vertical space. -pub(crate) fn measure_rows_height( - rows_all: &[GenericDisplayRow], - state: &ScrollState, - max_results: usize, - width: u16, -) -> u16 { - if rows_all.is_empty() { - return 1; // placeholder "no matches" line - } - - let content_width = width.saturating_sub(1).max(1); - - let visible_items = max_results.min(rows_all.len()); - let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); - if let Some(sel) = state.selected_idx { - if sel < start_idx { - start_idx = sel; - } else if visible_items > 0 { - let bottom = start_idx + visible_items - 1; - if sel > bottom { - start_idx = sel + 1 - visible_items; + cell = cell.style( + Style::default() + .fg(Color::Cyan) + .add_modifier(Modifier::BOLD), + ); } + rows.push(Row::new(vec![cell])); } } - let desc_col = compute_desc_col(rows_all, start_idx, visible_items, content_width); + let table = Table::new(rows, vec![Constraint::Percentage(100)]) + .block( + Block::default() + .borders(Borders::LEFT) + .border_type(BorderType::QuadrantOutside) + .border_style(Style::default().add_modifier(Modifier::DIM)), + ) + .widths([Constraint::Percentage(100)]); - use crate::wrapping::RtOptions; - use crate::wrapping::word_wrap_line; - let mut total: u16 = 0; - for row in rows_all - .iter() - .enumerate() - .skip(start_idx) - .take(visible_items) - .map(|(_, r)| r) - { - let full_line = build_full_line(row, desc_col); - let opts = RtOptions::new(content_width as usize) - .initial_indent(Line::from("")) - .subsequent_indent(Line::from(" ".repeat(desc_col))); - total = total.saturating_add(word_wrap_line(&full_line, opts).len() as u16); - } - total.max(1) + table.render(area, buf); } diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap index f908fb6144..b626f80ca1 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap @@ -4,5 +4,5 @@ expression: terminal.backend() --- "▌ /mo " "▌ " -"▌/model choose what model and reasoning effort to use " +"▌/model choose what model and reasoning effort to use " "▌/mention mention a file " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap index 65606ed7d0..aab7e7c9b6 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap @@ -6,6 +6,6 @@ expression: render_lines(&view) ▌ Switch between Codex approval presets ▌ ▌> 1. Read Only (current) Codex can read files -▌ 2. Full Access Codex can edit files +▌ 2. Full Access Codex can edit files Press Enter to confirm or Esc to go back diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap index b42a5f8c6b..77552f2b93 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap @@ -5,6 +5,6 @@ expression: render_lines(&view) ▌ Select Approval Mode ▌ ▌> 1. Read Only (current) Codex can read files -▌ 2. Full Access Codex can edit files +▌ 2. Full Access Codex can edit files Press Enter to confirm or Esc to go back