Disable nav history in vim scrolls (#32656)

Reland of #30345 to fix merge conflicts with the new skip-completions
option

Fixes #29431
Fixes #17592

Release Notes:

- vim: Scrolls are no longer added to the jumplist

crates/editor/src/editor.rs

@@ -1222,10 +1222,55 @@ impl Default for SelectionHistoryMode {
     }
 }
 
+#[derive(Debug)]
+pub struct SelectionEffects {
+    nav_history: bool,
+    completions: bool,
+    scroll: Option<Autoscroll>,
+}
+
+impl Default for SelectionEffects {
+    fn default() -> Self {
+        Self {
+            nav_history: true,
+            completions: true,
+            scroll: Some(Autoscroll::fit()),
+        }
+    }
+}
+impl SelectionEffects {
+    pub fn scroll(scroll: Autoscroll) -> Self {
+        Self {
+            scroll: Some(scroll),
+            ..Default::default()
+        }
+    }
+
+    pub fn no_scroll() -> Self {
+        Self {
+            scroll: None,
+            ..Default::default()
+        }
+    }
+
+    pub fn completions(self, completions: bool) -> Self {
+        Self {
+            completions,
+            ..self
+        }
+    }
+
+    pub fn nav_history(self, nav_history: bool) -> Self {
+        Self {
+            nav_history,
+            ..self
+        }
+    }
+}
+
 struct DeferredSelectionEffectsState {
     changed: bool,
-    should_update_completions: bool,
+    effects: SelectionEffects,
-    autoscroll: Option<Autoscroll>,
     old_cursor_position: Anchor,
     history_entry: SelectionHistoryEntry,
 }
@@ -2708,7 +2753,7 @@ impl Editor {
         &mut self,
         local: bool,
         old_cursor_position: &Anchor,
-        should_update_completions: bool,
+        effects: SelectionEffects,
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
@@ -2766,12 +2811,14 @@ impl Editor {
         let new_cursor_position = newest_selection.head();
         let selection_start = newest_selection.start;
 
-        self.push_to_nav_history(
+        if effects.nav_history {
-            *old_cursor_position,
+            self.push_to_nav_history(
-            Some(new_cursor_position.to_point(buffer)),
+                *old_cursor_position,
-            false,
+                Some(new_cursor_position.to_point(buffer)),
-            cx,
+                false,
-        );
+                cx,
+            );
+        }
 
         if local {
             if let Some(buffer_id) = new_cursor_position.buffer_id {
@@ -2802,7 +2849,7 @@ impl Editor {
             let completion_position = completion_menu.map(|menu| menu.initial_position);
             drop(context_menu);
 
-            if should_update_completions {
+            if effects.completions {
                 if let Some(completion_position) = completion_position {
                     let start_offset = selection_start.to_offset(buffer);
                     let position_matches = start_offset == completion_position.to_offset(buffer);
@@ -3009,43 +3056,23 @@ impl Editor {
     /// effects of selection change occur at the end of the transaction.
     pub fn change_selections<R>(
         &mut self,
-        autoscroll: Option<Autoscroll>,
+        effects: impl Into<SelectionEffects>,
-        window: &mut Window,
-        cx: &mut Context<Self>,
-        change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R,
-    ) -> R {
-        self.change_selections_inner(true, autoscroll, window, cx, change)
-    }
-
-    pub(crate) fn change_selections_without_updating_completions<R>(
-        &mut self,
-        autoscroll: Option<Autoscroll>,
-        window: &mut Window,
-        cx: &mut Context<Self>,
-        change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R,
-    ) -> R {
-        self.change_selections_inner(false, autoscroll, window, cx, change)
-    }
-
-    fn change_selections_inner<R>(
-        &mut self,
-        should_update_completions: bool,
-        autoscroll: Option<Autoscroll>,
         window: &mut Window,
         cx: &mut Context<Self>,
         change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R,
     ) -> R {
+        let effects = effects.into();
         if let Some(state) = &mut self.deferred_selection_effects_state {
-            state.autoscroll = autoscroll.or(state.autoscroll);
+            state.effects.scroll = effects.scroll.or(state.effects.scroll);
-            state.should_update_completions = should_update_completions;
+            state.effects.completions = effects.completions;
+            state.effects.nav_history |= effects.nav_history;
             let (changed, result) = self.selections.change_with(cx, change);
             state.changed |= changed;
             return result;
         }
         let mut state = DeferredSelectionEffectsState {
             changed: false,
-            should_update_completions,
+            effects,
-            autoscroll,
             old_cursor_position: self.selections.newest_anchor().head(),
             history_entry: SelectionHistoryEntry {
                 selections: self.selections.disjoint_anchors(),
@@ -3095,19 +3122,13 @@ impl Editor {
         if state.changed {
             self.selection_history.push(state.history_entry);
 
-            if let Some(autoscroll) = state.autoscroll {
+            if let Some(autoscroll) = state.effects.scroll {
                 self.request_autoscroll(autoscroll, cx);
             }
 
             let old_cursor_position = &state.old_cursor_position;
 
-            self.selections_did_change(
+            self.selections_did_change(true, &old_cursor_position, state.effects, window, cx);
-                true,
-                &old_cursor_position,
-                state.should_update_completions,
-                window,
-                cx,
-            );
 
             if self.should_open_signature_help_automatically(&old_cursor_position, cx) {
                 self.show_signature_help(&ShowSignatureHelp, window, cx);
@@ -3227,9 +3248,13 @@ impl Editor {
             _ => {}
         }
 
-        let auto_scroll = EditorSettings::get_global(cx).autoscroll_on_clicks;
+        let effects = if EditorSettings::get_global(cx).autoscroll_on_clicks {
+            SelectionEffects::scroll(Autoscroll::fit())
+        } else {
+            SelectionEffects::no_scroll()
+        };
 
-        self.change_selections(auto_scroll.then(Autoscroll::fit), window, cx, |s| {
+        self.change_selections(effects, window, cx, |s| {
             s.set_pending(pending_selection, pending_mode)
         });
     }
@@ -4016,8 +4041,8 @@ impl Editor {
             }
 
             let had_active_inline_completion = this.has_active_inline_completion();
-            this.change_selections_without_updating_completions(
+            this.change_selections(
-                Some(Autoscroll::fit()),
+                SelectionEffects::scroll(Autoscroll::fit()).completions(false),
                 window,
                 cx,
                 |s| s.select(new_selections),
@@ -16169,7 +16194,13 @@ impl Editor {
                 s.clear_pending();
             }
         });
-        self.selections_did_change(false, &old_cursor_position, true, window, cx);
+        self.selections_did_change(
+            false,
+            &old_cursor_position,
+            SelectionEffects::default(),
+            window,
+            cx,
+        );
     }
 
     pub fn transact(

crates/editor/src/items.rs

@@ -1,6 +1,7 @@
 use crate::{
     Anchor, Autoscroll, Editor, EditorEvent, EditorSettings, ExcerptId, ExcerptRange, FormatTarget,
-    MultiBuffer, MultiBufferSnapshot, NavigationData, SearchWithinRange, ToPoint as _,
+    MultiBuffer, MultiBufferSnapshot, NavigationData, SearchWithinRange, SelectionEffects,
+    ToPoint as _,
     editor_settings::SeedQuerySetting,
     persistence::{DB, SerializedEditor},
     scroll::ScrollAnchor,
@@ -611,12 +612,13 @@ impl Item for Editor {
             if newest_selection.head() == offset {
                 false
             } else {
-                let nav_history = self.nav_history.take();
                 self.set_scroll_anchor(scroll_anchor, window, cx);
-                self.change_selections(Some(Autoscroll::fit()), window, cx, |s| {
+                self.change_selections(
-                    s.select_ranges([offset..offset])
+                    SelectionEffects::default().nav_history(false),
-                });
+                    window,
-                self.nav_history = nav_history;
+                    cx,
+                    |s| s.select_ranges([offset..offset]),
+                );
                 true
             }
         } else {

crates/editor/src/jsx_tag_auto_close.rs

@@ -8,7 +8,7 @@ use util::ResultExt as _;
 use language::{BufferSnapshot, JsxTagAutoCloseConfig, Node};
 use text::{Anchor, OffsetRangeExt as _};
 
-use crate::Editor;
+use crate::{Editor, SelectionEffects};
 
 pub struct JsxTagCompletionState {
     edit_index: usize,
@@ -600,9 +600,14 @@ pub(crate) fn handle_from(
                     })
                     .collect::<Vec<_>>();
                 this.update_in(cx, |this, window, cx| {
-                    this.change_selections_without_updating_completions(None, window, cx, |s| {
+                    this.change_selections(
-                        s.select(base_selections);
+                        SelectionEffects::no_scroll().completions(false),
-                    });
+                        window,
+                        cx,
+                        |s| {
+                            s.select(base_selections);
+                        },
+                    );
                 })
                 .ok()?;
             }

crates/editor/src/scroll/autoscroll.rs

@@ -1,12 +1,13 @@
 use crate::{
-    DisplayRow, Editor, EditorMode, LineWithInvisibles, RowExt, display_map::ToDisplayPoint,
+    DisplayRow, Editor, EditorMode, LineWithInvisibles, RowExt, SelectionEffects,
+    display_map::ToDisplayPoint,
 };
 use gpui::{Bounds, Context, Pixels, Window, px};
 use language::Point;
 use multi_buffer::Anchor;
 use std::{cmp, f32};
 
-#[derive(PartialEq, Eq, Clone, Copy)]
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
 pub enum Autoscroll {
     Next,
     Strategy(AutoscrollStrategy, Option<Anchor>),
@@ -66,7 +67,16 @@ impl Autoscroll {
     }
 }
 
-#[derive(PartialEq, Eq, Default, Clone, Copy)]
+impl Into<SelectionEffects> for Option<Autoscroll> {
+    fn into(self) -> SelectionEffects {
+        match self {
+            Some(autoscroll) => SelectionEffects::scroll(autoscroll),
+            None => SelectionEffects::no_scroll(),
+        }
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Default, Clone, Copy)]
 pub enum AutoscrollStrategy {
     Fit,
     Newest,

crates/editor/src/test/editor_lsp_test_context.rs

@@ -169,6 +169,12 @@ impl EditorLspTestContext {
                 .expect("Opened test file wasn't an editor")
         });
         editor.update_in(&mut cx, |editor, window, cx| {
+            let nav_history = workspace
+                .read(cx)
+                .active_pane()
+                .read(cx)
+                .nav_history_for_item(&cx.entity());
+            editor.set_nav_history(Some(nav_history));
             window.focus(&editor.focus_handle(cx))
         });
 

crates/vim/src/normal/scroll.rs

@@ -1,6 +1,6 @@
 use crate::Vim;
 use editor::{
-    DisplayPoint, Editor, EditorSettings,
+    DisplayPoint, Editor, EditorSettings, SelectionEffects,
     display_map::{DisplayRow, ToDisplayPoint},
     scroll::ScrollAmount,
 };
@@ -101,69 +101,76 @@ fn scroll_editor(
     let top_anchor = editor.scroll_manager.anchor().anchor;
     let vertical_scroll_margin = EditorSettings::get_global(cx).vertical_scroll_margin;
 
-    editor.change_selections(None, window, cx, |s| {
+    editor.change_selections(
-        s.move_with(|map, selection| {
+        SelectionEffects::no_scroll().nav_history(false),
-            let mut head = selection.head();
+        window,
-            let top = top_anchor.to_display_point(map);
+        cx,
-            let starting_column = head.column();
+        |s| {
+            s.move_with(|map, selection| {
+                let mut head = selection.head();
+                let top = top_anchor.to_display_point(map);
+                let starting_column = head.column();
 
-            let vertical_scroll_margin =
+                let vertical_scroll_margin =
-                (vertical_scroll_margin as u32).min(visible_line_count as u32 / 2);
+                    (vertical_scroll_margin as u32).min(visible_line_count as u32 / 2);
 
-            if preserve_cursor_position {
+                if preserve_cursor_position {
-                let old_top = old_top_anchor.to_display_point(map);
+                    let old_top = old_top_anchor.to_display_point(map);
-                let new_row = if old_top.row() == top.row() {
+                    let new_row = if old_top.row() == top.row() {
-                    DisplayRow(
+                        DisplayRow(
-                        head.row()
+                            head.row()
-                            .0
+                                .0
-                            .saturating_add_signed(amount.lines(visible_line_count) as i32),
+                                .saturating_add_signed(amount.lines(visible_line_count) as i32),
-                    )
+                        )
+                    } else {
+                        DisplayRow(top.row().0 + selection.head().row().0 - old_top.row().0)
+                    };
+                    head = map.clip_point(DisplayPoint::new(new_row, head.column()), Bias::Left)
+                }
+
+                let min_row = if top.row().0 == 0 {
+                    DisplayRow(0)
                 } else {
-                    DisplayRow(top.row().0 + selection.head().row().0 - old_top.row().0)
+                    DisplayRow(top.row().0 + vertical_scroll_margin)
                 };
-                head = map.clip_point(DisplayPoint::new(new_row, head.column()), Bias::Left)
-            }
 
-            let min_row = if top.row().0 == 0 {
+                let max_visible_row = top.row().0.saturating_add(
-                DisplayRow(0)
+                    (visible_line_count as u32).saturating_sub(1 + vertical_scroll_margin),
-            } else {
+                );
-                DisplayRow(top.row().0 + vertical_scroll_margin)
+                // scroll off the end.
-            };
+                let max_row = if top.row().0 + visible_line_count as u32 >= map.max_point().row().0
+                {
+                    map.max_point().row()
+                } else {
+                    DisplayRow(
+                        (top.row().0 + visible_line_count as u32)
+                            .saturating_sub(1 + vertical_scroll_margin),
+                    )
+                };
 
-            let max_visible_row = top.row().0.saturating_add(
+                let new_row = if full_page_up {
-                (visible_line_count as u32).saturating_sub(1 + vertical_scroll_margin),
+                    // Special-casing ctrl-b/page-up, which is special-cased by Vim, it seems
-            );
+                    // to always put the cursor on the last line of the page, even if the cursor
-            // scroll off the end.
+                    // was before that.
-            let max_row = if top.row().0 + visible_line_count as u32 >= map.max_point().row().0 {
+                    DisplayRow(max_visible_row)
-                map.max_point().row()
+                } else if head.row() < min_row {
-            } else {
+                    min_row
-                DisplayRow(
+                } else if head.row() > max_row {
-                    (top.row().0 + visible_line_count as u32)
+                    max_row
-                        .saturating_sub(1 + vertical_scroll_margin),
+                } else {
-                )
+                    head.row()
-            };
+                };
+                let new_head =
+                    map.clip_point(DisplayPoint::new(new_row, starting_column), Bias::Left);
 
-            let new_row = if full_page_up {
+                if selection.is_empty() {
-                // Special-casing ctrl-b/page-up, which is special-cased by Vim, it seems
+                    selection.collapse_to(new_head, selection.goal)
-                // to always put the cursor on the last line of the page, even if the cursor
+                } else {
-                // was before that.
+                    selection.set_head(new_head, selection.goal)
-                DisplayRow(max_visible_row)
+                };
-            } else if head.row() < min_row {
+            })
-                min_row
+        },
-            } else if head.row() > max_row {
+    );
-                max_row
-            } else {
-                head.row()
-            };
-            let new_head = map.clip_point(DisplayPoint::new(new_row, starting_column), Bias::Left);
-
-            if selection.is_empty() {
-                selection.collapse_to(new_head, selection.goal)
-            } else {
-                selection.set_head(new_head, selection.goal)
-            };
-        })
-    });
 }
 
 #[cfg(test)]
@@ -424,4 +431,20 @@ mod test {
             cx.shared_state().await.assert_matches();
         }
     }
+
+    #[gpui::test]
+    async fn test_scroll_jumps(cx: &mut gpui::TestAppContext) {
+        let mut cx = NeovimBackedTestContext::new(cx).await;
+
+        cx.set_scroll_height(20).await;
+
+        let content = "ˇ".to_owned() + &sample_text(52, 2, 'a');
+        cx.set_shared_state(&content).await;
+
+        cx.simulate_shared_keystrokes("shift-g g g").await;
+        cx.simulate_shared_keystrokes("ctrl-d ctrl-d ctrl-o").await;
+        cx.shared_state().await.assert_matches();
+        cx.simulate_shared_keystrokes("ctrl-o").await;
+        cx.shared_state().await.assert_matches();
+    }
 }

crates/vim/test_data/test_scroll_jumps.json

@@ -0,0 +1,12 @@
+{"SetOption":{"value":"scrolloff=3"}}
+{"SetOption":{"value":"lines=22"}}
+{"Put":{"state":"ˇaa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\njj\nkk\nll\nmm\nnn\noo\npp\nqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz\n{{\n||\n}}\n~~\n\n€€\n\n‚‚\nƒƒ\n„„\n……\n††\n‡‡\nˆˆ\n‰‰\nŠŠ\n‹‹\nŒŒ\n\nŽŽ\n\n\n‘‘\n’’\n““\n””"}}
+{"Key":"shift-g"}
+{"Key":"g"}
+{"Key":"g"}
+{"Key":"ctrl-d"}
+{"Key":"ctrl-d"}
+{"Key":"ctrl-o"}
+{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\njj\nkk\nll\nmm\nnn\noo\npp\nqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz\n{{\n||\n}}\n~~\n\n€€\n\n‚‚\nƒƒ\n„„\n……\n††\n‡‡\nˆˆ\n‰‰\nŠŠ\n‹‹\nŒŒ\n\nŽŽ\n\n\n‘‘\n’’\n““\nˇ””","mode":"Normal"}}
+{"Key":"ctrl-o"}
+{"Get":{"state":"ˇaa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\njj\nkk\nll\nmm\nnn\noo\npp\nqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz\n{{\n||\n}}\n~~\n\n€€\n\n‚‚\nƒƒ\n„„\n……\n††\n‡‡\nˆˆ\n‰‰\nŠŠ\n‹‹\nŒŒ\n\nŽŽ\n\n\n‘‘\n’’\n““\n””","mode":"Normal"}}