From 341f143f5b795a7ced6dac44ea81e40246c5b3cb Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 14 May 2025 15:15:12 -0700 Subject: [PATCH] fix: wrap lines in the TUI --- .../tui/src/conversation_history_widget.rs | 195 +++++++++++------- 1 file changed, 119 insertions(+), 76 deletions(-) diff --git a/codex-rs/tui/src/conversation_history_widget.rs b/codex-rs/tui/src/conversation_history_widget.rs index f7a9405954..fc370c92d3 100644 --- a/codex-rs/tui/src/conversation_history_widget.rs +++ b/codex-rs/tui/src/conversation_history_widget.rs @@ -8,6 +8,7 @@ use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use ratatui::prelude::*; use ratatui::style::Style; +use ratatui::widgets::Wrap; use ratatui::widgets::*; use serde_json::Value as JsonValue; use std::cell::Cell as StdCell; @@ -16,6 +17,14 @@ use std::path::PathBuf; pub struct ConversationHistoryWidget { history: Vec, + /// Cached number of *wrapped* lines for every [`HistoryCell`] in + /// `history`. The length is always kept in sync with `history` so that + /// `line_counts[i]` corresponds to `history[i]`. + line_counts: std::cell::RefCell>, + /// The width (in terminal cells/columns) that `line_counts` was computed + /// for. When the available width changes we have to recompute the cached + /// values. + cached_width: StdCell, scroll_position: usize, /// Number of lines the last time render_ref() was called num_rendered_lines: StdCell, @@ -28,6 +37,8 @@ impl ConversationHistoryWidget { pub fn new() -> Self { Self { history: Vec::new(), + line_counts: std::cell::RefCell::new(Vec::new()), + cached_width: StdCell::new(0), scroll_position: usize::MAX, num_rendered_lines: StdCell::new(0), last_viewport_height: StdCell::new(0), @@ -97,9 +108,7 @@ impl ConversationHistoryWidget { // Compute the maximum explicit scroll offset that still shows a full // viewport. This mirrors the calculation in `scroll_page_down()` and // in the render path. - let max_scroll = num_rendered_lines - .saturating_sub(viewport_height) - .saturating_add(1); + let max_scroll = num_rendered_lines.saturating_sub(viewport_height); let new_pos = self.scroll_position.saturating_add(num_lines as usize); @@ -144,7 +153,7 @@ impl ConversationHistoryWidget { // Calculate the maximum explicit scroll offset that is still within // range. This matches the logic in `scroll_down()` and the render // method. - let max_scroll = num_lines.saturating_sub(viewport_height).saturating_add(1); + let max_scroll = num_lines.saturating_sub(viewport_height); // Attempt to move down by a full page. let new_pos = self.scroll_position.saturating_add(viewport_height); @@ -216,15 +225,43 @@ impl ConversationHistoryWidget { } fn add_to_history(&mut self, cell: HistoryCell) { + // Keep the cached line count vector in sync with `history`. If we + // already know the current width of the viewport, eagerly compute the + // wrapped line count for the newly appended cell. Otherwise we push a + // placeholder that will be filled in the next render cycle. + let width = self.cached_width.get(); + { + let mut counts = self.line_counts.borrow_mut(); + if width > 0 { + let count = Self::wrapped_line_count_for_cell(&cell, width); + counts.push(count); + } else { + counts.push(0); + } + } + self.history.push(cell); } /// Remove all history entries and reset scrolling. pub fn clear(&mut self) { self.history.clear(); + self.line_counts.borrow_mut().clear(); self.scroll_position = usize::MAX; } + /// Helper that returns the *wrapped* line count for the given + /// `HistoryCell` when rendered in a [`Paragraph`] with the provided + /// `width`. + fn wrapped_line_count_for_cell(cell: &HistoryCell, width: u16) -> usize { + // We do *not* enable trimming here because we want to exactly mirror + // how the lines will be rendered in `render_ref`, which uses + // `wrap(trim: false)` so that long words are not elided. + Paragraph::new(cell.lines().clone()) + .wrap(ratatui::widgets::Wrap { trim: false }) + .line_count(width) + } + pub fn record_completed_exec_command( &mut self, call_id: String, @@ -232,7 +269,8 @@ impl ConversationHistoryWidget { stderr: String, exit_code: i32, ) { - for cell in self.history.iter_mut() { + let width = self.cached_width.get(); + for (idx, cell) in self.history.iter_mut().enumerate() { if let HistoryCell::ActiveExecCommand { call_id: history_id, command, @@ -250,6 +288,13 @@ impl ConversationHistoryWidget { duration: start.elapsed(), }, ); + + // Update cached line count. + if width > 0 { + if let Some(slot) = self.line_counts.borrow_mut().get_mut(idx) { + *slot = Self::wrapped_line_count_for_cell(&self.history[idx], width); + } + } break; } } @@ -269,7 +314,8 @@ impl ConversationHistoryWidget { .unwrap_or_else(|_| serde_json::Value::String("".into())) }); - for cell in self.history.iter_mut() { + let width = self.cached_width.get(); + for (idx, cell) in self.history.iter_mut().enumerate() { if let HistoryCell::ActiveMcpToolCall { call_id: history_id, fq_tool_name, @@ -287,6 +333,13 @@ impl ConversationHistoryWidget { result_val, ); *cell = completed; + + if width > 0 { + if let Some(slot) = self.line_counts.borrow_mut().get_mut(idx) { + *slot = Self::wrapped_line_count_for_cell(&self.history[idx], width); + } + } + break; } } @@ -311,97 +364,83 @@ impl WidgetRef for ConversationHistoryWidget { .border_type(BorderType::Rounded) .border_style(border_style); - // ------------------------------------------------------------------ - // Build a *window* into the history instead of cloning the entire - // history into a brand‑new Vec every time we are asked to render. - // - // There can be an unbounded number of `Line` objects in the history, - // but the terminal will only ever display `height` of them at once. - // By materialising only the `height` lines that are scrolled into - // view we avoid the potentially expensive clone of the full - // conversation every frame. - // ------------------------------------------------------------------ - // Compute the inner area that will be available for the list after // the surrounding `Block` is drawn. let inner = block.inner(area); let viewport_height = inner.height as usize; - // Collect the lines that will actually be visible in the viewport - // while keeping track of the total number of lines so the scrollbar - // stays correct. - let num_lines: usize = self.history.iter().map(|c| c.lines().len()).sum(); + // ────────────────────────────────────────────────────────────────── + // Cache (and if necessary recalculate) the wrapped line counts for + // every [`HistoryCell`] so that our scrolling math accounts for text + // wrapping. + // ────────────────────────────────────────────────────────────────── - let max_scroll = num_lines.saturating_sub(viewport_height) + 1; + let width = inner.width; // Width of the viewport in terminal cells. + if width == 0 { + return; // Nothing to draw – avoid division by zero. + } + + // Recompute cache if the width changed or if we do not have counts + // for all entries yet. + if self.cached_width.get() != width || self.line_counts.borrow().len() != self.history.len() + { + self.cached_width.set(width); + + let mut counts = self.line_counts.borrow_mut(); + counts.clear(); + counts.reserve(self.history.len()); + for cell in &self.history { + let cnt = Self::wrapped_line_count_for_cell(cell, width); + counts.push(cnt); + } + } + + let num_lines: usize = self.line_counts.borrow().iter().sum(); + + // ------------------------------------------------------------------ + // Scroll position logic (largely unchanged but now using wrapped line + // counts instead of naïve line lengths). + // ------------------------------------------------------------------ + + let max_scroll = num_lines.saturating_sub(viewport_height); let scroll_pos = if self.scroll_position == usize::MAX { max_scroll } else { self.scroll_position.min(max_scroll) }; - let mut visible_lines: Vec> = Vec::with_capacity(viewport_height); + // Materialise *all* lines in a single Vec so we can hand it off to a + // Paragraph that takes care of wrapping and scrolling. Although this + // means cloning the full conversation buffer every frame, in + // practice the performance is perfectly adequate for typical + // workloads and keeps the rendering code straightforward. - if self.scroll_position == usize::MAX { - // Stick‑to‑bottom mode: walk the history backwards and keep the - // most recent `height` lines. This touches at most `height` - // lines regardless of how large the conversation grows. - 'outer_rev: for cell in self.history.iter().rev() { - for line in cell.lines().iter().rev() { - visible_lines.push(line.clone()); - if visible_lines.len() == viewport_height { - break 'outer_rev; - } - } - } - visible_lines.reverse(); - } else { - // Arbitrary scroll position. Skip lines until we reach the - // desired offset, then emit the next `height` lines. - let start_line = scroll_pos; - let mut current_index = 0usize; - 'outer_fwd: for cell in &self.history { - for line in cell.lines() { - if current_index >= start_line { - visible_lines.push(line.clone()); - if visible_lines.len() == viewport_height { - break 'outer_fwd; - } - } - current_index += 1; - } - } + let mut all_lines: Vec> = Vec::new(); + for cell in &self.history { + all_lines.extend(cell.lines().iter().cloned()); } - // We track the number of lines in the struct so can let the user take over from - // something other than usize::MAX when they start scrolling up. This could be - // removed once we have the vec in self. - self.num_rendered_lines.set(num_lines); - self.last_viewport_height.set(viewport_height); + // Build the Paragraph with wrapping enabled so long lines are not + // clipped. Horizontal trimming is disabled – we want long words to + // overflow onto subsequent lines instead of being elided. + let paragraph = Paragraph::new(all_lines) + .block(block) + .wrap(Wrap { trim: false }) + // Apply the vertical scroll so the correct portion of the text + // is visible. + .scroll((scroll_pos as u16, 0)); - // The widget takes care of drawing the `block` and computing its own - // inner area, so we render it over the full `area`. - // We *manually* sliced the set of `visible_lines` to fit within the - // viewport above, so there is no need to ask the `Paragraph` widget - // to apply an additional scroll offset. Doing so would cause the - // content to be shifted *twice* – once by our own logic and then a - // second time by the widget – which manifested as the entire block - // drifting off‑screen when the user attempted to scroll. - - // Currently, we do not use the `wrap` method on the `Paragraph` widget - // because it messes up our scrolling math above that assumes each Line - // contributes one line of height to the widget. Admittedly, this is - // bad because users cannot see content that is clipped without - // resizing the terminal. - let paragraph = Paragraph::new(visible_lines).block(block); paragraph.render(area, buf); + // ------------------------------------------------------------------ + // Draw scrollbar (unchanged except for using wrapped line counts). + // ------------------------------------------------------------------ + let needs_scrollbar = num_lines > viewport_height; if needs_scrollbar { let mut scroll_state = ScrollbarState::default() - // TODO(ragona): - // I don't totally understand this, but it appears to work exactly as expected - // if we set the content length as the lines minus the height. Maybe I was supposed - // to use viewport_content_length or something, but this works and I'm backing away. + // The Scrollbar widget expects the *content* height minus the + // viewport height, mirroring the calculation used previously. .content_length(num_lines.saturating_sub(viewport_height)) .position(scroll_pos); @@ -447,5 +486,9 @@ impl WidgetRef for ConversationHistoryWidget { &mut scroll_state, ); } + + // Update auxiliary stats that the scroll handlers rely on. + self.num_rendered_lines.set(num_lines); + self.last_viewport_height.set(viewport_height); } }