From 0841d9bba5e53104caa86eda39c9974afe63a639 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 17 Aug 2026 16:26:56 +0000 Subject: [PATCH] Hang soft-break whitespace in the TUI composer (#39022) ## Why Keeping overflowing separators with the following word can add whitespace-only rows and make composer wrapping differ from the queued-input preview. ## What changed - Hang breakable whitespace from the preceding visual row at soft word breaks while preserving the underlying editable text. - Keep indentation, trailing whitespace, mandatory breaks, and nonbreaking spaces visible and editable according to their existing semantics. - Align cursor placement, scrolling, vertical navigation, masked rendering, and styled overlays with the visible wrapped content, including after resizes. ## Testing Add focused wrapping, rendering, cursor, editing, scrolling, navigation, Unicode separator, long whitespace-run, and resize coverage. GitOrigin-RevId: 7fbdb0a4bf33cfee20c4c19dca5aee74df4f892c --- ...de_space_hangs_before_following_text.snap} | 6 +- ...ws_across_following_wrapped_fragments.snap | 7 +- ...eeps_fitting_remainder_on_current_row.snap | 4 +- ...ull_line_hangs_before_following_text.snap} | 4 +- ...led_overlays_stay_inside_the_textarea.snap | 12 +- codex-rs/tui/src/bottom_pane/textarea.rs | 189 +++++----- ..._tests__hanging_tab_cursor_and_scroll.snap | 8 + ...apping__tests__mandatory_break_cursor.snap | 9 + ...pping__tests__typing_after_eol_spaces.snap | 30 ++ ...sts__vertical_navigation_after_resize.snap | 23 ++ .../tui/src/bottom_pane/textarea/wrapping.rs | 88 ++++- .../bottom_pane/textarea/wrapping_tests.rs | 332 +++++++++++++++++- 12 files changed, 572 insertions(+), 140 deletions(-) rename codex-rs/tui/src/bottom_pane/snapshots/{codex_tui__bottom_pane__textarea__tests__textarea_breakable_unicode_space_stays_with_following_text.snap => codex_tui__bottom_pane__textarea__tests__textarea_breakable_unicode_space_hangs_before_following_text.snap} (64%) rename codex-rs/tui/src/bottom_pane/snapshots/{codex_tui__bottom_pane__textarea__tests__textarea_leading_space_after_full_line_stays_with_following_text.snap => codex_tui__bottom_pane__textarea__tests__textarea_separator_after_full_line_hangs_before_following_text.snap} (83%) create mode 100644 codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__hanging_tab_cursor_and_scroll.snap create mode 100644 codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__mandatory_break_cursor.snap create mode 100644 codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__typing_after_eol_spaces.snap create mode 100644 codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__vertical_navigation_after_resize.snap diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_breakable_unicode_space_stays_with_following_text.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_breakable_unicode_space_hangs_before_following_text.snap similarity index 64% rename from codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_breakable_unicode_space_stays_with_following_text.snap rename to codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_breakable_unicode_space_hangs_before_following_text.snap index a7769536fd..5fad1f50fa 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_breakable_unicode_space_stays_with_following_text.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_breakable_unicode_space_hangs_before_following_text.snap @@ -2,7 +2,7 @@ source: tui/src/bottom_pane/textarea.rs expression: "format!(\"cursor: {:?}\\n{}\", t.cursor_pos(area), terminal.backend())" --- -cursor: Some((3, 2)) +cursor: Some((1, 2)) "abad" -" ab" Hidden by multi-width symbols: [(1, " ")] -"cde " +"abcd" +"e " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_leading_space_reflows_across_following_wrapped_fragments.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_leading_space_reflows_across_following_wrapped_fragments.snap index e5cf6dd2db..b9e92f36bc 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_leading_space_reflows_across_following_wrapped_fragments.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_leading_space_reflows_across_following_wrapped_fragments.snap @@ -2,9 +2,8 @@ source: tui/src/bottom_pane/textarea.rs expression: "format!(\"cursor: {:?}\\n{}\", t.cursor_pos(area), terminal.backend())" --- -cursor: Some((2, 4)) +cursor: Some((2, 3)) "abad" -" abc" -"de " -"xy " +"abcd" +"e xy" "zz " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_reflow_keeps_fitting_remainder_on_current_row.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_reflow_keeps_fitting_remainder_on_current_row.snap index d87d057d97..7c8cdef020 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_reflow_keeps_fitting_remainder_on_current_row.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_reflow_keeps_fitting_remainder_on_current_row.snap @@ -2,6 +2,6 @@ source: tui/src/bottom_pane/textarea.rs expression: "format!(\"cursor: {:?}\\n{}\", t.cursor_pos(area), terminal.backend())" --- -cursor: Some((4, 1)) +cursor: Some((3, 1)) "p " -" a b " +"a b " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_leading_space_after_full_line_stays_with_following_text.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_separator_after_full_line_hangs_before_following_text.snap similarity index 83% rename from codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_leading_space_after_full_line_stays_with_following_text.snap rename to codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_separator_after_full_line_hangs_before_following_text.snap index 6adc45549a..3f81d108b1 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_leading_space_after_full_line_stays_with_following_text.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_separator_after_full_line_hangs_before_following_text.snap @@ -2,6 +2,6 @@ source: tui/src/bottom_pane/textarea.rs expression: "format!(\"cursor: {:?}\\n{}\", t.cursor_pos(area), terminal.backend())" --- -cursor: Some((2, 1)) +cursor: Some((1, 1)) "abad" -" a " +"a " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_styled_overlays_stay_inside_the_textarea.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_styled_overlays_stay_inside_the_textarea.snap index 2be86414e2..35501979d9 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_styled_overlays_stay_inside_the_textarea.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__textarea__tests__textarea_styled_overlays_stay_inside_the_textarea.snap @@ -3,12 +3,12 @@ source: tui/src/bottom_pane/textarea.rs expression: "snapshots.join(\"\\n\\n\")" --- element -" KEEP" -" nex " -"t " +"abadKEEP" +"next " +" " highlight -" KEEP" -" nex " -"t " +"abadKEEP" +"next " +" " diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index 9d521be7ed..61f1df1b9f 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -11,8 +11,8 @@ //! recent killed span. //! //! Wrapping also reserves a visible insertion point: full logical lines get continuation rows, -//! and overflowing spaces wrap instead of moving the cursor outside the textarea. A partial -//! whitespace continuation stays attached to the following word rather than becoming a blank row. +//! and trailing spaces wrap instead of moving the cursor outside the textarea. At soft word +//! breaks, interior separators hang off the preceding row without changing the editable text. use crate::key_hint::KeyBindingListExt; use crate::key_hint::is_altgr; @@ -451,16 +451,13 @@ impl TextArea { } let lines = self.wrapped_lines(area.width); - let effective_scroll = self.effective_scroll(area.height, &lines, state.scroll); - let i = Self::wrapped_line_index_by_start(&lines, self.cursor_pos)?; - let ls = &lines[i]; - let col = display_width(&self.text[ls.start..self.cursor_pos]) - .min(usize::from(area.width.saturating_sub(1))) as u16; + let effective_scroll = self.effective_scroll(area, &lines, state.scroll); + let (i, col) = wrapping::cursor_position(&self.text, &lines, area.width, self.cursor_pos)?; let screen_row = i .saturating_sub(effective_scroll as usize) .try_into() .unwrap_or(0); - Some((area.x + col, area.y + screen_row)) + Some((area.x + col as u16, area.y + screen_row)) } pub fn is_empty(&self) -> bool { @@ -472,13 +469,6 @@ impl TextArea { display_width(&self.text[bol..self.cursor_pos]) } - fn wrapped_line_index_by_start(lines: &[Range], pos: usize) -> Option { - // partition_point returns the index of the first element for which - // the predicate is false, i.e. the count of elements with start <= pos. - let idx = lines.partition_point(|r| r.start <= pos); - if idx == 0 { None } else { Some(idx - 1) } - } - fn move_to_display_col_on_line( &mut self, line_start: usize, @@ -1250,11 +1240,16 @@ impl TextArea { let cache_ref = self.wrap_cache.borrow(); if let Some(cache) = cache_ref.as_ref() { let lines = &cache.lines; - if let Some(idx) = Self::wrapped_line_index_by_start(lines, self.cursor_pos) { + if let Some((idx, col)) = + wrapping::cursor_position(&self.text, lines, cache.width, self.cursor_pos) + { let cur_range = &lines[idx]; - let target_col = self.preferred_col.unwrap_or_else(|| { - display_width(&self.text[cur_range.start..self.cursor_pos]) - }); + // A saved column can outlive a resize. Do not land in hanging whitespace + // that is displayed on the following row. + let target_col = self + .preferred_col + .unwrap_or(col) + .min(usize::from(cache.width.saturating_sub(1))); if idx > 0 { let prev = &lines[idx - 1]; let line_start = prev.start; @@ -1316,11 +1311,13 @@ impl TextArea { let cache_ref = self.wrap_cache.borrow(); if let Some(cache) = cache_ref.as_ref() { let lines = &cache.lines; - if let Some(idx) = Self::wrapped_line_index_by_start(lines, self.cursor_pos) { - let cur_range = &lines[idx]; - let target_col = self.preferred_col.unwrap_or_else(|| { - display_width(&self.text[cur_range.start..self.cursor_pos]) - }); + if let Some((idx, col)) = + wrapping::cursor_position(&self.text, lines, cache.width, self.cursor_pos) + { + let target_col = self + .preferred_col + .unwrap_or(col) + .min(usize::from(cache.width.saturating_sub(1))); if idx + 1 < lines.len() { let next = &lines[idx + 1]; let line_start = next.start; @@ -1926,30 +1923,24 @@ impl TextArea { /// /// - Cursor is always on screen. /// - No scrolling if content fits in the area. - fn effective_scroll( - &self, - area_height: u16, - lines: &[Range], - current_scroll: u16, - ) -> u16 { + fn effective_scroll(&self, area: Rect, lines: &[Range], current_scroll: u16) -> u16 { let total_lines = lines.len() as u16; - if area_height >= total_lines { + if area.height >= total_lines { return 0; } - // Where is the cursor within wrapped lines? Prefer assigning boundary positions - // (where pos equals the start of a wrapped line) to that later line. let cursor_line_idx = - Self::wrapped_line_index_by_start(lines, self.cursor_pos).unwrap_or(0) as u16; + wrapping::cursor_position(&self.text, lines, area.width, self.cursor_pos) + .map_or(0, |(row, _)| row) as u16; - let max_scroll = total_lines.saturating_sub(area_height); + let max_scroll = total_lines.saturating_sub(area.height); let mut scroll = current_scroll.min(max_scroll); // Ensure cursor is visible within [scroll, scroll + area_height) if cursor_line_idx < scroll { scroll = cursor_line_idx; - } else if cursor_line_idx >= scroll + area_height { - scroll = cursor_line_idx + 1 - area_height; + } else if cursor_line_idx >= scroll + area.height { + scroll = cursor_line_idx + 1 - area.height; } scroll } @@ -1974,7 +1965,7 @@ impl StatefulWidgetRef for &TextArea { fn render_ref(&self, area: Rect, buf: &mut Buffer, state: &mut Self::State) { let lines = self.wrapped_lines(area.width); - let scroll = self.effective_scroll(area.height, &lines, state.scroll); + let scroll = self.effective_scroll(area, &lines, state.scroll); state.scroll = scroll; let start = scroll as usize; @@ -1992,7 +1983,7 @@ impl TextArea { mask_char: char, ) { let lines = self.wrapped_lines(area.width); - let scroll = self.effective_scroll(area.height, &lines, state.scroll); + let scroll = self.effective_scroll(area, &lines, state.scroll); state.scroll = scroll; let start = scroll as usize; @@ -2013,7 +2004,7 @@ impl TextArea { highlights: &[(Range, Style)], ) { let lines = self.wrapped_lines(area.width); - let scroll = self.effective_scroll(area.height, &lines, state.scroll); + let scroll = self.effective_scroll(area, &lines, state.scroll); state.scroll = scroll; let start = scroll as usize; @@ -2031,31 +2022,40 @@ impl TextArea { base_style: Style, highlights: &[(Range, Style)], ) { + let element_style = base_style.fg(Color::Cyan); for (row, idx) in range.enumerate() { let r = &lines[idx]; let y = area.y + row as u16; - let line_range = r.start..r.end - 1; + let visible = wrapping::visible_prefix(&self.text[r.start..r.end - 1], area.width); + let line_range = r.start..r.start + visible.len(); buf.set_style(Rect::new(area.x, y, area.width, 1), base_style); // Draw base line with the provided style. buf.set_stringn( area.x, y, - text_for_display(&self.text[line_range.clone()]), + text_for_display(visible), usize::from(area.width), base_style, ); - // Overlay styled segments for elements that intersect this line. - for elem in &self.elements { - // Compute overlap with displayed slice. - let overlap_start = elem.range.start.max(line_range.start); - let overlap_end = elem.range.end.min(line_range.end); + // Apply search highlights last so they remain visible over styled elements. + let overlays = self + .elements + .iter() + .map(|element| (&element.range, element_style)) + .chain(highlights.iter().map(|(range, style)| (range, *style))); + for (overlay_range, style) in overlays { + let overlap_start = overlay_range.start.max(line_range.start); + let overlap_end = overlay_range.end.min(line_range.end); if overlap_start >= overlap_end { continue; } let styled = &self.text[overlap_start..overlap_end]; - let x_off = display_width(&self.text[line_range.start..overlap_start]) as u16; - let style = base_style.fg(Color::Cyan); + let x_off = display_width(&self.text[line_range.start..overlap_start]); + if x_off >= usize::from(area.width) { + continue; + } + let x_off = x_off as u16; buf.set_stringn( area.x + x_off, y, @@ -2064,25 +2064,6 @@ impl TextArea { style, ); } - - // Overlay render-only highlight ranges last so transient search highlighting remains - // visible even when it intersects attachment placeholders or other styled elements. - for (highlight_range, style) in highlights { - let overlap_start = highlight_range.start.max(line_range.start); - let overlap_end = highlight_range.end.min(line_range.end); - if overlap_start >= overlap_end { - continue; - } - let highlighted = &self.text[overlap_start..overlap_end]; - let x_off = display_width(&self.text[line_range.start..overlap_start]) as u16; - buf.set_stringn( - area.x + x_off, - y, - text_for_display(highlighted), - usize::from(area.width.saturating_sub(x_off)), - *style, - ); - } } } @@ -2098,8 +2079,8 @@ impl TextArea { for (row, idx) in range.enumerate() { let r = &lines[idx]; let y = area.y + row as u16; - let line_range = r.start..r.end - 1; - let masked = self.text[line_range.clone()] + let visible = wrapping::visible_prefix(&self.text[r.start..r.end - 1], area.width); + let masked = visible .graphemes(/*is_extended*/ true) .flat_map(|grapheme| std::iter::repeat_n(mask_char, display_width(grapheme))) .collect::(); @@ -3646,7 +3627,7 @@ mod tests { } #[test] - fn leading_space_after_full_line_stays_with_following_text() { + fn separator_after_full_line_hangs_before_following_text() { let mut t = ta_with("abad a"); t.set_cursor(t.text().len()); let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 2); @@ -3659,9 +3640,9 @@ mod tests { .unwrap(); assert_eq!(t.desired_height(area.width), 2); - assert_eq!(t.cursor_pos(area), Some((2, 1))); + assert_eq!(t.cursor_pos(area), Some((1, 1))); insta::assert_snapshot!( - "textarea_leading_space_after_full_line_stays_with_following_text", + "textarea_separator_after_full_line_hangs_before_following_text", format!("cursor: {:?}\n{}", t.cursor_pos(area), terminal.backend()) ); @@ -3670,12 +3651,12 @@ mod tests { t.set_cursor(t.text().len()); assert_eq!(t.desired_height(area.width), 2); - assert_eq!(t.cursor_pos(area), Some((2, 1))); + assert_eq!(t.cursor_pos(area), Some((1, 1))); } } #[test] - fn breakable_unicode_space_stays_with_following_text() { + fn breakable_unicode_space_hangs_before_following_text() { let mut t = ta_with("abad\u{3000}abcde"); t.set_cursor(t.text().len()); let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 3); @@ -3688,9 +3669,9 @@ mod tests { .unwrap(); assert_eq!(t.desired_height(area.width), 3); - assert_eq!(t.cursor_pos(area), Some((3, 2))); + assert_eq!(t.cursor_pos(area), Some((1, 2))); insta::assert_snapshot!( - "textarea_breakable_unicode_space_stays_with_following_text", + "textarea_breakable_unicode_space_hangs_before_following_text", format!("cursor: {:?}\n{}", t.cursor_pos(area), terminal.backend()) ); } @@ -3706,18 +3687,19 @@ mod tests { } let mut t = ta_with("abad abcde xy zz"); - assert_eq!(t.desired_height(area.width), 5); + let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 4); + assert_eq!(t.desired_height(area.width), 4); for (cursor, expected) in [ (4, (0, 1)), - (5, (1, 1)), - (8, (0, 2)), - (9, (1, 2)), - (10, (2, 2)), - (11, (0, 3)), - (12, (1, 3)), - (13, (2, 3)), - (14, (0, 4)), - (16, (2, 4)), + (5, (0, 1)), + (8, (3, 1)), + (9, (0, 2)), + (10, (1, 2)), + (11, (2, 2)), + (12, (3, 2)), + (13, (0, 3)), + (14, (0, 3)), + (16, (2, 3)), ] { t.set_cursor(cursor); assert_eq!(t.cursor_pos(area), Some(expected)); @@ -3741,8 +3723,8 @@ mod tests { fn reflow_keeps_fitting_remainder_on_current_row() { for (text, width, expected_height, expected_cursor) in [ (" -", 1, 4, (0, 3)), - ("p a b", 3, 3, (2, 2)), - ("p a b", 5, 2, (4, 1)), + ("p a b", 3, 3, (0, 2)), + ("p a b", 5, 2, (3, 1)), (" aaaa a ", 3, 4, (1, 3)), ] { let mut t = ta_with(text); @@ -3752,7 +3734,10 @@ mod tests { assert_eq!(t.desired_height(area.width), expected_height); assert_eq!(t.cursor_pos(area), Some(expected_cursor)); for range in t.wrapped_lines(width).iter() { - assert!(display_width(&text[range.start..range.end - 1]) <= usize::from(width)); + assert!( + display_width(text[range.start..range.end - 1].trim_end_matches(' ')) + <= usize::from(width) + ); } } @@ -3793,15 +3778,15 @@ mod tests { } #[test] - fn space_runs_wrap_before_later_text() { + fn space_runs_hang_before_later_text() { let mut t = ta_with("abad next"); - let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 4); + let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 3); - assert_eq!(t.desired_height(area.width), 4); + assert_eq!(t.desired_height(area.width), 3); - for (cursor, expected) in [(4, (0, 1)), (6, (2, 1)), (8, (0, 2)), (9, (1, 2))] { + for cursor in 4..=9 { t.set_cursor(cursor); - assert_eq!(t.cursor_pos(area), Some(expected)); + assert_eq!(t.cursor_pos(area), Some((0, 1))); } } @@ -4200,7 +4185,7 @@ mod tests { fn wrapped_navigation_with_newlines_and_spaces() { // Include spaces and an explicit newline to exercise boundaries let mut t = ta_with("word1 word2\nword3"); - // Width 6 wraps "word1 " and " word2" before the newline. + // Width 6 leaves both separators on the first visual row. let _ = t.desired_height(/*width*/ 6); // Put the cursor at column 1 of "word2". @@ -4209,20 +4194,16 @@ mod tests { // Up moves directly to the preceding visual line while preserving the visual column. t.move_cursor_up(); - assert_eq!(t.cursor(), 2); + assert_eq!(t.cursor(), 1); - // Down returns to the same visual column of " word2". + // Down returns to the same visual column of "word2". t.move_cursor_down(); assert_eq!(t.cursor(), start_word2 + 1); - // A full line reserves an insertion row immediately before the explicit newline. - t.move_cursor_down(); - assert_eq!(t.cursor(), t.text().find('\n').unwrap()); - - // Down again crosses the logical newline to the next visual line ("word3"). + // Down crosses the logical newline to the next visual line ("word3"). t.move_cursor_down(); let start_word3 = t.text().find("word3").unwrap(); - assert!(t.cursor() >= start_word3 && t.cursor() <= start_word3 + "word3".len()); + assert_eq!(t.cursor(), start_word3 + 1); } #[test] diff --git a/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__hanging_tab_cursor_and_scroll.snap b/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__hanging_tab_cursor_and_scroll.snap new file mode 100644 index 0000000000..9dc7bde124 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__hanging_tab_cursor_and_scroll.snap @@ -0,0 +1,8 @@ +--- +source: tui/src/bottom_pane/textarea/wrapping_tests.rs +expression: "format!(\"cursors: {cursors:?}\\nscroll: {}\\ncursor: {scrolled_cursor:?}\\n{}\",\nstate.scroll, terminal.backend())" +--- +cursors: [Some((0, 0)), Some((1, 0)), Some((2, 0)), Some((3, 0)), Some((0, 1)), Some((0, 1)), Some((1, 1))] +scroll: 1 +cursor: Some((0, 0)) +"a " diff --git a/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__mandatory_break_cursor.snap b/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__mandatory_break_cursor.snap new file mode 100644 index 0000000000..81509b0aab --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__mandatory_break_cursor.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/bottom_pane/textarea/wrapping_tests.rs +expression: "format!(\"before: {before:?}\\nafter: {after:?}\\n{}\",\nterminal.backend()).replace(separator, \"\\\\u{2028}\")" +--- +before: Some((0, 1)) +after: (7, Some((1, 1))) +"abad" +"\u{2028}nex" +"t " diff --git a/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__typing_after_eol_spaces.snap b/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__typing_after_eol_spaces.snap new file mode 100644 index 0000000000..f3522c6b93 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__typing_after_eol_spaces.snap @@ -0,0 +1,30 @@ +--- +source: tui/src/bottom_pane/textarea/wrapping_tests.rs +expression: "snapshots.join(\"\\n\\n\")" +--- +full line +text: "abad" +cursor: (0, 1) +"abad" +" " + + +trailing spaces +text: "abad " +cursor: (3, 1) +"abad" +" " + + +following word +text: "abad x" +cursor: (1, 1) +"abad" +"x " + + +delete following word +text: "abad " +cursor: (3, 1) +"abad" +" " diff --git a/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__vertical_navigation_after_resize.snap b/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__vertical_navigation_after_resize.snap new file mode 100644 index 0000000000..357c0ee2c7 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/textarea/snapshots/codex_tui__bottom_pane__textarea__wrapping__tests__vertical_navigation_after_resize.snap @@ -0,0 +1,23 @@ +--- +source: tui/src/bottom_pane/textarea/wrapping_tests.rs +expression: "format!(\"{}\\n\\n{}\", cursors.join(\"\\n\"), layouts.join(\"\\n\"))" +--- +up at width 10: (10, Some((0, 1))) +resize to width 4: (10, Some((2, 2))) +down to abcd: (14, Some((3, 3))) +down to xyz: (20, Some((3, 4))) +up to abcd: (14, Some((3, 3))) +up to ij: (10, Some((2, 2))) +restore width 10: (20, Some((9, 2))) + +width 4 +"abcd" +"efgh" +"ij " +"abcd" +"xyz " + +width 10 +"abcdefghij" +" " +"abcd xyz " diff --git a/codex-rs/tui/src/bottom_pane/textarea/wrapping.rs b/codex-rs/tui/src/bottom_pane/textarea/wrapping.rs index bd4b96690a..e125f8d68b 100644 --- a/codex-rs/tui/src/bottom_pane/textarea/wrapping.rs +++ b/codex-rs/tui/src/bottom_pane/textarea/wrapping.rs @@ -1,8 +1,9 @@ //! Grapheme-safe composer wrapping that preserves textwrap's semantic breakpoints. //! -//! Source ranges include a sentinel byte for cursor placement. Overflowing breakable whitespace -//! stays attached to the following word, while full logical lines reserve an insertion row. +//! Source ranges include a sentinel byte for cursor placement. Separators at soft breaks hang off +//! the preceding row without changing the source text. Full logical lines reserve an insertion row. +use super::text_for_display; use crate::width::display_width; use std::ops::Range; use textwrap::Options; @@ -14,11 +15,43 @@ struct WrappedWord { width: usize, } +/// Classifies breakable whitespace; the three Unicode nonbreaking spaces stay in their word. +fn is_breakable_space(ch: char) -> bool { + ch.is_whitespace() && !matches!(ch, '\u{a0}' | '\u{2007}' | '\u{202f}') +} + +/// Mandatory Unicode breaks retain their editable insertion positions instead of hanging. +fn is_hangable_space(ch: char) -> bool { + is_breakable_space(ch) + && !matches!( + ch, + '\n' | '\r' | '\u{b}' | '\u{c}' | '\u{85}' | '\u{2028}' | '\u{2029}' + ) +} + +/// Drops a row's hidden suffix once soft whitespace alone fills the viewport. +/// +/// Each hangable separator occupies at least one rendered cell. Keeping the complete grapheme +/// preserves any combining marks on the last visible space, while bounding work on long runs. +pub(super) fn visible_prefix(text: &str, width: u16) -> &str { + if width == 0 { + return ""; + } + if text.len() <= usize::from(width) { + return text; + } + text.grapheme_indices(/*is_extended*/ true) + .filter(|(_, grapheme)| grapheme.chars().any(is_hangable_space)) + .nth(usize::from(width) - 1) + .map_or(text, |(start, grapheme)| &text[..start + grapheme.len()]) +} + /// Returns grapheme-safe visual ranges, each including its cursor-position sentinel byte. /// -/// Breakable Unicode whitespace stays with the next word, nonbreaking whitespace remains part of -/// its word, and full logical lines receive an empty insertion row. Word boundaries and widths are -/// indexed once so maximum-size pasted lines remain linear to wrap. +/// Interior breakable whitespace can extend past the row width, but indentation and trailing +/// whitespace remain visible and editable. Nonbreaking whitespace remains part of its word, and +/// full logical lines receive an empty insertion row. Word boundaries and widths are indexed once +/// so maximum-size pasted lines remain linear to wrap. pub(super) fn wrapped_lines(text: &str, width: u16) -> Vec> { let width = usize::from(width); let options = Options::new(width).wrap_algorithm(textwrap::WrapAlgorithm::FirstFit); @@ -52,6 +85,7 @@ pub(super) fn wrapped_lines(text: &str, width: u16) -> Vec> { let mut line_width = 0; let mut line_has_text = false; let mut line_active = false; + let mut logical_content_end = 0; let mut processed_end = 0; let mut previous_was_whitespace = false; let mut word_index = 0; @@ -73,6 +107,12 @@ pub(super) fn wrapped_lines(text: &str, width: u16) -> Vec> { if !line_active { line_start = fragment_start; line_active = true; + let logical_line = text[fragment_start..] + .split('\n') + .next() + .unwrap_or_default(); + logical_content_end = + fragment_start + logical_line.trim_end_matches(is_hangable_space).len(); } else if line_has_text && breakpoints[fragment_start] { let remaining_word_width = words .get(word_index) @@ -97,8 +137,7 @@ pub(super) fn wrapped_lines(text: &str, width: u16) -> Vec> { } let grapheme_end = grapheme_start + grapheme.len(); - let is_whitespace = grapheme.chars().all(char::is_whitespace) - && (grapheme == " " || breakpoints[grapheme_end]); + let is_whitespace = grapheme.chars().all(is_breakable_space); // Partial whitespace rows absorb the next word; text rows keep it intact. if !is_whitespace && line_has_text && previous_was_whitespace { let word_end = words @@ -114,7 +153,10 @@ pub(super) fn wrapped_lines(text: &str, width: u16) -> Vec> { } let grapheme_width = display_width(grapheme); - if line_width > 0 && line_width + grapheme_width > width { + let hanging_space = line_has_text + && grapheme_end < logical_content_end + && grapheme.chars().all(is_hangable_space); + if !hanging_space && line_width > 0 && line_width + grapheme_width > width { lines.push(line_start..grapheme_start + 1); line_start = grapheme_start; line_width = 0; @@ -148,6 +190,36 @@ pub(super) fn wrapped_lines(text: &str, width: u16) -> Vec> { lines } +/// Maps a source insertion point to a zero-based visual row and column. +/// +/// `lines` must come from [`wrapped_lines`] for the display-normalized text and the same width, +/// and `pos` must be a valid source boundary. The measured prefix uses the same normalization. +/// Positions in overflowing separators share the following row's first column without changing +/// their distinct editing offsets. Other columns stay inside the viewport. +pub(super) fn cursor_position( + text: &str, + lines: &[Range], + width: u16, + pos: usize, +) -> Option<(usize, usize)> { + let row = lines + .partition_point(|line| line.start <= pos) + .checked_sub(1)?; + let before_cursor = visible_prefix(&text[lines[row].start..pos], width); + let before_cursor = text_for_display(before_cursor); + let col = display_width(before_cursor.as_ref()); + let width = usize::from(width); + if col >= width + && lines + .get(row + 1) + .is_some_and(|next| next.start == lines[row].end - 1) + { + Some((row + 1, 0)) + } else { + Some((row, col.min(width.saturating_sub(1)))) + } +} + #[cfg(test)] #[path = "wrapping_tests.rs"] mod tests; diff --git a/codex-rs/tui/src/bottom_pane/textarea/wrapping_tests.rs b/codex-rs/tui/src/bottom_pane/textarea/wrapping_tests.rs index 989689c875..08557b8e9d 100644 --- a/codex-rs/tui/src/bottom_pane/textarea/wrapping_tests.rs +++ b/codex-rs/tui/src/bottom_pane/textarea/wrapping_tests.rs @@ -1,9 +1,102 @@ use super::super::TextArea; +use super::super::TextAreaState; +use super::visible_prefix; use super::wrapped_lines; use crate::width::display_width; +use crate::wrapping::RtOptions; +use crate::wrapping::adaptive_wrap_lines; use codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS; use pretty_assertions::assert_eq; +use ratatui::Terminal; +use ratatui::backend::TestBackend; use ratatui::layout::Rect; +use ratatui::text::Line; +use ratatui::widgets::StatefulWidgetRef; +use ratatui::widgets::WidgetRef; + +fn wrapped_rows(text: &str, width: u16) -> Vec<&str> { + wrapped_lines(text, width) + .into_iter() + .map(|range| &text[range.start..range.end - 1]) + .collect() +} + +#[test] +fn vertical_navigation_preserves_destination_tab_columns() { + let mut t = TextArea::new(); + t.insert_str("a\tb\nxyz"); + let area = Rect::new(0, 0, /*width*/ 8, /*height*/ 2); + t.set_cursor(/*pos*/ 5); + let _ = t.desired_height(area.width); + t.move_cursor_up(); + assert_eq!((t.cursor(), t.cursor_pos(area)), (1, Some((1, 0)))); + t.move_cursor_down(); + assert_eq!((t.cursor(), t.cursor_pos(area)), (5, Some((1, 1)))); +} + +#[test] +fn mandatory_breaks_keep_distinct_cursor_positions() { + let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 3); + for separator in ["\u{b}", "\u{c}", "\u{85}", "\u{2028}", "\u{2029}", "\r"] { + let text = format!("abad{separator}next"); + let rows = wrapped_rows(&text, area.width); + assert_eq!(rows, ["abad", &format!("{separator}nex"), "t"]); + + let mut t = TextArea::new(); + t.insert_str(&text); + t.set_cursor(/*pos*/ 4); + let before = t.cursor_pos(area); + t.move_cursor_right(); + let after = (t.cursor(), t.cursor_pos(area)); + assert_eq!( + (before, after), + (Some((0, 1)), (4 + separator.len(), Some((1, 1)))) + ); + t.move_cursor_left(); + assert_eq!((t.cursor(), t.cursor_pos(area)), (4, before)); + assert_eq!(t.text(), text); + + if separator == "\u{2028}" { + let mut terminal = Terminal::new(TestBackend::new(area.width, area.height)).unwrap(); + terminal + .draw(|frame| { + WidgetRef::render_ref(&(&t), frame.area(), frame.buffer_mut()); + }) + .unwrap(); + insta::assert_snapshot!( + "mandatory_break_cursor", + format!( + "before: {before:?}\nafter: {after:?}\n{}", + terminal.backend() + ) + .replace(separator, "\\u{2028}") + ); + } + } +} + +#[test] +fn visible_prefix_bounds_long_hanging_runs_without_splitting_graphemes() { + let width = 80; + let count = MAX_USER_INPUT_TEXT_CHARS - 2; + let area = Rect::new(0, 0, width, /*height*/ 2); + for separator in [" ", "\t", "\u{2003}", "\u{3000}"] { + let text = format!("a{}x", separator.repeat(count)); + assert_eq!( + visible_prefix(&text, width), + format!("a{}", separator.repeat(usize::from(width))) + ); + let mut t = TextArea::new(); + t.insert_str(&text); + t.set_cursor(text.len() - 1 - separator.len()); + assert_eq!(t.cursor_pos(area), Some((0, 1))); + assert_eq!(t.text(), text); + } + assert_eq!( + visible_prefix("a \u{301} hidden", /*width*/ 2), + "a \u{301}" + ); +} #[test] fn preserves_textwrap_word_boundaries() { @@ -28,18 +121,234 @@ fn preserves_textwrap_word_boundaries() { } #[test] -fn breakable_unicode_spaces_stay_with_following_words() { +fn breakable_unicode_spaces_hang_before_following_words() { for (text, expected_rows) in [ - ("abad abcde", ["abad", " abc", "de"]), - ("abad\u{2003}abcde", ["abad", "\u{2003}abc", "de"]), - ("abad\u{3000}abcde", ["abad", "\u{3000}ab", "cde"]), + ("abad abcde", ["abad ", "abcd", "e"]), + ("abad\u{2003}abcde", ["abad\u{2003}", "abcd", "e"]), + ( + "abad\u{2003}\u{2003}abcde", + ["abad\u{2003}\u{2003}", "abcd", "e"], + ), + ("abad\u{3000}abcde", ["abad\u{3000}", "abcd", "e"]), + ( + "abad\u{3000} \u{2003}abcde", + ["abad\u{3000} \u{2003}", "abcd", "e"], + ), ] { - let rows = wrapped_lines(text, /*width*/ 4) - .iter() - .map(|range| &text[range.start..range.end - 1]) - .collect::>(); + assert_eq!( + wrapped_rows(text, /*width*/ 4), + expected_rows, + "text={text:?}" + ); + } +} - assert_eq!(rows, expected_rows, "text={text:?}"); +#[test] +fn hanging_spaces_preserve_source_editing_and_visual_navigation() { + let text = "abad next"; + let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 3); + let mut t = TextArea::new(); + t.insert_str(text); + assert_eq!(t.desired_height(area.width), 3); + assert_eq!(t.text(), text); + + for cursor in 4..=6 { + t.set_cursor(cursor); + assert_eq!(t.cursor_pos(area), Some((0, 1))); + } + t.move_cursor_left(); + assert_eq!(t.cursor(), 5); + t.move_cursor_up(); + assert_eq!(t.cursor(), 0); + t.move_cursor_down(); + assert_eq!(t.cursor(), 6); + t.delete_backward(/*n*/ 1); + assert_eq!(t.text(), "abad next"); + assert_eq!(t.cursor_pos(area), Some((0, 1))); + t.delete_backward(/*n*/ 1); + assert_eq!(t.text(), "abadnext"); + assert_eq!(t.cursor_pos(area), Some((0, 1))); +} + +#[test] +fn hanging_tabs_keep_cursor_scrolling_and_navigation_aligned() { + let text = "abc\t a"; + let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 2); + let mut t = TextArea::new(); + t.insert_str(text); + let cursors = (0..=text.len()) + .map(|pos| { + t.set_cursor(pos); + t.cursor_pos(area) + }) + .collect::>(); + assert_eq!( + cursors, + [(0, 0), (1, 0), (2, 0), (3, 0), (0, 1), (0, 1), (1, 1)].map(Some) + ); + + t.set_cursor(/*pos*/ 4); + let mut state = TextAreaState::default(); + let mut terminal = Terminal::new(TestBackend::new(area.width, /*height*/ 1)).unwrap(); + terminal + .draw(|frame| { + StatefulWidgetRef::render_ref(&(&t), frame.area(), frame.buffer_mut(), &mut state); + }) + .unwrap(); + let scrolled_cursor = t.cursor_pos_with_state(terminal.backend().buffer().area, state); + assert_eq!((state.scroll, scrolled_cursor), (1, Some((0, 0)))); + insta::assert_snapshot!( + "hanging_tab_cursor_and_scroll", + format!( + "cursors: {cursors:?}\nscroll: {}\ncursor: {scrolled_cursor:?}\n{}", + state.scroll, + terminal.backend() + ) + ); + + t.move_cursor_up(); + assert_eq!((t.cursor(), t.cursor_pos(area)), (0, Some((0, 0)))); + t.move_cursor_down(); + assert_eq!((t.cursor(), t.cursor_pos(area)), (5, Some((0, 1)))); + assert_eq!(t.text(), text); +} + +#[test] +fn vertical_navigation_clamps_saved_column_after_resize() { + let text = "abcdefghij\nabcd xyz"; + let mut t = TextArea::new(); + t.insert_str(text); + let mut cursors = Vec::new(); + let mut record = |t: &TextArea, label: &str, width: u16, expected| { + let area = Rect::new(0, 0, width, t.desired_height(width)); + let cursor = (t.cursor(), t.cursor_pos(area)); + assert_eq!(cursor, expected, "{label}"); + cursors.push(format!("{label}: {cursor:?}")); + }; + + let _ = t.desired_height(/*width*/ 10); + t.move_cursor_up(); + record(&t, "up at width 10", /*width*/ 10, (10, Some((0, 1)))); + record( + &t, + "resize to width 4", + /*width*/ 4, + (10, Some((2, 2))), + ); + t.move_cursor_down(); + record(&t, "down to abcd", /*width*/ 4, (14, Some((3, 3)))); + t.move_cursor_down(); + record(&t, "down to xyz", /*width*/ 4, (20, Some((3, 4)))); + t.move_cursor_up(); + record(&t, "up to abcd", /*width*/ 4, (14, Some((3, 3)))); + t.move_cursor_up(); + record(&t, "up to ij", /*width*/ 4, (10, Some((2, 2)))); + let _ = t.desired_height(/*width*/ 10); + t.move_cursor_down(); + record( + &t, + "restore width 10", + /*width*/ 10, + (20, Some((9, 2))), + ); + + assert_eq!(t.text(), text); + let layouts = [4, 10].map(|width| { + let mut terminal = Terminal::new(TestBackend::new(width, t.desired_height(width))).unwrap(); + terminal + .draw(|frame| { + WidgetRef::render_ref(&(&t), frame.area(), frame.buffer_mut()); + }) + .unwrap(); + format!("width {width}\n{}", terminal.backend()) + }); + insta::assert_snapshot!( + "vertical_navigation_after_resize", + format!("{}\n\n{}", cursors.join("\n"), layouts.join("\n")) + ); +} + +#[test] +fn typing_after_eol_spaces_reflows_without_changing_text() { + let area = Rect::new(0, 0, /*width*/ 4, /*height*/ 2); + let mut t = TextArea::new(); + let mut terminal = Terminal::new(TestBackend::new(area.width, area.height)).unwrap(); + let mut snapshots = Vec::new(); + for (label, input, expected_cursor) in [ + ("full line", "abad", (0, 1)), + ("trailing spaces", " ", (3, 1)), + ("following word", "x", (1, 1)), + ("delete following word", "", (3, 1)), + ] { + if input.is_empty() { + t.delete_backward(/*n*/ 1); + } else { + t.insert_str(input); + } + terminal + .draw(|frame| { + WidgetRef::render_ref(&(&t), frame.area(), frame.buffer_mut()); + }) + .unwrap(); + assert_eq!(t.cursor_pos(area), Some(expected_cursor)); + snapshots.push(format!( + "{label}\ntext: {:?}\ncursor: {expected_cursor:?}\n{}", + t.text(), + terminal.backend() + )); + } + assert_eq!(t.text(), "abad "); + insta::assert_snapshot!("typing_after_eol_spaces", snapshots.join("\n\n")); +} + +#[test] +fn explicit_indentation_and_trailing_spaces_do_not_hang() { + for (text, expected_rows) in [ + ("abad\n a", vec!["abad", "", " a"]), + ("abad \n a", vec!["abad", " ", " a"]), + (" a", vec![" ", "a"]), + ("abad ", vec!["abad", " "]), + ("abad\u{2003}", vec!["abad", "\u{2003}"]), + ("abad\u{3000}\n a", vec!["abad", "\u{3000}", " a"]), + ] { + assert_eq!( + wrapped_rows(text, /*width*/ 4), + expected_rows, + "text={text:?}" + ); + } +} + +#[test] +fn nonbreaking_spaces_never_hang() { + for space in ['\u{a0}', '\u{2007}', '\u{202f}'] { + let text = format!("abcd{space} x"); + let last = format!("{space} x"); + assert_eq!(wrapped_rows(&text, /*width*/ 4), ["abcd", last.as_str()]); + } +} + +#[test] +fn prose_wraps_like_the_queued_input_preview() { + for text in [ + "new access tokens", + "aaaa next", + "a foo-barbaz", + "some ordinary prose", + ] { + for width in 3..=16 { + let composer = wrapped_rows(text, width) + .into_iter() + .map(|row| row.trim_end_matches(' ')) + .filter(|row| !row.is_empty()) + .collect::>(); + let preview = + adaptive_wrap_lines([Line::from(text)], RtOptions::new(usize::from(width))) + .into_iter() + .map(|line| line.to_string()) + .collect::>(); + assert_eq!(composer, preview, "text={text:?}, width={width}"); + } } } @@ -80,7 +389,7 @@ fn ascii_wrapped_rows_fit_and_preserve_cursor_positions() { end = range.end - 1; let row = &text[range.start..end]; assert!( - display_width(row) <= usize::from(width), + display_width(row.trim_end_matches(' ')) <= usize::from(width), "text={text:?}, width={width}, row={row:?}" ); } @@ -94,7 +403,8 @@ fn ascii_wrapped_rows_fit_and_preserve_cursor_positions() { if let Some(previous) = previous { assert!( position.1 > previous.1 - || (position.1 == previous.1 && position.0 > previous.0), + || (position.1 == previous.1 && position.0 > previous.0) + || (position == previous && text.as_bytes()[cursor - 1] == b' '), "text={text:?}, width={width}, ranges={ranges:?}, cursor={cursor}, previous={previous:?}, position={position:?}" ); }