Fix diff_hunk_before in a multibuffer (#26059)

Conrad Irwin and Cole created

Also simplify it to avoid doing a bunch of unnecessary work.

Co-Authored-By: Cole <cole@zed.dev>

Closes #ISSUE

Release Notes:

- git: Fix jumping to the previous diff hunk

---------

Co-authored-by: Cole <cole@zed.dev>

Change summary

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(-)

Detailed changes

crates/editor/src/editor.rs ๐Ÿ”—

@@ -11544,15 +11544,16 @@ impl Editor {
         direction: Direction,
         window: &mut Window,
         cx: &mut Context<Editor>,
-    ) -> Option<MultiBufferDiffHunk> {
-        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<MultiBufferDiffHunk> {
+    ) -> Option<MultiBufferRow> {
         snapshot
             .buffer_snapshot
             .diff_hunk_before(position)

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::<Vec<_>>();
 
-        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::<Vec<_>>();
 
-            (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::<String>(),
                 expected_text
             );

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::<ProjectDiff>(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
+        "
+        ));
+    }
 }

crates/multi_buffer/src/multi_buffer.rs ๐Ÿ”—

@@ -3818,104 +3818,57 @@ impl MultiBufferSnapshot {
         })
     }
 
-    pub fn diff_hunk_before<T: ToOffset>(&self, position: T) -> Option<MultiBufferDiffHunk> {
+    pub fn diff_hunk_before<T: ToOffset>(&self, position: T) -> Option<MultiBufferRow> {
         let offset = position.to_offset(self);
 
-        // Go to the region containing the given offset.
         let mut cursor = self.cursor::<DimensionPair<usize, Point>>();
         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::<D>(&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();

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))
     );
 }