Diff View: Scroll to center of hunks when reviewing (#25846)

João Marcos created 10 months ago

When reviewing hunks, scroll to put them at the center of the screen
so you can better see the context around that hunk.

The field `center_cursor` was added to the actions `editor::GoToHunk`
and `editor::GoToPrevHunk`, this was set to `false` by default in
keymaps, as it wouldn't help with in-editor navigation.

The field is set to `true` for when you trigger `git::StageAndNext`
and `git::UnstageAndNext`, this is also `true` for the buttons in the
Diff View toolbar.

Release Notes:

- N/A

Change summary

assets/keymaps/default-linux.json      |  8 +-
assets/keymaps/default-macos.json      |  4 
assets/keymaps/linux/jetbrains.json    |  4 
assets/keymaps/linux/sublime_text.json |  4 
assets/keymaps/macos/jetbrains.json    |  4 
assets/keymaps/macos/sublime_text.json |  4 
assets/keymaps/vim.json                |  4 
crates/editor/src/actions.rs           | 18 +++++
crates/editor/src/editor.rs            | 87 ++++++++++++++++++---------
crates/editor/src/editor_tests.rs      | 18 ++--
crates/editor/src/element.rs           | 13 ++-
crates/git_ui/src/project_diff.rs      | 24 ++++++-
crates/zed/src/zed/quick_action_bar.rs | 14 +++
13 files changed, 140 insertions(+), 66 deletions(-)

Detailed changes

assets/keymaps/default-linux.json

@@ -372,8 +372,8 @@
       "ctrl-alt-y": "git::ToggleStaged",
       "alt-y": ["git::StageAndNext", { "whole_excerpt": false }],
       "alt-shift-y": ["git::UnstageAndNext", { "whole_excerpt": false }],
-      "alt-.": "editor::GoToHunk",
-      "alt-,": "editor::GoToPrevHunk"
+      "alt-.": ["editor::GoToHunk", { "center_cursor": true }],
+      "alt-,": ["editor::GoToPrevHunk", { "center_cursor": true }]
     }
   },
   {
@@ -564,8 +564,8 @@
       "shift-enter": "editor::ExpandExcerpts",
       "ctrl-alt-enter": "editor::OpenExcerptsSplit",
       "ctrl-shift-e": "pane::RevealInProjectPanel",
-      "ctrl-f8": "editor::GoToHunk",
-      "ctrl-shift-f8": "editor::GoToPrevHunk",
+      "ctrl-f8": ["editor::GoToHunk", { "center_cursor": true }],
+      "ctrl-shift-f8": ["editor::GoToPrevHunk", { "center_cursor": true }],
       "ctrl-enter": "assistant::InlineAssist",
       "ctrl-:": "editor::ToggleInlayHints"
     }

assets/keymaps/default-macos.json

@@ -640,8 +640,8 @@
       "shift-enter": "editor::ExpandExcerpts",
       "cmd-alt-enter": "editor::OpenExcerptsSplit",
       "cmd-shift-e": "pane::RevealInProjectPanel",
-      "cmd-f8": "editor::GoToHunk",
-      "cmd-shift-f8": "editor::GoToPrevHunk",
+      "cmd-f8": ["editor::GoToHunk", { "center_cursor": true }],
+      "cmd-shift-f8": ["editor::GoToPrevHunk", { "center_cursor": true }],
       "ctrl-enter": "assistant::InlineAssist",
       "ctrl-:": "editor::ToggleInlayHints"
     }

assets/keymaps/linux/jetbrains.json

@@ -42,8 +42,8 @@
       "ctrl-alt-shift-b": "editor::GoToTypeDefinitionSplit",
       "f2": "editor::GoToDiagnostic",
       "shift-f2": "editor::GoToPrevDiagnostic",
-      "ctrl-alt-shift-down": "editor::GoToHunk",
-      "ctrl-alt-shift-up": "editor::GoToPrevHunk",
+      "ctrl-alt-shift-down": ["editor::GoToHunk", { "center_cursor": true }],
+      "ctrl-alt-shift-up": ["editor::GoToPrevHunk", { "center_cursor": true }],
       "ctrl-alt-z": "git::Restore",
       "ctrl-home": "editor::MoveToBeginning",
       "ctrl-end": "editor::MoveToEnd",

assets/keymaps/linux/sublime_text.json

@@ -43,8 +43,8 @@
       "ctrl-f12": "editor::GoToDefinitionSplit",
       "shift-f12": "editor::FindAllReferences",
       "ctrl-shift-f12": "editor::FindAllReferences",
-      "ctrl-.": "editor::GoToHunk",
-      "ctrl-,": "editor::GoToPrevHunk",
+      "ctrl-.": ["editor::GoToHunk", { "center_cursor": true }],
+      "ctrl-,": ["editor::GoToPrevHunk", { "center_cursor": true }],
       "ctrl-k ctrl-u": "editor::ConvertToUpperCase",
       "ctrl-k ctrl-l": "editor::ConvertToLowerCase",
       "shift-alt-m": "markdown::OpenPreviewToTheSide",

assets/keymaps/macos/jetbrains.json

@@ -40,8 +40,8 @@
       "cmd-alt-shift-b": "editor::GoToTypeDefinitionSplit",
       "f2": "editor::GoToDiagnostic",
       "shift-f2": "editor::GoToPrevDiagnostic",
-      "ctrl-alt-shift-down": "editor::GoToHunk",
-      "ctrl-alt-shift-up": "editor::GoToPrevHunk",
+      "ctrl-alt-shift-down": ["editor::GoToHunk", { "center_cursor": true }],
+      "ctrl-alt-shift-up": ["editor::GoToPrevHunk", { "center_cursor": true }],
       "cmd-home": "editor::MoveToBeginning",
       "cmd-end": "editor::MoveToEnd",
       "cmd-shift-home": "editor::SelectToBeginning",

assets/keymaps/macos/sublime_text.json

@@ -44,8 +44,8 @@
       "alt-cmd-down": "editor::GoToDefinition",
       "ctrl-alt-cmd-down": "editor::GoToDefinitionSplit",
       "alt-shift-cmd-down": "editor::FindAllReferences",
-      "ctrl-.": "editor::GoToHunk",
-      "ctrl-,": "editor::GoToPrevHunk",
+      "ctrl-.": ["editor::GoToHunk", { "center_cursor": true }],
+      "ctrl-,": ["editor::GoToPrevHunk", { "center_cursor": true }],
       "cmd-k cmd-u": "editor::ConvertToUpperCase",
       "cmd-k cmd-l": "editor::ConvertToLowerCase",
       "cmd-shift-j": "editor::JoinLines",

assets/keymaps/vim.json

@@ -238,8 +238,8 @@
       "] x": "vim::SelectSmallerSyntaxNode",
       "] d": "editor::GoToDiagnostic",
       "[ d": "editor::GoToPrevDiagnostic",
-      "] c": "editor::GoToHunk",
-      "[ c": "editor::GoToPrevHunk",
+      "] c": ["editor::GoToHunk", { "center_cursor": true }],
+      "[ c": ["editor::GoToPrevHunk", { "center_cursor": true }],
       "g c": "vim::PushToggleComments"
     }
   },

crates/editor/src/actions.rs

@@ -189,6 +189,20 @@ pub struct DeleteToPreviousWordStart {
     pub ignore_newlines: bool,
 }
 
+#[derive(PartialEq, Clone, Deserialize, Default, JsonSchema)]
+#[serde(deny_unknown_fields)]
+pub struct GoToHunk {
+    #[serde(default)]
+    pub center_cursor: bool,
+}
+
+#[derive(PartialEq, Clone, Deserialize, Default, JsonSchema)]
+#[serde(deny_unknown_fields)]
+pub struct GoToPrevHunk {
+    #[serde(default)]
+    pub center_cursor: bool,
+}
+
 #[derive(PartialEq, Clone, Deserialize, Default, JsonSchema)]
 pub struct FoldAtLevel(pub u32);
 
@@ -218,6 +232,8 @@ impl_actions!(
         ExpandExcerptsDown,
         ExpandExcerptsUp,
         FoldAt,
+        GoToHunk,
+        GoToPrevHunk,
         HandleInput,
         MoveDownByLines,
         MovePageDown,
@@ -300,11 +316,9 @@ gpui::actions!(
         GoToDefinition,
         GoToDefinitionSplit,
         GoToDiagnostic,
-        GoToHunk,
         GoToImplementation,
         GoToImplementationSplit,
         GoToPrevDiagnostic,
-        GoToPrevHunk,
         GoToTypeDefinition,
         GoToTypeDefinitionSplit,
         HalfPageDown,

crates/editor/src/editor.rs

@@ -11411,27 +11411,45 @@ impl Editor {
         }
     }
 
-    fn go_to_next_hunk(&mut self, _: &GoToHunk, window: &mut Window, cx: &mut Context<Self>) {
+    fn go_to_next_hunk(&mut self, action: &GoToHunk, window: &mut Window, cx: &mut Context<Self>) {
         let snapshot = self.snapshot(window, cx);
         let selection = self.selections.newest::<Point>(cx);
-        self.go_to_hunk_after_position(&snapshot, selection.head(), window, cx);
+        self.go_to_hunk_after_or_before_position(
+            &snapshot,
+            selection.head(),
+            true,
+            action.center_cursor,
+            window,
+            cx,
+        );
     }
 
-    fn go_to_hunk_after_position(
+    fn go_to_hunk_after_or_before_position(
         &mut self,
         snapshot: &EditorSnapshot,
         position: Point,
+        after: bool,
+        scroll_center: bool,
         window: &mut Window,
         cx: &mut Context<Editor>,
     ) -> Option<MultiBufferDiffHunk> {
-        let hunk = self.hunk_after_position(snapshot, position);
+        let hunk = if after {
+            self.hunk_after_position(snapshot, position)
+        } else {
+            self.hunk_before_position(snapshot, position)
+        };
 
         if let Some(hunk) = &hunk {
-            let point = Point::new(hunk.row_range.start.0, 0);
+            let destination = Point::new(hunk.row_range.start.0, 0);
+            let autoscroll = if scroll_center {
+                Autoscroll::center()
+            } else {
+                Autoscroll::fit()
+            };
 
-            self.unfold_ranges(&[point..point], false, false, cx);
-            self.change_selections(Some(Autoscroll::fit()), window, cx, |s| {
-                s.select_ranges([point..point]);
+            self.unfold_ranges(&[destination..destination], false, false, cx);
+            self.change_selections(Some(autoscroll), window, cx, |s| {
+                s.select_ranges([destination..destination]);
             });
         }
 
@@ -11455,32 +11473,33 @@ impl Editor {
             })
     }
 
-    fn go_to_prev_hunk(&mut self, _: &GoToPrevHunk, window: &mut Window, cx: &mut Context<Self>) {
+    fn go_to_prev_hunk(
+        &mut self,
+        action: &GoToPrevHunk,
+        window: &mut Window,
+        cx: &mut Context<Self>,
+    ) {
         let snapshot = self.snapshot(window, cx);
         let selection = self.selections.newest::<Point>(cx);
-        self.go_to_hunk_before_position(&snapshot, selection.head(), window, cx);
+        self.go_to_hunk_after_or_before_position(
+            &snapshot,
+            selection.head(),
+            false,
+            action.center_cursor,
+            window,
+            cx,
+        );
     }
 
-    fn go_to_hunk_before_position(
+    fn hunk_before_position(
         &mut self,
         snapshot: &EditorSnapshot,
         position: Point,
-        window: &mut Window,
-        cx: &mut Context<Editor>,
     ) -> Option<MultiBufferDiffHunk> {
-        let mut hunk = snapshot.buffer_snapshot.diff_hunk_before(position);
-        if hunk.is_none() {
-            hunk = snapshot.buffer_snapshot.diff_hunk_before(Point::MAX);
-        }
-        if let Some(hunk) = &hunk {
-            let destination = Point::new(hunk.row_range.start.0, 0);
-            self.unfold_ranges(&[destination..destination], false, false, cx);
-            self.change_selections(Some(Autoscroll::fit()), window, cx, |s| {
-                s.select_ranges(vec![destination..destination]);
-            });
-        }
-
-        hunk
+        snapshot
+            .buffer_snapshot
+            .diff_hunk_before(position)
+            .or_else(|| snapshot.buffer_snapshot.diff_hunk_before(Point::MAX))
     }
 
     pub fn go_to_definition(
@@ -13598,7 +13617,13 @@ impl Editor {
                 });
 
             if run_twice {
-                self.go_to_next_hunk(&Default::default(), window, cx);
+                self.go_to_next_hunk(
+                    &GoToHunk {
+                        center_cursor: true,
+                    },
+                    window,
+                    cx,
+                );
             }
         } else if !self.buffer().read(cx).is_singleton() {
             self.stage_or_unstage_diff_hunks(stage, &ranges[..], window, cx);
@@ -13656,7 +13681,13 @@ impl Editor {
             }
         }
         self.stage_or_unstage_diff_hunks(stage, &ranges[..], window, cx);
-        self.go_to_next_hunk(&Default::default(), window, cx);
+        self.go_to_next_hunk(
+            &GoToHunk {
+                center_cursor: true,
+            },
+            window,
+            cx,
+        );
     }
 
     fn do_stage_or_unstage(

crates/editor/src/editor_tests.rs

@@ -11302,7 +11302,7 @@ async fn test_go_to_hunk(executor: BackgroundExecutor, cx: &mut TestAppContext)
     cx.update_editor(|editor, window, cx| {
         //Wrap around the bottom of the buffer
         for _ in 0..3 {
-            editor.go_to_next_hunk(&GoToHunk, window, cx);
+            editor.go_to_next_hunk(&GoToHunk::default(), window, cx);
         }
     });
 
@@ -11324,7 +11324,7 @@ async fn test_go_to_hunk(executor: BackgroundExecutor, cx: &mut TestAppContext)
     cx.update_editor(|editor, window, cx| {
         //Wrap around the top of the buffer
         for _ in 0..2 {
-            editor.go_to_prev_hunk(&GoToPrevHunk, window, cx);
+            editor.go_to_prev_hunk(&GoToPrevHunk::default(), window, cx);
         }
     });
 
@@ -11344,7 +11344,7 @@ async fn test_go_to_hunk(executor: BackgroundExecutor, cx: &mut TestAppContext)
     );
 
     cx.update_editor(|editor, window, cx| {
-        editor.go_to_prev_hunk(&GoToPrevHunk, window, cx);
+        editor.go_to_prev_hunk(&GoToPrevHunk::default(), window, cx);
     });
 
     cx.assert_editor_state(
@@ -11363,7 +11363,7 @@ async fn test_go_to_hunk(executor: BackgroundExecutor, cx: &mut TestAppContext)
     );
 
     cx.update_editor(|editor, window, cx| {
-        editor.go_to_prev_hunk(&GoToPrevHunk, window, cx);
+        editor.go_to_prev_hunk(&GoToPrevHunk::default(), window, cx);
     });
 
     cx.assert_editor_state(
@@ -11383,7 +11383,7 @@ async fn test_go_to_hunk(executor: BackgroundExecutor, cx: &mut TestAppContext)
 
     cx.update_editor(|editor, window, cx| {
         for _ in 0..2 {
-            editor.go_to_prev_hunk(&GoToPrevHunk, window, cx);
+            editor.go_to_prev_hunk(&GoToPrevHunk::default(), window, cx);
         }
     });
 
@@ -11407,7 +11407,7 @@ async fn test_go_to_hunk(executor: BackgroundExecutor, cx: &mut TestAppContext)
     });
 
     cx.update_editor(|editor, window, cx| {
-        editor.go_to_next_hunk(&GoToHunk, window, cx);
+        editor.go_to_next_hunk(&GoToHunk::default(), window, cx);
     });
 
     cx.assert_editor_state(
@@ -13414,7 +13414,7 @@ async fn test_toggle_selected_diff_hunks(executor: BackgroundExecutor, cx: &mut
     executor.run_until_parked();
 
     cx.update_editor(|editor, window, cx| {
-        editor.go_to_next_hunk(&GoToHunk, window, cx);
+        editor.go_to_next_hunk(&GoToHunk::default(), window, cx);
         editor.toggle_selected_diff_hunks(&ToggleSelectedDiffHunks, window, cx);
     });
     executor.run_until_parked();
@@ -13436,7 +13436,7 @@ async fn test_toggle_selected_diff_hunks(executor: BackgroundExecutor, cx: &mut
 
     cx.update_editor(|editor, window, cx| {
         for _ in 0..2 {
-            editor.go_to_next_hunk(&GoToHunk, window, cx);
+            editor.go_to_next_hunk(&GoToHunk::default(), window, cx);
             editor.toggle_selected_diff_hunks(&ToggleSelectedDiffHunks, window, cx);
         }
     });
@@ -13459,7 +13459,7 @@ async fn test_toggle_selected_diff_hunks(executor: BackgroundExecutor, cx: &mut
     );
 
     cx.update_editor(|editor, window, cx| {
-        editor.go_to_next_hunk(&GoToHunk, window, cx);
+        editor.go_to_next_hunk(&GoToHunk::default(), window, cx);
         editor.toggle_selected_diff_hunks(&ToggleSelectedDiffHunks, window, cx);
     });
     executor.run_until_parked();

crates/editor/src/element.rs

@@ -8873,7 +8873,7 @@ fn diff_hunk_controls(
                             move |window, cx| {
                                 Tooltip::for_action_in(
                                     "Next Hunk",
-                                    &GoToHunk,
+                                    &GoToHunk::default(),
                                     &focus_handle,
                                     window,
                                     cx,
@@ -8887,8 +8887,9 @@ fn diff_hunk_controls(
                                     let snapshot = editor.snapshot(window, cx);
                                     let position =
                                         hunk_range.end.to_point(&snapshot.buffer_snapshot);
-                                    editor
-                                        .go_to_hunk_after_position(&snapshot, position, window, cx);
+                                    editor.go_to_hunk_after_or_before_position(
+                                        &snapshot, position, true, true, window, cx,
+                                    );
                                     editor.expand_selected_diff_hunks(cx);
                                 });
                             }
@@ -8904,7 +8905,7 @@ fn diff_hunk_controls(
                             move |window, cx| {
                                 Tooltip::for_action_in(
                                     "Previous Hunk",
-                                    &GoToPrevHunk,
+                                    &GoToPrevHunk::default(),
                                     &focus_handle,
                                     window,
                                     cx,
@@ -8918,7 +8919,9 @@ fn diff_hunk_controls(
                                     let snapshot = editor.snapshot(window, cx);
                                     let point =
                                         hunk_range.start.to_point(&snapshot.buffer_snapshot);
-                                    editor.go_to_hunk_before_position(&snapshot, point, window, cx);
+                                    editor.go_to_hunk_after_or_before_position(
+                                        &snapshot, point, false, true, window, cx,
+                                    );
                                     editor.expand_selected_diff_hunks(cx);
                                 });
                             }

crates/git_ui/src/project_diff.rs

@@ -869,12 +869,20 @@ impl Render for ProjectDiffToolbar {
                             .shape(ui::IconButtonShape::Square)
                             .tooltip(Tooltip::for_action_title_in(
                                 "Go to previous hunk",
-                                &GoToPrevHunk,
+                                &GoToPrevHunk {
+                                    center_cursor: false,
+                                },
                                 &focus_handle,
                             ))
                             .disabled(!button_states.prev_next)
                             .on_click(cx.listener(|this, _, window, cx| {
-                                this.dispatch_action(&GoToPrevHunk, window, cx)
+                                this.dispatch_action(
+                                    &GoToPrevHunk {
+                                        center_cursor: true,
+                                    },
+                                    window,
+                                    cx,
+                                )
                             })),
                     )
                     .child(
@@ -882,12 +890,20 @@ impl Render for ProjectDiffToolbar {
                             .shape(ui::IconButtonShape::Square)
                             .tooltip(Tooltip::for_action_title_in(
                                 "Go to next hunk",
-                                &GoToHunk,
+                                &GoToHunk {
+                                    center_cursor: false,
+                                },
                                 &focus_handle,
                             ))
                             .disabled(!button_states.prev_next)
                             .on_click(cx.listener(|this, _, window, cx| {
-                                this.dispatch_action(&GoToHunk, window, cx)
+                                this.dispatch_action(
+                                    &GoToHunk {
+                                        center_cursor: true,
+                                    },
+                                    window,
+                                    cx,
+                                )
                             })),
                     ),
             )

crates/zed/src/zed/quick_action_bar.rs

@@ -182,8 +182,18 @@ impl Render for QuickActionBar {
                             .action("Next Problem", Box::new(GoToDiagnostic))
                             .action("Previous Problem", Box::new(GoToPrevDiagnostic))
                             .separator()
-                            .action("Next Hunk", Box::new(GoToHunk))
-                            .action("Previous Hunk", Box::new(GoToPrevHunk))
+                            .action(
+                                "Next Hunk",
+                                Box::new(GoToHunk {
+                                    center_cursor: true,
+                                }),
+                            )
+                            .action(
+                                "Previous Hunk",
+                                Box::new(GoToPrevHunk {
+                                    center_cursor: true,
+                                }),
+                            )
                             .separator()
                             .action("Move Line Up", Box::new(MoveLineUp))
                             .action("Move Line Down", Box::new(MoveLineDown))