From d7b90f4204028888aa9da0c1dc2208b5bff7bf18 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 4 Mar 2025 20:07:19 -0700 Subject: [PATCH] Fix diff_hunk_before in a multibuffer (#26059) Also simplify it to avoid doing a bunch of unnecessary work. Co-Authored-By: Cole Closes #ISSUE Release Notes: - git: Fix jumping to the previous diff hunk --------- Co-authored-by: Cole --- crates/editor/src/editor.rs | 13 +- crates/editor/src/test/editor_test_context.rs | 21 ++- crates/git_ui/src/project_diff.rs | 93 +++++++++++- crates/multi_buffer/src/multi_buffer.rs | 138 +++++------------- crates/multi_buffer/src/multi_buffer_tests.rs | 31 ++-- 5 files changed, 160 insertions(+), 136 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index adc611125d7be206b755e08d2089b0afc0a70c00..18439c4fc20477ae3b443e469ae7015157d69fd1 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -11544,15 +11544,16 @@ impl Editor { direction: Direction, window: &mut Window, cx: &mut Context, - ) -> Option { - let hunk = if direction == Direction::Next { + ) { + let row = if direction == Direction::Next { self.hunk_after_position(snapshot, position) + .map(|hunk| hunk.row_range.start) } else { self.hunk_before_position(snapshot, position) }; - if let Some(hunk) = &hunk { - let destination = Point::new(hunk.row_range.start.0, 0); + if let Some(row) = row { + let destination = Point::new(row.0, 0); let autoscroll = Autoscroll::center(); self.unfold_ranges(&[destination..destination], false, false, cx); @@ -11560,8 +11561,6 @@ impl Editor { s.select_ranges([destination..destination]); }); } - - hunk } fn hunk_after_position( @@ -11602,7 +11601,7 @@ impl Editor { &mut self, snapshot: &EditorSnapshot, position: Point, - ) -> Option { + ) -> Option { snapshot .buffer_snapshot .diff_hunk_before(position) diff --git a/crates/editor/src/test/editor_test_context.rs b/crates/editor/src/test/editor_test_context.rs index 8fe9f293ee98f4fbb53dd4785f4e03614a08c539..c5132773e3238e3a23371098f77230406c7a8ea8 100644 --- a/crates/editor/src/test/editor_test_context.rs +++ b/crates/editor/src/test/editor_test_context.rs @@ -12,7 +12,7 @@ use gpui::{ }; use itertools::Itertools; use language::{Buffer, BufferSnapshot, LanguageRegistry}; -use multi_buffer::{ExcerptRange, MultiBufferRow}; +use multi_buffer::{Anchor, ExcerptRange, MultiBufferRow}; use parking_lot::RwLock; use project::{FakeFs, Project}; use std::{ @@ -399,7 +399,7 @@ impl EditorTestContext { .split("[EXCERPT]\n") .collect::>(); - let (selections, excerpts) = self.update_editor(|editor, _, cx| { + let (multibuffer_snapshot, selections, excerpts) = self.update_editor(|editor, _, cx| { let multibuffer_snapshot = editor.buffer.read(cx).snapshot(cx); let selections = editor.selections.disjoint_anchors(); @@ -408,10 +408,15 @@ impl EditorTestContext { .map(|(e_id, snapshot, range)| (e_id, snapshot.clone(), range)) .collect::>(); - (selections, excerpts) + (multibuffer_snapshot, selections, excerpts) }); - assert_eq!(excerpts.len(), expected_excerpts.len()); + assert!( + excerpts.len() == expected_excerpts.len(), + "should have {} excerpts, got {}", + expected_excerpts.len(), + excerpts.len() + ); for (ix, (excerpt_id, snapshot, range)) in excerpts.into_iter().enumerate() { let is_folded = self @@ -435,8 +440,12 @@ impl EditorTestContext { } assert!(!is_folded, "excerpt {} should not be folded", ix); assert_eq!( - snapshot - .text_for_range(range.context.clone()) + multibuffer_snapshot + .text_for_range(Anchor::range_in_buffer( + excerpt_id, + snapshot.remote_id(), + range.context.clone() + )) .collect::(), expected_text ); diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs index c7960e5c5cef845caf105fbd6b3d6cadd8b101d0..ab929e8fb20fadb52294403e426636523e228b36 100644 --- a/crates/git_ui/src/project_diff.rs +++ b/crates/git_ui/src/project_diff.rs @@ -926,12 +926,14 @@ impl Render for ProjectDiffToolbar { } } +#[cfg(not(target_os = "windows"))] #[cfg(test)] mod tests { use std::path::Path; use collections::HashMap; - use editor::test::editor_test_context::assert_state_with_diff; + use db::indoc; + use editor::test::editor_test_context::{assert_state_with_diff, EditorTestContext}; use git::status::{StatusCode, TrackedStatus}; use gpui::TestAppContext; use project::FakeFs; @@ -1222,4 +1224,93 @@ mod tests { .unindent(), ); } + + use crate::project_diff::{self, ProjectDiff}; + + #[gpui::test] + async fn test_go_to_prev_hunk_multibuffer(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + "/a", + json!({ + ".git":{}, + "a.txt": "created\n", + "b.txt": "really changed\n", + "c.txt": "unchanged\n" + }), + ) + .await; + + fs.set_git_content_for_repo( + Path::new("/a/.git"), + &[ + ("b.txt".into(), "before\n".to_string(), None), + ("c.txt".into(), "unchanged\n".to_string(), None), + ("d.txt".into(), "deleted\n".to_string(), None), + ], + ); + + let project = Project::test(fs, [Path::new("/a")], cx).await; + let (workspace, cx) = + cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx)); + + cx.run_until_parked(); + + cx.focus(&workspace); + cx.update(|window, cx| { + window.dispatch_action(project_diff::Diff.boxed_clone(), cx); + }); + + cx.run_until_parked(); + + let item = workspace.update(cx, |workspace, cx| { + workspace.active_item_as::(cx).unwrap() + }); + cx.focus(&item); + let editor = item.update(cx, |item, _| item.editor.clone()); + + let mut cx = EditorTestContext::for_editor_in(editor, cx).await; + + cx.assert_excerpts_with_selections(indoc!( + " + [EXCERPT] + before + really changed + [EXCERPT] + [FOLDED] + [EXCERPT] + ˇcreated + " + )); + + cx.dispatch_action(editor::actions::GoToPreviousHunk); + + cx.assert_excerpts_with_selections(indoc!( + " + [EXCERPT] + before + really changed + [EXCERPT] + ˇ[FOLDED] + [EXCERPT] + created + " + )); + + cx.dispatch_action(editor::actions::GoToPreviousHunk); + + cx.assert_excerpts_with_selections(indoc!( + " + [EXCERPT] + ˇbefore + really changed + [EXCERPT] + [FOLDED] + [EXCERPT] + created + " + )); + } } diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index a7350736b910cc0cb07c8c0b0694041e495fb81e..20fa4bb94eeba9112fcee08f65fc7bc6def234d9 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -3818,104 +3818,57 @@ impl MultiBufferSnapshot { }) } - pub fn diff_hunk_before(&self, position: T) -> Option { + pub fn diff_hunk_before(&self, position: T) -> Option { let offset = position.to_offset(self); - // Go to the region containing the given offset. let mut cursor = self.cursor::>(); cursor.seek(&DimensionPair { key: offset, value: None, }); - let mut region = cursor.region()?; - if region.range.start.key == offset || !region.is_main_buffer { - cursor.prev(); - region = cursor.region()?; - } - - // Find the corresponding buffer offset. - let overshoot = if region.is_main_buffer { - offset - region.range.start.key - } else { - 0 - }; - let mut max_buffer_offset = region + cursor.seek_to_start_of_current_excerpt(); + let excerpt = cursor.excerpt()?; + + let excerpt_end = excerpt.range.context.end.to_offset(&excerpt.buffer); + let current_position = self + .anchor_before(offset) + .text_anchor + .to_offset(&excerpt.buffer); + let excerpt_end = excerpt .buffer - .clip_offset(region.buffer_range.start.key + overshoot, Bias::Right); - - loop { - let excerpt = cursor.excerpt()?; - let excerpt_end = excerpt.range.context.end.to_offset(&excerpt.buffer); - let buffer_offset = excerpt_end.min(max_buffer_offset); - let buffer_end = excerpt.buffer.anchor_before(buffer_offset); - let buffer_end_row = buffer_end.to_point(&excerpt.buffer).row; - - if let Some(diff) = self.diffs.get(&excerpt.buffer_id) { - for hunk in diff.hunks_intersecting_range_rev( - excerpt.range.context.start..buffer_end, - &excerpt.buffer, - ) { - let hunk_range = hunk.buffer_range.to_offset(&excerpt.buffer); - if hunk.range.end >= Point::new(buffer_end_row, 0) { - continue; - } - - let hunk_start = hunk.range.start; - let hunk_end = hunk.range.end; - - cursor.seek_to_buffer_position_in_current_excerpt(&DimensionPair { - key: hunk_range.start, - value: None, - }); - - let mut region = cursor.region()?; - while !region.is_main_buffer || region.buffer_range.start.key >= hunk_range.end - { - cursor.prev(); - region = cursor.region()?; - } - - let overshoot = if region.is_main_buffer { - hunk_start.saturating_sub(region.buffer_range.start.value.unwrap()) - } else { - Point::zero() - }; - let start = region.range.start.value.unwrap() + overshoot; - - while let Some(region) = cursor.region() { - if !region.is_main_buffer - || region.buffer_range.end.value.unwrap() <= hunk_end - { - cursor.next(); - } else { - break; - } - } + .anchor_before(excerpt_end.min(current_position)); - let end = if let Some(region) = cursor.region() { - let overshoot = if region.is_main_buffer { - hunk_end.saturating_sub(region.buffer_range.start.value.unwrap()) - } else { - Point::zero() - }; - region.range.start.value.unwrap() + overshoot - } else { - self.max_point() - }; - - return Some(MultiBufferDiffHunk { - row_range: MultiBufferRow(start.row)..MultiBufferRow(end.row), - buffer_id: excerpt.buffer_id, - excerpt_id: excerpt.id, - buffer_range: hunk.buffer_range.clone(), - diff_base_byte_range: hunk.diff_base_byte_range.clone(), - secondary_status: hunk.secondary_status, - }); + if let Some(diff) = self.diffs.get(&excerpt.buffer_id) { + for hunk in diff.hunks_intersecting_range_rev( + excerpt.range.context.start..excerpt_end, + &excerpt.buffer, + ) { + let hunk_end = hunk.buffer_range.end.to_offset(&excerpt.buffer); + if hunk_end >= current_position { + continue; } + let start = + Anchor::in_buffer(excerpt.id, excerpt.buffer_id, hunk.buffer_range.start) + .to_point(&self); + return Some(MultiBufferRow(start.row)); } + } + loop { cursor.prev_excerpt(); - max_buffer_offset = usize::MAX; + let excerpt = cursor.excerpt()?; + + let Some(diff) = self.diffs.get(&excerpt.buffer_id) else { + continue; + }; + let mut hunks = + diff.hunks_intersecting_range_rev(excerpt.range.context.clone(), &excerpt.buffer); + let Some(hunk) = hunks.next() else { + continue; + }; + let start = Anchor::in_buffer(excerpt.id, excerpt.buffer_id, hunk.buffer_range.start) + .to_point(&self); + return Some(MultiBufferRow(start.row)); } } @@ -6132,21 +6085,6 @@ where } } - fn seek_to_buffer_position_in_current_excerpt(&mut self, position: &D) { - self.cached_region.take(); - if let Some(excerpt) = self.excerpts.item() { - let excerpt_start = excerpt.range.context.start.summary::(&excerpt.buffer); - let position_in_excerpt = *position - excerpt_start; - let mut excerpt_position = self.excerpts.start().0; - excerpt_position.add_assign(&position_in_excerpt); - self.diff_transforms - .seek(&ExcerptDimension(excerpt_position), Bias::Left, &()); - if self.diff_transforms.item().is_none() { - self.diff_transforms.next(&()); - } - } - } - fn next_excerpt(&mut self) { self.excerpts.next(&()); self.seek_to_start_of_current_excerpt(); diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index d0e34e198074182f4583275296f9a18dcfcbf8aa..2a3d1727cdb4c982bde77e42ccf609133f3af40d 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/crates/multi_buffer/src/multi_buffer_tests.rs @@ -440,23 +440,14 @@ fn test_diff_hunks_in_range(cx: &mut TestAppContext) { vec![1..3, 4..6, 7..8] ); + assert_eq!(snapshot.diff_hunk_before(Point::new(1, 1)), None,); assert_eq!( - snapshot - .diff_hunk_before(Point::new(1, 1)) - .map(|hunk| hunk.row_range.start.0..hunk.row_range.end.0), - None, + snapshot.diff_hunk_before(Point::new(7, 0)), + Some(MultiBufferRow(4)) ); assert_eq!( - snapshot - .diff_hunk_before(Point::new(7, 0)) - .map(|hunk| hunk.row_range.start.0..hunk.row_range.end.0), - Some(4..6) - ); - assert_eq!( - snapshot - .diff_hunk_before(Point::new(4, 0)) - .map(|hunk| hunk.row_range.start.0..hunk.row_range.end.0), - Some(1..3) + snapshot.diff_hunk_before(Point::new(4, 0)), + Some(MultiBufferRow(1)) ); multibuffer.update(cx, |multibuffer, cx| { @@ -478,16 +469,12 @@ fn test_diff_hunks_in_range(cx: &mut TestAppContext) { ); assert_eq!( - snapshot - .diff_hunk_before(Point::new(2, 0)) - .map(|hunk| hunk.row_range.start.0..hunk.row_range.end.0), - Some(1..1), + snapshot.diff_hunk_before(Point::new(2, 0)), + Some(MultiBufferRow(1)), ); assert_eq!( - snapshot - .diff_hunk_before(Point::new(4, 0)) - .map(|hunk| hunk.row_range.start.0..hunk.row_range.end.0), - Some(2..2) + snapshot.diff_hunk_before(Point::new(4, 0)), + Some(MultiBufferRow(2)) ); }