From 682427324d5a821edb7cad469e8980df3f0f21f1 Mon Sep 17 00:00:00 2001 From: Valdemar Lorenzen Date: Fri, 16 Oct 2026 11:02:44 +0200 Subject: [PATCH] fix: overlay clipped scope labels without reflow --- TODO.md | 4 +- docs/editing.md | 2 +- src/tui.rs | 271 +++++++++++++++++++++++++++++++++++++----------- 3 files changed, 215 insertions(+), 62 deletions(-) diff --git a/TODO.md b/TODO.md index 0041192..9e50a2e 100644 --- a/TODO.md +++ b/TODO.md @@ -13,8 +13,8 @@ - [x] action to scroll viewport without having to move the cursor (like vim's `ctrl+e|u`). - [x] scroll viewport action (`ctrl+u|d`) scroll distance should be configurable. - [x] file picker file search should also be fuzzy (we should be able to use the same fuzzy implemented for diff view search, i believe) -- [ ] diff view: labels are currently attached also when scrolling the view port horizontally (such that e.g. a function name is not fully visible in the viewport), which is good, but it currently attaches at that line, thus pushing down the function by one line, which causes a one-line jump when scrolling horizontally. - - we should in this case attach the label above the function instead of on the same line that it's defined such that it's not pushed down. +- [x] diff view: labels are currently attached also when scrolling the view port horizontally (such that e.g. a function name is not fully visible in the viewport), which is good, but it currently attaches at that line, thus pushing down the function by one line, which causes a one-line jump when scrolling horizontally. + - horizontal labels overlay the row above the declaration, or the pane's top border, without shifting source rows. - [ ] diff view: github integration (code review) - add comments on a pr, add review comments at specific lines of files, request changes, approve pr, etc. - [ ] diff view: local review comments (same features as for github reviews) that allows review of local changes to deliver specific reviews to ai agents used for coding. - design question: would this require a server that the agent can interact with, or would local review files be fine? don't want to use server client stuff, if it would be easier and better to just use local files. diff --git a/docs/editing.md b/docs/editing.md index 4a6b7ac..2a47d90 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -449,7 +449,7 @@ Review marks belong only to the open comparison and survive filtering, file navi Text panes show change excerpts with **10 unchanged lines above and below** each change, preserving original file line numbers. Overlapping or touching excerpts merge; omitted stretches show an unchanged-line count in both panes. Files with only metadata changes show their notice or “No text changes” rather than unrelated source. -**Scope labels** identify named enclosing functions/classes and containers, such as `impl Review › run`. Muted, unnumbered rows supply context only when the declaration's symbol is outside that pane's viewport. Scrolling the symbol back into view removes the label and reclaims its row; decorators alone do not count as the declaration name. Visibility follows source columns and wrapped segments independently for each side. Each split side uses its own source, so renamed functions keep distinct old/new names. Unified labels use `−` for Before, `+` for After, and `±` for shared names. Narrow panes prefer the innermost name. Labels are presentation only: motions, selections, copying, editing, and line jumps retain original source coordinates. Context comes from the existing Tree-sitter trees, including supported injected code, without requiring an LSP; unsupported syntax gets no guessed label. +**Scope labels** identify named enclosing functions/classes and containers, such as `impl Review › run`. Muted, unnumbered rows supply context when the declaration is vertically outside that pane's viewport; scrolling it back into view removes the label and reclaims its row. If an unwrapped declaration remains vertically visible but its symbol is clipped horizontally, its label instead overlays the preceding row, or the pane's top border for the first visible row. Horizontal scrolling never inserts a row or shifts source text; scrolling the symbol back into view restores the covered row or border. Decorators alone do not count as the declaration name. Visibility follows source columns and wrapped segments independently for each side. Each split side uses its own source, so renamed functions keep distinct old/new names. Unified labels use `−` for Before, `+` for After, and `±` for shared names. Narrow panes prefer the innermost name. Labels are presentation only: motions, selections, copying, editing, and line jumps retain original source coordinates. Context comes from the existing Tree-sitter trees, including supported injected code, without requiring an LSP; unsupported syntax gets no guessed label. Within an excerpt, additional hunks and alignment gaps do not repeat an unchanged scope label. A new excerpt or an actual scope change gets a fresh label when needed; sticky context is retained independently for each side. In **Normal mode** with a text pane focused, **`[` expands above** and **`]` expands below** the excerpt containing the caret, revealing up to **15 lines** per keypress in either pane. In editable After's Insert mode these keys insert literal text. Expansion clamps at file boundaries and merges excerpts when they meet. Existing visible source stays anchored. Opening a file resets its excerpts to the configured initial context. Live edits recompute changes while retaining expanded context; the caret stays visible even when the last text change is reverted. diff --git a/src/tui.rs b/src/tui.rs index c053988..5503324 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -3774,7 +3774,6 @@ fn draw_diff( let viewport = panels .diff_wrap .get_or_insert_with(DiffWrapViewport::default); - viewport.lefts = [before_left, after_left]; viewport.update( diff, file, @@ -3958,6 +3957,22 @@ fn draw_diff( content, ); } + if let (Some(file), Some(syntax), Some(viewport)) = ( + selected.filter(|file| file.loaded), + panels.diff_syntax.as_ref(), + panels.diff_wrap.as_ref(), + ) { + draw_diff_scope_labels( + frame, + diff, + file, + syntax, + viewport, + [before_area, after_area], + [before_left, after_left], + &mut document_caret, + ); + } panels.diff = Some(DiffGeometry { regions: [ (navigator.unwrap_or_default(), Focus::Files), @@ -4090,6 +4105,116 @@ fn diff_pane_panel( diff_panel(frame, area, &title, access, &revision, diff.focus == focus) } +#[allow(clippy::too_many_arguments)] +fn draw_diff_scope_labels( + frame: &mut Frame, + diff: &DiffState, + file: &ChangedFile, + syntax: &DiffSyntaxCache, + viewport: &DiffWrapViewport, + areas: [Rect; 2], + lefts: [usize; 2], + caret: &mut Option<(Rect, Position)>, +) { + let unified = diff.view_mode == ViewMode::Unified; + for (pane, focus) in [Focus::Before, Focus::After].into_iter().enumerate() { + if areas[pane].is_empty() || (unified && (diff.focus == Focus::Before) != (pane == 0)) { + continue; + } + let content = diff_inner(areas[pane]); + let gutter = diff_gutter(diff, file, usize::from(content.width), focus); + let width = usize::from(content.width).saturating_sub(gutter); + for (screen, row) in viewport + .rows + .iter() + .take(usize::from(content.height)) + .enumerate() + { + let overlay = row.scope == [false; 2]; + if overlay && !row.unwrapped { + continue; + } + let contexts = syntax.scopes(diff, row.position.display); + let side = if !unified { + pane + } else if overlay { + usize::from(diff.display_focus(row.position.display) != Some(Focus::Before)) + } else { + usize::from(!row.scope[0]) + }; + let Some(scope) = contexts[side] else { + continue; + }; + let y = content.y + screen as u16; + let y = if overlay { + let source_focus = if side == 0 { + Focus::Before + } else { + Focus::After + }; + if diff.display_source_row(source_focus, row.position.display) + != Some(scope.name.row) + { + continue; + } + let source = if side == 0 { &file.before } else { &file.after }; + let prefix = &source[scope.name.row][..scope.name.column]; + let column = display_column(prefix, usize::MAX); + if lefts[pane] <= column && column < lefts[pane].saturating_add(width) { + continue; + } + let Some(y) = y.checked_sub(1).filter(|&y| y >= areas[pane].y) else { + continue; + }; + y + } else { + if !row.scope[side] { + continue; + } + y + }; + let shared = if overlay { + contexts[0].map(|scope| scope.label) == contexts[1].map(|scope| scope.label) + } else { + row.scope == [true; 2] + }; + let prefix = if width < 4 { + "" + } else if !unified { + "› " + } else if shared { + "± " + } else if side == 0 { + "− " + } else { + "+ " + }; + let prefix_width = UnicodeWidthStr::width(prefix); + let available = width.saturating_sub(prefix_width); + let label = if UnicodeWidthStr::width(scope.label) > available { + scope.label.rsplit(" › ").next().unwrap_or(scope.label) + } else { + scope.label + }; + let style = Style::default() + .fg(Color::DarkGray) + .add_modifier(Modifier::ITALIC); + let x = content.x + gutter as u16; + let buffer = frame.buffer_mut(); + for x in content.x..content.right() { + buffer[(x, y)].reset(); + } + buffer.set_stringn(x, y, prefix, width, style); + buffer.set_stringn(x + prefix_width as u16, y, label, available, style); + if caret.is_some_and(|(_, position)| { + position.y == y && content.x <= position.x && position.x < content.right() + }) { + *caret = None; + } + } + } +} + #[allow(clippy::too_many_arguments)] fn draw_diff_pane( frame: &mut Frame, @@ -4142,58 +4267,6 @@ fn draw_diff_pane( *left = diff.bound_horizontal(focus, *left, width); } let left = *left; - if let Some(rows) = wrapped { - for (screen, row) in rows.iter().take(usize::from(content.height)).enumerate() { - if row.scope == [false; 2] { - continue; - } - let side = if unified { - usize::from(!row.scope[0]) - } else { - usize::from(!before) - }; - if !row.scope[side] { - continue; - } - let source_focus = if side == 0 { - Focus::Before - } else { - Focus::After - }; - let Some(scope) = diff - .display_source_row(source_focus, row.position.display) - .and_then(|source| syntax[side].scope_at(source)) - else { - continue; - }; - let prefix = if width < 4 { - "" - } else if !unified { - "› " - } else if row.scope == [true; 2] { - "± " - } else if side == 0 { - "− " - } else { - "+ " - }; - let prefix_width = UnicodeWidthStr::width(prefix); - let available = width.saturating_sub(prefix_width); - let label = if UnicodeWidthStr::width(scope.label) > available { - scope.label.rsplit(" › ").next().unwrap_or(scope.label) - } else { - scope.label - }; - let style = Style::default() - .fg(Color::DarkGray) - .add_modifier(Modifier::ITALIC); - let x = content.x + gutter as u16; - let y = content.y + screen as u16; - let buffer = frame.buffer_mut(); - buffer.set_stringn(x, y, prefix, width, style); - buffer.set_stringn(x + prefix_width as u16, y, label, available, style); - } - } for (screen, display_row, segments) in diff_screen_rows(diff, top, content.height, wrapped) { let row = &diff.display_rows[display_row]; let row_area = Rect::new(content.x, content.y + screen as u16, content.width, 1); @@ -4463,7 +4536,6 @@ struct DiffWrapViewport { before: crate::wrap::WrapCache, after: crate::wrap::WrapCache, widths: [usize; 2], - lefts: [usize; 2], heights: [usize; 2], visible_names: [Vec; 2], top: DiffWrapPosition, @@ -4628,13 +4700,13 @@ impl DiffWrapViewport { { return false; } - let (left, right) = if row.unwrapped { - (self.lefts[side], self.lefts[side] + self.widths[side]) - } else if let Some(segment) = row.segments[side] { - (segment.start, segment.end) - } else { + if row.unwrapped { + return true; + } + let Some(segment) = row.segments[side] else { return false; }; + let (left, right) = (segment.start, segment.end); left <= column && column < right }) } @@ -8189,6 +8261,87 @@ mod tests { assert_eq!(viewport.rows[0].scope, [false, true]); } + #[test] + fn diff_scope_horizontal_scroll_keeps_source_rows_fixed() { + let before = + "keep_previous_code();\nfn review() {\n old_call_with_a_long_argument_name();\n}\n"; + let after = before.replace("old_call", "new_call"); + for (mode, axis, focus) in [ + (ViewMode::Split, SplitAxis::Vertical, Focus::After), + (ViewMode::Split, SplitAxis::Horizontal, Focus::After), + (ViewMode::Unified, SplitAxis::Vertical, Focus::Before), + (ViewMode::Unified, SplitAxis::Vertical, Focus::After), + ] { + let mut file = syntax_diff_file("scope.rs", before, &after); + file.hunks = crate::diff::line_hunks(&file.before, false, &file.after, false); + let mut diff = diff_comparison(vec![file]); + diff.files_visible = false; + diff.split_axis = axis; + diff.set_view_mode(mode); + diff.focus = focus; + let declaration = diff.source_display_row(Focus::After, 1).unwrap(); + let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(120, 30)).unwrap(); + let mut panels = WheelPanels::default(); + // A declaration at the top uses the border rather than displacing its first row. + for top in [0, declaration] { + diff.top = top; + let mut original = None; + for lefts in [[0, 0], [8, 0], [0, 8], [8, 8], [0, 0]] { + [diff.left_before, diff.left_after] = lefts; + render_diff(&mut terminal, &diff, &mut panels); + let viewport = panels.diff_wrap.as_ref().unwrap(); + let source_rows = viewport + .rows + .iter() + .enumerate() + .filter(|(_, row)| row.scope == [false; 2]) + .map(|(screen, row)| (screen, row.position)) + .collect::>(); + if let Some(original) = &original { + assert_eq!( + &source_rows, original, + "Horizontal scrolling must not move source rows: {mode:?}, {axis:?}" + ); + } else { + original = Some(source_rows); + } + let screen = viewport + .screen_row(DiffWrapPosition { + display: declaration, + continuation: 0, + }) + .unwrap(); + for (pane, left) in lefts.into_iter().enumerate() { + if mode == ViewMode::Unified && (focus == Focus::Before) != (pane == 0) { + continue; + } + let area = diff_inner(panels.diff.as_ref().unwrap().regions[pane + 1].0); + let above = pane_screen( + &terminal, + Rect::new(area.x, area.y + screen as u16 - 1, area.width, 1), + ); + assert_eq!( + above.contains("review"), + left > 0, + "Clipped names belong above the declaration, without adding a row" + ); + if left > 0 { + assert_eq!( + above.trim(), + if mode == ViewMode::Unified { + "± review" + } else { + "› review" + }, + "The overlay must clear source text and its line number" + ); + } + } + } + } + } + } + #[test] fn diff_scope_labels_follow_declaration_visibility() { let before = "@decorate()\nclass PolicyActiveStatusEnum(BaseEnum):\n ACTIVE = 1\n EXPIRED = 2\n FUTURE = 3\n UNKNOWN = 4\n";