From 380f3ea368f1449bfa3a31bf88651c726a348c73 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 18:42:45 +0200 Subject: [PATCH 1/7] feat(diff): fold a file away from its header row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The @@ line went with it. Both gutters already carry the numbers it repeated, so a hunk boundary is now an untexted muted band, between hunks and never before the first, where the file header already parts them. It yields no selection run and no text to a copy. Collapsing is applied where the rows are derived, not where they are painted: a collapsed file does not emit its body, so the element windows, paints and selects over a shorter list and needs to know only how to map a click to a row. The set of collapsed files is keyed by index into the patch rather than by display path, because a file with neither an old nor a new path renders as the empty string and two of them would collapse together; it is cleared whenever the detail changes, so an index never outlives what it indexes. Toggling clears the window selection for the reason set_detail does — gpui-base anchors an endpoint to a pixel offset, which after a collapse resolves onto rows the reader never dragged across, and Cmd-C would copy them. It keeps the scroll offset, pulling it up only as far as the shortened content allows, because the element chooses its rows from that offset one phase before the scrolling container gets to clamp it, and an unclamped one leaves it laying out nothing at all. --- crates/ui/src/detail/diff/body.rs | 160 ++++++++++++++++---- crates/ui/src/detail/diff/mod.rs | 28 +++- crates/ui/src/detail/diff/model.rs | 226 +++++++++++++++++++++++------ crates/ui/src/detail/diff/split.rs | 135 +++++++++++++---- crates/ui/src/detail/format.rs | 38 +---- crates/ui/src/detail/mod.rs | 72 +++++++-- 6 files changed, 498 insertions(+), 161 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index fd44881..d6a8350 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -3,19 +3,19 @@ use std::rc::Rc; use domain::LineOrigin; use gpui::{ - App, Bounds, Element, ElementId, FlexDirection, GlobalElementId, Half as _, HighlightStyle, - Hitbox, HitboxBehavior, Hsla, InspectorElementId, IntoElement, LayoutId, Length, Pixels, Point, - ScrollHandle, ShapedLine, SharedString, Style, StyledText, TextAlign, TextLayout, TextStyle, - Window, fill, point, px, relative, size, + App, Bounds, DispatchPhase, Element, ElementId, FlexDirection, GlobalElementId, Half as _, + HighlightStyle, Hitbox, HitboxBehavior, Hsla, InspectorElementId, IntoElement, LayoutId, + Length, MouseButton, MouseDownEvent, Pixels, Point, ScrollHandle, ShapedLine, SharedString, + Style, StyledText, TextAlign, TextLayout, TextStyle, Window, fill, point, px, relative, size, }; use gpui_base::{TextSelectionHandle, TextSelectionRegistration, TextSelectionRun}; use gpui_component::{ThemeColor, ThemeMode}; -use super::DiffContent; use super::model::{Row, header_line}; use super::pairing::SideLine; use super::palette::line_colors; use super::split::SplitRow; +use super::{DiffContent, ToggleFile}; pub(super) const ROW_HEIGHT: f32 = 18.; @@ -35,7 +35,7 @@ pub(super) enum Rows { } impl Rows { - fn len(&self) -> usize { + pub(super) fn len(&self) -> usize { match self { Rows::Unified(rows) => rows.len(), Rows::Split(rows) => rows.len(), @@ -64,6 +64,20 @@ impl Rows { self.len() * self.columns() } + fn file_at(&self, row: usize) -> Option { + let row = match self { + Rows::Unified(rows) => rows.get(row)?, + Rows::Split(rows) => match rows.get(row)? { + SplitRow::Full(full) => full, + SplitRow::Sides { .. } => return None, + }, + }; + match row { + Row::FileHeader { file, .. } => Some(*file), + _ => None, + } + } + fn side(&self, row: usize, column: usize) -> Option<&SideLine> { let Rows::Split(rows) = self else { return None; @@ -90,6 +104,7 @@ pub(super) struct DiffBody { select_all: bool, selection: TextSelectionHandle, scroll: ScrollHandle, + toggle_file: ToggleFile, theme: ThemeColor, mode: ThemeMode, visible: Range, @@ -102,6 +117,7 @@ pub(super) fn body( select_all: bool, selection: TextSelectionHandle, scroll: ScrollHandle, + toggle_file: ToggleFile, theme: ThemeColor, mode: ThemeMode, ) -> DiffBody { @@ -110,6 +126,7 @@ pub(super) fn body( select_all, selection, scroll, + toggle_file, theme, mode, visible: 0..0, @@ -143,8 +160,10 @@ fn row_text(row: &Row) -> SharedString { status, added, deleted, - } => header_line(path, status, *added, *deleted).into(), - Row::HunkHeader { text } => text.clone().into(), + collapsed, + .. + } => header_line(path, status, *added, *deleted, *collapsed).into(), + Row::Separator => SharedString::default(), Row::Line { content, .. } => content.clone().into(), Row::Placeholder { message } => (*message).into(), } @@ -173,14 +192,14 @@ fn cell_foreground(rows: &Rows, cell: usize, theme: &ThemeColor) -> Hsla { fn row_foreground(row: &Row, theme: &ThemeColor) -> Hsla { match row { Row::FileHeader { .. } | Row::Line { .. } => theme.foreground, - Row::HunkHeader { .. } | Row::Placeholder { .. } => theme.muted_foreground, + Row::Separator | Row::Placeholder { .. } => theme.muted_foreground, } } fn row_background(row: &Row, theme: &ThemeColor, mode: ThemeMode) -> Option { match row { Row::FileHeader { .. } => Some(theme.secondary), - Row::HunkHeader { .. } => Some(theme.muted), + Row::Separator => Some(theme.muted), Row::Placeholder { .. } => None, Row::Line { origin, .. } => line_colors(*origin, mode, theme).background, } @@ -259,6 +278,11 @@ fn row_window(offset_y: Pixels, viewport: Pixels, rows: usize) -> Range { first.min(rows)..first.saturating_add(count).min(rows) } +fn row_at(origin_y: Pixels, y: Pixels, rows: usize) -> Option { + let row = ((y - origin_y) / px(ROW_HEIGHT)).floor(); + (row >= 0. && row < rows as f32).then_some(row as usize) +} + fn selected_rows( origin_y: Pixels, top: Pixels, @@ -656,6 +680,28 @@ impl DiffBody { let line = pen.shape(marker(origin).into(), marker_color, window); paint_line(&line, point(marker_left, top), window, cx); } + + fn on_mouse_down(&self, bounds: Bounds, hitbox: &Hitbox, window: &mut Window) { + let hitbox = hitbox.clone(); + let content = Rc::clone(&self.content); + let toggle_file = Rc::clone(&self.toggle_file); + let origin_y = bounds.origin.y; + window.on_mouse_event(move |event: &MouseDownEvent, phase, window, cx| { + if phase != DispatchPhase::Bubble + || event.button != MouseButton::Left + || event.click_count != 1 + || !hitbox.is_hovered(window) + { + return; + } + let Some(row) = row_at(origin_y, event.position.y, content.rows.len()) else { + return; + }; + if let Some(file) = content.rows.file_at(row) { + toggle_file(&file, window, cx); + } + }); + } } impl IntoElement for DiffBody { @@ -755,10 +801,12 @@ impl Element for DiffBody { _: Option<&InspectorElementId>, bounds: Bounds, _: &mut Self::RequestLayoutState, - _: &mut Self::PrepaintState, + hitbox: &mut Self::PrepaintState, window: &mut Window, cx: &mut App, ) { + self.on_mouse_down(bounds, hitbox, window); + let first_cell = self.visible_cells().start; let runs: Vec = self .texts @@ -824,16 +872,12 @@ mod tests { fn file_header() -> Row { Row::FileHeader { + file: 0, path: "src/main.rs".to_string(), status: FileStatus::Modified, added: 3, deleted: 1, - } - } - - fn hunk_header() -> Row { - Row::HunkHeader { - text: "@@ -1,3 +1,4 @@".to_string(), + collapsed: false, } } @@ -856,7 +900,7 @@ mod tests { fn split_rows() -> Rows { Rows::Split(vec![ - SplitRow::Full(hunk_header()), + SplitRow::Full(file_header()), SplitRow::Sides { left: Some(side(LineOrigin::Deletion, "gone")), right: None, @@ -903,15 +947,32 @@ mod tests { } #[test] - fn a_row_renders_its_own_kind_of_text() { + fn only_a_file_header_row_names_a_file() { + let unified = Rows::Unified(vec![ + file_header(), + Row::Separator, + line(LineOrigin::Context, "keep"), + ]); + + assert_eq!(unified.file_at(0), Some(0)); + assert_eq!(unified.file_at(1), None); + assert_eq!(unified.file_at(2), None); + assert_eq!(unified.file_at(3), None, "there is no fourth row"); + assert_eq!(split_rows().file_at(0), Some(0)); assert_eq!( - row_text(&file_header()), - SharedString::from("src/main.rs +3 \u{2212}1") + split_rows().file_at(1), + None, + "a two-sided row is never a header" ); + } + + #[test] + fn a_row_renders_its_own_kind_of_text() { assert_eq!( - row_text(&hunk_header()), - SharedString::from("@@ -1,3 +1,4 @@") + row_text(&file_header()), + SharedString::from("\u{25be} src/main.rs +3 \u{2212}1") ); + assert_eq!(row_text(&Row::Separator), SharedString::default()); assert_eq!( row_text(&line(LineOrigin::Addition, "let x = 1;")), SharedString::from("let x = 1;") @@ -929,7 +990,7 @@ mod tests { let rows = Rows::Unified(vec![file_header(), line(LineOrigin::Deletion, "gone")]); assert_eq!( cell_text(&rows, 0), - SharedString::from("src/main.rs +3 \u{2212}1") + SharedString::from("\u{25be} src/main.rs +3 \u{2212}1") ); assert_eq!(cell_text(&rows, 1), SharedString::from("gone")); } @@ -937,10 +998,40 @@ mod tests { #[test] fn a_full_width_split_row_renders_in_the_first_column_and_blank_in_the_second() { let rows = split_rows(); - assert_eq!(cell_text(&rows, 0), SharedString::from("@@ -1,3 +1,4 @@")); + assert_eq!( + cell_text(&rows, 0), + SharedString::from("\u{25be} src/main.rs +3 \u{2212}1") + ); assert_eq!(cell_text(&rows, 1), SharedString::default()); } + #[test] + fn a_separator_carries_no_text_in_either_view() { + let unified = Rows::Unified(vec![Row::Separator]); + let split = Rows::Split(vec![SplitRow::Full(Row::Separator)]); + + assert_eq!(cell_text(&unified, 0), SharedString::default()); + assert_eq!(cell_text(&split, 0), SharedString::default()); + assert_eq!(cell_text(&split, 1), SharedString::default()); + } + + #[test] + fn a_collapsed_header_turns_its_disclosure_marker_sideways() { + let collapsed = Row::FileHeader { + file: 0, + path: "src/main.rs".to_string(), + status: FileStatus::Modified, + added: 3, + deleted: 1, + collapsed: true, + }; + + assert_eq!( + row_text(&collapsed), + SharedString::from("\u{25b8} src/main.rs +3 \u{2212}1") + ); + } + #[test] fn a_split_row_puts_each_side_in_its_own_column_and_pads_the_missing_one() { let rows = split_rows(); @@ -956,7 +1047,7 @@ mod tests { } #[test] - fn only_a_changed_line_and_the_two_headers_are_banded() { + fn only_a_changed_line_the_file_header_and_a_separator_are_banded() { let theme = ThemeColor::light(); let mode = ThemeMode::Light; @@ -965,7 +1056,7 @@ mod tests { Some(theme.secondary) ); assert_eq!( - row_background(&hunk_header(), &theme, mode), + row_background(&Row::Separator, &theme, mode), Some(theme.muted) ); assert_eq!( @@ -1144,6 +1235,21 @@ mod tests { ); } + #[test] + fn a_click_lands_on_the_row_its_height_puts_it_in() { + assert_eq!(row_at(px(100.), px(100.), 3), Some(0)); + assert_eq!(row_at(px(100.), px(100. + ROW_HEIGHT - 1.), 3), Some(0)); + assert_eq!(row_at(px(100.), px(100. + ROW_HEIGHT), 3), Some(1)); + assert_eq!(row_at(px(100.), px(100. + 2.5 * ROW_HEIGHT), 3), Some(2)); + } + + #[test] + fn a_click_outside_the_rows_lands_on_none_of_them() { + assert_eq!(row_at(px(100.), px(99.), 3), None); + assert_eq!(row_at(px(100.), px(100. + 3. * ROW_HEIGHT), 3), None); + assert_eq!(row_at(px(100.), px(100.), 0), None); + } + #[test] fn a_selection_spans_the_rows_its_two_endpoints_land_in() { assert_eq!( diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index a870240..8a2636a 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -13,7 +13,7 @@ //! A row carries one cell in [`DiffViewMode::Unified`] and two in [`DiffViewMode::Split`], //! which is the only difference between the two views: [`body::Rows`] answers how many //! columns a body has, and every horizontal position — the content width, a cell's bounds, -//! a gutter's origin — is that column's share of the element's width. A file, hunk or +//! a gutter's origin — is that column's share of the element's width. A file, separator or //! placeholder row keeps one full-width cell in either view, so the columns stay uniform //! and a cell is `row * columns + column` rather than a lookup. Because a column is laid //! out against `content_width / columns` and the width is measured over every cell, a @@ -70,7 +70,10 @@ //! natural width — which is what keeps a row one line tall and `ROW_HEIGHT` true. That is //! affordable only because the text it measures is not rebuilt with it: [`DiffContent`] //! holds the rows and one [`gpui::SharedString`] per cell, derived by [`content`] when the -//! patch or the view mode changes and handed to the element behind an `Rc` thereafter. So +//! patch, the view mode or the set of collapsed files changes and handed to the element +//! behind an `Rc` thereafter. Collapsing is applied in that derivation — the body rows of a +//! collapsed file are never emitted — so the element windows, paints and selects over a +//! shorter list and knows nothing of collapsing beyond mapping a click to a row. So //! after the first frame each cell is a hit in gpui's line-layout cache and the steady-state //! cost is a hash of bytes that are already there, with no allocation and no reshape. //! @@ -102,12 +105,13 @@ mod pairing; mod palette; mod split; +use std::collections::HashSet; use std::rc::Rc; use domain::Patch; use gpui::{ - AnyElement, App, InteractiveElement as _, IntoElement, ParentElement as _, ScrollHandle, - SharedString, StatefulInteractiveElement as _, Styled as _, div, px, + AnyElement, App, InteractiveElement as _, IntoElement, ParentElement as _, Pixels, + ScrollHandle, SharedString, StatefulInteractiveElement as _, Styled as _, Window, div, px, }; use gpui_base::TextSelectionHandle; use gpui_component::{ @@ -121,6 +125,10 @@ use body::{ROW_HEIGHT, Rows, body, cell_strings}; use model::rows; use split::split_rows; +pub(super) type Collapsed = HashSet; + +pub(super) type ToggleFile = Rc; + pub(super) struct DiffContent { rows: Rows, strings: Vec, @@ -130,12 +138,16 @@ impl DiffContent { pub(super) fn is_empty(&self) -> bool { self.rows.is_empty() } + + pub(super) fn height(&self) -> Pixels { + px(self.rows.len() as f32 * ROW_HEIGHT) + } } -pub(super) fn content(patch: &Patch, mode: DiffViewMode) -> DiffContent { +pub(super) fn content(patch: &Patch, mode: DiffViewMode, collapsed: &Collapsed) -> DiffContent { let rows = match mode { - DiffViewMode::Unified => Rows::Unified(rows(patch)), - DiffViewMode::Split => Rows::Split(split_rows(patch)), + DiffViewMode::Unified => Rows::Unified(rows(patch, collapsed)), + DiffViewMode::Split => Rows::Split(split_rows(patch, collapsed)), }; let strings = cell_strings(&rows); DiffContent { rows, strings } @@ -146,6 +158,7 @@ pub(super) fn render( select_all: bool, selection: &TextSelectionHandle, scroll: &ScrollHandle, + toggle_file: ToggleFile, cx: &App, ) -> AnyElement { let Some(content) = content.filter(|content| !content.is_empty()) else { @@ -178,6 +191,7 @@ pub(super) fn render( select_all, selection.clone(), scroll.clone(), + toggle_file, theme.colors, theme.mode, )), diff --git a/crates/ui/src/detail/diff/model.rs b/crates/ui/src/detail/diff/model.rs index f691e5b..c5386e2 100644 --- a/crates/ui/src/detail/diff/model.rs +++ b/crates/ui/src/detail/diff/model.rs @@ -1,20 +1,23 @@ use std::fmt::Write as _; -use domain::{DiffLine, FilePatch, FileStatus, Hunk, LineOrigin, Patch}; +use domain::{DiffLine, FilePatch, FileStatus, LineOrigin, Patch}; -use crate::detail::format; +use super::Collapsed; + +const EXPANDED_MARKER: &str = "\u{25be}"; +const COLLAPSED_MARKER: &str = "\u{25b8}"; #[derive(Clone, Debug, PartialEq, Eq)] pub(super) enum Row { FileHeader { + file: usize, path: String, status: FileStatus, added: usize, deleted: usize, + collapsed: bool, }, - HunkHeader { - text: String, - }, + Separator, Line { origin: LineOrigin, old_number: Option, @@ -26,26 +29,42 @@ pub(super) enum Row { }, } -pub(super) fn rows(patch: &Patch) -> Vec { +pub(super) fn rows(patch: &Patch, collapsed: &Collapsed) -> Vec { let mut rows = Vec::new(); - for file in &patch.files { - rows.push(file_header(file)); - push_body(&mut rows, file); + for (file, patch) in patch.files.iter().enumerate() { + rows.push(file_header(file, patch, collapsed.contains(&file))); + if collapsed.contains(&file) { + continue; + } + push_body(&mut rows, patch); } rows } -pub(super) fn file_header(file: &FilePatch) -> Row { +pub(super) fn file_header(file: usize, patch: &FilePatch, collapsed: bool) -> Row { Row::FileHeader { - path: header_path(file), - status: file.status.clone(), - added: file.added_lines(), - deleted: file.deleted_lines(), + file, + path: header_path(patch), + status: patch.status.clone(), + added: patch.added_lines(), + deleted: patch.deleted_lines(), + collapsed, } } -pub(super) fn header_line(path: &str, status: &FileStatus, added: usize, deleted: usize) -> String { - let mut text = path.to_string(); +pub(super) fn header_line( + path: &str, + status: &FileStatus, + added: usize, + deleted: usize, + collapsed: bool, +) -> String { + let marker = if collapsed { + COLLAPSED_MARKER + } else { + EXPANDED_MARKER + }; + let mut text = format!("{marker} {path}"); if let Some(label) = status_label(status) { let _ = write!(text, " {label}"); } @@ -94,19 +113,15 @@ pub(super) fn placeholder(file: &FilePatch) -> Option { None } -pub(super) fn hunk_header(hunk: &Hunk) -> Row { - Row::HunkHeader { - text: format::hunk_heading(hunk), - } -} - fn push_body(rows: &mut Vec, file: &FilePatch) { if let Some(placeholder) = placeholder(file) { rows.push(placeholder); return; } - for hunk in &file.hunks { - rows.push(hunk_header(hunk)); + for (position, hunk) in file.hunks.iter().enumerate() { + if position > 0 { + rows.push(Row::Separator); + } rows.extend(hunk.lines.iter().map(line_row)); } } @@ -166,8 +181,20 @@ mod tests { } } + fn expanded() -> Collapsed { + Collapsed::new() + } + + fn collapsed(files: impl IntoIterator) -> Collapsed { + files.into_iter().collect() + } + + fn one_line() -> Hunk { + hunk(vec![line(LineOrigin::Addition, None, Some(1), "new")]) + } + #[test] - fn a_modified_file_yields_a_header_a_hunk_header_and_one_row_per_line() { + fn a_modified_file_yields_a_header_and_one_row_per_line() { let patch = Patch { files: vec![file( vec![hunk(vec![ @@ -179,11 +206,11 @@ mod tests { )], }; - let rows = rows(&patch); + let rows = rows(&patch, &expanded()); assert!(matches!(rows[0], Row::FileHeader { .. })); - assert!(matches!(rows[1], Row::HunkHeader { .. })); - assert_eq!(rows.len(), 5); + assert!(matches!(rows[1], Row::Line { .. })); + assert_eq!(rows.len(), 4); } #[test] @@ -200,10 +227,10 @@ mod tests { )], }; - let rows = rows(&patch); + let rows = rows(&patch, &expanded()); assert_eq!( - rows[2], + rows[1], Row::Line { origin: LineOrigin::Deletion, old_number: Some(7), @@ -219,7 +246,7 @@ mod tests { let patch = Patch { files: vec![file(Vec::new(), true)], }; - let rows = rows(&patch); + let rows = rows(&patch, &expanded()); assert_eq!( rows[1], Row::Placeholder { @@ -233,7 +260,7 @@ mod tests { let patch = Patch { files: vec![file(Vec::new(), false)], }; - let rows = rows(&patch); + let rows = rows(&patch, &expanded()); assert_eq!( rows[1], Row::Placeholder { @@ -254,7 +281,8 @@ mod tests { status, added, deleted, - } = file_header(&file) + .. + } = file_header(0, &file, false) else { panic!("a file yields a header row"); }; @@ -262,8 +290,8 @@ mod tests { assert_eq!(path, "src/old.rs \u{2192} src/new.rs"); assert_eq!(status, FileStatus::Renamed { similarity: 87 }); assert_eq!( - header_line(&path, &status, added, deleted), - "src/old.rs \u{2192} src/new.rs renamed 87% +0 \u{2212}0" + header_line(&path, &status, added, deleted, false), + "\u{25be} src/old.rs \u{2192} src/new.rs renamed 87% +0 \u{2212}0" ); } @@ -279,15 +307,16 @@ mod tests { status, added, deleted, - } = file_header(&file) + .. + } = file_header(0, &file, false) else { panic!("a file yields a header row"); }; assert_eq!(path, "src/old.rs \u{2192} src/copy.rs"); assert_eq!( - header_line(&path, &status, added, deleted), - "src/old.rs \u{2192} src/copy.rs copied 100% +0 \u{2212}0" + header_line(&path, &status, added, deleted, false), + "\u{25be} src/old.rs \u{2192} src/copy.rs copied 100% +0 \u{2212}0" ); } @@ -298,7 +327,7 @@ mod tests { "src/a.rs", FileStatus::Renamed { similarity: 100 }, ); - let Row::FileHeader { path, .. } = file_header(&file) else { + let Row::FileHeader { path, .. } = file_header(0, &file, false) else { panic!("a file yields a header row"); }; assert_eq!(path, "src/a.rs"); @@ -307,24 +336,22 @@ mod tests { #[test] fn a_modified_file_carries_no_status_label() { let patch = Patch { - files: vec![file( - vec![hunk(vec![line(LineOrigin::Addition, None, Some(1), "new")])], - false, - )], + files: vec![file(vec![one_line()], false)], }; let Row::FileHeader { path, status, added, deleted, - } = rows(&patch).remove(0) + .. + } = rows(&patch, &expanded()).remove(0) else { panic!("the first row is the file header"); }; assert_eq!( - header_line(&path, &status, added, deleted), - "src/main.rs +1 \u{2212}0" + header_line(&path, &status, added, deleted, false), + "\u{25be} src/main.rs +1 \u{2212}0" ); } @@ -333,10 +360,115 @@ mod tests { let patch = Patch { files: vec![file(Vec::new(), true), file(Vec::new(), true)], }; - let headers = rows(&patch) + let headers = rows(&patch, &expanded()) .iter() .filter(|r| matches!(r, Row::FileHeader { .. })) .count(); assert_eq!(headers, 2); } + + #[test] + fn a_separator_stands_between_two_hunks_and_never_ahead_of_the_first() { + let patch = Patch { + files: vec![file(vec![one_line(), one_line()], false)], + }; + + let rows = rows(&patch, &expanded()); + + assert!(matches!(rows[0], Row::FileHeader { .. })); + assert!(matches!(rows[1], Row::Line { .. })); + assert_eq!(rows[2], Row::Separator); + assert!(matches!(rows[3], Row::Line { .. })); + assert_eq!(rows.len(), 4); + } + + #[test] + fn a_collapsed_file_emits_its_header_and_nothing_else() { + let patch = Patch { + files: vec![file(vec![one_line(), one_line()], false)], + }; + + let rows = rows(&patch, &collapsed([0])); + + assert_eq!(rows.len(), 1); + assert!(matches!( + rows[0], + Row::FileHeader { + collapsed: true, + .. + } + )); + assert!(!rows.contains(&Row::Separator)); + } + + #[test] + fn a_collapsed_binary_file_drops_its_placeholder_too() { + let patch = Patch { + files: vec![file(Vec::new(), true)], + }; + assert_eq!(rows(&patch, &collapsed([0])).len(), 1); + } + + #[test] + fn collapsing_one_file_leaves_the_others_whole() { + let patch = Patch { + files: vec![ + file(vec![one_line()], false), + file(vec![one_line()], false), + file(vec![one_line()], false), + ], + }; + + let rows = rows(&patch, &collapsed([1])); + + assert_eq!(rows.len(), 5); + assert!(matches!( + rows[0], + Row::FileHeader { + file: 0, + collapsed: false, + .. + } + )); + assert!(matches!(rows[1], Row::Line { .. })); + assert!(matches!( + rows[2], + Row::FileHeader { + file: 1, + collapsed: true, + .. + } + )); + assert!(matches!( + rows[3], + Row::FileHeader { + file: 2, + collapsed: false, + .. + } + )); + assert!(matches!(rows[4], Row::Line { .. })); + } + + #[test] + fn a_collapsed_header_turns_its_disclosure_marker_sideways() { + let file = file(vec![one_line()], false); + let Row::FileHeader { + path, + status, + added, + deleted, + collapsed, + .. + } = file_header(0, &file, true) + else { + panic!("a file yields a header row"); + }; + + assert!(collapsed); + assert_eq!( + header_line(&path, &status, added, deleted, collapsed), + "\u{25b8} src/main.rs +1 \u{2212}0" + ); + } } diff --git a/crates/ui/src/detail/diff/split.rs b/crates/ui/src/detail/diff/split.rs index f711c83..9ed5764 100644 --- a/crates/ui/src/detail/diff/split.rs +++ b/crates/ui/src/detail/diff/split.rs @@ -1,6 +1,7 @@ use domain::Patch; -use super::model::{Row, file_header, hunk_header, placeholder}; +use super::Collapsed; +use super::model::{Row, file_header, placeholder}; use super::pairing::{SideLine, pair}; #[derive(Clone, Debug, PartialEq, Eq)] @@ -12,16 +13,25 @@ pub(super) enum SplitRow { }, } -pub(super) fn split_rows(patch: &Patch) -> Vec { +pub(super) fn split_rows(patch: &Patch, collapsed: &Collapsed) -> Vec { let mut rows = Vec::new(); - for file in &patch.files { - rows.push(SplitRow::Full(file_header(file))); - if let Some(placeholder) = placeholder(file) { + for (file, patch) in patch.files.iter().enumerate() { + rows.push(SplitRow::Full(file_header( + file, + patch, + collapsed.contains(&file), + ))); + if collapsed.contains(&file) { + continue; + } + if let Some(placeholder) = placeholder(patch) { rows.push(SplitRow::Full(placeholder)); continue; } - for hunk in &file.hunks { - rows.push(SplitRow::Full(hunk_header(hunk))); + for (position, hunk) in patch.hunks.iter().enumerate() { + if position > 0 { + rows.push(SplitRow::Full(Row::Separator)); + } rows.extend(pair(&hunk.lines).into_iter().map(|row| SplitRow::Sides { left: row.left, right: row.right, @@ -93,6 +103,14 @@ mod tests { .collect() } + fn expanded() -> Collapsed { + Collapsed::new() + } + + fn collapsed(files: impl IntoIterator) -> Collapsed { + files.into_iter().collect() + } + #[test] fn a_two_file_patch_yields_both_files_in_order_each_behind_its_own_header() { let patch = Patch { @@ -102,12 +120,11 @@ mod tests { ], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); assert_eq!(paths(&rows), vec!["src/a.rs", "src/b.rs"]); - assert!(matches!(rows[1], SplitRow::Full(Row::HunkHeader { .. }))); - assert!(matches!(rows[2], SplitRow::Sides { .. })); - assert_eq!(rows.len(), 6); + assert!(matches!(rows[1], SplitRow::Sides { .. })); + assert_eq!(rows.len(), 4); } #[test] @@ -116,10 +133,10 @@ mod tests { files: vec![file("src/a.rs", vec![replacement()], false)], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); - let SplitRow::Sides { left, right } = &rows[2] else { - panic!("the third row is the paired line"); + let SplitRow::Sides { left, right } = &rows[1] else { + panic!("the second row is the paired line"); }; assert_eq!( left.as_ref().map(|side| side.content.as_str()), @@ -141,10 +158,10 @@ mod tests { )], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); - let SplitRow::Sides { left, right } = &rows[2] else { - panic!("the third row is the added line"); + let SplitRow::Sides { left, right } = &rows[1] else { + panic!("the second row is the added line"); }; assert!(left.is_none()); assert!(right.is_some()); @@ -165,10 +182,10 @@ mod tests { )], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); - let SplitRow::Sides { left, right } = &rows[2] else { - panic!("the third row is the deleted line"); + let SplitRow::Sides { left, right } = &rows[1] else { + panic!("the second row is the deleted line"); }; assert!(left.is_some()); assert!(right.is_none()); @@ -180,7 +197,7 @@ mod tests { files: vec![file("src/a.png", Vec::new(), true)], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); assert_eq!( rows[1], @@ -197,7 +214,7 @@ mod tests { files: vec![file("src/a.rs", Vec::new(), false)], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); assert_eq!( rows[1], @@ -217,16 +234,18 @@ mod tests { )], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); assert_eq!(paths(&rows), vec!["src/old.rs \u{2192} src/new.rs"]); assert_eq!( rows[0], SplitRow::Full(Row::FileHeader { + file: 0, path: "src/old.rs \u{2192} src/new.rs".to_string(), status: FileStatus::Renamed { similarity: 87 }, added: 0, deleted: 0, + collapsed: false, }) ); assert_eq!( @@ -247,30 +266,86 @@ mod tests { )], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); assert_eq!( rows[0], SplitRow::Full(Row::FileHeader { + file: 0, path: "src/old.rs \u{2192} src/copy.rs".to_string(), status: FileStatus::Copied { similarity: 100 }, added: 0, deleted: 0, + collapsed: false, }) ); } #[test] - fn every_hunk_of_a_file_keeps_its_own_header() { + fn two_hunks_of_a_file_are_parted_by_a_single_separator() { + let patch = Patch { + files: vec![file("src/a.rs", vec![replacement(), replacement()], false)], + }; + + let rows = split_rows(&patch, &expanded()); + + assert!(matches!(rows[0], SplitRow::Full(Row::FileHeader { .. }))); + assert!(matches!(rows[1], SplitRow::Sides { .. })); + assert_eq!(rows[2], SplitRow::Full(Row::Separator)); + assert!(matches!(rows[3], SplitRow::Sides { .. })); + assert_eq!(rows.len(), 4); + } + + #[test] + fn a_collapsed_file_emits_its_header_and_nothing_else() { let patch = Patch { files: vec![file("src/a.rs", vec![replacement(), replacement()], false)], }; - let headers = split_rows(&patch) - .iter() - .filter(|row| matches!(row, SplitRow::Full(Row::HunkHeader { .. }))) - .count(); + let rows = split_rows(&patch, &collapsed([0])); + + assert_eq!(rows.len(), 1); + assert!(matches!( + rows[0], + SplitRow::Full(Row::FileHeader { + collapsed: true, + .. + }) + )); + assert!(!rows.contains(&SplitRow::Full(Row::Separator))); + } + + #[test] + fn collapsing_one_file_leaves_the_others_whole() { + let patch = Patch { + files: vec![ + file("src/a.rs", vec![replacement()], false), + file("src/b.rs", vec![replacement()], false), + file("src/c.rs", vec![replacement()], false), + ], + }; + + let rows = split_rows(&patch, &collapsed([1])); + + assert_eq!(paths(&rows), vec!["src/a.rs", "src/b.rs", "src/c.rs"]); + assert_eq!(rows.len(), 5); + assert!(matches!(rows[1], SplitRow::Sides { .. })); + assert!(matches!( + rows[2], + SplitRow::Full(Row::FileHeader { + file: 1, + collapsed: true, + .. + }) + )); + assert!(matches!(rows[4], SplitRow::Sides { .. })); + } - assert_eq!(headers, 2); + #[test] + fn a_collapsed_binary_file_drops_its_placeholder_too() { + let patch = Patch { + files: vec![file("src/a.png", Vec::new(), true)], + }; + assert_eq!(split_rows(&patch, &collapsed([0])).len(), 1); } } diff --git a/crates/ui/src/detail/format.rs b/crates/ui/src/detail/format.rs index 5888f11..857ba0d 100644 --- a/crates/ui/src/detail/format.rs +++ b/crates/ui/src/detail/format.rs @@ -3,7 +3,7 @@ //! Nothing here touches gpui's `App` or `Window`: every function is a plain transformation //! from domain types to strings, so it is testable without a running window. -use domain::{Hunk, ObjectId, Timestamp}; +use domain::{ObjectId, Timestamp}; /// Hexadecimal characters kept when an identifier is shown abbreviated, matching Git's /// own default abbreviation length. @@ -79,19 +79,6 @@ pub fn escape_markdown(text: &str) -> String { escaped } -/// A hunk's `@@ -old,len +new,len @@ heading` marker line. -pub fn hunk_heading(hunk: &Hunk) -> String { - let marker = format!( - "@@ -{},{} +{},{} @@", - hunk.old_start, hunk.old_lines, hunk.new_start, hunk.new_lines - ); - if hunk.heading.is_empty() { - marker - } else { - format!("{marker} {}", hunk.heading) - } -} - #[cfg(test)] mod tests { use super::*; @@ -100,17 +87,6 @@ mod tests { nibble.to_string().repeat(40).parse().unwrap() } - fn hunk() -> Hunk { - Hunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 2, - heading: "fn existing()".to_string(), - lines: vec![], - } - } - #[test] fn abbreviate_truncates_to_seven_hex_characters() { assert_eq!(abbreviate(id('a')), "aaaaaaa"); @@ -152,18 +128,6 @@ mod tests { assert_eq!(format_timestamp(timestamp), "31 December 1999 at 19:00:00"); } - #[test] - fn hunk_heading_includes_the_function_context_when_git_found_one() { - assert_eq!(hunk_heading(&hunk()), "@@ -1,1 +1,2 @@ fn existing()"); - } - - #[test] - fn hunk_heading_omits_the_trailing_space_when_git_found_no_context() { - let mut hunk = hunk(); - hunk.heading = String::new(); - assert_eq!(hunk_heading(&hunk), "@@ -1,1 +1,2 @@"); - } - #[test] fn escape_markdown_neutralises_ascii_punctuation_without_touching_letters_digits_or_spaces() { assert_eq!( diff --git a/crates/ui/src/detail/mod.rs b/crates/ui/src/detail/mod.rs index 067adce..f2c4ea5 100644 --- a/crates/ui/src/detail/mod.rs +++ b/crates/ui/src/detail/mod.rs @@ -21,10 +21,13 @@ //! string of every cell in them are derived from the patch, and deriving them per frame //! would copy every line of the patch two or three times over on every repaint — //! `refresh_window_on_change` repaints on each mouse move of a selection drag, so that is -//! not a rare frame. They are rebuilt by [`DetailPanel::set_detail`] and -//! [`DetailPanel::set_diff_view_mode`], which are the only two places the patch or the -//! view mode can change. [`DetailPanel::selected_tab`] is never touched by `set_detail`, -//! which is what lets picking a different commit leave the open tab alone. +//! not a rare frame. They are rebuilt by [`DetailPanel::set_detail`], +//! [`DetailPanel::set_diff_view_mode`] and [`DetailPanel::toggle_file`], the three places +//! the patch, the view mode or the set of collapsed files can change. +//! [`DetailPanel::selected_tab`] is never touched by `set_detail`, which is what lets +//! picking a different commit leave the open tab alone. The collapsed set is cleared by +//! `set_detail` rather than carried over: it names files by their index in the patch, and +//! another commit's patch holds other files at those indices. //! //! The diff view mode is read once from disk in [`DetailPanel::new`], before the first //! frame, and written back through `cx.background_executor()`: @@ -33,8 +36,12 @@ //! than inline. //! //! Both `set_detail` and a mode change route through `DetailPanel::reset_diff_view`, which -//! zeroes the diff's scroll offset and clears the window selection. Clearing is the reason -//! both take a `&mut Window`, which is why [`crate::workspace::Workspace`] threads one into +//! zeroes the diff's scroll offset and clears the window selection. Collapsing a file clears +//! the selection as well but keeps the scroll offset — the rows the reader was looking at are +//! still there — pulling it up only as far as the shortened content's last screenful, +//! because the element chooses the rows to lay out from that offset one phase before the +//! scrolling container gets to clamp it. Clearing is the reason all three take a +//! `&mut Window`, which is why [`crate::workspace::Workspace`] threads one into //! `sync_panels_from_repository`. It is not optional: `gpui-base` stores a selection //! endpoint relative to the participant's registered origin (`text_selection.rs:1336`), so it //! survives the content underneath it changing, and a stored `y` then resolves onto whatever row now sits @@ -42,8 +49,8 @@ //! copy. `TextSelection::clear` is window-wide rather than per-participant, and this panel //! has participants beyond the diff body: [`metadata`] renders every value through //! [`gpui_component::text::markdown`], and each `TextView` registers one of its own. They -//! are cleared too, which is the right outcome — they are selections over the commit that -//! is being replaced. +//! are cleared too, which is the right outcome — they are selections over content this panel +//! is replacing. mod diff; mod format; @@ -73,7 +80,7 @@ use crate::diff_view_mode::DiffViewMode; use crate::persistence; use crate::repository::{CommitDetail, LoadState}; -use diff::DiffContent; +use diff::{Collapsed, DiffContent, ToggleFile}; #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] enum DetailTab { @@ -104,6 +111,7 @@ impl DetailTab { pub struct DetailPanel { detail: LoadState>, diff_content: Option>, + diff_collapsed: Collapsed, diff_selection: TextSelectionHandle, diff_select_all: bool, diff_auto_scroll: AutoScroll, @@ -145,6 +153,7 @@ impl DetailPanel { Self { detail: LoadState::Idle, diff_content: None, + diff_collapsed: Collapsed::new(), diff_selection, diff_select_all: false, diff_auto_scroll: AutoScroll::default(), @@ -163,11 +172,22 @@ impl DetailPanel { cx: &mut Context, ) { self.detail = detail; + self.diff_collapsed.clear(); self.rebuild_diff_content(); self.reset_diff_view(window, cx); cx.notify(); } + fn toggle_file(&mut self, file: usize, window: &mut Window, cx: &mut Context) { + if !self.diff_collapsed.remove(&file) { + self.diff_collapsed.insert(file); + } + self.rebuild_diff_content(); + self.clear_diff_selection(window, cx); + self.clamp_diff_scroll(); + cx.notify(); + } + fn set_diff_view_mode( &mut self, mode: DiffViewMode, @@ -194,20 +214,38 @@ impl DetailPanel { fn rebuild_diff_content(&mut self) { self.diff_content = match &self.detail { - LoadState::Ready(detail) => { - Some(Rc::new(diff::content(&detail.patch, self.diff_view_mode))) - } + LoadState::Ready(detail) => Some(Rc::new(diff::content( + &detail.patch, + self.diff_view_mode, + &self.diff_collapsed, + ))), _ => None, }; } fn reset_diff_view(&mut self, window: &mut Window, cx: &mut Context) { + self.clear_diff_selection(window, cx); + self.diff_scroll_handle.set_offset(Point::default()); + } + + fn clear_diff_selection(&mut self, window: &mut Window, cx: &mut Context) { self.diff_auto_scroll.stop(); self.diff_select_all = false; - self.diff_scroll_handle.set_offset(Point::default()); TextSelection::clear(window, cx); } + fn clamp_diff_scroll(&mut self) { + let height = self + .diff_content + .as_ref() + .map_or(px(0.), |content| content.height()); + let lowest = -(height - self.diff_scroll_handle.bounds().size.height).max(px(0.)); + let offset = self.diff_scroll_handle.offset(); + if offset.y < lowest { + self.diff_scroll_handle.set_offset(point(offset.x, lowest)); + } + } + fn forget_select_all(&mut self, cx: &mut Context) { if !self.diff_select_all { return; @@ -278,6 +316,9 @@ impl Render for DetailPanel { fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { let selected_tab = self.selected_tab; let diff_view_mode = self.diff_view_mode; + let toggle_file: ToggleFile = Rc::new(cx.listener(|this, file: &usize, window, cx| { + this.toggle_file(*file, window, cx); + })); div() .track_focus(&self.focus_handle) .on_action(cx.listener(Self::on_copy)) @@ -298,6 +339,7 @@ impl Render for DetailPanel { &self.diff_selection, &self.general_scroll_handle, &self.diff_scroll_handle, + toggle_file, cx, ), }) @@ -398,6 +440,7 @@ fn ready_state( diff_selection: &TextSelectionHandle, general_scroll_handle: &ScrollHandle, diff_scroll_handle: &ScrollHandle, + toggle_file: ToggleFile, cx: &App, ) -> AnyElement { match selected_tab { @@ -407,6 +450,7 @@ fn ready_state( diff_select_all, diff_selection, diff_scroll_handle, + toggle_file, cx, ), } @@ -442,6 +486,7 @@ fn diff_tab( select_all: bool, selection: &TextSelectionHandle, scroll_handle: &ScrollHandle, + toggle_file: ToggleFile, cx: &App, ) -> AnyElement { div() @@ -453,6 +498,7 @@ fn diff_tab( select_all, selection, scroll_handle, + toggle_file, cx, )) .into_any_element() From 368540affe5eb53c57334f3cfed09dab4cb310d4 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 19:02:39 +0200 Subject: [PATCH 2/7] fix(diff): paint the disclosure marker instead of registering it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the header's text the marker was part of a selection run, so Cmd-C over a header and Select All both emitted "▾ src/main.rs +3 −1" — against the rule the module doc states, that only code text becomes a run and everything in the gutter is painted directly so line numbers and markers cannot reach the clipboard. paint_gutter already returned early for a header row, which leaves that gutter free: the glyph is shaped and painted there, at the offset a line's +/− uses, and header_line goes back to what it was. A hunk carrying no lines put a separator directly under the file header, which is what "between hunks only" exists to prevent, and two of them stacked two bands. Only git's own output rules that out; the parser accepts @@ -0,0 +0,0 @@, so the rows skip an empty hunk rather than trusting it. Row::FileHeader names its file `index`, leaving `file` free for the &FilePatch it means everywhere else in these two modules, and the collapsed lookup is hashed once per file rather than twice. Rows::full answers the full-width row that file_at, cell_foreground and paint_background each used to match out for themselves, and Rows::marker_left names the offset the three marker sites share. --- crates/ui/src/detail/diff/body.rs | 96 ++++++++++++++++----------- crates/ui/src/detail/diff/mod.rs | 8 ++- crates/ui/src/detail/diff/model.rs | 101 +++++++++++++++++------------ crates/ui/src/detail/diff/split.rs | 40 ++++++++---- 4 files changed, 150 insertions(+), 95 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index d6a8350..8c0522a 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -64,16 +64,23 @@ impl Rows { self.len() * self.columns() } - fn file_at(&self, row: usize) -> Option { - let row = match self { - Rows::Unified(rows) => rows.get(row)?, + fn marker_left(&self) -> f32 { + self.code_left() - MARKER_WIDTH + MARKER_PADDING + } + + fn full(&self, row: usize) -> Option<&Row> { + match self { + Rows::Unified(rows) => rows.get(row), Rows::Split(rows) => match rows.get(row)? { - SplitRow::Full(full) => full, - SplitRow::Sides { .. } => return None, + SplitRow::Full(full) => Some(full), + SplitRow::Sides { .. } => None, }, - }; - match row { - Row::FileHeader { file, .. } => Some(*file), + } + } + + fn file_at(&self, row: usize) -> Option { + match self.full(row)? { + Row::FileHeader { index, .. } => Some(*index), _ => None, } } @@ -160,9 +167,8 @@ fn row_text(row: &Row) -> SharedString { status, added, deleted, - collapsed, .. - } => header_line(path, status, *added, *deleted, *collapsed).into(), + } => header_line(path, status, *added, *deleted).into(), Row::Separator => SharedString::default(), Row::Line { content, .. } => content.clone().into(), Row::Placeholder { message } => (*message).into(), @@ -179,14 +185,8 @@ fn styled_cell(rows: &Rows, cell: usize, text: SharedString, theme: &ThemeColor) } fn cell_foreground(rows: &Rows, cell: usize, theme: &ThemeColor) -> Hsla { - let row = cell / rows.columns(); - match rows { - Rows::Unified(rows) => row_foreground(&rows[row], theme), - Rows::Split(split) => match &split[row] { - SplitRow::Full(full) => row_foreground(full, theme), - SplitRow::Sides { .. } => theme.foreground, - }, - } + rows.full(cell / rows.columns()) + .map_or(theme.foreground, |full| row_foreground(full, theme)) } fn row_foreground(row: &Row, theme: &ThemeColor) -> Hsla { @@ -213,6 +213,10 @@ fn marker(origin: LineOrigin) -> &'static str { } } +fn disclosure(collapsed: bool) -> &'static str { + if collapsed { "\u{25b8}" } else { "\u{25be}" } +} + fn column_width(bounds: Bounds, columns: usize) -> Pixels { bounds.size.width / columns as f32 } @@ -573,15 +577,7 @@ impl DiffBody { window: &mut Window, ) { let columns = self.rows().columns(); - let full_width = match self.rows() { - Rows::Unified(rows) => Some(&rows[row]), - Rows::Split(split) => match &split[row] { - SplitRow::Full(full) => Some(full), - SplitRow::Sides { .. } => None, - }, - }; - - match full_width { + match self.rows().full(row) { Some(full) => { if let Some(background) = row_background(full, &self.theme, self.mode) { let band = Bounds::new( @@ -627,8 +623,19 @@ impl DiffBody { ) { let left = cell_bounds.origin.x - px(self.rows().code_left()); let top = cell_bounds.origin.y; + let marker_left = left + px(self.rows().marker_left()); let muted = self.theme.muted_foreground; - let (origin, marker_left) = match self.rows() { + + if let Some(header @ Row::FileHeader { collapsed, .. }) = self.rows().full(row) { + if column == 0 { + let color = row_foreground(header, &self.theme); + let line = pen.shape(disclosure(*collapsed).into(), color, window); + paint_line(&line, point(marker_left, top), window, cx); + } + return; + } + + let origin = match self.rows() { Rows::Unified(rows) => { let Row::Line { origin, @@ -657,7 +664,7 @@ impl DiffBody { window, cx, ); - (origin, left + px(2. * GUTTER_WIDTH + MARKER_PADDING)) + origin } Rows::Split(_) => { let Some(side) = self.rows().side(row, column) else { @@ -672,7 +679,7 @@ impl DiffBody { window, cx, ); - (side.origin, left + px(GUTTER_WIDTH + MARKER_PADDING)) + side.origin } }; @@ -872,7 +879,7 @@ mod tests { fn file_header() -> Row { Row::FileHeader { - file: 0, + index: 0, path: "src/main.rs".to_string(), status: FileStatus::Modified, added: 3, @@ -922,6 +929,14 @@ mod tests { assert_eq!(split.code_left(), SPLIT_CODE_LEFT); } + #[test] + fn a_marker_sits_in_the_last_gutter_a_view_has() { + let unified = Rows::Unified(vec![file_header()]); + + assert_eq!(unified.marker_left(), 2. * GUTTER_WIDTH + MARKER_PADDING); + assert_eq!(split_rows().marker_left(), GUTTER_WIDTH + MARKER_PADDING); + } + #[test] fn an_empty_row_list_is_empty_in_either_view() { assert!(Rows::Unified(Vec::new()).is_empty()); @@ -970,7 +985,7 @@ mod tests { fn a_row_renders_its_own_kind_of_text() { assert_eq!( row_text(&file_header()), - SharedString::from("\u{25be} src/main.rs +3 \u{2212}1") + SharedString::from("src/main.rs +3 \u{2212}1") ); assert_eq!(row_text(&Row::Separator), SharedString::default()); assert_eq!( @@ -990,7 +1005,7 @@ mod tests { let rows = Rows::Unified(vec![file_header(), line(LineOrigin::Deletion, "gone")]); assert_eq!( cell_text(&rows, 0), - SharedString::from("\u{25be} src/main.rs +3 \u{2212}1") + SharedString::from("src/main.rs +3 \u{2212}1") ); assert_eq!(cell_text(&rows, 1), SharedString::from("gone")); } @@ -1000,7 +1015,7 @@ mod tests { let rows = split_rows(); assert_eq!( cell_text(&rows, 0), - SharedString::from("\u{25be} src/main.rs +3 \u{2212}1") + SharedString::from("src/main.rs +3 \u{2212}1") ); assert_eq!(cell_text(&rows, 1), SharedString::default()); } @@ -1016,9 +1031,15 @@ mod tests { } #[test] - fn a_collapsed_header_turns_its_disclosure_marker_sideways() { + fn a_disclosure_marker_turns_sideways_when_its_file_is_collapsed() { + assert_eq!(disclosure(false), "\u{25be}"); + assert_eq!(disclosure(true), "\u{25b8}"); + } + + #[test] + fn a_collapsed_header_reads_exactly_as_an_expanded_one() { let collapsed = Row::FileHeader { - file: 0, + index: 0, path: "src/main.rs".to_string(), status: FileStatus::Modified, added: 3, @@ -1028,7 +1049,8 @@ mod tests { assert_eq!( row_text(&collapsed), - SharedString::from("\u{25b8} src/main.rs +3 \u{2212}1") + row_text(&file_header()), + "the marker is painted in the gutter, so no run and no copy can carry it" ); } diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 8a2636a..5260910 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -71,11 +71,13 @@ //! affordable only because the text it measures is not rebuilt with it: [`DiffContent`] //! holds the rows and one [`gpui::SharedString`] per cell, derived by [`content`] when the //! patch, the view mode or the set of collapsed files changes and handed to the element -//! behind an `Rc` thereafter. Collapsing is applied in that derivation — the body rows of a -//! collapsed file are never emitted — so the element windows, paints and selects over a -//! shorter list and knows nothing of collapsing beyond mapping a click to a row. So +//! behind an `Rc` thereafter. So //! after the first frame each cell is a hit in gpui's line-layout cache and the steady-state //! cost is a hash of bytes that are already there, with no allocation and no reshape. +//! Collapsing is applied in that same derivation — the body rows of a collapsed file are +//! never emitted — so the element windows, paints and selects over a shorter list and knows +//! nothing of collapsing beyond mapping a click to a row and drawing a disclosure marker in +//! the header's own gutter, which keeps that marker out of every run and every copy. //! //! Copying depends on a fact `gpui-base`'s own doc comment does not state. A participant's //! runs concatenate with no separator when `update_runs` projects them diff --git a/crates/ui/src/detail/diff/model.rs b/crates/ui/src/detail/diff/model.rs index c5386e2..2ff61da 100644 --- a/crates/ui/src/detail/diff/model.rs +++ b/crates/ui/src/detail/diff/model.rs @@ -1,16 +1,13 @@ use std::fmt::Write as _; -use domain::{DiffLine, FilePatch, FileStatus, LineOrigin, Patch}; +use domain::{DiffLine, FilePatch, FileStatus, Hunk, LineOrigin, Patch}; use super::Collapsed; -const EXPANDED_MARKER: &str = "\u{25be}"; -const COLLAPSED_MARKER: &str = "\u{25b8}"; - #[derive(Clone, Debug, PartialEq, Eq)] pub(super) enum Row { FileHeader { - file: usize, + index: usize, path: String, status: FileStatus, added: usize, @@ -31,40 +28,29 @@ pub(super) enum Row { pub(super) fn rows(patch: &Patch, collapsed: &Collapsed) -> Vec { let mut rows = Vec::new(); - for (file, patch) in patch.files.iter().enumerate() { - rows.push(file_header(file, patch, collapsed.contains(&file))); - if collapsed.contains(&file) { - continue; + for (index, file) in patch.files.iter().enumerate() { + let is_collapsed = collapsed.contains(&index); + rows.push(file_header(index, file, is_collapsed)); + if !is_collapsed { + push_body(&mut rows, file); } - push_body(&mut rows, patch); } rows } -pub(super) fn file_header(file: usize, patch: &FilePatch, collapsed: bool) -> Row { +pub(super) fn file_header(index: usize, file: &FilePatch, collapsed: bool) -> Row { Row::FileHeader { - file, - path: header_path(patch), - status: patch.status.clone(), - added: patch.added_lines(), - deleted: patch.deleted_lines(), + index, + path: header_path(file), + status: file.status.clone(), + added: file.added_lines(), + deleted: file.deleted_lines(), collapsed, } } -pub(super) fn header_line( - path: &str, - status: &FileStatus, - added: usize, - deleted: usize, - collapsed: bool, -) -> String { - let marker = if collapsed { - COLLAPSED_MARKER - } else { - EXPANDED_MARKER - }; - let mut text = format!("{marker} {path}"); +pub(super) fn header_line(path: &str, status: &FileStatus, added: usize, deleted: usize) -> String { + let mut text = path.to_string(); if let Some(label) = status_label(status) { let _ = write!(text, " {label}"); } @@ -99,6 +85,10 @@ fn status_label(status: &FileStatus) -> Option { } } +pub(super) fn filled_hunks(file: &FilePatch) -> impl Iterator { + file.hunks.iter().filter(|hunk| !hunk.lines.is_empty()) +} + pub(super) fn placeholder(file: &FilePatch) -> Option { if file.is_binary { return Some(Row::Placeholder { @@ -118,7 +108,7 @@ fn push_body(rows: &mut Vec, file: &FilePatch) { rows.push(placeholder); return; } - for (position, hunk) in file.hunks.iter().enumerate() { + for (position, hunk) in filled_hunks(file).enumerate() { if position > 0 { rows.push(Row::Separator); } @@ -290,8 +280,8 @@ mod tests { assert_eq!(path, "src/old.rs \u{2192} src/new.rs"); assert_eq!(status, FileStatus::Renamed { similarity: 87 }); assert_eq!( - header_line(&path, &status, added, deleted, false), - "\u{25be} src/old.rs \u{2192} src/new.rs renamed 87% +0 \u{2212}0" + header_line(&path, &status, added, deleted), + "src/old.rs \u{2192} src/new.rs renamed 87% +0 \u{2212}0" ); } @@ -315,8 +305,8 @@ mod tests { assert_eq!(path, "src/old.rs \u{2192} src/copy.rs"); assert_eq!( - header_line(&path, &status, added, deleted, false), - "\u{25be} src/old.rs \u{2192} src/copy.rs copied 100% +0 \u{2212}0" + header_line(&path, &status, added, deleted), + "src/old.rs \u{2192} src/copy.rs copied 100% +0 \u{2212}0" ); } @@ -350,8 +340,8 @@ mod tests { }; assert_eq!( - header_line(&path, &status, added, deleted, false), - "\u{25be} src/main.rs +1 \u{2212}0" + header_line(&path, &status, added, deleted), + "src/main.rs +1 \u{2212}0" ); } @@ -425,7 +415,7 @@ mod tests { assert!(matches!( rows[0], Row::FileHeader { - file: 0, + index: 0, collapsed: false, .. } @@ -434,7 +424,7 @@ mod tests { assert!(matches!( rows[2], Row::FileHeader { - file: 1, + index: 1, collapsed: true, .. } @@ -442,7 +432,7 @@ mod tests { assert!(matches!( rows[3], Row::FileHeader { - file: 2, + index: 2, collapsed: false, .. } @@ -451,7 +441,7 @@ mod tests { } #[test] - fn a_collapsed_header_turns_its_disclosure_marker_sideways() { + fn a_collapsed_header_carries_the_flag_and_reads_exactly_as_an_expanded_one() { let file = file(vec![one_line()], false); let Row::FileHeader { path, @@ -467,8 +457,35 @@ mod tests { assert!(collapsed); assert_eq!( - header_line(&path, &status, added, deleted, collapsed), - "\u{25b8} src/main.rs +1 \u{2212}0" + header_line(&path, &status, added, deleted), + "src/main.rs +1 \u{2212}0", + "the disclosure marker is painted in the gutter, because a run would copy it" ); } + + #[test] + fn an_empty_hunk_yields_neither_a_line_nor_a_separator() { + let patch = Patch { + files: vec![file(vec![hunk(Vec::new()), one_line()], false)], + }; + + let rows = rows(&patch, &expanded()); + + assert!(matches!(rows[0], Row::FileHeader { .. })); + assert!(matches!(rows[1], Row::Line { .. })); + assert_eq!(rows.len(), 2); + } + + #[test] + fn an_empty_hunk_between_two_filled_ones_parts_them_only_once() { + let patch = Patch { + files: vec![file(vec![one_line(), hunk(Vec::new()), one_line()], false)], + }; + + let rows = rows(&patch, &expanded()); + + assert_eq!(rows[2], Row::Separator); + assert_eq!(rows.iter().filter(|row| **row == Row::Separator).count(), 1); + assert_eq!(rows.len(), 4); + } } diff --git a/crates/ui/src/detail/diff/split.rs b/crates/ui/src/detail/diff/split.rs index 9ed5764..95f9f75 100644 --- a/crates/ui/src/detail/diff/split.rs +++ b/crates/ui/src/detail/diff/split.rs @@ -1,7 +1,7 @@ use domain::Patch; use super::Collapsed; -use super::model::{Row, file_header, placeholder}; +use super::model::{Row, file_header, filled_hunks, placeholder}; use super::pairing::{SideLine, pair}; #[derive(Clone, Debug, PartialEq, Eq)] @@ -15,20 +15,17 @@ pub(super) enum SplitRow { pub(super) fn split_rows(patch: &Patch, collapsed: &Collapsed) -> Vec { let mut rows = Vec::new(); - for (file, patch) in patch.files.iter().enumerate() { - rows.push(SplitRow::Full(file_header( - file, - patch, - collapsed.contains(&file), - ))); - if collapsed.contains(&file) { + for (index, file) in patch.files.iter().enumerate() { + let is_collapsed = collapsed.contains(&index); + rows.push(SplitRow::Full(file_header(index, file, is_collapsed))); + if is_collapsed { continue; } - if let Some(placeholder) = placeholder(patch) { + if let Some(placeholder) = placeholder(file) { rows.push(SplitRow::Full(placeholder)); continue; } - for (position, hunk) in patch.hunks.iter().enumerate() { + for (position, hunk) in filled_hunks(file).enumerate() { if position > 0 { rows.push(SplitRow::Full(Row::Separator)); } @@ -240,7 +237,7 @@ mod tests { assert_eq!( rows[0], SplitRow::Full(Row::FileHeader { - file: 0, + index: 0, path: "src/old.rs \u{2192} src/new.rs".to_string(), status: FileStatus::Renamed { similarity: 87 }, added: 0, @@ -271,7 +268,7 @@ mod tests { assert_eq!( rows[0], SplitRow::Full(Row::FileHeader { - file: 0, + index: 0, path: "src/old.rs \u{2192} src/copy.rs".to_string(), status: FileStatus::Copied { similarity: 100 }, added: 0, @@ -333,7 +330,7 @@ mod tests { assert!(matches!( rows[2], SplitRow::Full(Row::FileHeader { - file: 1, + index: 1, collapsed: true, .. }) @@ -348,4 +345,21 @@ mod tests { }; assert_eq!(split_rows(&patch, &collapsed([0])).len(), 1); } + + #[test] + fn an_empty_hunk_yields_neither_a_row_nor_a_separator() { + let patch = Patch { + files: vec![file( + "src/a.rs", + vec![hunk(Vec::new()), replacement(), hunk(Vec::new())], + false, + )], + }; + + let rows = split_rows(&patch, &expanded()); + + assert!(matches!(rows[0], SplitRow::Full(Row::FileHeader { .. }))); + assert!(matches!(rows[1], SplitRow::Sides { .. })); + assert_eq!(rows.len(), 2); + } } From d348e0bbe900874927ff85cb6643acc80a81e966 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 19:07:53 +0200 Subject: [PATCH 3/7] test(diff): mirror the empty-hunk separator case in the split view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two derivations share filled_hunks today, but they are separate functions and one could be inlined or specialised without the other noticing. The split view had only two empty hunks flanking a filled one, which never puts an empty hunk between two filled ones — the arrangement that would produce a doubled separator. It passes as written. --- crates/ui/src/detail/diff/split.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/ui/src/detail/diff/split.rs b/crates/ui/src/detail/diff/split.rs index 95f9f75..6f37236 100644 --- a/crates/ui/src/detail/diff/split.rs +++ b/crates/ui/src/detail/diff/split.rs @@ -362,4 +362,26 @@ mod tests { assert!(matches!(rows[1], SplitRow::Sides { .. })); assert_eq!(rows.len(), 2); } + + #[test] + fn an_empty_hunk_between_two_filled_ones_parts_them_only_once() { + let patch = Patch { + files: vec![file( + "src/a.rs", + vec![replacement(), hunk(Vec::new()), replacement()], + false, + )], + }; + + let rows = split_rows(&patch, &expanded()); + + assert_eq!(rows[2], SplitRow::Full(Row::Separator)); + assert_eq!( + rows.iter() + .filter(|row| **row == SplitRow::Full(Row::Separator)) + .count(), + 1 + ); + assert_eq!(rows.len(), 4); + } } From 9bd891006417f5fa4375efb15eb3dc41a4001451 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 20:46:48 +0200 Subject: [PATCH 4/7] feat(diff): restyle a file header as a chevron, a pastille and a change bar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The header row read as one line of text — path, status label, `+22 −0` — indented to the code column, with only the disclosure marker painted separately in the gutter. It now spans the row: chevron, status pastille and path flush left at the row's own left edge, and a green/red bar with the total in bold right-aligned at the other end. Only the path is a selection run. `header_line` reduces to the path, so `cell_strings` — the one source both the runs and the clipboard read — carries nothing else, and copying a header yields a bare path. The chevron, pastille, bar and count are painted the way `paint_gutter` already paints line numbers and markers, which is what keeps them out of every run. A bar's total width is its file's share of the most-changed file in the patch, and that maximum is derived in `content` alongside the rows and the strings rather than reached for per frame: sizing a bar from the element would mean walking every file of the patch on every repaint, and a selection drag repaints on each mouse move. Green is `theme.green` and red `theme.red`, the pair `line_colors` already tints a `+` and a `−` with, so a segment and a marker mean the same thing. The pastille is `theme.green` for an addition, `theme.red` for a deletion and `theme.blue` for a modification. Renamed, Copied and TypeChanged share `theme.muted_foreground`: none of the three is a statement about content — they describe the file's identity or its kind moving, and whatever content changed with it is already told by the bar beside them. The similarity percentage those two carried in the old header text is dropped; the `→` in the path still says a rename happened. Four degenerate cases decide the geometry. A patch of pure renames has no scale, so `max_changes` of zero draws no bar rather than dividing by it, and a file of no changes draws none either. A file with only additions gets no red segment and one with only deletions no green, each skipped rather than painted zero-wide. And a bar too short to see is widened twice over: the bar takes a floor of `BAR_MIN_WIDTH`, so one change beside a thousand is 4px rather than 0.064px, and a segment takes a floor of `BAR_MIN_SEGMENT` by moving the boundary rather than growing the bar, so the sum stays exactly proportional. `BAR_MIN_WIDTH` is written as twice `BAR_MIN_SEGMENT` because that is what makes the two clamps impossible to invert. The bar and count are right-aligned to the element's right edge, mirroring the left side being flush to its left edge, rather than pinned to the viewport. Both ends of the header therefore scroll with the content. Only the header's geometry moves: `Rows::cell_left` answers `HEADER_TEXT_LEFT` for a header and `code_left()` for every other row, so lines, separators and placeholders are untouched. Both copy paths go through the same `cell_bounds_at`, so the projection and the off-screen arithmetic still agree cell for cell. The click target is unchanged — `on_mouse_down` reads only `y`. --- crates/ui/src/detail/diff/body.rs | 323 +++++++++++++++++++++++++---- crates/ui/src/detail/diff/mod.rs | 35 ++-- crates/ui/src/detail/diff/model.rs | 114 +++++----- 3 files changed, 365 insertions(+), 107 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index 8c0522a..9bf4c0d 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -1,12 +1,13 @@ use std::ops::{Range, RangeInclusive}; use std::rc::Rc; -use domain::LineOrigin; +use domain::{FileStatus, LineOrigin}; use gpui::{ - App, Bounds, DispatchPhase, Element, ElementId, FlexDirection, GlobalElementId, Half as _, - HighlightStyle, Hitbox, HitboxBehavior, Hsla, InspectorElementId, IntoElement, LayoutId, - Length, MouseButton, MouseDownEvent, Pixels, Point, ScrollHandle, ShapedLine, SharedString, - Style, StyledText, TextAlign, TextLayout, TextStyle, Window, fill, point, px, relative, size, + App, Bounds, DispatchPhase, Element, ElementId, FlexDirection, FontWeight, GlobalElementId, + Half as _, HighlightStyle, Hitbox, HitboxBehavior, Hsla, InspectorElementId, IntoElement, + LayoutId, Length, MouseButton, MouseDownEvent, Pixels, Point, ScrollHandle, ShapedLine, + SharedString, Style, StyledText, TextAlign, TextLayout, TextStyle, Window, fill, point, px, + relative, size, }; use gpui_base::{TextSelectionHandle, TextSelectionRegistration, TextSelectionRun}; use gpui_component::{ThemeColor, ThemeMode}; @@ -29,6 +30,22 @@ const COLUMN_RULE_WIDTH: f32 = 1.; const TRAILING_SPACE: f32 = 16.; const UNMEASURED_ROWS: usize = 100; +const HEADER_PADDING: f32 = 8.; +const DISCLOSURE_WIDTH: f32 = 12.; +const PASTILLE_SIZE: f32 = 8.; +const PASTILLE_RADIUS: f32 = 2.; +const PASTILLE_GAP: f32 = 8.; +const PASTILLE_LEFT: f32 = HEADER_PADDING + DISCLOSURE_WIDTH; +const PASTILLE_TOP: f32 = (ROW_HEIGHT - PASTILLE_SIZE) / 2.; +const HEADER_TEXT_LEFT: f32 = PASTILLE_LEFT + PASTILLE_SIZE + PASTILLE_GAP; +const BAR_WIDTH: f32 = 64.; +const BAR_HEIGHT: f32 = 8.; +const BAR_TOP: f32 = (ROW_HEIGHT - BAR_HEIGHT) / 2.; +const BAR_MIN_SEGMENT: f32 = 2.; +const BAR_MIN_WIDTH: f32 = 2. * BAR_MIN_SEGMENT; +const BAR_GAP: f32 = 8.; +const COUNT_WIDTH: f32 = 34.; + pub(super) enum Rows { Unified(Vec), Split(Vec), @@ -60,6 +77,13 @@ impl Rows { } } + fn cell_left(&self, row: usize) -> f32 { + match self.full(row) { + Some(Row::FileHeader { .. }) => HEADER_TEXT_LEFT, + _ => self.code_left(), + } + } + fn cells(&self) -> usize { self.len() * self.columns() } @@ -162,13 +186,7 @@ fn cell_text(rows: &Rows, cell: usize) -> SharedString { fn row_text(row: &Row) -> SharedString { match row { - Row::FileHeader { - path, - status, - added, - deleted, - .. - } => header_line(path, status, *added, *deleted).into(), + Row::FileHeader { path, .. } => header_line(path).into(), Row::Separator => SharedString::default(), Row::Line { content, .. } => content.clone().into(), Row::Placeholder { message } => (*message).into(), @@ -228,21 +246,57 @@ fn column_left(bounds: Bounds, columns: usize, column: usize) -> Pixels fn bounds_for_cell( bounds: Bounds, columns: usize, - code_left: Pixels, + left: Pixels, cell: usize, ) -> Bounds { Bounds::new( point( - column_left(bounds, columns, cell % columns) + code_left, + column_left(bounds, columns, cell % columns) + left, bounds.origin.y + px((cell / columns) as f32 * ROW_HEIGHT), ), size( - (column_width(bounds, columns) - code_left).max(px(0.)), + (column_width(bounds, columns) - left).max(px(0.)), px(ROW_HEIGHT), ), ) } +#[derive(Clone, Copy, Debug, PartialEq)] +struct Bar { + added: f32, + deleted: f32, +} + +fn bar_widths(added: usize, deleted: usize, max_changes: usize) -> Option { + let total = added + deleted; + if total == 0 || max_changes == 0 { + return None; + } + let width = (BAR_WIDTH * total as f32 / max_changes as f32).max(BAR_MIN_WIDTH); + let mut green = width * added as f32 / total as f32; + if added > 0 { + green = green.max(BAR_MIN_SEGMENT); + } + if deleted > 0 { + green = green.min(width - BAR_MIN_SEGMENT); + } + Some(Bar { + added: green, + deleted: width - green, + }) +} + +fn status_color(status: &FileStatus, theme: &ThemeColor) -> Hsla { + match status { + FileStatus::Added => theme.green, + FileStatus::Deleted => theme.red, + FileStatus::Modified => theme.blue, + FileStatus::Renamed { .. } | FileStatus::Copied { .. } | FileStatus::TypeChanged => { + theme.muted_foreground + } + } +} + fn selection_quad_bounds( start: Point, end: Point, @@ -428,8 +482,23 @@ impl Pen { } fn shape(&self, text: SharedString, color: Hsla, window: &Window) -> ShapedLine { + self.weighted(text, color, self.style.font_weight, window) + } + + fn bold(&self, text: SharedString, color: Hsla, window: &Window) -> ShapedLine { + self.weighted(text, color, FontWeight::BOLD, window) + } + + fn weighted( + &self, + text: SharedString, + color: Hsla, + weight: FontWeight, + window: &Window, + ) -> ShapedLine { let mut run = self.style.to_run(text.len()); run.color = color; + run.font.weight = weight; window .text_system() .shape_line(text, self.font_size, &[run], None) @@ -475,10 +544,11 @@ impl DiffBody { } fn cell_bounds_at(&self, bounds: Bounds, cell: usize) -> Bounds { + let columns = self.rows().columns(); bounds_for_cell( bounds, - self.rows().columns(), - px(self.rows().code_left()), + columns, + px(self.rows().cell_left(cell / columns)), cell, ) } @@ -626,15 +696,6 @@ impl DiffBody { let marker_left = left + px(self.rows().marker_left()); let muted = self.theme.muted_foreground; - if let Some(header @ Row::FileHeader { collapsed, .. }) = self.rows().full(row) { - if column == 0 { - let color = row_foreground(header, &self.theme); - let line = pen.shape(disclosure(*collapsed).into(), color, window); - paint_line(&line, point(marker_left, top), window, cx); - } - return; - } - let origin = match self.rows() { Rows::Unified(rows) => { let Row::Line { @@ -688,6 +749,80 @@ impl DiffBody { paint_line(&line, point(marker_left, top), window, cx); } + fn paint_header( + &self, + header: &Row, + bounds: Bounds, + top: Pixels, + pen: &Pen, + window: &mut Window, + cx: &mut App, + ) { + let Row::FileHeader { + status, + added, + deleted, + collapsed, + .. + } = header + else { + return; + }; + + let chevron = pen.shape( + disclosure(*collapsed).into(), + self.theme.muted_foreground, + window, + ); + paint_line( + &chevron, + point(bounds.origin.x + px(HEADER_PADDING), top), + window, + cx, + ); + + window.paint_quad( + fill( + Bounds::new( + point(bounds.origin.x + px(PASTILLE_LEFT), top + px(PASTILLE_TOP)), + size(px(PASTILLE_SIZE), px(PASTILLE_SIZE)), + ), + status_color(status, &self.theme), + ) + .corner_radii(px(PASTILLE_RADIUS)), + ); + + let count = pen.bold( + (added + deleted).to_string().into(), + self.theme.foreground, + window, + ); + let count_right = bounds.right() - px(HEADER_PADDING); + paint_line(&count, point(count_right - count.width(), top), window, cx); + + let Some(bar) = bar_widths(*added, *deleted, self.content.max_changes) else { + return; + }; + let bar_right = count_right - px(BAR_GAP) - px(COUNT_WIDTH).max(count.width()); + let bar_left = bar_right - px(bar.added + bar.deleted); + let bar_top = top + px(BAR_TOP); + for (offset, width, color) in [ + (0., bar.added, self.theme.green), + (bar.added, bar.deleted, self.theme.red), + ] { + if width <= 0. { + continue; + } + window.paint_quad(fill( + Bounds::new( + point(bar_left + px(offset), bar_top), + size(px(width), px(BAR_HEIGHT)), + ), + color, + )); + } + } + fn on_mouse_down(&self, bounds: Bounds, hitbox: &Hitbox, window: &mut Window) { let hitbox = hitbox.clone(); let content = Rc::clone(&self.content); @@ -857,7 +992,13 @@ impl Element for DiffBody { ); } - self.paint_gutter(row, column, cell_bounds, &pen, window, cx); + match self.rows().full(row) { + Some(header @ Row::FileHeader { .. }) if column == 0 => { + self.paint_header(header, bounds, top, &pen, window, cx); + } + Some(Row::FileHeader { .. }) => {} + _ => self.paint_gutter(row, column, cell_bounds, &pen, window, cx), + } self.texts[cell_offset].paint( None, None, @@ -875,7 +1016,6 @@ impl Element for DiffBody { #[cfg(test)] mod tests { use super::*; - use domain::FileStatus; fn file_header() -> Row { Row::FileHeader { @@ -983,10 +1123,7 @@ mod tests { #[test] fn a_row_renders_its_own_kind_of_text() { - assert_eq!( - row_text(&file_header()), - SharedString::from("src/main.rs +3 \u{2212}1") - ); + assert_eq!(row_text(&file_header()), SharedString::from("src/main.rs")); assert_eq!(row_text(&Row::Separator), SharedString::default()); assert_eq!( row_text(&line(LineOrigin::Addition, "let x = 1;")), @@ -1003,20 +1140,14 @@ mod tests { #[test] fn a_unified_cell_is_its_row() { let rows = Rows::Unified(vec![file_header(), line(LineOrigin::Deletion, "gone")]); - assert_eq!( - cell_text(&rows, 0), - SharedString::from("src/main.rs +3 \u{2212}1") - ); + assert_eq!(cell_text(&rows, 0), SharedString::from("src/main.rs")); assert_eq!(cell_text(&rows, 1), SharedString::from("gone")); } #[test] fn a_full_width_split_row_renders_in_the_first_column_and_blank_in_the_second() { let rows = split_rows(); - assert_eq!( - cell_text(&rows, 0), - SharedString::from("src/main.rs +3 \u{2212}1") - ); + assert_eq!(cell_text(&rows, 0), SharedString::from("src/main.rs")); assert_eq!(cell_text(&rows, 1), SharedString::default()); } @@ -1030,6 +1161,118 @@ mod tests { assert_eq!(cell_text(&split, 1), SharedString::default()); } + #[test] + fn a_header_starts_at_the_left_edge_and_every_other_row_past_its_gutters() { + let unified = Rows::Unified(vec![file_header(), line(LineOrigin::Context, "keep")]); + let split = split_rows(); + + assert_eq!(unified.cell_left(0), HEADER_TEXT_LEFT); + assert_eq!(unified.cell_left(1), CODE_LEFT); + assert_eq!(split.cell_left(0), HEADER_TEXT_LEFT); + assert_eq!(split.cell_left(1), SPLIT_CODE_LEFT); + assert!( + split.cell_left(0) < split.cell_left(1), + "a header is flush left, ahead of the narrowest code column" + ); + } + + #[test] + fn a_bar_is_the_files_share_of_the_widest_one_in_the_patch() { + assert_eq!( + bar_widths(30, 10, 40), + Some(Bar { + added: BAR_WIDTH * 0.75, + deleted: BAR_WIDTH * 0.25 + }), + "the widest file fills the bar and splits it by its own two counts" + ); + let half = bar_widths(10, 10, 40).expect("a changed file has a bar"); + assert_eq!(half.added + half.deleted, BAR_WIDTH / 2.); + assert_eq!(half.added, half.deleted); + } + + #[test] + fn a_patch_that_changes_nothing_draws_no_bar_and_divides_by_nothing() { + assert_eq!(bar_widths(0, 0, 0), None); + assert_eq!( + bar_widths(0, 0, 40), + None, + "a pure rename beside real changes is a bar of no length" + ); + } + + #[test] + fn a_file_of_one_kind_of_change_gets_one_segment_and_no_other() { + assert_eq!( + bar_widths(40, 0, 40), + Some(Bar { + added: BAR_WIDTH, + deleted: 0. + }) + ); + assert_eq!( + bar_widths(0, 40, 40), + Some(Bar { + added: 0., + deleted: BAR_WIDTH + }) + ); + } + + #[test] + fn a_bar_too_short_to_draw_is_widened_rather_than_lost() { + let one = bar_widths(1, 0, 1000).expect("one change still draws"); + assert_eq!(one.added, BAR_MIN_WIDTH); + assert_eq!(one.deleted, 0.); + + let pair = bar_widths(1, 1, 1000).expect("two changes still draw"); + assert_eq!(pair.added, BAR_MIN_SEGMENT); + assert_eq!(pair.deleted, BAR_MIN_SEGMENT); + } + + #[test] + fn a_segment_too_thin_to_see_keeps_its_minimum_without_lengthening_the_bar() { + let lopsided = bar_widths(1, 999, 1000).expect("a changed file has a bar"); + assert_eq!(lopsided.added, BAR_MIN_SEGMENT); + assert_eq!(lopsided.added + lopsided.deleted, BAR_WIDTH); + + let mirrored = bar_widths(999, 1, 1000).expect("a changed file has a bar"); + assert_eq!(mirrored.deleted, BAR_MIN_SEGMENT); + assert_eq!(mirrored.added + mirrored.deleted, BAR_WIDTH); + } + + #[test] + fn a_pastille_tells_an_addition_a_deletion_and_a_modification_apart() { + for theme in [ThemeColor::light(), ThemeColor::dark()] { + let colors = [ + status_color(&FileStatus::Added, &theme), + status_color(&FileStatus::Deleted, &theme), + status_color(&FileStatus::Modified, &theme), + ]; + for (i, a) in colors.iter().enumerate() { + for (j, b) in colors.iter().enumerate().skip(i + 1) { + let distance = crate::theme_palette::rendered_distance(theme.secondary, *a, *b); + assert!( + distance > 0.06, + "statuses {i} and {j} read as the same pastille (distance {distance:.3})" + ); + } + } + } + } + + #[test] + fn a_move_and_a_type_change_share_one_pastille() { + let theme = ThemeColor::light(); + let moved = status_color(&FileStatus::Renamed { similarity: 90 }, &theme); + assert_eq!( + status_color(&FileStatus::Copied { similarity: 90 }, &theme), + moved + ); + assert_eq!(status_color(&FileStatus::TypeChanged, &theme), moved); + assert_ne!(moved, status_color(&FileStatus::Modified, &theme)); + } + #[test] fn a_disclosure_marker_turns_sideways_when_its_file_is_collapsed() { assert_eq!(disclosure(false), "\u{25be}"); diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 5260910..19929ba 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -22,9 +22,12 @@ //! Selection comes back through `gpui-base`'s window-level participant system rather than //! from the editor. [`body`] is the element that joins it: it registers one participant and //! declares one run per cell on screen, left before right within a row, and only the code -//! text becomes a run — the gutters and the marker are painted directly and never -//! registered, which is what keeps line numbers and markers out of the clipboard. The rows -//! scroll on both axes rather than soft-wrapping, +//! text — or, on a file header, the path alone — becomes a run. Everything else a row draws +//! is painted directly and never registered: a line's gutters and its `+`/`−` marker, and a +//! header's disclosure chevron, status pastille, change bar and change count. That is what +//! keeps line numbers, markers and a file's statistics out of the clipboard, and it is why +//! copying a header yields a bare path. The rows scroll on both axes rather than +//! soft-wrapping, //! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the //! reason it usually does: the container's `overflow_scroll()` puts both axes in //! `Overflow::Scroll`, and gpui's vertical-onto-horizontal remap (`div.rs:3220-3224`, @@ -69,15 +72,18 @@ //! because a cell is laid out against a column width that provably exceeds every cell's //! natural width — which is what keeps a row one line tall and `ROW_HEIGHT` true. That is //! affordable only because the text it measures is not rebuilt with it: [`DiffContent`] -//! holds the rows and one [`gpui::SharedString`] per cell, derived by [`content`] when the -//! patch, the view mode or the set of collapsed files changes and handed to the element -//! behind an `Rc` thereafter. So -//! after the first frame each cell is a hit in gpui's line-layout cache and the steady-state -//! cost is a hash of bytes that are already there, with no allocation and no reshape. +//! holds the rows, one [`gpui::SharedString`] per cell, and the total changes of the patch's +//! largest file, derived by [`content`] when the patch, the view mode or the set of collapsed +//! files changes and handed to the element behind an `Rc` thereafter. So after the first +//! frame each cell is a hit in gpui's line-layout cache and the steady-state cost is a hash +//! of bytes that are already there, with no allocation and no reshape. That last number is +//! the scale every header's change bar is drawn against, and it belongs to the derivation for +//! the same reason the strings do: a bar is a share of the widest file in the patch, so +//! sizing one from the element would mean walking every file of the patch on every frame. //! Collapsing is applied in that same derivation — the body rows of a collapsed file are //! never emitted — so the element windows, paints and selects over a shorter list and knows -//! nothing of collapsing beyond mapping a click to a row and drawing a disclosure marker in -//! the header's own gutter, which keeps that marker out of every run and every copy. +//! nothing of collapsing beyond mapping a click to a row and drawing a disclosure chevron at +//! the header's own left edge, which keeps that chevron out of every run and every copy. //! //! Copying depends on a fact `gpui-base`'s own doc comment does not state. A participant's //! runs concatenate with no separator when `update_runs` projects them @@ -124,7 +130,7 @@ use gpui_component::{ use crate::diff_view_mode::DiffViewMode; use body::{ROW_HEIGHT, Rows, body, cell_strings}; -use model::rows; +use model::{max_changes, rows}; use split::split_rows; pub(super) type Collapsed = HashSet; @@ -134,6 +140,7 @@ pub(super) type ToggleFile = Rc; pub(super) struct DiffContent { rows: Rows, strings: Vec, + max_changes: usize, } impl DiffContent { @@ -152,7 +159,11 @@ pub(super) fn content(patch: &Patch, mode: DiffViewMode, collapsed: &Collapsed) DiffViewMode::Split => Rows::Split(split_rows(patch, collapsed)), }; let strings = cell_strings(&rows); - DiffContent { rows, strings } + DiffContent { + rows, + strings, + max_changes: max_changes(patch), + } } pub(super) fn render( diff --git a/crates/ui/src/detail/diff/model.rs b/crates/ui/src/detail/diff/model.rs index 2ff61da..b671c00 100644 --- a/crates/ui/src/detail/diff/model.rs +++ b/crates/ui/src/detail/diff/model.rs @@ -1,5 +1,3 @@ -use std::fmt::Write as _; - use domain::{DiffLine, FilePatch, FileStatus, Hunk, LineOrigin, Patch}; use super::Collapsed; @@ -49,13 +47,17 @@ pub(super) fn file_header(index: usize, file: &FilePatch, collapsed: bool) -> Ro } } -pub(super) fn header_line(path: &str, status: &FileStatus, added: usize, deleted: usize) -> String { - let mut text = path.to_string(); - if let Some(label) = status_label(status) { - let _ = write!(text, " {label}"); - } - let _ = write!(text, " +{added} \u{2212}{deleted}"); - text +pub(super) fn header_line(path: &str) -> String { + path.to_string() +} + +pub(super) fn max_changes(patch: &Patch) -> usize { + patch + .files + .iter() + .map(|file| file.added_lines() + file.deleted_lines()) + .max() + .unwrap_or(0) } fn header_path(file: &FilePatch) -> String { @@ -74,17 +76,6 @@ fn header_path(file: &FilePatch) -> String { } } -fn status_label(status: &FileStatus) -> Option { - match status { - FileStatus::Modified => None, - FileStatus::Added => Some("added".to_string()), - FileStatus::Deleted => Some("deleted".to_string()), - FileStatus::Renamed { similarity } => Some(format!("renamed {similarity}%")), - FileStatus::Copied { similarity } => Some(format!("copied {similarity}%")), - FileStatus::TypeChanged => Some("type changed".to_string()), - } -} - pub(super) fn filled_hunks(file: &FilePatch) -> impl Iterator { file.hunks.iter().filter(|hunk| !hunk.lines.is_empty()) } @@ -266,22 +257,16 @@ mod tests { "src/new.rs", FileStatus::Renamed { similarity: 87 }, ); - let Row::FileHeader { - path, - status, - added, - deleted, - .. - } = file_header(0, &file, false) - else { + let Row::FileHeader { path, status, .. } = file_header(0, &file, false) else { panic!("a file yields a header row"); }; assert_eq!(path, "src/old.rs \u{2192} src/new.rs"); assert_eq!(status, FileStatus::Renamed { similarity: 87 }); assert_eq!( - header_line(&path, &status, added, deleted), - "src/old.rs \u{2192} src/new.rs renamed 87% +0 \u{2212}0" + header_line(&path), + "src/old.rs \u{2192} src/new.rs", + "the similarity rides on the status, which is painted as a pastille" ); } @@ -292,22 +277,13 @@ mod tests { "src/copy.rs", FileStatus::Copied { similarity: 100 }, ); - let Row::FileHeader { - path, - status, - added, - deleted, - .. - } = file_header(0, &file, false) - else { + let Row::FileHeader { path, status, .. } = file_header(0, &file, false) else { panic!("a file yields a header row"); }; assert_eq!(path, "src/old.rs \u{2192} src/copy.rs"); - assert_eq!( - header_line(&path, &status, added, deleted), - "src/old.rs \u{2192} src/copy.rs copied 100% +0 \u{2212}0" - ); + assert_eq!(status, FileStatus::Copied { similarity: 100 }); + assert_eq!(header_line(&path), "src/old.rs \u{2192} src/copy.rs"); } #[test] @@ -324,13 +300,12 @@ mod tests { } #[test] - fn a_modified_file_carries_no_status_label() { + fn a_header_line_carries_the_path_and_nothing_else() { let patch = Patch { files: vec![file(vec![one_line()], false)], }; let Row::FileHeader { path, - status, added, deleted, .. @@ -339,9 +314,12 @@ mod tests { panic!("the first row is the file header"); }; + assert_eq!(added, 1); + assert_eq!(deleted, 0); assert_eq!( - header_line(&path, &status, added, deleted), - "src/main.rs +1 \u{2212}0" + header_line(&path), + "src/main.rs", + "the counts are painted, so a run cannot copy them with the path" ); } @@ -444,12 +422,7 @@ mod tests { fn a_collapsed_header_carries_the_flag_and_reads_exactly_as_an_expanded_one() { let file = file(vec![one_line()], false); let Row::FileHeader { - path, - status, - added, - deleted, - collapsed, - .. + path, collapsed, .. } = file_header(0, &file, true) else { panic!("a file yields a header row"); @@ -457,12 +430,43 @@ mod tests { assert!(collapsed); assert_eq!( - header_line(&path, &status, added, deleted), - "src/main.rs +1 \u{2212}0", - "the disclosure marker is painted in the gutter, because a run would copy it" + header_line(&path), + "src/main.rs", + "the disclosure chevron is painted, because a run would copy it" ); } + #[test] + fn the_scale_is_the_largest_file_of_the_patch() { + let patch = Patch { + files: vec![ + file(vec![one_line()], false), + file( + vec![hunk(vec![ + line(LineOrigin::Addition, None, Some(1), "a"), + line(LineOrigin::Addition, None, Some(2), "b"), + line(LineOrigin::Deletion, Some(1), None, "c"), + ])], + false, + ), + ], + }; + assert_eq!(max_changes(&patch), 3); + } + + #[test] + fn a_patch_of_pure_renames_has_no_scale_to_divide_by() { + let patch = Patch { + files: vec![moved( + "src/old.rs", + "src/new.rs", + FileStatus::Renamed { similarity: 100 }, + )], + }; + assert_eq!(max_changes(&patch), 0); + assert_eq!(max_changes(&Patch::default()), 0); + } + #[test] fn an_empty_hunk_yields_neither_a_line_nor_a_separator() { let patch = Patch { From c508c4b378fdf57628adcad03f4a83e2b68ab876 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 20:57:46 +0200 Subject: [PATCH 5/7] fix(diff): pin a file header to the viewport instead of scrolling it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A header row was laid out in document space: flush to the element's left edge and right-aligned to the element's right edge. With no soft wrap, a Rust diff's content is routinely wider than the dock, so the bar and the count sat off-screen until the reader scrolled right. A statistic you have to go looking for is not a statistic. The mistake was treating the header as document. The code rows scroll horizontally because they are the thing being read; a header describes what is being read and belongs to the frame around it. The chevron, pastille and path now pin to the viewport's left edge and the bar and count to its right, inset by `Scrollbar::width()` — gpui-component's own 16px, taken from the constant rather than copied, so it breaks loudly if it moves instead of drifting into the overlay scrollbar. Only the background band still spans the element, which is what keeps a scrolled header reading as one continuous row rather than as a label floating over the code. The viewport is unmeasured on the first frame. `row_window` already answers that with `UNMEASURED_ROWS`, and this takes the same shape: `header_budget` returns `None`, the header falls back to the element's own edges, nothing is elided, and the second frame corrects it. One `Option` carries that state, so the pin, the elision and the cell bounds cannot disagree about whether the viewport is known. Pinning is what bounds the path, and the bound is one number per frame: the viewport less the furniture at both ends, computed in `request_layout` beside the visible range. A path wider than it is elided from the left — `…detail/diff/split.rs` — because the tail of a path names the file and the head is the part that can be lost. `elide_path` binary-searches the tails, so it shapes about six candidates rather than one per character. The elided string is the one that becomes the run. Every reader of a cell's text now goes through one method, `cell_string`, so the layout, the run, the select-all range and both copy paths cannot disagree — a projected byte range indexing a different string than the one it was measured against would slice a `String` off a character boundary and panic. The price is the other half of that trade: copying a header whose path is too long for the panel yields the elided form rather than the whole path. Highlight and clipboard staying equal is worth more than the full path here, and it is the invariant the module already existed to protect. The click target is unmoved. `on_mouse_down` reads only `event.position.y` against a hitbox that is the whole element, so the entire row width still toggles and still resolves through `file_at` to the same file. Nothing else about a row changed: `content_width` still measures the unelided strings, so a header cell is still laid out against a width that exceeds it and a row is still one line tall. --- crates/ui/src/detail/diff/body.rs | 213 ++++++++++++++++++++++++------ crates/ui/src/detail/diff/mod.rs | 29 +++- 2 files changed, 203 insertions(+), 39 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index 9bf4c0d..03825ad 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -10,7 +10,7 @@ use gpui::{ relative, size, }; use gpui_base::{TextSelectionHandle, TextSelectionRegistration, TextSelectionRun}; -use gpui_component::{ThemeColor, ThemeMode}; +use gpui_component::{ThemeColor, ThemeMode, scroll::Scrollbar}; use super::model::{Row, header_line}; use super::pairing::SideLine; @@ -45,6 +45,8 @@ const BAR_MIN_SEGMENT: f32 = 2.; const BAR_MIN_WIDTH: f32 = 2. * BAR_MIN_SEGMENT; const BAR_GAP: f32 = 8.; const COUNT_WIDTH: f32 = 34.; +const HEADER_STATS_WIDTH: f32 = BAR_GAP + BAR_WIDTH + BAR_GAP + COUNT_WIDTH + HEADER_PADDING; +const ELLIPSIS: &str = "\u{2026}"; pub(super) enum Rows { Unified(Vec), @@ -77,10 +79,15 @@ impl Rows { } } + fn is_header(&self, row: usize) -> bool { + matches!(self.full(row), Some(Row::FileHeader { .. })) + } + fn cell_left(&self, row: usize) -> f32 { - match self.full(row) { - Some(Row::FileHeader { .. }) => HEADER_TEXT_LEFT, - _ => self.code_left(), + if self.is_header(row) { + HEADER_TEXT_LEFT + } else { + self.code_left() } } @@ -139,6 +146,8 @@ pub(super) struct DiffBody { theme: ThemeColor, mode: ThemeMode, visible: Range, + path_budget: Option, + display: Vec, texts: Vec, cell_bounds: Vec>, } @@ -161,6 +170,8 @@ pub(super) fn body( theme, mode, visible: 0..0, + path_budget: None, + display: Vec::new(), texts: Vec::new(), cell_bounds: Vec::new(), } @@ -261,6 +272,33 @@ fn bounds_for_cell( ) } +fn header_budget(viewport: Pixels) -> Option { + if viewport <= px(0.) { + return None; + } + let furniture = px(HEADER_TEXT_LEFT + HEADER_STATS_WIDTH) + Scrollbar::width(); + Some((viewport - furniture).max(px(0.))) +} + +fn elide_path(path: &str, budget: f32, width: impl Fn(&str) -> f32) -> Option { + if width(path) <= budget { + return None; + } + let tails: Vec = path + .char_indices() + .map(|(offset, _)| offset) + .skip(1) + .chain([path.len()]) + .collect(); + let elided = |tail: usize| format!("{ELLIPSIS}{}", &path[tail..]); + let index = tails.partition_point(|tail| width(&elided(*tail)) > budget); + Some( + tails + .get(index) + .map_or_else(|| ELLIPSIS.to_string(), |tail| elided(*tail)), + ) +} + #[derive(Clone, Copy, Debug, PartialEq)] struct Bar { added: f32, @@ -543,13 +581,41 @@ impl DiffBody { &self.content.strings } + fn header_frame(&self, bounds: Bounds) -> Bounds { + if self.path_budget.is_some() { + self.scroll.bounds() + } else { + bounds + } + } + + fn cell_string(&self, cell: usize, pen: &Pen, window: &Window) -> SharedString { + let text = self.content.strings[cell].clone(); + let columns = self.rows().columns(); + let Some(budget) = self + .path_budget + .filter(|_| self.rows().is_header(cell / columns)) + else { + return text; + }; + elide_path(&text, f32::from(budget), |candidate| { + f32::from(pen.width(candidate.to_string().into(), window)) + }) + .map_or(text, SharedString::from) + } + fn cell_bounds_at(&self, bounds: Bounds, cell: usize) -> Bounds { let columns = self.rows().columns(); - bounds_for_cell( - bounds, - columns, - px(self.rows().cell_left(cell / columns)), - cell, + let row = cell / columns; + let Some(budget) = self.path_budget.filter(|_| self.rows().is_header(row)) else { + return bounds_for_cell(bounds, columns, px(self.rows().cell_left(row)), cell); + }; + Bounds::new( + point( + self.header_frame(bounds).origin.x + px(HEADER_TEXT_LEFT), + bounds.origin.y + px(row as f32 * ROW_HEIGHT), + ), + size(budget, px(ROW_HEIGHT)), ) } @@ -575,13 +641,13 @@ impl DiffBody { self.visible.start * columns..self.visible.end * columns } - fn whole_text(&self) -> String { - let ranges: Vec>> = self - .strings() - .iter() - .map(|text| Some(0..text.len())) + fn whole_text(&self, pen: &Pen, window: &Window) -> String { + let texts: Vec = (0..self.rows().cells()) + .map(|cell| self.cell_string(cell, pen, window)) .collect(); - copy_text(self.strings(), &ranges, 0, self.rows().columns()) + let ranges: Vec>> = + texts.iter().map(|text| Some(0..text.len())).collect(); + copy_text(&texts, &ranges, 0, self.rows().columns()) } fn copy_selection( @@ -593,7 +659,7 @@ impl DiffBody { cx: &App, ) -> String { if self.select_all { - return self.whole_text(); + return self.whole_text(pen, window); } let Some(points) = self .selection @@ -616,27 +682,27 @@ impl DiffBody { let columns = self.rows().columns(); let visible = self.visible_cells(); let cells = rows.start() * columns..(rows.end() + 1) * columns; + let texts: Vec = cells + .clone() + .map(|cell| self.cell_string(cell, pen, window)) + .collect(); let ranges: Vec>> = cells .clone() - .map(|cell| { + .enumerate() + .map(|(offset, cell)| { if visible.contains(&cell) { return projected.get(cell - visible.start).and_then(Clone::clone); } let cell_bounds = self.cell_bounds_at(bounds, cell); let band = selection_band(cell_bounds.origin.y, anchor, cursor)?; - let text = &self.strings()[cell]; + let text = &texts[offset]; selected_range(text, band, cell_bounds.origin.x, || { pen.measure(text.clone(), window) }) }) .collect(); - copy_text( - &self.strings()[cells.clone()], - &ranges, - cells.start, - columns, - ) + copy_text(&texts, &ranges, cells.start, columns) } fn paint_background( @@ -769,6 +835,7 @@ impl DiffBody { return; }; + let frame = self.header_frame(bounds); let chevron = pen.shape( disclosure(*collapsed).into(), self.theme.muted_foreground, @@ -776,7 +843,7 @@ impl DiffBody { ); paint_line( &chevron, - point(bounds.origin.x + px(HEADER_PADDING), top), + point(frame.origin.x + px(HEADER_PADDING), top), window, cx, ); @@ -784,7 +851,7 @@ impl DiffBody { window.paint_quad( fill( Bounds::new( - point(bounds.origin.x + px(PASTILLE_LEFT), top + px(PASTILLE_TOP)), + point(frame.origin.x + px(PASTILLE_LEFT), top + px(PASTILLE_TOP)), size(px(PASTILLE_SIZE), px(PASTILLE_SIZE)), ), status_color(status, &self.theme), @@ -797,7 +864,7 @@ impl DiffBody { self.theme.foreground, window, ); - let count_right = bounds.right() - px(HEADER_PADDING); + let count_right = frame.right() - Scrollbar::width() - px(HEADER_PADDING); paint_line(&count, point(count_right - count.width(), top), window, cx); let Some(bar) = bar_widths(*added, *deleted, self.content.max_changes) else { @@ -874,16 +941,18 @@ impl Element for DiffBody { cx: &mut App, ) -> (LayoutId, Self::RequestLayoutState) { self.visible = self.visible_rows(); + self.path_budget = header_budget(self.scroll.bounds().size.width); + + let pen = Pen::new(window); + let display: Vec = self + .visible_cells() + .map(|cell| self.cell_string(cell, &pen, window)) + .collect(); + self.display = display; self.texts = self .visible_cells() - .map(|cell| { - styled_cell( - self.rows(), - cell, - self.content.strings[cell].clone(), - &self.theme, - ) - }) + .zip(&self.display) + .map(|(cell, text)| styled_cell(self.rows(), cell, text.clone(), &self.theme)) .collect(); let children: Vec = self @@ -957,7 +1026,7 @@ impl Element for DiffBody { .map(|(offset, text)| { let cell = first_cell + offset; TextSelectionRun::new( - self.content.strings[cell].clone(), + self.display[offset].clone(), text.layout().clone(), self.cell_bounds[offset], ) @@ -979,7 +1048,7 @@ impl Element for DiffBody { let cell_offset = offset * columns + column; let cell_bounds = self.cell_bounds[cell_offset]; let range = if self.select_all { - Some(0..self.content.strings[first_cell + cell_offset].len()) + Some(0..self.display[cell_offset].len()) } else { projection.ranges().get(cell_offset).and_then(Clone::clone) }; @@ -1176,6 +1245,76 @@ mod tests { ); } + fn monospace(text: &str) -> f32 { + text.chars().count() as f32 + } + + #[test] + fn a_path_that_fits_its_budget_is_left_alone() { + assert_eq!(elide_path("src/main.rs", 40., monospace), None); + assert_eq!( + elide_path("0123456789", 10., monospace), + None, + "a path exactly as wide as its budget still fits" + ); + assert_eq!(elide_path("", 0., monospace), None); + } + + #[test] + fn a_path_over_its_budget_loses_its_head_rather_than_its_tail() { + assert_eq!( + elide_path("crates/ui/src/detail.rs", 10., monospace), + Some("\u{2026}detail.rs".to_string()), + "the tail identifies the file, so the ellipsis leads" + ); + assert_eq!( + elide_path("0123456789", 9.9, monospace), + Some("\u{2026}23456789".to_string()), + "one character over budget drops two, since the ellipsis takes one" + ); + } + + #[test] + fn an_elided_path_never_exceeds_the_budget_it_was_given() { + let path = "crates/ui/src/detail/diff/body.rs"; + for budget in 2..=40 { + let budget = budget as f32; + let elided = elide_path(path, budget, monospace).unwrap_or_else(|| path.to_string()); + assert!( + monospace(&elided) <= budget, + "{elided:?} overruns a budget of {budget}" + ); + assert!( + path.ends_with(elided.trim_start_matches(ELLIPSIS)), + "{elided:?} is not a tail of {path:?}" + ); + } + } + + #[test] + fn a_budget_too_small_for_the_ellipsis_itself_keeps_the_ellipsis() { + assert_eq!( + elide_path("src/main.rs", 0.5, monospace), + Some(ELLIPSIS.to_string()), + "nothing fits, so the row says so rather than drawing a misleading tail" + ); + assert_eq!( + elide_path("src/main.rs", 1., monospace), + Some(ELLIPSIS.to_string()) + ); + } + + #[test] + fn an_unmeasured_viewport_has_no_budget_and_a_narrow_one_has_no_negative_budget() { + assert_eq!(header_budget(px(0.)), None); + assert_eq!(header_budget(px(-10.)), None); + assert_eq!(header_budget(px(1.)), Some(px(0.))); + let wide = header_budget(px(1000.)).expect("a measured viewport has a budget"); + let narrow = header_budget(px(600.)).expect("a measured viewport has a budget"); + assert_eq!(wide - narrow, px(400.), "the furniture is a fixed width"); + assert!(narrow > px(0.)); + } + #[test] fn a_bar_is_the_files_share_of_the_widest_one_in_the_patch() { assert_eq!( diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 19929ba..fec51ef 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -19,6 +19,27 @@ //! out against `content_width / columns` and the width is measured over every cell, a //! full-width header still fits in the half it is drawn in. //! +//! A file header is the one row that is not placed that way, because it is the one row that +//! is not document. The code rows scroll horizontally because they *are* the thing being +//! read; a header describes what is being read and belongs to the frame around it. So its +//! chevron, pastille and path pin to the viewport's left edge and its bar and count to the +//! viewport's right edge, inset by `gpui_component::scroll::Scrollbar::width()` so the count +//! never sits under the overlay scrollbar. Only the background band still spans the element, +//! which is what keeps a scrolled header reading as one continuous row rather than as a +//! label floating over the code. The viewport is unmeasured on the first frame, exactly as +//! it is for [`body::row_window`], and the fallback has the same shape: for that one frame +//! the header falls back to the element's own edges and nothing is elided, and the second +//! frame corrects it. +//! +//! Pinning is what bounds the path, and the bound is a single number per frame: +//! `body::header_budget` is the viewport less the furniture at both ends, computed in +//! `request_layout` beside the visible range. A path wider than it is elided from the +//! *left* — `…detail/diff/split.rs` — because the tail of a path is what names the file and +//! the head is the part that can be lost. The elided string is the one that becomes the run, +//! so what is highlighted and what is copied still cannot drift apart. The price is the +//! other half of that trade: copying a header whose path is too long for the panel yields +//! the elided form, not the whole path. +//! //! Selection comes back through `gpui-base`'s window-level participant system rather than //! from the editor. [`body`] is the element that joins it: it registers one participant and //! declares one run per cell on screen, left before right within a row, and only the code @@ -26,7 +47,7 @@ //! is painted directly and never registered: a line's gutters and its `+`/`−` marker, and a //! header's disclosure chevron, status pastille, change bar and change count. That is what //! keeps line numbers, markers and a file's statistics out of the clipboard, and it is why -//! copying a header yields a bare path. The rows scroll on both axes rather than +//! copying a header yields a bare path. The code rows scroll on both axes rather than //! soft-wrapping, //! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the //! reason it usually does: the container's `overflow_scroll()` puts both axes in @@ -76,7 +97,11 @@ //! largest file, derived by [`content`] when the patch, the view mode or the set of collapsed //! files changes and handed to the element behind an `Rc` thereafter. So after the first //! frame each cell is a hit in gpui's line-layout cache and the steady-state cost is a hash -//! of bytes that are already there, with no allocation and no reshape. That last number is +//! of bytes that are already there, with no allocation and no reshape. A header row is the +//! exception in one respect: its elision binary-searches the tail against the shaped width, +//! so it allocates a handful of candidates per header per frame. They are the same +//! candidates every frame, so they settle into the same cache, and there are at most a +//! screenful of headers — but it is a search, not a lookup. That last number is //! the scale every header's change bar is drawn against, and it belongs to the derivation for //! the same reason the strings do: a bar is a share of the widest file in the patch, so //! sizing one from the element would mean walking every file of the patch on every frame. From 64154b2d3e05e5a29c86550cdea8478dbbfb97f8 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 21:11:45 +0200 Subject: [PATCH 6/7] refactor(diff): drop the +/- marker and give its column to the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The row's background tint carries the addition/deletion signal on its own, which is the whole point of moving off the code editor: a full-width band does what a tint hugging the glyphs could not. Seen running, the marker read as leftover furniture between the line numbers and the code. So the column goes rather than being blanked. code_left() is the gutters alone now — 2 * GUTTER_WIDTH unified, GUTTER_WIDTH split — and every code cell gains sixteen pixels. The gutter numbers do not move: paint_gutter recovers the row's left edge as origin.x - code_left(), and both terms shrank by the same sixteen. LineColors::foreground had one caller, the marker's tint, so it is dead the moment the glyph goes and a dead field is a -D warnings failure. line_colors becomes line_background(origin, mode) -> Option; the theme argument went with the field, since only the foreground arm ever read it. The four tint constants and the two-bar contrast reasoning behind them are untouched. The disclosure chevron stays. It positions itself from HEADER_PADDING and DISCLOSURE_WIDTH, not from the marker geometry — verified before deleting anything — so nothing in the header moved. This reverses a Decisions bullet in the design spec, which kept the marker on the grounds that it carried the signal for anyone who reads green and red poorly. That argument was made against a mock-up and is outweighed rather than answered, so the bullet is rewritten rather than dropped, and it records the cheapest mitigation if it is revisited: a 2px coloured edge down the row, which puts shape back to work without putting a character back in the text. --- crates/ui/src/detail/diff/body.rs | 82 ++++++++----------- crates/ui/src/detail/diff/mod.rs | 13 +-- crates/ui/src/detail/diff/palette.rs | 68 ++++----------- ...026-08-29-github-style-diff-view-design.md | 27 ++++-- 4 files changed, 74 insertions(+), 116 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index 03825ad..904fa8b 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -1,7 +1,7 @@ use std::ops::{Range, RangeInclusive}; use std::rc::Rc; -use domain::{FileStatus, LineOrigin}; +use domain::FileStatus; use gpui::{ App, Bounds, DispatchPhase, Element, ElementId, FlexDirection, FontWeight, GlobalElementId, Half as _, HighlightStyle, Hitbox, HitboxBehavior, Hsla, InspectorElementId, IntoElement, @@ -14,18 +14,16 @@ use gpui_component::{ThemeColor, ThemeMode, scroll::Scrollbar}; use super::model::{Row, header_line}; use super::pairing::SideLine; -use super::palette::line_colors; +use super::palette::line_background; use super::split::SplitRow; use super::{DiffContent, ToggleFile}; pub(super) const ROW_HEIGHT: f32 = 18.; const GUTTER_WIDTH: f32 = 44.; -const MARKER_WIDTH: f32 = 16.; const GUTTER_PADDING: f32 = 8.; -const MARKER_PADDING: f32 = 5.; -const CODE_LEFT: f32 = 2. * GUTTER_WIDTH + MARKER_WIDTH; -const SPLIT_CODE_LEFT: f32 = GUTTER_WIDTH + MARKER_WIDTH; +const CODE_LEFT: f32 = 2. * GUTTER_WIDTH; +const SPLIT_CODE_LEFT: f32 = GUTTER_WIDTH; const COLUMN_RULE_WIDTH: f32 = 1.; const TRAILING_SPACE: f32 = 16.; const UNMEASURED_ROWS: usize = 100; @@ -95,10 +93,6 @@ impl Rows { self.len() * self.columns() } - fn marker_left(&self) -> f32 { - self.code_left() - MARKER_WIDTH + MARKER_PADDING - } - fn full(&self, row: usize) -> Option<&Row> { match self { Rows::Unified(rows) => rows.get(row), @@ -230,15 +224,7 @@ fn row_background(row: &Row, theme: &ThemeColor, mode: ThemeMode) -> Option Some(theme.secondary), Row::Separator => Some(theme.muted), Row::Placeholder { .. } => None, - Row::Line { origin, .. } => line_colors(*origin, mode, theme).background, - } -} - -fn marker(origin: LineOrigin) -> &'static str { - match origin { - LineOrigin::Addition => "+", - LineOrigin::Deletion => "\u{2212}", - LineOrigin::Context => " ", + Row::Line { origin, .. } => line_background(*origin, mode), } } @@ -725,9 +711,11 @@ impl DiffBody { } None => { for column in 0..columns { - let Some(background) = self.rows().side(row, column).and_then(|side| { - line_colors(side.origin, self.mode, &self.theme).background - }) else { + let Some(background) = self + .rows() + .side(row, column) + .and_then(|side| line_background(side.origin, self.mode)) + else { continue; }; let band = Bounds::new( @@ -759,13 +747,11 @@ impl DiffBody { ) { let left = cell_bounds.origin.x - px(self.rows().code_left()); let top = cell_bounds.origin.y; - let marker_left = left + px(self.rows().marker_left()); let muted = self.theme.muted_foreground; - let origin = match self.rows() { + match self.rows() { Rows::Unified(rows) => { let Row::Line { - origin, old_number, new_number, .. @@ -791,7 +777,6 @@ impl DiffBody { window, cx, ); - origin } Rows::Split(_) => { let Some(side) = self.rows().side(row, column) else { @@ -806,13 +791,8 @@ impl DiffBody { window, cx, ); - side.origin } - }; - - let marker_color = line_colors(origin, self.mode, &self.theme).foreground; - let line = pen.shape(marker(origin).into(), marker_color, window); - paint_line(&line, point(marker_left, top), window, cx); + } } fn paint_header( @@ -1085,6 +1065,7 @@ impl Element for DiffBody { #[cfg(test)] mod tests { use super::*; + use domain::LineOrigin; fn file_header() -> Row { Row::FileHeader { @@ -1139,11 +1120,11 @@ mod tests { } #[test] - fn a_marker_sits_in_the_last_gutter_a_view_has() { + fn code_starts_immediately_after_the_gutters_a_view_has() { let unified = Rows::Unified(vec![file_header()]); - assert_eq!(unified.marker_left(), 2. * GUTTER_WIDTH + MARKER_PADDING); - assert_eq!(split_rows().marker_left(), GUTTER_WIDTH + MARKER_PADDING); + assert_eq!(unified.code_left(), 2. * GUTTER_WIDTH); + assert_eq!(split_rows().code_left(), GUTTER_WIDTH); } #[test] @@ -1432,7 +1413,7 @@ mod tests { assert_eq!( row_text(&collapsed), row_text(&file_header()), - "the marker is painted in the gutter, so no run and no copy can carry it" + "the chevron is painted beside the path, so no run and no copy can carry it" ); } @@ -1443,13 +1424,6 @@ mod tests { assert_eq!(cell_text(&rows, 3), SharedString::default()); } - #[test] - fn a_marker_names_the_origin_and_a_context_line_keeps_the_column_wide() { - assert_eq!(marker(LineOrigin::Addition), "+"); - assert_eq!(marker(LineOrigin::Deletion), "\u{2212}"); - assert_eq!(marker(LineOrigin::Context), " "); - } - #[test] fn only_a_changed_line_the_file_header_and_a_separator_are_banded() { let theme = ThemeColor::light(); @@ -1479,7 +1453,7 @@ mod tests { ); assert_eq!( row_background(&line(LineOrigin::Addition, "new"), &theme, mode), - line_colors(LineOrigin::Addition, mode, &theme).background + line_background(LineOrigin::Addition, mode) ); } @@ -1498,23 +1472,31 @@ mod tests { let bounds = Bounds::new(point(px(10.), px(20.)), size(px(200.), px(400.))); assert_eq!( - bounds_for_cell(bounds, 2, px(60.), 3), + bounds_for_cell(bounds, 2, px(SPLIT_CODE_LEFT), 3), Bounds::new( - point(px(170.), px(20. + ROW_HEIGHT)), - size(px(40.), px(ROW_HEIGHT)) + point(px(110. + SPLIT_CODE_LEFT), px(20. + ROW_HEIGHT)), + size(px(100. - SPLIT_CODE_LEFT), px(ROW_HEIGHT)) ), "cell 3 is the right column of the second row" ); assert_eq!( - bounds_for_cell(bounds, 1, px(104.), 0), - Bounds::new(point(px(114.), px(20.)), size(px(96.), px(ROW_HEIGHT))) + bounds_for_cell(bounds, 1, px(CODE_LEFT), 0), + Bounds::new( + point(px(10. + CODE_LEFT), px(20.)), + size(px(200. - CODE_LEFT), px(ROW_HEIGHT)) + ) ); } #[test] fn a_column_narrower_than_its_gutters_leaves_no_width_rather_than_a_negative_one() { let bounds = Bounds::new(point(px(0.), px(0.)), size(px(80.), px(400.))); - assert_eq!(bounds_for_cell(bounds, 2, px(60.), 0).size.width, px(0.)); + assert_eq!( + bounds_for_cell(bounds, 2, px(SPLIT_CODE_LEFT), 0) + .size + .width, + px(0.) + ); } #[test] diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index fec51ef..9d09329 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -7,8 +7,9 @@ //! full-width row background outside the cursor line, so an addition's tint hugs the glyphs //! and a blank added line gets none at all; and it has no per-line element hook to work //! around either. The `+`/`-` markers were carrying the signal the colour only half -//! carried. All three follow from the diff being one text document, so the rows are built -//! here instead — see the design note under `docs/superpowers/specs/`. +//! carried; a full-width tint carries it whole, so this view draws no markers at all. All +//! three follow from the diff being one text document, so the rows are built here instead — +//! see the design note under `docs/superpowers/specs/`. //! //! A row carries one cell in [`DiffViewMode::Unified`] and two in [`DiffViewMode::Split`], //! which is the only difference between the two views: [`body::Rows`] answers how many @@ -44,10 +45,10 @@ //! from the editor. [`body`] is the element that joins it: it registers one participant and //! declares one run per cell on screen, left before right within a row, and only the code //! text — or, on a file header, the path alone — becomes a run. Everything else a row draws -//! is painted directly and never registered: a line's gutters and its `+`/`−` marker, and a -//! header's disclosure chevron, status pastille, change bar and change count. That is what -//! keeps line numbers, markers and a file's statistics out of the clipboard, and it is why -//! copying a header yields a bare path. The code rows scroll on both axes rather than +//! is painted directly and never registered: a line's two gutters, and a header's disclosure +//! chevron, status pastille, change bar and change count. That is what keeps line numbers and +//! a file's statistics out of the clipboard, and it is why copying a header yields a bare +//! path. The code rows scroll on both axes rather than //! soft-wrapping, //! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the //! reason it usually does: the container's `overflow_scroll()` puts both axes in diff --git a/crates/ui/src/detail/diff/palette.rs b/crates/ui/src/detail/diff/palette.rs index 74015ff..59f84ff 100644 --- a/crates/ui/src/detail/diff/palette.rs +++ b/crates/ui/src/detail/diff/palette.rs @@ -1,6 +1,6 @@ use domain::LineOrigin; use gpui::{Hsla, rgb}; -use gpui_component::{ThemeColor, ThemeMode}; +use gpui_component::ThemeMode; const LIGHT_ADDITION_BACKGROUND: u32 = 0xdafbe1; const LIGHT_DELETION_BACKGROUND: u32 = 0xffebe9; @@ -17,36 +17,19 @@ const DARK_ADDITION_BACKGROUND: u32 = 0x355a40; /// Mirrors [`DARK_ADDITION_BACKGROUND`]'s two-bar reasoning: 1.30:1 against Frappé's /// background, 6.20:1 under the same `#c6d0f5`. A deletion can afford a darker plate than -/// an addition because nothing has to read *as* red on it — only the marker is tinted, and -/// it is drawn in `theme.red` rather than in the plate's own hue. +/// an addition because nothing has to read *as* red on it. const DARK_DELETION_BACKGROUND: u32 = 0x5f3c45; -/// The plate a line of a given origin sits on, and the colour of its `+`/`−` marker. -/// -/// `foreground` is the marker's colour alone. The code itself is painted in -/// `theme.foreground` whatever its origin — which is the measurement the two dark -/// constants above are chosen against. -pub(super) struct LineColors { - pub background: Option, - pub foreground: Hsla, -} - -pub(super) fn line_colors(origin: LineOrigin, mode: ThemeMode, theme: &ThemeColor) -> LineColors { - let background = match (origin, mode.is_dark()) { +/// The plate a line of a given origin sits on, and the whole of the signal that it is an +/// addition or a deletion. The code itself is painted in `theme.foreground` whatever its +/// origin — which is the measurement the two dark constants above are chosen against. +pub(super) fn line_background(origin: LineOrigin, mode: ThemeMode) -> Option { + match (origin, mode.is_dark()) { (LineOrigin::Context, _) => None, (LineOrigin::Addition, false) => Some(rgb(LIGHT_ADDITION_BACKGROUND).into()), (LineOrigin::Addition, true) => Some(rgb(DARK_ADDITION_BACKGROUND).into()), (LineOrigin::Deletion, false) => Some(rgb(LIGHT_DELETION_BACKGROUND).into()), (LineOrigin::Deletion, true) => Some(rgb(DARK_DELETION_BACKGROUND).into()), - }; - let foreground = match origin { - LineOrigin::Context => theme.muted_foreground, - LineOrigin::Addition => theme.green, - LineOrigin::Deletion => theme.red, - }; - LineColors { - background, - foreground, } } @@ -56,55 +39,34 @@ mod tests { #[test] fn light_mode_uses_the_exact_given_hex_values() { - let theme = ThemeColor::light(); assert_eq!( - line_colors(LineOrigin::Addition, ThemeMode::Light, &theme).background, + line_background(LineOrigin::Addition, ThemeMode::Light), Some(rgb(0xdafbe1).into()) ); assert_eq!( - line_colors(LineOrigin::Deletion, ThemeMode::Light, &theme).background, + line_background(LineOrigin::Deletion, ThemeMode::Light), Some(rgb(0xffebe9).into()) ); } - #[test] - fn a_marker_is_tinted_by_its_origin_rather_than_by_the_code_colour() { - let theme = ThemeColor::dark(); - let added = line_colors(LineOrigin::Addition, ThemeMode::Dark, &theme).foreground; - let deleted = line_colors(LineOrigin::Deletion, ThemeMode::Dark, &theme).foreground; - let context = line_colors(LineOrigin::Context, ThemeMode::Dark, &theme).foreground; - - assert_eq!(added, theme.green); - assert_eq!(deleted, theme.red); - assert_eq!(context, theme.muted_foreground); - assert_ne!(added, theme.foreground); - assert_ne!(deleted, theme.foreground); - } - #[test] fn a_context_line_has_no_background() { - let theme = ThemeColor::light(); - assert!( - line_colors(LineOrigin::Context, ThemeMode::Light, &theme) - .background - .is_none() - ); + assert!(line_background(LineOrigin::Context, ThemeMode::Light).is_none()); + assert!(line_background(LineOrigin::Context, ThemeMode::Dark).is_none()); } #[test] fn an_addition_and_a_deletion_do_not_share_a_background() { - let theme = ThemeColor::light(); - let added = line_colors(LineOrigin::Addition, ThemeMode::Light, &theme).background; - let deleted = line_colors(LineOrigin::Deletion, ThemeMode::Light, &theme).background; + let added = line_background(LineOrigin::Addition, ThemeMode::Light); + let deleted = line_background(LineOrigin::Deletion, ThemeMode::Light); assert!(added.is_some() && deleted.is_some()); assert_ne!(added, deleted); } #[test] fn dark_mode_does_not_reuse_the_light_pair() { - let theme = ThemeColor::dark(); - let light = line_colors(LineOrigin::Addition, ThemeMode::Light, &theme).background; - let dark = line_colors(LineOrigin::Addition, ThemeMode::Dark, &theme).background; + let light = line_background(LineOrigin::Addition, ThemeMode::Light); + let dark = line_background(LineOrigin::Addition, ThemeMode::Dark); assert_ne!(light, dark); } } diff --git a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md index 4565ba9..21b01a3 100644 --- a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md +++ b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md @@ -70,10 +70,23 @@ every row's wrapped height depends on the viewport width and has to be computed anything can be placed. The choice stands on fidelity and on that cost, and can be revisited without invalidating the rest of this design. -**The `+`/`-` marker stays, in a column of its own.** Sixteen pixels, between the gutters -and the code, as in the deleted renderer. It no longer shifts the code, it carries the -signal for anyone who reads the green and the red poorly, and being a separate element it -never reaches the clipboard. +**The `+`/`-` marker goes, column and all.** This reverses the original decision, which was +to keep it in sixteen pixels of its own between the gutters and the code, on the grounds +that it carried the signal for anyone who reads the green and the red poorly. That argument +was made against a mock-up. Seen running, the full-width row tint — the thing this whole +design exists to make possible, and the thing the editor could not draw — turned out to +carry the signal on its own, and the marker read as leftover furniture between the line +numbers and the code. So `MARKER_WIDTH`, `MARKER_PADDING` and the glyph itself are deleted +rather than blanked, and `code_left()` is now the gutters alone: `2 × GUTTER_WIDTH` unified, +`GUTTER_WIDTH` split. The code gains the sixteen pixels. + +The accessibility argument is not answered, only outweighed, and it is the reason to record +this rather than to drop the bullet. If it is ever revisited, the cheapest mitigation is not +to put the character back: it is a 2px coloured edge down the left of the row, inside the +tint. That makes *shape* carry the information — which is what the marker was really for — +without returning a glyph to the text, so it stays out of every run and every clipboard and +costs the code no width. `palette::line_background` is where the per-origin colour it would +need used to live. **The domain layer does not change.** `DiffLine` already stores `old_number`, `new_number`, and a `content` with the marker stripped (`crates/domain/src/patch.rs:27`, parser at @@ -150,9 +163,9 @@ in `prepaint`/`paint`: difference of the two origins keeps the sum identical by construction and puts the trigger zone where the user can reach it. - `TextSelectionRun::new(text, layout, bounds)` — **only for the code content**. The - gutters and the marker are neighbouring elements and are never registered, which is what - makes a copied diff come out as clean code with no line numbers and no markers. This is - strictly better than the editor, which copies its markers today. + gutters are painted directly and are never registered, which is what makes a copied diff + come out as clean code with no line numbers. This is strictly better than the editor, + which copies its markers today. - `TextSelectionContentKey` turns out not to be needed. It is a `u64` the participant computes from an endpoint's content point through `TextSelectionHandle::resolve_content_key_with`, which `gpui-base` stores on the endpoint From ca9cd9e7dad4fc1ec68f2d2f8d64e307ab3a1aeb Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 21:29:06 +0200 Subject: [PATCH 7/7] fix(diff): index a copied range into the string it was measured against MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit copy_selection's visible branch took its range from the projection, which gpui-base computed against display[cell - visible.start] in request_layout, and then sliced texts[offset], which this function rebuilt in paint with a second cell_string call and its own Pen. The two agree today: path_budget is a field, so it is fixed within a frame, and the text style stack is the same in both phases. But that is two things happening to agree, not an invariant. The cell_string funnel prevents the strings being formatted differently; it cannot prevent them being measured differently, and the failure mode is copy_text slicing on a char boundary that the other string does not have — a panic, not a wrong character. So the visible branch reads its string out of display as well. One closure answers whether a cell is displayed, and both the text map and the range map consult it, which also makes the fallback symmetric: a cell display has no entry for falls through to cell_string in both maps together. Five findings from the same review, none load-bearing: header_budget and paint_header derived the same clearance from the same constants independently. count_right_edge and bar_right_edge now name that arithmetic, and a test pins the widest bar the painter can draw to exactly BAR_GAP right of where the budget stops the path. elide_path is careful about char boundaries and was only ever tested on ASCII. The sweep gains an accented path, and one case pins that the cut is a byte offset landing on a boundary rather than a char count. "That last number" in diff/mod.rs pointed at max_changes until c76c5a6 put the elision aside between them; it is named outright now. Reflowed the two paragraphs my previous commit left ragged. header_line was path.to_string(), called by row_text to build a string it already held. Dropped rather than kept: the seam carried no decision, and three of its four test assertions sat beside an identical assertion on the path itself. --- crates/ui/src/detail/diff/body.rs | 82 +++++++++++++++++++++++------- crates/ui/src/detail/diff/mod.rs | 23 ++++----- crates/ui/src/detail/diff/model.rs | 19 ++----- 3 files changed, 80 insertions(+), 44 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index 904fa8b..0636c61 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -12,7 +12,7 @@ use gpui::{ use gpui_base::{TextSelectionHandle, TextSelectionRegistration, TextSelectionRun}; use gpui_component::{ThemeColor, ThemeMode, scroll::Scrollbar}; -use super::model::{Row, header_line}; +use super::model::Row; use super::pairing::SideLine; use super::palette::line_background; use super::split::SplitRow; @@ -191,7 +191,7 @@ fn cell_text(rows: &Rows, cell: usize) -> SharedString { fn row_text(row: &Row) -> SharedString { match row { - Row::FileHeader { path, .. } => header_line(path).into(), + Row::FileHeader { path, .. } => path.clone().into(), Row::Separator => SharedString::default(), Row::Line { content, .. } => content.clone().into(), Row::Placeholder { message } => (*message).into(), @@ -266,6 +266,14 @@ fn header_budget(viewport: Pixels) -> Option { Some((viewport - furniture).max(px(0.))) } +fn count_right_edge(frame_right: Pixels) -> Pixels { + frame_right - Scrollbar::width() - px(HEADER_PADDING) +} + +fn bar_right_edge(frame_right: Pixels, count_width: Pixels) -> Pixels { + count_right_edge(frame_right) - px(BAR_GAP) - px(COUNT_WIDTH).max(count_width) +} + fn elide_path(path: &str, budget: f32, width: impl Fn(&str) -> f32) -> Option { if width(path) <= budget { return None; @@ -668,15 +676,25 @@ impl DiffBody { let columns = self.rows().columns(); let visible = self.visible_cells(); let cells = rows.start() * columns..(rows.end() + 1) * columns; + let displayed = |cell: usize| { + visible + .contains(&cell) + .then(|| self.display.get(cell - visible.start)) + .flatten() + }; let texts: Vec = cells .clone() - .map(|cell| self.cell_string(cell, pen, window)) + .map(|cell| { + displayed(cell) + .cloned() + .unwrap_or_else(|| self.cell_string(cell, pen, window)) + }) .collect(); let ranges: Vec>> = cells .clone() .enumerate() .map(|(offset, cell)| { - if visible.contains(&cell) { + if displayed(cell).is_some() { return projected.get(cell - visible.start).and_then(Clone::clone); } let cell_bounds = self.cell_bounds_at(bounds, cell); @@ -844,13 +862,13 @@ impl DiffBody { self.theme.foreground, window, ); - let count_right = frame.right() - Scrollbar::width() - px(HEADER_PADDING); + let count_right = count_right_edge(frame.right()); paint_line(&count, point(count_right - count.width(), top), window, cx); let Some(bar) = bar_widths(*added, *deleted, self.content.max_changes) else { return; }; - let bar_right = count_right - px(BAR_GAP) - px(COUNT_WIDTH).max(count.width()); + let bar_right = bar_right_edge(frame.right(), count.width()); let bar_left = bar_right - px(bar.added + bar.deleted); let bar_top = top + px(BAR_TOP); for (offset, width, color) in [ @@ -1253,22 +1271,32 @@ mod tests { Some("\u{2026}23456789".to_string()), "one character over budget drops two, since the ellipsis takes one" ); + assert_eq!( + elide_path("d\u{e9}tail.rs", 5., monospace), + Some("\u{2026}l.rs".to_string()), + "the cut is a byte offset that lands on a character boundary, not a char count" + ); } #[test] fn an_elided_path_never_exceeds_the_budget_it_was_given() { - let path = "crates/ui/src/detail/diff/body.rs"; - for budget in 2..=40 { - let budget = budget as f32; - let elided = elide_path(path, budget, monospace).unwrap_or_else(|| path.to_string()); - assert!( - monospace(&elided) <= budget, - "{elided:?} overruns a budget of {budget}" - ); - assert!( - path.ends_with(elided.trim_start_matches(ELLIPSIS)), - "{elided:?} is not a tail of {path:?}" - ); + for path in [ + "crates/ui/src/detail/diff/body.rs", + "crates/ui/src/d\u{e9}tail/diff/b\u{f4}dy.rs", + ] { + for budget in 2..=40 { + let budget = budget as f32; + let elided = + elide_path(path, budget, monospace).unwrap_or_else(|| path.to_string()); + assert!( + monospace(&elided) <= budget, + "{elided:?} overruns a budget of {budget}" + ); + assert!( + path.ends_with(elided.trim_start_matches(ELLIPSIS)), + "{elided:?} is not a tail of {path:?}" + ); + } } } @@ -1296,6 +1324,24 @@ mod tests { assert!(narrow > px(0.)); } + #[test] + fn the_path_budget_stops_a_gap_short_of_the_leftmost_bar_the_painter_can_draw() { + let width = px(1000.); + let budget = header_budget(width).expect("a measured viewport has a budget"); + let path_right = px(HEADER_TEXT_LEFT) + budget; + + assert_eq!( + bar_right_edge(width, px(0.)) - px(BAR_WIDTH) - path_right, + px(BAR_GAP), + "the budget and the painter derive the same clearance from the same constants" + ); + assert_eq!( + bar_right_edge(width, px(COUNT_WIDTH + BAR_GAP)) - px(BAR_WIDTH) - path_right, + px(0.), + "a count wider than its column eats the gap before it reaches the path" + ); + } + #[test] fn a_bar_is_the_files_share_of_the_widest_one_in_the_patch() { assert_eq!( diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 9d09329..45ff62d 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -48,9 +48,8 @@ //! is painted directly and never registered: a line's two gutters, and a header's disclosure //! chevron, status pastille, change bar and change count. That is what keeps line numbers and //! a file's statistics out of the clipboard, and it is why copying a header yields a bare -//! path. The code rows scroll on both axes rather than -//! soft-wrapping, -//! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the +//! path. The code rows scroll on both axes rather than soft-wrapping, matching GitHub. +//! `restrict_scroll_to_axis` still earns its place here, but not for the //! reason it usually does: the container's `overflow_scroll()` puts both axes in //! `Overflow::Scroll`, and gpui's vertical-onto-horizontal remap (`div.rs:3220-3224`, //! `:3229-3233`) only fires when one axis is *not* `Overflow::Scroll`, so that remap is @@ -81,12 +80,12 @@ //! projection still agree cell for cell. Endpoints survive scrolling because `gpui-base` //! stores them relative to `bounds.origin + scroll_offset`, and the participant reports the //! two so that their sum is this element's own origin, which already carries the scroll: a -//! point off the top of the viewport is then a negative `y` rather than a lost one. Rows -//! that *are* on screen keep the projection's -//! own range, so what is highlighted and what is copied cannot drift apart. Select All is -//! the one selection with no window points to derive anything from — it is participant-local -//! (`set_local_selection`), so `copy_selection` answers the whole document for it directly -//! and every visible cell is highlighted whole. +//! point off the top of the viewport is then a negative `y` rather than a lost one. Rows that +//! *are* on screen keep both the projection's range and the string it was measured against, +//! read out of the one `display` array the runs were declared from, so what is highlighted and +//! what is copied cannot drift apart. Select All is the one selection with no window points to +//! derive anything from — it is participant-local (`set_local_selection`), so `copy_selection` +//! answers the whole document for it directly and every visible cell is highlighted whole. //! //! What stays unwindowed is the content width: `body::DiffBody::content_width` shapes every //! cell on every layout pass, because the horizontal scroll extent has to consider rows that @@ -102,9 +101,9 @@ //! exception in one respect: its elision binary-searches the tail against the shaped width, //! so it allocates a handful of candidates per header per frame. They are the same //! candidates every frame, so they settle into the same cache, and there are at most a -//! screenful of headers — but it is a search, not a lookup. That last number is -//! the scale every header's change bar is drawn against, and it belongs to the derivation for -//! the same reason the strings do: a bar is a share of the widest file in the patch, so +//! screenful of headers — but it is a search, not a lookup. The largest file's total changes +//! is the scale every header's change bar is drawn against, and it belongs to the derivation +//! for the same reason the strings do: a bar is a share of the widest file in the patch, so //! sizing one from the element would mean walking every file of the patch on every frame. //! Collapsing is applied in that same derivation — the body rows of a collapsed file are //! never emitted — so the element windows, paints and selects over a shorter list and knows diff --git a/crates/ui/src/detail/diff/model.rs b/crates/ui/src/detail/diff/model.rs index b671c00..4936a3b 100644 --- a/crates/ui/src/detail/diff/model.rs +++ b/crates/ui/src/detail/diff/model.rs @@ -47,10 +47,6 @@ pub(super) fn file_header(index: usize, file: &FilePatch, collapsed: bool) -> Ro } } -pub(super) fn header_line(path: &str) -> String { - path.to_string() -} - pub(super) fn max_changes(patch: &Patch) -> usize { patch .files @@ -261,13 +257,11 @@ mod tests { panic!("a file yields a header row"); }; - assert_eq!(path, "src/old.rs \u{2192} src/new.rs"); - assert_eq!(status, FileStatus::Renamed { similarity: 87 }); assert_eq!( - header_line(&path), - "src/old.rs \u{2192} src/new.rs", + path, "src/old.rs \u{2192} src/new.rs", "the similarity rides on the status, which is painted as a pastille" ); + assert_eq!(status, FileStatus::Renamed { similarity: 87 }); } #[test] @@ -283,7 +277,6 @@ mod tests { assert_eq!(path, "src/old.rs \u{2192} src/copy.rs"); assert_eq!(status, FileStatus::Copied { similarity: 100 }); - assert_eq!(header_line(&path), "src/old.rs \u{2192} src/copy.rs"); } #[test] @@ -300,7 +293,7 @@ mod tests { } #[test] - fn a_header_line_carries_the_path_and_nothing_else() { + fn a_header_carries_the_path_and_nothing_else() { let patch = Patch { files: vec![file(vec![one_line()], false)], }; @@ -317,8 +310,7 @@ mod tests { assert_eq!(added, 1); assert_eq!(deleted, 0); assert_eq!( - header_line(&path), - "src/main.rs", + path, "src/main.rs", "the counts are painted, so a run cannot copy them with the path" ); } @@ -430,8 +422,7 @@ mod tests { assert!(collapsed); assert_eq!( - header_line(&path), - "src/main.rs", + path, "src/main.rs", "the disclosure chevron is painted, because a run would copy it" ); }