From 029bf2f284b4e59f20175d78443e630468f3a3e5 Mon Sep 17 00:00:00 2001 From: TwoClocks <5883156+TwoClocks@users.noreply.github.com> Date: Wed, 8 Jul 2026 09:37:08 -0700 Subject: [PATCH] Make editor::LineUp & editor::LineDown honor vertical_scroll_margin like vim::LineUp & vim::LineDown (#52057) ## Context This is an implementation of this feature: https://github.com/zed-industries/zed/discussions/49821 Although, I'd argue it's a bug fix, not a feature. Either way : This copies the window scroll logic from the `vim` mode versions with out all the extra logic for visual mode. Could refactor the common logic out of the vim code and make it common. But that seems like a bigger PR. Happy to take a stab at it if that's what you prefer. Could also add new commands for the new behavior if you prefer. I didn't do that, because it seems like more clutter in the commands, and my belief that the existing behavior is a bug. But happy to do that if you prefer. ## How to Review creates new function `scroll_screen_with_cursor_margin` in `scroll.rs` wires up editor::LineUp/LineDown to new function in `elements.rs` adds a test in `editor_tests.rs` ## 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 Release Notes: - editor::LineUp/LineDown commands now honor vertical_scroll_margin (same as vim::LineUp/LineDown) --------- Co-authored-by: Kirill Bulatov --- crates/editor/src/editor_tests.rs | 84 +++++++++++++++++++++++++++++++ crates/editor/src/element.rs | 12 ++--- crates/editor/src/scroll.rs | 59 +++++++++++++++++++++- 3 files changed, 148 insertions(+), 7 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 2fa319300e5..ca5cc9a6a2f 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -2958,6 +2958,90 @@ async fn test_scroll_page_up_page_down(cx: &mut TestAppContext) { }); } +#[gpui::test] +async fn test_scroll_line_up_down_cursor_margin(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(2, cx); + editor + .style(cx) + .text + .line_height_in_pixels(window.rem_size()) + }); + let window = cx.window; + // 5 visible lines with margin=2: valid cursor rows are [top+2 .. top+2] (only the middle row) + cx.simulate_window_resize(window, size(px(1000.), 5. * line_height)); + + // Cursor at row 0 — autoscroll leaves viewport at y=0. + cx.set_state(indoc! {" + ˇone + two + three + four + five + six + seven + eight + nine + ten + "}); + + cx.update_editor(|editor, window, cx| { + assert_eq!(editor.snapshot(window, cx).scroll_position().y, 0.); + + // Scroll down 1: top=1, visible rows 1-5, margin=2 → min_row=3, max_row=3. + // Cursor at row 0 < min_row=3 → clamped to row 3. + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Line(1.), window, cx); + assert_eq!(editor.snapshot(window, cx).scroll_position().y, 1.); + let snapshot = editor.display_snapshot(cx); + assert_eq!( + editor.selections.newest_display(&snapshot).head().row().0, + 3, + "cursor clamped to min_row after scrolling down" + ); + }); + + cx.update_editor(|editor, window, cx| { + // Scroll up 1: top=0, visible rows 0-4, margin=2 (top==0 special case) → min_row=0, max_row=2. + // Cursor at row 3 > max_row=2 → clamped to row 2. + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Line(-1.), window, cx); + assert_eq!(editor.snapshot(window, cx).scroll_position().y, 0.); + let snapshot = editor.display_snapshot(cx); + assert_eq!( + editor.selections.newest_display(&snapshot).head().row().0, + 2, + "cursor clamped to max_row after scrolling up" + ); + }); + + cx.update_editor(|editor, window, cx| { + // Half page down (2 lines): top=2, visible rows 2-6, margin=2 → min_row=4, max_row=4. + // Cursor at row 2 < min_row=4 → clamped to row 4. + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Page(0.5), window, cx); + assert_eq!(editor.snapshot(window, cx).scroll_position().y, 2.); + let snapshot = editor.display_snapshot(cx); + assert_eq!( + editor.selections.newest_display(&snapshot).head().row().0, + 4, + "cursor clamped to min_row after scrolling half page down" + ); + }); + + cx.update_editor(|editor, window, cx| { + // Half page up (2 lines): top=0, visible rows 0-4, margin=2 (top==0 special case) → min_row=0, max_row=2. + // Cursor at row 4 > max_row=2 → clamped to row 2. + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Page(-0.5), window, cx); + assert_eq!(editor.snapshot(window, cx).scroll_position().y, 0.); + let snapshot = editor.display_snapshot(cx); + assert_eq!( + editor.selections.newest_display(&snapshot).head().row().0, + 2, + "cursor clamped to max_row after scrolling half page up" + ); + }); +} + #[gpui::test] async fn test_autoscroll(cx: &mut TestAppContext) { init_test(cx, |_| {}); diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index a82b3ea4488..4ce36632f25 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -287,22 +287,22 @@ impl EditorElement { register_action(editor, window, Editor::scroll_cursor_bottom); register_action(editor, window, Editor::scroll_cursor_center_top_bottom); register_action(editor, window, |editor, _: &LineDown, window, cx| { - editor.scroll_screen(&ScrollAmount::Line(1.), window, cx) + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Line(1.), window, cx) }); register_action(editor, window, |editor, _: &LineUp, window, cx| { - editor.scroll_screen(&ScrollAmount::Line(-1.), window, cx) + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Line(-1.), window, cx) }); register_action(editor, window, |editor, _: &HalfPageDown, window, cx| { - editor.scroll_screen(&ScrollAmount::Page(0.5), window, cx) + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Page(0.5), window, cx) }); register_action(editor, window, |editor, _: &HalfPageUp, window, cx| { - editor.scroll_screen(&ScrollAmount::Page(-0.5), window, cx) + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Page(-0.5), window, cx) }); register_action(editor, window, |editor, _: &PageDown, window, cx| { - editor.scroll_screen(&ScrollAmount::Page(1.), window, cx) + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Page(1.), window, cx) }); register_action(editor, window, |editor, _: &PageUp, window, cx| { - editor.scroll_screen(&ScrollAmount::Page(-1.), window, cx) + editor.scroll_screen_with_cursor_margin(&ScrollAmount::Page(-1.), window, cx) }); register_action(editor, window, Editor::move_to_previous_word_start); register_action(editor, window, Editor::move_to_previous_subword_start); diff --git a/crates/editor/src/scroll.rs b/crates/editor/src/scroll.rs index ec7f9036c4a..dcd96c675c5 100644 --- a/crates/editor/src/scroll.rs +++ b/crates/editor/src/scroll.rs @@ -5,7 +5,7 @@ pub(crate) mod scroll_amount; use crate::editor_settings::ScrollBeyondLastLine; use crate::{ Anchor, DisplayPoint, DisplayRow, Editor, EditorEvent, EditorMode, EditorSettings, - MultiBufferSnapshot, RowExt, SizingBehavior, ToPoint, + MultiBufferSnapshot, RowExt, SelectionEffects, SizingBehavior, ToPoint, display_map::{DisplaySnapshot, ToDisplayPoint}, hover_popover::hide_hover, persistence::EditorDb, @@ -945,6 +945,63 @@ impl Editor { self.set_scroll_position(new_position, window, cx); } + pub fn scroll_screen_with_cursor_margin( + &mut self, + amount: &ScrollAmount, + window: &mut Window, + cx: &mut Context, + ) { + self.scroll_screen(amount, window, cx); + + let Some(visible_line_count) = self.visible_line_count() else { + return; + }; + let display_snapshot = self.display_map.update(cx, |map, cx| map.snapshot(cx)); + let top = self + .scroll_manager + .scroll_top_display_point(&display_snapshot, cx); + let vertical_scroll_margin = + (self.vertical_scroll_margin() as u32).min(visible_line_count as u32 / 2); + + let max_point = display_snapshot.max_point(); + let min_row = if top.row().0 == 0 { + DisplayRow(0) + } else { + DisplayRow(top.row().0 + vertical_scroll_margin) + }; + let max_row = if top.row().0 + visible_line_count as u32 >= max_point.row().0 { + max_point.row() + } else { + DisplayRow( + (top.row().0 + visible_line_count as u32) + .saturating_sub(1 + vertical_scroll_margin), + ) + }; + + self.change_selections( + SelectionEffects::no_scroll().nav_history(false), + window, + cx, + |s| { + s.move_with(&mut |map, selection| { + let head = selection.head(); + let new_row = if head.row() < min_row { + min_row + } else if head.row() > max_row { + max_row + } else { + head.row() + }; + if new_row != head.row() { + let new_head = + map.clip_point(DisplayPoint::new(new_row, head.column()), Bias::Left); + selection.collapse_to(new_head, selection.goal); + } + }) + }, + ); + } + /// Returns an ordering. The newest selection is: /// Ordering::Equal => on screen /// Ordering::Less => above or to the left of the screen