From b103d7621b0ec48d5134d3c7c7f174a214e80c51 Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Mon, 9 Jun 2025 14:11:31 -0700 Subject: [PATCH] Improve handling of large output in embedded terminals (#32416) #31922 made embedded terminals automatically grow to fit the content. We since found some issues with large output which this PR addresses by: - Only shaping / laying out lines that are visible in the viewport (based on `window.content_mask`) - Falling back to embedded scrolling after 1K lines. The perf fix above actually makes it possible to handle a lot of lines, but: - Alacrity uses a `u16` for rows internally, so we needed a limit to prevent overflow. - Scrolling through thousands of lines to get to the other side of a terminal tool call isn't great UX, so we might as well set the limit low. - We can consider raising the limit when we make card headers sticky. Release Notes: - Agent: Improve handling of large terminal output --- crates/assistant_tools/src/terminal_tool.rs | 53 ++++--- crates/repl/src/outputs/plain.rs | 2 +- crates/terminal_view/src/terminal_element.rs | 152 ++++++++++++------- crates/terminal_view/src/terminal_view.rs | 79 ++++++++-- 4 files changed, 199 insertions(+), 87 deletions(-) diff --git a/crates/assistant_tools/src/terminal_tool.rs b/crates/assistant_tools/src/terminal_tool.rs index 4059eac2cf..5ec0ce7b8f 100644 --- a/crates/assistant_tools/src/terminal_tool.rs +++ b/crates/assistant_tools/src/terminal_tool.rs @@ -638,29 +638,36 @@ impl ToolCard for TerminalToolCard { .bg(cx.theme().colors().editor_background) .rounded_b_md() .text_ui_sm(cx) - .child( - ToolOutputPreview::new( - terminal.clone().into_any_element(), - terminal.entity_id(), - ) - .with_total_lines(self.content_line_count) - .toggle_state(!terminal.read(cx).is_content_limited(window)) - .on_toggle({ - let terminal = terminal.clone(); - move |is_expanded, _, cx| { - terminal.update(cx, |terminal, cx| { - terminal.set_embedded_mode( - if is_expanded { - None - } else { - Some(COLLAPSED_LINES) - }, - cx, - ); - }); - } - }), - ), + .child({ + let content_mode = terminal.read(cx).content_mode(window, cx); + + if content_mode.is_scrollable() { + div().h_72().child(terminal.clone()).into_any_element() + } else { + ToolOutputPreview::new( + terminal.clone().into_any_element(), + terminal.entity_id(), + ) + .with_total_lines(self.content_line_count) + .toggle_state(!content_mode.is_limited()) + .on_toggle({ + let terminal = terminal.clone(); + move |is_expanded, _, cx| { + terminal.update(cx, |terminal, cx| { + terminal.set_embedded_mode( + if is_expanded { + None + } else { + Some(COLLAPSED_LINES) + }, + cx, + ); + }); + } + }) + .into_any_element() + } + }), ) }, ) diff --git a/crates/repl/src/outputs/plain.rs b/crates/repl/src/outputs/plain.rs index 89ae10eee3..237c86d8cc 100644 --- a/crates/repl/src/outputs/plain.rs +++ b/crates/repl/src/outputs/plain.rs @@ -258,7 +258,7 @@ impl Render for TerminalOutput { cell: ic.cell.clone(), }); let (cells, rects) = - TerminalElement::layout_grid(grid, &text_style, text_system, None, window, cx); + TerminalElement::layout_grid(grid, 0, &text_style, text_system, None, window, cx); // lines are 0-indexed, so we must add 1 to get the number of lines let text_line_height = text_style.line_height_in_pixels(window.rem_size()); diff --git a/crates/terminal_view/src/terminal_element.rs b/crates/terminal_view/src/terminal_element.rs index 3c68c0501d..155073c1ef 100644 --- a/crates/terminal_view/src/terminal_element.rs +++ b/crates/terminal_view/src/terminal_element.rs @@ -2,7 +2,7 @@ use editor::{CursorLayout, HighlightedRange, HighlightedRangeLine}; use gpui::{ AnyElement, App, AvailableSpace, Bounds, ContentMask, Context, DispatchPhase, Element, ElementId, Entity, FocusHandle, Font, FontStyle, FontWeight, GlobalElementId, HighlightStyle, - Hitbox, Hsla, InputHandler, InteractiveElement, Interactivity, IntoElement, LayoutId, + Hitbox, Hsla, InputHandler, InteractiveElement, Interactivity, IntoElement, LayoutId, Length, ModifiersChangedEvent, MouseButton, MouseMoveEvent, Pixels, Point, ShapedLine, StatefulInteractiveElement, StrikethroughStyle, Styled, TextRun, TextStyle, UTF16Selection, UnderlineStyle, WeakEntity, WhiteSpace, Window, WindowTextSystem, div, fill, point, px, @@ -32,7 +32,7 @@ use workspace::Workspace; use std::mem; use std::{fmt::Debug, ops::RangeInclusive, rc::Rc}; -use crate::{BlockContext, BlockProperties, TerminalMode, TerminalView}; +use crate::{BlockContext, BlockProperties, ContentMode, TerminalMode, TerminalView}; /// The information generated during layout that is necessary for painting. pub struct LayoutState { @@ -49,6 +49,7 @@ pub struct LayoutState { gutter: Pixels, block_below_cursor_element: Option, base_text_style: TextStyle, + content_mode: ContentMode, } /// Helper struct for converting data between Alacritty's cursor points, and displayed cursor points. @@ -202,6 +203,7 @@ impl TerminalElement { pub fn layout_grid( grid: impl Iterator, + start_line_offset: i32, text_style: &TextStyle, // terminal_theme: &TerminalStyle, text_system: &WindowTextSystem, @@ -218,6 +220,8 @@ impl TerminalElement { let linegroups = grid.into_iter().chunk_by(|i| i.point.line); for (line_index, (_, line)) in linegroups.into_iter().enumerate() { + let alac_line = start_line_offset + line_index as i32; + for cell in line { let mut fg = cell.fg; let mut bg = cell.bg; @@ -245,7 +249,7 @@ impl TerminalElement { || { Some(LayoutRect::new( AlacPoint::new( - line_index as i32, + alac_line, cell.point.column.0 as i32, ), 1, @@ -260,10 +264,7 @@ impl TerminalElement { rects.push(cur_rect.take().unwrap()); } cur_rect = Some(LayoutRect::new( - AlacPoint::new( - line_index as i32, - cell.point.column.0 as i32, - ), + AlacPoint::new(alac_line, cell.point.column.0 as i32), 1, convert_color(&bg, theme), )); @@ -272,7 +273,7 @@ impl TerminalElement { None => { cur_alac_color = Some(bg); cur_rect = Some(LayoutRect::new( - AlacPoint::new(line_index as i32, cell.point.column.0 as i32), + AlacPoint::new(alac_line, cell.point.column.0 as i32), 1, convert_color(&bg, theme), )); @@ -295,7 +296,7 @@ impl TerminalElement { ); cells.push(LayoutCell::new( - AlacPoint::new(line_index as i32, cell.point.column.0 as i32), + AlacPoint::new(alac_line, cell.point.column.0 as i32), layout_cell, )) }; @@ -430,7 +431,13 @@ impl TerminalElement { } } - fn register_mouse_listeners(&mut self, mode: TermMode, hitbox: &Hitbox, window: &mut Window) { + fn register_mouse_listeners( + &mut self, + mode: TermMode, + hitbox: &Hitbox, + content_mode: &ContentMode, + window: &mut Window, + ) { let focus = self.focus.clone(); let terminal = self.terminal.clone(); let terminal_view = self.terminal_view.clone(); @@ -512,14 +519,18 @@ impl TerminalElement { ), ); - if !matches!(self.mode, TerminalMode::Embedded { .. }) { + if content_mode.is_scrollable() { self.interactivity.on_scroll_wheel({ let terminal_view = self.terminal_view.downgrade(); - move |e, _window, cx| { + move |e, window, cx| { terminal_view .update(cx, |terminal_view, cx| { - terminal_view.scroll_wheel(e, cx); - cx.notify(); + if matches!(terminal_view.mode, TerminalMode::Standalone) + || terminal_view.focus_handle.is_focused(window) + { + terminal_view.scroll_wheel(e, cx); + cx.notify(); + } }) .ok(); } @@ -605,6 +616,32 @@ impl Element for TerminalElement { window: &mut Window, cx: &mut App, ) -> (LayoutId, Self::RequestLayoutState) { + let height: Length = match self.terminal_view.read(cx).content_mode(window, cx) { + ContentMode::Inline { + displayed_lines, + total_lines: _, + } => { + let rem_size = window.rem_size(); + let line_height = window.text_style().font_size.to_pixels(rem_size) + * TerminalSettings::get_global(cx) + .line_height + .value() + .to_pixels(rem_size) + .0; + (displayed_lines * line_height).into() + } + ContentMode::Scrollable => { + if let TerminalMode::Embedded { .. } = &self.mode { + let term = self.terminal.read(cx); + if !term.scrolled_to_top() && !term.scrolled_to_bottom() && self.focused { + self.interactivity.occlude_mouse(); + } + } + + relative(1.).into() + } + }; + let layout_id = self.interactivity.request_layout( global_id, inspector_id, @@ -612,29 +649,7 @@ impl Element for TerminalElement { cx, |mut style, window, cx| { style.size.width = relative(1.).into(); - - match &self.mode { - TerminalMode::Scrollable => { - style.size.height = relative(1.).into(); - } - TerminalMode::Embedded { max_lines } => { - let rem_size = window.rem_size(); - let line_height = window.text_style().font_size.to_pixels(rem_size) - * TerminalSettings::get_global(cx) - .line_height - .value() - .to_pixels(rem_size) - .0; - - let mut line_count = self.terminal.read(cx).total_lines(); - if !self.focused { - if let Some(max_lines) = max_lines { - line_count = line_count.min(*max_lines); - } - } - style.size.height = (line_count * line_height).into(); - } - } + style.size.height = height; window.request_layout(style, None, cx) }, @@ -693,7 +708,7 @@ impl Element for TerminalElement { TerminalMode::Embedded { .. } => { window.text_style().font_size.to_pixels(window.rem_size()) } - TerminalMode::Scrollable => terminal_settings + TerminalMode::Standalone => terminal_settings .font_size .map_or(buffer_font_size, |size| theme::adjusted_font_size(size, cx)), }; @@ -733,7 +748,7 @@ impl Element for TerminalElement { let player_color = theme.players().local(); let match_color = theme.colors().search_match_background; let gutter; - let dimensions = { + let (dimensions, line_height_px) = { let rem_size = window.rem_size(); let font_pixels = text_style.font_size.to_pixels(rem_size); // TODO: line_height should be an f32 not an AbsoluteLength. @@ -759,7 +774,10 @@ impl Element for TerminalElement { let mut origin = bounds.origin; origin.x += gutter; - TerminalBounds::new(line_height, cell_width, Bounds { origin, size }) + ( + TerminalBounds::new(line_height, cell_width, Bounds { origin, size }), + line_height, + ) }; let search_matches = self.terminal.read(cx).matches.clone(); @@ -827,16 +845,42 @@ impl Element for TerminalElement { // then have that representation be converted to the appropriate highlight data structure - let (cells, rects) = TerminalElement::layout_grid( - cells.iter().cloned(), - &text_style, - window.text_system(), - last_hovered_word - .as_ref() - .map(|last_hovered_word| (link_style, &last_hovered_word.word_match)), - window, - cx, - ); + let content_mode = self.terminal_view.read(cx).content_mode(window, cx); + let (cells, rects) = match content_mode { + ContentMode::Scrollable => TerminalElement::layout_grid( + cells.iter().cloned(), + 0, + &text_style, + window.text_system(), + last_hovered_word + .as_ref() + .map(|last_hovered_word| (link_style, &last_hovered_word.word_match)), + window, + cx, + ), + ContentMode::Inline { .. } => { + let intersection = window.content_mask().bounds.intersect(&bounds); + let start_row = (intersection.top() - bounds.top()) / line_height_px; + let end_row = start_row + intersection.size.height / line_height_px; + let line_range = (start_row as i32)..=(end_row as i32); + + TerminalElement::layout_grid( + cells + .iter() + .skip_while(|i| &i.point.line < line_range.start()) + .take_while(|i| &i.point.line <= line_range.end()) + .cloned(), + *line_range.start(), + &text_style, + window.text_system(), + last_hovered_word.as_ref().map(|last_hovered_word| { + (link_style, &last_hovered_word.word_match) + }), + window, + cx, + ) + } + }; // Layout cursor. Rectangle is used for IME, so we should lay it out even // if we don't end up showing it. @@ -932,6 +976,7 @@ impl Element for TerminalElement { gutter, block_below_cursor_element, base_text_style: text_style, + content_mode, } }, ) @@ -969,7 +1014,12 @@ impl Element for TerminalElement { workspace: self.workspace.clone(), }; - self.register_mouse_listeners(layout.mode, &layout.hitbox, window); + self.register_mouse_listeners( + layout.mode, + &layout.hitbox, + &layout.content_mode, + window, + ); if window.modifiers().secondary() && bounds.contains(&window.mouse_position()) && self.terminal_view.read(cx).hover.is_some() diff --git a/crates/terminal_view/src/terminal_view.rs b/crates/terminal_view/src/terminal_view.rs index ebc84aad42..148d74ec2e 100644 --- a/crates/terminal_view/src/terminal_view.rs +++ b/crates/terminal_view/src/terminal_view.rs @@ -140,12 +140,37 @@ pub struct TerminalView { #[derive(Default, Clone)] pub enum TerminalMode { #[default] - Scrollable, + Standalone, Embedded { - max_lines: Option, + max_lines_when_unfocused: Option, }, } +#[derive(Clone)] +pub enum ContentMode { + Scrollable, + Inline { + displayed_lines: usize, + total_lines: usize, + }, +} + +impl ContentMode { + pub fn is_limited(&self) -> bool { + match self { + ContentMode::Scrollable => false, + ContentMode::Inline { + displayed_lines, + total_lines, + } => displayed_lines < total_lines, + } + } + + pub fn is_scrollable(&self) -> bool { + matches!(self, ContentMode::Scrollable) + } +} + #[derive(Debug)] struct HoverTarget { tooltip: String, @@ -223,7 +248,7 @@ impl TerminalView { blink_epoch: 0, hover: None, hover_tooltip_update: Task::ready(()), - mode: TerminalMode::Scrollable, + mode: TerminalMode::Standalone, workspace_id, show_breadcrumbs: TerminalSettings::get_global(cx).toolbar.breadcrumbs, block_below_cursor: None, @@ -245,16 +270,46 @@ impl TerminalView { } /// Enable 'embedded' mode where the terminal displays the full content with an optional limit of lines. - pub fn set_embedded_mode(&mut self, max_lines: Option, cx: &mut Context) { - self.mode = TerminalMode::Embedded { max_lines }; + pub fn set_embedded_mode( + &mut self, + max_lines_when_unfocused: Option, + cx: &mut Context, + ) { + self.mode = TerminalMode::Embedded { + max_lines_when_unfocused, + }; cx.notify(); } - pub fn is_content_limited(&self, window: &Window) -> bool { + const MAX_EMBEDDED_LINES: usize = 1_000; + + /// Returns the current `ContentMode` depending on the set `TerminalMode` and the current number of lines + /// + /// Note: Even in embedded mode, the terminal will fallback to scrollable when its content exceeds `MAX_EMBEDDED_LINES` + pub fn content_mode(&self, window: &Window, cx: &App) -> ContentMode { match &self.mode { - TerminalMode::Scrollable => false, - TerminalMode::Embedded { max_lines } => { - !self.focus_handle.is_focused(window) && max_lines.is_some() + TerminalMode::Standalone => ContentMode::Scrollable, + TerminalMode::Embedded { + max_lines_when_unfocused, + } => { + let total_lines = self.terminal.read(cx).total_lines(); + + if total_lines > Self::MAX_EMBEDDED_LINES { + ContentMode::Scrollable + } else { + let mut displayed_lines = total_lines; + + if !self.focus_handle.is_focused(window) { + if let Some(max_lines) = max_lines_when_unfocused { + displayed_lines = displayed_lines.min(*max_lines) + } + } + + ContentMode::Inline { + displayed_lines, + total_lines, + } + } } } } @@ -840,10 +895,10 @@ impl TerminalView { })) } - fn render_scrollbar(&self, cx: &mut Context) -> Option> { + fn render_scrollbar(&self, window: &Window, cx: &mut Context) -> Option> { if !Self::should_show_scrollbar(cx) || !(self.show_scrollbar || self.scrollbar_state.is_dragging()) - || matches!(self.mode, TerminalMode::Embedded { .. }) + || !self.content_mode(window, cx).is_scrollable() { return None; } @@ -1493,7 +1548,7 @@ impl Render for TerminalView { self.block_below_cursor.clone(), self.mode.clone(), )) - .when_some(self.render_scrollbar(cx), |div, scrollbar| { + .when_some(self.render_scrollbar(window, cx), |div, scrollbar| { div.child(scrollbar) }), )