-
Notifications
You must be signed in to change notification settings - Fork 7.4k
fix(tui): avoid hard-wrapping URL lines in history #8757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 19 commits
756324a
cd88cbe
27f8434
368cd4a
1f460aa
2a94512
2718474
e29c887
3924eb9
d02735e
76598df
844261f
d38a801
b1a639b
e7ac563
b69e937
e65bde3
ae5d64e
1b28ad7
6976202
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,8 @@ use std::fmt; | |
| use std::io; | ||
| use std::io::Write; | ||
|
|
||
| use crate::wrapping::word_wrap_lines_borrowed; | ||
| use crate::wrapping::RtOptions; | ||
| use crate::wrapping::word_wrap_line_url_aware; | ||
| use crossterm::Command; | ||
| use crossterm::cursor::MoveTo; | ||
| use crossterm::queue; | ||
|
|
@@ -22,6 +23,37 @@ use ratatui::style::Modifier; | |
| use ratatui::text::Line; | ||
| use ratatui::text::Span; | ||
|
|
||
| fn wrap_history_lines<'a>(lines: &'a [Line<'a>], width: u16) -> Vec<Line<'a>> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit (subjective preference): I'd prefer to see this below where it's called as it makes the code easier generally to read top-to-bottom outside-in. |
||
| let base_opts = RtOptions::new(width.max(1) as usize); | ||
| let mut wrapped: Vec<Line<'a>> = Vec::new(); | ||
| let mut first = true; | ||
| for line in lines { | ||
| let opts = if first { | ||
| base_opts.clone() | ||
| } else { | ||
| base_opts | ||
| .clone() | ||
| .initial_indent(base_opts.subsequent_indent.clone()) | ||
| }; | ||
|
Comment on lines
+33
to
+37
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a bit odd. I'm not sure I understand why we need to do this rather than relying on initial indent directly here. |
||
| wrapped.extend(word_wrap_line_url_aware(line, opts)); | ||
| first = false; | ||
| } | ||
| wrapped | ||
| } | ||
|
|
||
| /// Counts the number of terminal rows needed after wrapping `lines` at `width`. | ||
| /// This accounts for any long tokens (like URLs) that are still forced to | ||
| /// hard-wrap by the terminal even after word-aware wrapping. | ||
| fn wrapped_row_count(lines: &[Line<'_>], width: u16) -> u16 { | ||
| let width = width.max(1) as usize; | ||
| let mut rows = 0usize; | ||
| for line in lines { | ||
| let line_width = line.width().max(1); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love the Not a big deal, but if there's a simple fix to avoid this (like storing the line + width computed once here) that would be great. |
||
| rows = rows.saturating_add(line_width.div_ceil(width)); | ||
| } | ||
|
Comment on lines
+50
to
+53
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can probably be more written more succinctly / obviously with map + sum |
||
| u16::try_from(rows).unwrap_or(u16::MAX) | ||
| } | ||
Daniel-Santiago-Acosta-1013 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// Insert `lines` above the viewport using the terminal's backend writer | ||
| /// (avoids direct stdout references). | ||
| pub fn insert_history_lines<B>( | ||
|
|
@@ -40,8 +72,8 @@ where | |
|
|
||
| // Pre-wrap lines using word-aware wrapping so terminal scrollback sees the same | ||
| // formatting as the TUI. This avoids character-level hard wrapping by the terminal. | ||
| let wrapped = word_wrap_lines_borrowed(&lines, area.width.max(1) as usize); | ||
| let wrapped_lines = wrapped.len() as u16; | ||
| let wrapped = wrap_history_lines(&lines, area.width); | ||
| let wrapped_lines = wrapped_row_count(&wrapped, area.width); | ||
| let cursor_top = if area.bottom() < screen_size.height { | ||
| // If the viewport is not at the bottom of the screen, scroll it down to make room. | ||
| // Don't scroll it past the bottom of the screen. | ||
|
|
@@ -287,6 +319,7 @@ mod tests { | |
| use super::*; | ||
| use crate::markdown_render::render_markdown_text; | ||
| use crate::test_backend::VT100Backend; | ||
| use pretty_assertions::assert_eq; | ||
| use ratatui::layout::Rect; | ||
| use ratatui::style::Color; | ||
|
|
||
|
|
@@ -318,6 +351,55 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn wrap_history_lines_wraps_before_url() { | ||
| let url = "http://foobar"; | ||
| let line: Line<'static> = | ||
| Line::from("Here is some text and a http://foobar url in the middle"); | ||
|
|
||
| let lines = [line]; | ||
| let wrapped = wrap_history_lines(&lines, 24); | ||
| let rendered: Vec<String> = wrapped | ||
| .iter() | ||
| .map(|line| { | ||
| line.spans | ||
| .iter() | ||
| .map(|span| span.content.as_ref()) | ||
| .collect() | ||
| }) | ||
| .collect(); | ||
|
|
||
| let Some(url_line) = rendered.iter().find(|line| line.contains(url)) else { | ||
| panic!("expected wrapped output to contain url"); | ||
| }; | ||
|
|
||
| assert!(url_line.starts_with(url)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn wrap_history_lines_breaks_long_url_when_too_long() { | ||
| let url = "https://github.com/openai/codex/issues/new?template=2-bug-report.yml&steps=Uploaded%20thread:thread-1"; | ||
| let line: Line<'static> = Line::from(url); | ||
| let width = 20u16; | ||
|
|
||
| let lines = [line]; | ||
| let wrapped = wrap_history_lines(&lines, width); | ||
| let rows = wrapped_row_count(&wrapped, width); | ||
| let rendered: Vec<String> = wrapped | ||
| .iter() | ||
| .map(|line| { | ||
| line.spans | ||
| .iter() | ||
| .map(|span| span.content.as_ref()) | ||
| .collect() | ||
| }) | ||
| .collect(); | ||
|
|
||
| assert!(wrapped.len() > 1); | ||
| assert!(rendered.iter().all(|line| !line.contains(url))); | ||
| assert_eq!(rows as usize, wrapped.len()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn vt100_blockquote_line_emits_green_fg() { | ||
| // Set up a small off-screen terminal | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add covering tests (make codex produce these so that all the edge cases / behavior are properly tested). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a doc comment clarifying intent