From 6357a85e8fd30da52590364eee5213d7ff978947 Mon Sep 17 00:00:00 2001 From: Dino Date: Wed, 29 Apr 2026 08:29:15 +0100 Subject: [PATCH] editor: Add preserve scroll strategy for go to definition (#55036) These changes attempt to expand on the work introduced by https://github.com/zed-industries/zed/pull/54778 by introducing a new `GoToDefinitionScrollStrategy::Preserve` variant that attempts to keep the cursor at the same vertical offset within the viewport when navigating to a definition. Most of the machinery for this was already in place. To support cases where the user's scroll position isn't snapped to an exact display row, for example, after scrolling with the mmouse, `Autoscroll::TopRelative` and `Autoscroll::BottomRelative` were updated from `usize` to `ScrollOffset`, allowing fractional offsets. When the cursor is offscreen at the moment the `editor: go to definition` action is invoked, `Preserve` falls back to `Autoscroll::center`, matching the existing default for `go_to_definition_scroll_strategy`. This avoids attempting to preserve an offset where the cursor isn't visible which would lead to the cursor being offscreen when jumping to the definition. Documentation has also been updated to reflect this new strategy value. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Relates to #52173 Release Notes: - Added a new `preserve` option to `go_to_definition_scroll_strategy` that keeps the cursor at the same vertical position within the viewport when navigating to a definition --- assets/settings/default.json | 2 + crates/editor/src/editor.rs | 49 ++++- crates/editor/src/editor_tests.rs | 238 ++++++++++++++++++++++++- crates/editor/src/element.rs | 4 +- crates/editor/src/scroll/autoscroll.rs | 27 +-- crates/repl/src/session.rs | 2 +- crates/settings_content/src/editor.rs | 3 + docs/src/reference/all-settings.md | 9 + 8 files changed, 312 insertions(+), 22 deletions(-) diff --git a/assets/settings/default.json b/assets/settings/default.json index 464587e629..02d93ece4a 100644 --- a/assets/settings/default.json +++ b/assets/settings/default.json @@ -361,6 +361,8 @@ // 1. Vertically center the target in the viewport: `center` (default) // 2. Scroll the minimum amount needed to make the target visible: `minimum` // 3. Scroll so the target appears near the top of the viewport: `top` + // 4. Preserve the cursor's vertical position within the viewport, falling back to `center` when the cursor is + // offscreen: `preserve` "go_to_definition_scroll_strategy": "center", // Which level to use to filter out diagnostics displayed in the editor. // diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index ee6b2be18b..42e4ebb606 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -18340,7 +18340,11 @@ impl Editor { }; let anchor_range = range.to_anchors(&multibuffer.snapshot(cx)); self.change_selections( - SelectionEffects::scroll(Autoscroll::for_go_to_definition(cx)).nav_history(true), + SelectionEffects::scroll(Autoscroll::for_go_to_definition( + self.cursor_top_offset(cx), + cx, + )) + .nav_history(true), window, cx, |s| s.select_anchor_ranges([anchor_range]), @@ -19146,8 +19150,11 @@ impl Editor { } editor.change_selections( - SelectionEffects::scroll(Autoscroll::for_go_to_definition(cx)) - .nav_history(true), + SelectionEffects::scroll(Autoscroll::for_go_to_definition( + editor.cursor_top_offset(cx), + cx, + )) + .nav_history(true), window, cx, |s| s.select_anchor_ranges(target_ranges), @@ -19163,6 +19170,8 @@ impl Editor { return Navigated::No; }; let pane = workspace.read(cx).active_pane().clone(); + let offset = editor.cursor_top_offset(cx); + window.defer(cx, move |window, cx| { let (target_editor, target_pane): (Entity, Entity) = workspace.update(cx, |workspace, cx| { @@ -19226,8 +19235,10 @@ impl Editor { } target_editor.change_selections( - SelectionEffects::scroll(Autoscroll::for_go_to_definition(cx)) - .nav_history(true), + SelectionEffects::scroll(Autoscroll::for_go_to_definition( + offset, cx, + )) + .nav_history(true), window, cx, |s| s.select_anchor_ranges(target_ranges), @@ -19507,7 +19518,10 @@ impl Editor { let Range { start, end } = locations[destination_location_index]; editor.update_in(cx, |editor, window, cx| { - let effects = SelectionEffects::scroll(Autoscroll::for_go_to_definition(cx)); + let effects = SelectionEffects::scroll(Autoscroll::for_go_to_definition( + editor.cursor_top_offset(cx), + cx, + )); editor.unfold_ranges(&[start..end], false, false, cx); editor.change_selections(effects, window, cx, |s| { @@ -25672,7 +25686,7 @@ impl Editor { } let autoscroll = match scroll_offset { Some(scroll_offset) => { - Autoscroll::top_relative(scroll_offset as usize) + Autoscroll::top_relative(scroll_offset as ScrollOffset) } None => Autoscroll::newest(), }; @@ -26747,6 +26761,27 @@ impl Editor { self.refresh_runnables(None, window, cx); } } + + /// Returns the current cursor's vertical offset, in display rows, from the + /// top of the visible viewport. + /// Returns `None` if the cursor is not currently on screen. + pub fn cursor_top_offset(&self, cx: &mut Context) -> Option { + let visible = self.visible_line_count()?; + let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx)); + let scroll_top = self.scroll_manager.scroll_position(&display_map, cx).y; + let cursor_display_row = self + .selections + .newest::(&display_map) + .head() + .to_display_point(&display_map) + .row() + .as_f64(); + + match cursor_display_row - scroll_top { + offset if offset < 0.0 || offset >= visible => None, + offset => Some(offset), + } + } } fn edit_for_markdown_paste<'a>( diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 3a23bb1147..b0ea1b8674 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -49,9 +49,9 @@ use project::{ use serde_json::{self, json}; use settings::{ AllLanguageSettingsContent, DelayMs, EditorSettingsContent, GlobalLspSettingsContent, - IndentGuideBackgroundColoring, IndentGuideColoring, InlayHintSettingsContent, - ProjectSettingsContent, ScrollBeyondLastLine, SearchSettingsContent, SettingsContent, - SettingsStore, + GoToDefinitionScrollStrategy, IndentGuideBackgroundColoring, IndentGuideColoring, + InlayHintSettingsContent, ProjectSettingsContent, ScrollBeyondLastLine, SearchSettingsContent, + SettingsContent, SettingsStore, }; use std::{borrow::Cow, sync::Arc}; use std::{cell::RefCell, future::Future, rc::Rc, sync::atomic::AtomicBool, time::Instant}; @@ -2962,6 +2962,102 @@ async fn test_autoscroll(cx: &mut TestAppContext) { }); } +#[gpui::test] +async fn test_autoscroll_relative(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + let mut cx = EditorTestContext::new(cx).await; + + let line_height = cx.update_editor(|editor, window, cx| { + editor.set_vertical_scroll_margin(0, cx); + editor + .style(cx) + .text + .line_height_in_pixels(window.rem_size()) + }); + let window = cx.window; + + // Resize the window such that only 6 lines of text fit on screen. + cx.simulate_window_resize(window, size(px(1000.), 6. * line_height)); + + cx.set_state( + r#"ˇone + two + three + four + five + six + seven + eight + nine + ten + eleven + twelve + thirteen + fourteen + fifteen + "#, + ); + cx.update_editor(|editor, window, cx| { + assert_eq!( + editor.snapshot(window, cx).scroll_position(), + gpui::Point::new(0., 0.0) + ); + }); + + // Placing the cursor at row 7 with a top-relative autoscroll of 2 display + // rows, should land the scroll position's y coordinate at 5.0 (7 - 2). + cx.update_editor(|editor, window, cx| { + editor.change_selections( + SelectionEffects::scroll(Autoscroll::top_relative(2.0)), + window, + cx, + |selections| selections.select_ranges([Point::new(7, 0)..Point::new(7, 0)]), + ); + }); + cx.update_editor(|editor, window, cx| { + assert_eq!( + editor.snapshot(window, cx).scroll_position(), + gpui::Point::new(0., 5.0) + ); + }); + + // Seeing as fractional offsets are supported, with the cursor at row 10 and + // a top-relative autoscroll of 2.5 display rows, the scroll position's y + // coordinate lands at 7.5 (10 - 2.5). + cx.update_editor(|editor, window, cx| { + editor.change_selections( + SelectionEffects::scroll(Autoscroll::top_relative(2.5)), + window, + cx, + |selections| selections.select_ranges([Point::new(10, 0)..Point::new(10, 0)]), + ); + }); + cx.update_editor(|editor, window, cx| { + assert_eq!( + editor.snapshot(window, cx).scroll_position(), + gpui::Point::new(0., 7.5) + ); + }); + + // When the requested offset would scroll past the top of the buffer, + // `scroll_position.y` is clamped to 0 rather than going negative. + cx.update_editor(|editor, window, cx| { + editor.change_selections( + SelectionEffects::scroll(Autoscroll::top_relative(4.0)), + window, + cx, + |selections| selections.select_ranges([Point::new(1, 0)..Point::new(1, 0)]), + ); + }); + + cx.update_editor(|editor, window, cx| { + assert_eq!( + editor.snapshot(window, cx).scroll_position(), + gpui::Point::new(0., 0.0) + ); + }); +} + #[gpui::test] async fn test_exclude_overscroll_margin_clamps_scroll_position(cx: &mut TestAppContext) { init_test(cx, |_| {}); @@ -26371,6 +26467,142 @@ async fn test_goto_definition_contained_ranges(cx: &mut TestAppContext) { assert_eq!(navigated, Navigated::Yes); } +#[gpui::test] +async fn test_goto_definition_preserve_scroll_strategy(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + update_test_editor_settings(cx, &|settings| { + settings.go_to_definition_scroll_strategy = Some(GoToDefinitionScrollStrategy::Preserve); + settings.vertical_scroll_margin = Some(0.0); + }); + + let mut cx = EditorLspTestContext::new_rust( + lsp::ServerCapabilities { + definition_provider: Some(lsp::OneOf::Left(true)), + ..lsp::ServerCapabilities::default() + }, + cx, + ) + .await; + + let window = cx.window; + let line_height = cx.update_editor(|editor, window, cx| { + editor + .style(cx) + .text + .line_height_in_pixels(window.rem_size()) + }); + cx.simulate_window_resize(window, size(px(1000.), 8. * line_height)); + + // Build a buffer where `target` is defined on row 10 and called from + // row 20, with the cursor placed on the call site. + let buffer = indoc! { " + // 0 + // 1 + // 2 + // 3 + // 4 + // 5 + // 6 + // 7 + // 8 + // 9 + fn target() // 10 + // 11 + // 12 + // 13 + // 14 + // 15 + // 16 + // 17 + // 18 + // 19 + fn caller() { ˇtarget(); } // 20 + // 21 + // 22 + // 23 + // 24 + // 25 + // 26 + // 27 + // 28 + // 29 + // 30 + "}; + + // Mock the response from the LSP server when requesting to go to a + // definition so as to always jump to the `target` function. + cx.set_request_handler::(|url, _, _| async move { + Ok(Some(lsp::GotoDefinitionResponse::Scalar(lsp::Location { + uri: url.clone(), + range: lsp::Range::new(lsp::Position::new(10, 3), lsp::Position::new(10, 9)), + }))) + }); + + let caller_row = 20.0; + let target_row = 10.0; + let offset = 1.5; + let center_offset = cx.update_editor(|editor, _, _| { + editor + .visible_line_count() + .map(|count| ((count - 1.0) / 2.0).floor()) + .expect("Visible line count should be available") + }); + + // When the cursor is visible inside the viewport, going to a definition + // should preserve that same offset value. + // In this case, with the cursor set at row 20 and the scroll position set + // to 18.5 (20 - 1.5), when going to the definition of `target` in row 10, + // the scroll position should end up at 8.5 (10 - 1.5), so as to preserve + // that same offset of 1.5. + cx.set_state(&buffer); + cx.update_editor(|editor, window, cx| { + editor.set_scroll_position(gpui::Point::new(0.0, caller_row - offset), window, cx); + }); + cx.update_editor(|editor, window, cx| editor.go_to_definition(&GoToDefinition, window, cx)) + .await + .expect("Failed to navigate to definition"); + cx.run_until_parked(); + cx.update_editor(|editor, window, cx| { + assert_eq!( + editor.snapshot(window, cx).scroll_position(), + gpui::Point::new(0.0, target_row - offset), + ); + }); + + // In the case where the cursor ends up outside of the visible viewport, the + // scroll position's offset should be ignored and the center of the viewport + // should be used instead. + // Since the cursor is jumping to row 10, the scroll position's y coordinate + // should end up at 10 minus the offset from the center of the viewport. + cx.set_state(&buffer); + cx.update_editor(|editor, window, cx| { + editor.set_scroll_position(gpui::Point::new(0.0, 0.0), window, cx); + let snapshot = editor.display_snapshot(cx); + let cursor_row = editor + .selections + .newest_display(&snapshot) + .start + .row() + .as_f64(); + let visible_lines = editor + .visible_line_count() + .expect("Visible line count should be available"); + + assert!(cursor_row >= visible_lines, "Cursor should be offscreen"); + }); + + cx.update_editor(|editor, window, cx| editor.go_to_definition(&GoToDefinition, window, cx)) + .await + .expect("Failed to navigate to definition"); + cx.run_until_parked(); + cx.update_editor(|editor, window, cx| { + assert_eq!( + editor.snapshot(window, cx).scroll_position(), + gpui::Point::new(0.0, (target_row - center_offset).max(0.0)), + ); + }); +} + #[gpui::test] async fn test_find_all_references_editor_reuse(cx: &mut TestAppContext) { init_test(cx, |_| {}); diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index 4b719b1474..a6be884058 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -6814,7 +6814,9 @@ impl EditorElement { .display_snapshot .display_point_to_anchor(point_for_position.nearest_valid, Bias::Left); editor.change_selections( - SelectionEffects::scroll(Autoscroll::top_relative(line_index)), + SelectionEffects::scroll(Autoscroll::top_relative( + line_index as ScrollOffset, + )), window, cx, |selections| { diff --git a/crates/editor/src/scroll/autoscroll.rs b/crates/editor/src/scroll/autoscroll.rs index 38f0f4b022..6bdb60a470 100644 --- a/crates/editor/src/scroll/autoscroll.rs +++ b/crates/editor/src/scroll/autoscroll.rs @@ -36,11 +36,14 @@ impl Autoscroll { /// Returns the autoscroll strategy configured for navigation to definitions /// and references, based on `go_to_definition_scroll_strategy`. - pub fn for_go_to_definition(cx: &App) -> Self { + pub fn for_go_to_definition(offset: Option, cx: &App) -> Self { match EditorSettings::get_global(cx).go_to_definition_scroll_strategy { GoToDefinitionScrollStrategy::Center => Self::center(), GoToDefinitionScrollStrategy::Minimum => Self::fit(), GoToDefinitionScrollStrategy::Top => Self::focused(), + GoToDefinitionScrollStrategy::Preserve => { + offset.map(Self::top_relative).unwrap_or_else(Self::center) + } } } @@ -50,9 +53,10 @@ impl Autoscroll { Self::Strategy(AutoscrollStrategy::Focused, None) } - /// Scrolls so that the newest cursor is roughly an n-th line from the top. - pub fn top_relative(n: usize) -> Self { - Self::Strategy(AutoscrollStrategy::TopRelative(n), None) + /// Scrolls so that the newest cursor is the given offset (in display rows) + /// from the top of the viewport. + pub fn top_relative(offset: ScrollOffset) -> Self { + Self::Strategy(AutoscrollStrategy::TopRelative(offset), None) } /// Scrolls so that the newest cursor is at the top. @@ -60,9 +64,10 @@ impl Autoscroll { Self::Strategy(AutoscrollStrategy::Top, None) } - /// Scrolls so that the newest cursor is roughly an n-th line from the bottom. - pub fn bottom_relative(n: usize) -> Self { - Self::Strategy(AutoscrollStrategy::BottomRelative(n), None) + /// Scrolls so that the newest cursor is the given offset (in display rows) + /// from the bottom of the viewport. + pub fn bottom_relative(offset: ScrollOffset) -> Self { + Self::Strategy(AutoscrollStrategy::BottomRelative(offset), None) } /// Scrolls so that the newest cursor is at the bottom. @@ -91,7 +96,7 @@ impl Into for Option { } } -#[derive(Debug, PartialEq, Eq, Default, Clone, Copy)] +#[derive(Debug, PartialEq, Default, Clone, Copy)] pub enum AutoscrollStrategy { Fit, Newest, @@ -100,10 +105,12 @@ pub enum AutoscrollStrategy { Focused, Top, Bottom, - TopRelative(usize), - BottomRelative(usize), + TopRelative(ScrollOffset), + BottomRelative(ScrollOffset), } +impl Eq for AutoscrollStrategy {} + impl AutoscrollStrategy { fn next(&self) -> Self { match self { diff --git a/crates/repl/src/session.rs b/crates/repl/src/session.rs index 3849138448..9b7bd75950 100644 --- a/crates/repl/src/session.rs +++ b/crates/repl/src/session.rs @@ -795,7 +795,7 @@ impl Session { if move_down { editor.update(cx, move |editor, cx| { editor.change_selections( - SelectionEffects::scroll(Autoscroll::top_relative(8)), + SelectionEffects::scroll(Autoscroll::top_relative(8.0)), window, cx, |selections| { diff --git a/crates/settings_content/src/editor.rs b/crates/settings_content/src/editor.rs index 18a7bd5fd4..5c0dd93968 100644 --- a/crates/settings_content/src/editor.rs +++ b/crates/settings_content/src/editor.rs @@ -831,6 +831,9 @@ pub enum GoToDefinitionScrollStrategy { Minimum, /// Scroll so the target appears near the top of the viewport. Top, + /// Preserve the cursor's vertical position within the viewport, falling + /// back to centering when the cursor is offscreen. + Preserve, } /// Determines when the mouse cursor should be hidden in an editor or input box. diff --git a/docs/src/reference/all-settings.md b/docs/src/reference/all-settings.md index c6c33b8b5e..f175d51a39 100644 --- a/docs/src/reference/all-settings.md +++ b/docs/src/reference/all-settings.md @@ -2456,6 +2456,15 @@ Example: } ``` +4. Preserve the cursor's vertical position within the viewport, falling back to + `center` when the cursor is offscreen. + +```json [settings] +{ + "go_to_definition_scroll_strategy": "preserve" +} +``` + ## Hard Tabs - Description: Whether to indent lines using tab characters or multiple spaces.