diff --git a/crates/terminal_view/src/terminal_element.rs b/crates/terminal_view/src/terminal_element.rs index c5289a34d6cbc9ecc4ddcdaacb6aaf2ce0831846..0a21aecf1ae30039a746b9061551ca1761a6b90e 100644 --- a/crates/terminal_view/src/terminal_element.rs +++ b/crates/terminal_view/src/terminal_element.rs @@ -1065,43 +1065,73 @@ impl Element for TerminalElement { // then have that representation be converted to the appropriate highlight data structure let content_mode = self.terminal_view.read(cx).content_mode(window, cx); - let (rects, batched_text_runs) = match content_mode { - ContentMode::Scrollable => { - // In scrollable mode, the terminal already provides cells - // that are correctly positioned for the current viewport - // based on its display_offset. We don't need additional filtering. - TerminalElement::layout_grid( - cells.iter().cloned(), - 0, - &text_style, - last_hovered_word.as_ref().map(|last_hovered_word| { - (link_style, &last_hovered_word.word_match) - }), - minimum_contrast, - 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, - last_hovered_word.as_ref().map(|last_hovered_word| { - (link_style, &last_hovered_word.word_match) - }), - minimum_contrast, - cx, - ) - } + + // Calculate the intersection of the terminal's bounds with the current + // content mask (the visible viewport after all parent clipping). + // This allows us to only render cells that are actually visible, which is + // critical for performance when terminals are inside scrollable containers + // like the Agent Panel thread view. + // + // This optimization is analogous to the editor optimization in PR #45077 + // which fixed performance issues with large AutoHeight editors inside Lists. + let visible_bounds = window.content_mask().bounds; + let intersection = visible_bounds.intersect(&bounds); + + // If the terminal is entirely outside the viewport, skip all cell processing. + // This handles the case where the terminal has been scrolled past (above or + // below the viewport), similar to the editor fix in PR #45077 where start_row + // could exceed max_row when the editor was positioned above the viewport. + let (rects, batched_text_runs) = if intersection.size.height <= px(0.) + || intersection.size.width <= px(0.) + { + (Vec::new(), Vec::new()) + } else if intersection == bounds { + // Fast path: terminal fully visible, no clipping needed. + // Avoid grouping/allocation overhead by streaming cells directly. + TerminalElement::layout_grid( + cells.iter().cloned(), + 0, + &text_style, + last_hovered_word + .as_ref() + .map(|last_hovered_word| (link_style, &last_hovered_word.word_match)), + minimum_contrast, + cx, + ) + } else { + // Calculate which screen rows are visible based on pixel positions. + // This works for both Scrollable and Inline modes because we filter + // by screen position (enumerated line group index), not by the cell's + // internal line number (which can be negative in Scrollable mode for + // scrollback history). + let rows_above_viewport = + ((intersection.top() - bounds.top()).max(px(0.)) / line_height_px) as usize; + let visible_row_count = + (intersection.size.height / line_height_px).ceil() as usize + 1; + + // Group cells by line and filter to only the visible screen rows. + // skip() and take() work on enumerated line groups (screen position), + // making this work regardless of the actual cell.point.line values. + let visible_cells: Vec<_> = cells + .iter() + .chunk_by(|c| c.point.line) + .into_iter() + .skip(rows_above_viewport) + .take(visible_row_count) + .flat_map(|(_, line_cells)| line_cells) + .cloned() + .collect(); + + TerminalElement::layout_grid( + visible_cells.into_iter(), + rows_above_viewport as i32, + &text_style, + last_hovered_word + .as_ref() + .map(|last_hovered_word| (link_style, &last_hovered_word.word_match)), + minimum_contrast, + cx, + ) }; // Layout cursor. Rectangle is used for IME, so we should lay it out even @@ -2059,4 +2089,248 @@ mod tests { let merged2 = merge_background_regions(regions2); assert_eq!(merged2.len(), 3); } + + #[test] + fn test_screen_position_filtering_with_positive_lines() { + // Test the unified screen-position-based filtering approach. + // This works for both Scrollable and Inline modes because we filter + // by enumerated line group index, not by cell.point.line values. + use itertools::Itertools; + use terminal::IndexedCell; + use terminal::alacritty_terminal::index::{Column, Line, Point as AlacPoint}; + use terminal::alacritty_terminal::term::cell::Cell; + + // Create mock cells for lines 0-23 (typical terminal with 24 visible lines) + let mut cells = Vec::new(); + for line in 0..24i32 { + for col in 0..3i32 { + cells.push(IndexedCell { + point: AlacPoint::new(Line(line), Column(col as usize)), + cell: Cell::default(), + }); + } + } + + // Scenario: Terminal partially scrolled above viewport + // First 5 lines (0-4) are clipped, lines 5-15 should be visible + let rows_above_viewport = 5usize; + let visible_row_count = 11usize; + + // Apply the same filtering logic as in the render code + let filtered: Vec<_> = cells + .iter() + .chunk_by(|c| c.point.line) + .into_iter() + .skip(rows_above_viewport) + .take(visible_row_count) + .flat_map(|(_, line_cells)| line_cells) + .collect(); + + // Should have lines 5-15 (11 lines * 3 cells each = 33 cells) + assert_eq!(filtered.len(), 11 * 3, "Should have 33 cells for 11 lines"); + + // First filtered cell should be line 5 + assert_eq!( + filtered.first().unwrap().point.line, + Line(5), + "First cell should be on line 5" + ); + + // Last filtered cell should be line 15 + assert_eq!( + filtered.last().unwrap().point.line, + Line(15), + "Last cell should be on line 15" + ); + } + + #[test] + fn test_screen_position_filtering_with_negative_lines() { + // This is the key test! In Scrollable mode, cells have NEGATIVE line numbers + // for scrollback history. The screen-position filtering approach works because + // we filter by enumerated line group index, not by cell.point.line values. + use itertools::Itertools; + use terminal::IndexedCell; + use terminal::alacritty_terminal::index::{Column, Line, Point as AlacPoint}; + use terminal::alacritty_terminal::term::cell::Cell; + + // Simulate cells from a scrolled terminal with scrollback + // These have negative line numbers representing scrollback history + let mut scrollback_cells = Vec::new(); + for line in -588i32..=-578i32 { + for col in 0..80i32 { + scrollback_cells.push(IndexedCell { + point: AlacPoint::new(Line(line), Column(col as usize)), + cell: Cell::default(), + }); + } + } + + // Scenario: First 3 screen rows clipped, show next 5 rows + let rows_above_viewport = 3usize; + let visible_row_count = 5usize; + + // Apply the same filtering logic as in the render code + let filtered: Vec<_> = scrollback_cells + .iter() + .chunk_by(|c| c.point.line) + .into_iter() + .skip(rows_above_viewport) + .take(visible_row_count) + .flat_map(|(_, line_cells)| line_cells) + .collect(); + + // Should have 5 lines * 80 cells = 400 cells + assert_eq!(filtered.len(), 5 * 80, "Should have 400 cells for 5 lines"); + + // First filtered cell should be line -585 (skipped 3 lines from -588) + assert_eq!( + filtered.first().unwrap().point.line, + Line(-585), + "First cell should be on line -585" + ); + + // Last filtered cell should be line -581 (5 lines: -585, -584, -583, -582, -581) + assert_eq!( + filtered.last().unwrap().point.line, + Line(-581), + "Last cell should be on line -581" + ); + } + + #[test] + fn test_screen_position_filtering_skip_all() { + // Test what happens when we skip more rows than exist + use itertools::Itertools; + use terminal::IndexedCell; + use terminal::alacritty_terminal::index::{Column, Line, Point as AlacPoint}; + use terminal::alacritty_terminal::term::cell::Cell; + + let mut cells = Vec::new(); + for line in 0..10i32 { + cells.push(IndexedCell { + point: AlacPoint::new(Line(line), Column(0)), + cell: Cell::default(), + }); + } + + // Skip more rows than exist + let rows_above_viewport = 100usize; + let visible_row_count = 5usize; + + let filtered: Vec<_> = cells + .iter() + .chunk_by(|c| c.point.line) + .into_iter() + .skip(rows_above_viewport) + .take(visible_row_count) + .flat_map(|(_, line_cells)| line_cells) + .collect(); + + assert_eq!( + filtered.len(), + 0, + "Should have no cells when all are skipped" + ); + } + + #[test] + fn test_layout_grid_positioning_math() { + // Test the math that layout_grid uses for positioning. + // When we skip N rows, we pass N as start_line_offset to layout_grid, + // which positions the first visible line at screen row N. + + // Scenario: Terminal at y=-100px, line_height=20px + // First 5 screen rows are above viewport (clipped) + // So we skip 5 rows and pass offset=5 to layout_grid + + let terminal_origin_y = -100.0f32; + let line_height = 20.0f32; + let rows_skipped = 5; + + // The first visible line (at offset 5) renders at: + // y = terminal_origin + offset * line_height = -100 + 5*20 = 0 + let first_visible_y = terminal_origin_y + rows_skipped as f32 * line_height; + assert_eq!( + first_visible_y, 0.0, + "First visible line should be at viewport top (y=0)" + ); + + // The 6th visible line (at offset 10) renders at: + let sixth_visible_y = terminal_origin_y + (rows_skipped + 5) as f32 * line_height; + assert_eq!( + sixth_visible_y, 100.0, + "6th visible line should be at y=100" + ); + } + + #[test] + fn test_unified_filtering_works_for_both_modes() { + // This test proves that the unified screen-position filtering approach + // works for BOTH positive line numbers (Inline mode) and negative line + // numbers (Scrollable mode with scrollback). + // + // The key insight: we filter by enumerated line group index (screen position), + // not by cell.point.line values. This makes the filtering agnostic to the + // actual line numbers in the cells. + use itertools::Itertools; + use terminal::IndexedCell; + use terminal::alacritty_terminal::index::{Column, Line, Point as AlacPoint}; + use terminal::alacritty_terminal::term::cell::Cell; + + // Test with positive line numbers (Inline mode style) + let positive_cells: Vec<_> = (0..10i32) + .flat_map(|line| { + (0..3i32).map(move |col| IndexedCell { + point: AlacPoint::new(Line(line), Column(col as usize)), + cell: Cell::default(), + }) + }) + .collect(); + + // Test with negative line numbers (Scrollable mode with scrollback) + let negative_cells: Vec<_> = (-10i32..0i32) + .flat_map(|line| { + (0..3i32).map(move |col| IndexedCell { + point: AlacPoint::new(Line(line), Column(col as usize)), + cell: Cell::default(), + }) + }) + .collect(); + + let rows_to_skip = 3usize; + let rows_to_take = 4usize; + + // Filter positive cells + let positive_filtered: Vec<_> = positive_cells + .iter() + .chunk_by(|c| c.point.line) + .into_iter() + .skip(rows_to_skip) + .take(rows_to_take) + .flat_map(|(_, cells)| cells) + .collect(); + + // Filter negative cells + let negative_filtered: Vec<_> = negative_cells + .iter() + .chunk_by(|c| c.point.line) + .into_iter() + .skip(rows_to_skip) + .take(rows_to_take) + .flat_map(|(_, cells)| cells) + .collect(); + + // Both should have same count: 4 lines * 3 cells = 12 + assert_eq!(positive_filtered.len(), 12); + assert_eq!(negative_filtered.len(), 12); + + // Positive: lines 3, 4, 5, 6 + assert_eq!(positive_filtered.first().unwrap().point.line, Line(3)); + assert_eq!(positive_filtered.last().unwrap().point.line, Line(6)); + + // Negative: lines -7, -6, -5, -4 + assert_eq!(negative_filtered.first().unwrap().point.line, Line(-7)); + assert_eq!(negative_filtered.last().unwrap().point.line, Line(-4)); + } }