From 9166e66519cd39663ce26ab7912db46a39e66adb Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 12 Jun 2025 22:18:22 -0600 Subject: [PATCH] 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 | 129 ++++++++++------- crates/editor/src/items.rs | 14 +- crates/editor/src/jsx_tag_auto_close.rs | 13 +- crates/editor/src/scroll/autoscroll.rs | 16 +- .../src/test/editor_lsp_test_context.rs | 6 + crates/vim/src/normal/scroll.rs | 137 ++++++++++-------- crates/vim/test_data/test_scroll_jumps.json | 12 ++ 7 files changed, 208 insertions(+), 119 deletions(-) create mode 100644 crates/vim/test_data/test_scroll_jumps.json diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 6bbcebdbc6..cf9dc99232 100644 --- a/crates/editor/src/editor.rs +++ b/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, +} + +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, - autoscroll: Option, + effects: SelectionEffects, 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, ) { @@ -2766,12 +2811,14 @@ impl Editor { let new_cursor_position = newest_selection.head(); let selection_start = newest_selection.start; - self.push_to_nav_history( - *old_cursor_position, - Some(new_cursor_position.to_point(buffer)), - false, - cx, - ); + if effects.nav_history { + self.push_to_nav_history( + *old_cursor_position, + Some(new_cursor_position.to_point(buffer)), + 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( &mut self, - autoscroll: Option, - window: &mut Window, - cx: &mut Context, - change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R, - ) -> R { - self.change_selections_inner(true, autoscroll, window, cx, change) - } - - pub(crate) fn change_selections_without_updating_completions( - &mut self, - autoscroll: Option, - window: &mut Window, - cx: &mut Context, - change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R, - ) -> R { - self.change_selections_inner(false, autoscroll, window, cx, change) - } - - fn change_selections_inner( - &mut self, - should_update_completions: bool, - autoscroll: Option, + effects: impl Into, window: &mut Window, cx: &mut Context, 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.should_update_completions = should_update_completions; + state.effects.scroll = effects.scroll.or(state.effects.scroll); + 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, - autoscroll, + effects, 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( - true, - &old_cursor_position, - state.should_update_completions, - window, - cx, - ); + self.selections_did_change(true, &old_cursor_position, state.effects, 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( - Some(Autoscroll::fit()), + this.change_selections( + 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( diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 9e18c40cf4..f5b8b0b64d 100644 --- a/crates/editor/src/items.rs +++ b/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| { - s.select_ranges([offset..offset]) - }); - self.nav_history = nav_history; + self.change_selections( + SelectionEffects::default().nav_history(false), + window, + cx, + |s| s.select_ranges([offset..offset]), + ); true } } else { diff --git a/crates/editor/src/jsx_tag_auto_close.rs b/crates/editor/src/jsx_tag_auto_close.rs index c365b285c7..f24fe46100 100644 --- a/crates/editor/src/jsx_tag_auto_close.rs +++ b/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::>(); this.update_in(cx, |this, window, cx| { - this.change_selections_without_updating_completions(None, window, cx, |s| { - s.select(base_selections); - }); + this.change_selections( + SelectionEffects::no_scroll().completions(false), + window, + cx, + |s| { + s.select(base_selections); + }, + ); }) .ok()?; } diff --git a/crates/editor/src/scroll/autoscroll.rs b/crates/editor/src/scroll/autoscroll.rs index 7c022e62a4..55998aa2fd 100644 --- a/crates/editor/src/scroll/autoscroll.rs +++ b/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), @@ -66,7 +67,16 @@ impl Autoscroll { } } -#[derive(PartialEq, Eq, Default, Clone, Copy)] +impl Into for Option { + 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, diff --git a/crates/editor/src/test/editor_lsp_test_context.rs b/crates/editor/src/test/editor_lsp_test_context.rs index 1f625b4510..51ae9306a3 100644 --- a/crates/editor/src/test/editor_lsp_test_context.rs +++ b/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)) }); diff --git a/crates/vim/src/normal/scroll.rs b/crates/vim/src/normal/scroll.rs index 5586f6ff71..5f3c6073be 100644 --- a/crates/vim/src/normal/scroll.rs +++ b/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| { - s.move_with(|map, selection| { - let mut head = selection.head(); - let top = top_anchor.to_display_point(map); - let starting_column = head.column(); + editor.change_selections( + SelectionEffects::no_scroll().nav_history(false), + window, + cx, + |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 = - (vertical_scroll_margin as u32).min(visible_line_count as u32 / 2); + let vertical_scroll_margin = + (vertical_scroll_margin as u32).min(visible_line_count as u32 / 2); - if preserve_cursor_position { - let old_top = old_top_anchor.to_display_point(map); - let new_row = if old_top.row() == top.row() { - DisplayRow( - head.row() - .0 - .saturating_add_signed(amount.lines(visible_line_count) as i32), - ) + if preserve_cursor_position { + let old_top = old_top_anchor.to_display_point(map); + let new_row = if old_top.row() == top.row() { + DisplayRow( + head.row() + .0 + .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 { - DisplayRow(0) - } else { - DisplayRow(top.row().0 + vertical_scroll_margin) - }; + let max_visible_row = top.row().0.saturating_add( + (visible_line_count as u32).saturating_sub(1 + 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( - (visible_line_count as u32).saturating_sub(1 + 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 new_row = if full_page_up { + // 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 + // was before that. + 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); - let new_row = if full_page_up { - // 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 - // was before that. - 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) - }; - }) - }); + 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(); + } } diff --git a/crates/vim/test_data/test_scroll_jumps.json b/crates/vim/test_data/test_scroll_jumps.json new file mode 100644 index 0000000000..8d44457bf9 --- /dev/null +++ b/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"}}