diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index fd44881..0636c61 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -1,41 +1,58 @@ use std::ops::{Range, RangeInclusive}; use std::rc::Rc; -use domain::LineOrigin; +use domain::FileStatus; 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, 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}; +use gpui_component::{ThemeColor, ThemeMode, scroll::Scrollbar}; -use super::DiffContent; -use super::model::{Row, header_line}; +use super::model::Row; 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; +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.; +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), Split(Vec), } impl Rows { - fn len(&self) -> usize { + pub(super) fn len(&self) -> usize { match self { Rows::Unified(rows) => rows.len(), Rows::Split(rows) => rows.len(), @@ -60,10 +77,39 @@ impl Rows { } } + fn is_header(&self, row: usize) -> bool { + matches!(self.full(row), Some(Row::FileHeader { .. })) + } + + fn cell_left(&self, row: usize) -> f32 { + if self.is_header(row) { + HEADER_TEXT_LEFT + } else { + self.code_left() + } + } + fn cells(&self) -> usize { self.len() * self.columns() } + 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) => Some(full), + SplitRow::Sides { .. } => None, + }, + } + } + + fn file_at(&self, row: usize) -> Option { + match self.full(row)? { + Row::FileHeader { index, .. } => Some(*index), + _ => None, + } + } + fn side(&self, row: usize, column: usize) -> Option<&SideLine> { let Rows::Split(rows) = self else { return None; @@ -90,9 +136,12 @@ pub(super) struct DiffBody { select_all: bool, selection: TextSelectionHandle, scroll: ScrollHandle, + toggle_file: ToggleFile, theme: ThemeColor, mode: ThemeMode, visible: Range, + path_budget: Option, + display: Vec, texts: Vec, cell_bounds: Vec>, } @@ -102,6 +151,7 @@ pub(super) fn body( select_all: bool, selection: TextSelectionHandle, scroll: ScrollHandle, + toggle_file: ToggleFile, theme: ThemeColor, mode: ThemeMode, ) -> DiffBody { @@ -110,9 +160,12 @@ pub(super) fn body( select_all, selection, scroll, + toggle_file, theme, mode, visible: 0..0, + path_budget: None, + display: Vec::new(), texts: Vec::new(), cell_bounds: Vec::new(), } @@ -138,13 +191,8 @@ 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::HunkHeader { text } => text.clone().into(), + Row::FileHeader { path, .. } => path.clone().into(), + Row::Separator => SharedString::default(), Row::Line { content, .. } => content.clone().into(), Row::Placeholder { message } => (*message).into(), } @@ -160,38 +208,28 @@ 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 { 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, + Row::Line { origin, .. } => line_background(*origin, mode), } } -fn marker(origin: LineOrigin) -> &'static str { - match origin { - LineOrigin::Addition => "+", - LineOrigin::Deletion => "\u{2212}", - LineOrigin::Context => " ", - } +fn disclosure(collapsed: bool) -> &'static str { + if collapsed { "\u{25b8}" } else { "\u{25be}" } } fn column_width(bounds: Bounds, columns: usize) -> Pixels { @@ -205,21 +243,92 @@ 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), ), ) } +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 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; + } + 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, + 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, @@ -259,6 +368,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, @@ -400,8 +514,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) @@ -446,12 +575,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 { - bounds_for_cell( - bounds, - self.rows().columns(), - px(self.rows().code_left()), - cell, + let columns = self.rows().columns(); + 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)), ) } @@ -477,13 +635,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( @@ -495,7 +653,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 @@ -518,27 +676,37 @@ impl DiffBody { let columns = self.rows().columns(); let visible = self.visible_cells(); let cells = rows.start() * columns..(rows.end() + 1) * columns; - let ranges: Vec>> = cells + let displayed = |cell: usize| { + visible + .contains(&cell) + .then(|| self.display.get(cell - visible.start)) + .flatten() + }; + let texts: Vec = cells .clone() .map(|cell| { - if visible.contains(&cell) { + displayed(cell) + .cloned() + .unwrap_or_else(|| self.cell_string(cell, pen, window)) + }) + .collect(); + let ranges: Vec>> = cells + .clone() + .enumerate() + .map(|(offset, 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); 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( @@ -549,15 +717,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( @@ -569,9 +729,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( @@ -604,10 +766,10 @@ impl DiffBody { let left = cell_bounds.origin.x - px(self.rows().code_left()); let top = cell_bounds.origin.y; let muted = self.theme.muted_foreground; - let (origin, marker_left) = match self.rows() { + + match self.rows() { Rows::Unified(rows) => { let Row::Line { - origin, old_number, new_number, .. @@ -633,7 +795,6 @@ impl DiffBody { window, cx, ); - (origin, left + px(2. * GUTTER_WIDTH + MARKER_PADDING)) } Rows::Split(_) => { let Some(side) = self.rows().side(row, column) else { @@ -648,13 +809,105 @@ impl DiffBody { window, cx, ); - (side.origin, left + px(GUTTER_WIDTH + MARKER_PADDING)) } + } + } + + 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 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); + let frame = self.header_frame(bounds); + let chevron = pen.shape( + disclosure(*collapsed).into(), + self.theme.muted_foreground, + window, + ); + paint_line( + &chevron, + point(frame.origin.x + px(HEADER_PADDING), top), + window, + cx, + ); + + window.paint_quad( + fill( + Bounds::new( + point(frame.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 = 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 = 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 [ + (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); + 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); + } + }); } } @@ -686,16 +939,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 @@ -755,10 +1010,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 @@ -767,7 +1024,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], ) @@ -789,7 +1046,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) }; @@ -802,7 +1059,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, @@ -820,20 +1083,16 @@ impl Element for DiffBody { #[cfg(test)] mod tests { use super::*; - use domain::FileStatus; + use domain::LineOrigin; fn file_header() -> Row { Row::FileHeader { + index: 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 +1115,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, @@ -878,6 +1137,14 @@ mod tests { assert_eq!(split.code_left(), SPLIT_CODE_LEFT); } + #[test] + fn code_starts_immediately_after_the_gutters_a_view_has() { + let unified = Rows::Unified(vec![file_header()]); + + assert_eq!(unified.code_left(), 2. * GUTTER_WIDTH); + assert_eq!(split_rows().code_left(), GUTTER_WIDTH); + } + #[test] fn an_empty_row_list_is_empty_in_either_view() { assert!(Rows::Unified(Vec::new()).is_empty()); @@ -903,15 +1170,29 @@ 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") - ); - assert_eq!( - row_text(&hunk_header()), - SharedString::from("@@ -1,3 +1,4 @@") + 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(&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;")), SharedString::from("let x = 1;") @@ -927,20 +1208,261 @@ 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("@@ -1,3 +1,4 @@")); + assert_eq!(cell_text(&rows, 0), SharedString::from("src/main.rs")); 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_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" + ); + } + + 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" + ); + 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() { + 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:?}" + ); + } + } + } + + #[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 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!( + 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}"); + assert_eq!(disclosure(true), "\u{25b8}"); + } + + #[test] + fn a_collapsed_header_reads_exactly_as_an_expanded_one() { + let collapsed = Row::FileHeader { + index: 0, + path: "src/main.rs".to_string(), + status: FileStatus::Modified, + added: 3, + deleted: 1, + collapsed: true, + }; + + assert_eq!( + row_text(&collapsed), + row_text(&file_header()), + "the chevron is painted beside the path, so no run and no copy can carry it" + ); + } + #[test] fn a_split_row_puts_each_side_in_its_own_column_and_pads_the_missing_one() { let rows = split_rows(); @@ -949,14 +1471,7 @@ mod tests { } #[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_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 +1480,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!( @@ -984,7 +1499,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) ); } @@ -1003,23 +1518,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] @@ -1144,6 +1667,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..45ff62d 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -7,25 +7,49 @@ //! 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 //! 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 //! 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 -//! 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, -//! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the +//! 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 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 //! `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 @@ -56,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 @@ -69,10 +93,22 @@ //! 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 or the view mode 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. 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. 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 +//! 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 @@ -102,12 +138,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::{ @@ -118,27 +155,40 @@ 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; + +pub(super) type ToggleFile = Rc; + pub(super) struct DiffContent { rows: Rows, strings: Vec, + max_changes: usize, } 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 } + DiffContent { + rows, + strings, + max_changes: max_changes(patch), + } } pub(super) fn render( @@ -146,6 +196,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 +229,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..4936a3b 100644 --- a/crates/ui/src/detail/diff/model.rs +++ b/crates/ui/src/detail/diff/model.rs @@ -1,20 +1,18 @@ -use std::fmt::Write as _; - use domain::{DiffLine, FilePatch, FileStatus, Hunk, LineOrigin, Patch}; -use crate::detail::format; +use super::Collapsed; #[derive(Clone, Debug, PartialEq, Eq)] pub(super) enum Row { FileHeader { + index: usize, path: String, status: FileStatus, added: usize, deleted: usize, + collapsed: bool, }, - HunkHeader { - text: String, - }, + Separator, Line { origin: LineOrigin, old_number: Option, @@ -26,31 +24,36 @@ 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 (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); + } } rows } -pub(super) fn file_header(file: &FilePatch) -> Row { +pub(super) fn file_header(index: usize, file: &FilePatch, collapsed: bool) -> Row { Row::FileHeader { + 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) -> 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 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 { @@ -69,15 +72,8 @@ 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()) } pub(super) fn placeholder(file: &FilePatch) -> Option { @@ -94,19 +90,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 filled_hunks(file).enumerate() { + if position > 0 { + rows.push(Row::Separator); + } rows.extend(hunk.lines.iter().map(line_row)); } } @@ -166,8 +158,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 +183,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 +204,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 +223,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 +237,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 { @@ -249,22 +253,15 @@ mod tests { "src/new.rs", FileStatus::Renamed { similarity: 87 }, ); - let Row::FileHeader { - path, - status, - added, - deleted, - } = file_header(&file) - 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" + 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] @@ -274,21 +271,12 @@ mod tests { "src/copy.rs", FileStatus::Copied { similarity: 100 }, ); - let Row::FileHeader { - path, - status, - added, - deleted, - } = file_header(&file) - 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 }); } #[test] @@ -298,33 +286,32 @@ 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"); } #[test] - fn a_modified_file_carries_no_status_label() { + fn a_header_carries_the_path_and_nothing_else() { 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!(added, 1); + assert_eq!(deleted, 0); assert_eq!( - header_line(&path, &status, added, deleted), - "src/main.rs +1 \u{2212}0" + path, "src/main.rs", + "the counts are painted, so a run cannot copy them with the path" ); } @@ -333,10 +320,167 @@ 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 { + index: 0, + collapsed: false, + .. + } + )); + assert!(matches!(rows[1], Row::Line { .. })); + assert!(matches!( + rows[2], + Row::FileHeader { + index: 1, + collapsed: true, + .. + } + )); + assert!(matches!( + rows[3], + Row::FileHeader { + index: 2, + collapsed: false, + .. + } + )); + assert!(matches!(rows[4], Row::Line { .. })); + } + + #[test] + 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, collapsed, .. + } = file_header(0, &file, true) + else { + panic!("a file yields a header row"); + }; + + assert!(collapsed); + assert_eq!( + 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 { + 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/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/crates/ui/src/detail/diff/split.rs b/crates/ui/src/detail/diff/split.rs index f711c83..6f37236 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, filled_hunks, placeholder}; use super::pairing::{SideLine, pair}; #[derive(Clone, Debug, PartialEq, Eq)] @@ -12,16 +13,22 @@ 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))); + 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(file) { rows.push(SplitRow::Full(placeholder)); continue; } - for hunk in &file.hunks { - rows.push(SplitRow::Full(hunk_header(hunk))); + for (position, hunk) in filled_hunks(file).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 +100,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 +117,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 +130,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 +155,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 +179,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 +194,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 +211,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 +231,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 { + index: 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 +263,125 @@ mod tests { )], }; - let rows = split_rows(&patch); + let rows = split_rows(&patch, &expanded()); assert_eq!( rows[0], SplitRow::Full(Row::FileHeader { + index: 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!(headers, 2); + 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 { + index: 1, + collapsed: true, + .. + }) + )); + assert!(matches!(rows[4], SplitRow::Sides { .. })); + } + + #[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); + } + + #[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); + } + + #[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); } } 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() 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