editor: Fix vertical scroll margin calculation (#63944)
# Objective - Fixes an issue where setting an arbitrarily high `Vertical Scroll Margin` fails to perfectly lock the cursor in the center of the viewport (Typewriter Mode?). In certain cases one could go up/down a line before auto-scroll gets triggered, deviating from user intent. - Fixes #63862 ## Solution - **The Issue:** When `Vertical Scroll Margin` is very high, the effective margin applied above and below the cursor is derived from the viewport's free space: ```rust ((visible_lines - (target_bottom - target_top)) / 2.0).floor() ``` - **The Fix:** Instead of using the same rounded number for both the top and bottom margins, calculate them separately: 1. The top margin stays the same as before. 2. The bottom margin is calculated by subtracting the top margin from the total free screen space. - This ensures the bottom margin automatically catches whatever fraction the floor operation on the top margin drops, so the two margins always sum to the full available space. ## Testing - Did you test these changes? If so, how? Ran `cargo test -p editor`, all passed. I've also attached the results of manual testing in the Showcase section below. ## 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 adheres to Zed's UI standards ([[UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [[icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md)](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable ## Showcase Before and after screen recording <details> <summary>Click to view showcase</summary> **Before:** https://private-user-images.githubusercontent.com/63847494/646999418-4dd388b8-4aba-4004-ac4f-6f1e0dad0f70.mp4?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3ODg5MTE3MTUsIm5iZiI6MTc4ODkxMTQxNSwicGF0aCI6Ii82Mzg0NzQ5NC82NDY5OTk0MTgtNGRkMzg4YjgtNGFiYS00MDA0LWFjNGYtNmYxZTBkYWQwZjcwLm1wND9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjA5MDglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYwOTA4VDIzNTAxNVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWRhZGM5MWExMDkyMDIzZjVlYjcxYmQ4YTRmOTM3MDliNDcyOTUzZjFhYmY1ZTgwMDY0YzhlYmQ1NzllYWYyN2EmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JnJlc3BvbnNlLWNvbnRlbnQtdHlwZT12aWRlbyUyRm1wNCJ9.DG6RpiEM-781ZctlvtQeWhw3kAYJ4KS6rNsAmMU38fg **After:** https://github.com/user-attachments/assets/4bf870e0-84ba-449a-bf2a-925c4a90d5ed </details> Release Notes: - Fixed the cursor not staying perfectly centered when using a high Vertical Scroll Margin (Typewriter Mode). --------- Co-authored-by: Smit Barmase <heysmitbarmase@gmail.com>
This commit is contained in:
rn23thakurandSmit Barmase authored and GitHub committed
1 parent
1753d7f52b
commit
108fd3ac11
2 files changed
+228
-3
No files matched your search
@@ -3756,6 +3756,115 @@ async fn test_autoscroll(cx: &mut TestAppContext) {
|
||||
});
|
||||
}
|
||||
|
||||
#[track_caller]
|
||||
fn assert_cursor_row_and_scroll_top(
|
||||
cx: &mut EditorTestContext,
|
||||
expected_cursor_row: u32,
|
||||
expected_scroll_top: f64,
|
||||
) {
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
let snapshot = editor.snapshot(window, cx);
|
||||
let cursor_row = editor
|
||||
.selections
|
||||
.newest_display(&snapshot.display_snapshot)
|
||||
.head()
|
||||
.row();
|
||||
|
||||
assert_eq!(cursor_row, DisplayRow(expected_cursor_row));
|
||||
assert_eq!(snapshot.scroll_position().y, expected_scroll_top);
|
||||
});
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_autoscroll_high_margin_keeps_cursor_centered(cx: &mut TestAppContext) {
|
||||
init_test(cx, |_| {});
|
||||
let mut cx = EditorTestContext::new(cx).await;
|
||||
|
||||
let line_height = cx.update_editor(|editor, window, cx| {
|
||||
// Let viewport space, rather than the configured margin, limit the padding.
|
||||
editor.set_vertical_scroll_margin(999, cx);
|
||||
editor
|
||||
.style(cx)
|
||||
.text
|
||||
.line_height_in_pixels(window.rem_size())
|
||||
});
|
||||
let window = cx.window;
|
||||
|
||||
for autoscroll in [Autoscroll::fit(), Autoscroll::newest()] {
|
||||
// After allowing for the cursor row, the free space is odd, fractional, or even.
|
||||
for (visible_lines, expected_top_margin) in [(6., 2.), (6.5, 2.), (7., 3.)] {
|
||||
cx.simulate_window_resize(window, size(px(1000.), visible_lines * line_height));
|
||||
|
||||
// Start at zero-based row 6, away from file boundaries that could clamp scrolling.
|
||||
cx.set_state("0\n1\n2\n3\n4\n5\nˇ6\n7\n8\n9\n10\n11\n12\n13\n14");
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
editor.set_scroll_position(gpui::Point::new(0., 0.), window, cx);
|
||||
editor.request_autoscroll(autoscroll, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
// The cursor's viewport offset is its row minus the scroll top.
|
||||
assert_cursor_row_and_scroll_top(&mut cx, 6, 6. - expected_top_margin);
|
||||
|
||||
// Moving down to row 7 must scroll with the cursor, preserving that offset.
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
editor.move_down(&MoveDown, window, cx);
|
||||
editor.request_autoscroll(autoscroll, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
assert_cursor_row_and_scroll_top(&mut cx, 7, 7. - expected_top_margin);
|
||||
|
||||
// Returning to row 6 must restore the viewport, not leave room for cursor drift.
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
editor.move_up(&MoveUp, window, cx);
|
||||
editor.request_autoscroll(autoscroll, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
assert_cursor_row_and_scroll_top(&mut cx, 6, 6. - expected_top_margin);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_autoscroll_auto_height_has_no_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_mode(EditorMode::AutoHeight {
|
||||
min_lines: 1,
|
||||
max_lines: Some(4),
|
||||
});
|
||||
|
||||
// Auto-height inputs must ignore even a nonzero configured margin.
|
||||
editor.set_vertical_scroll_margin(3, cx);
|
||||
editor
|
||||
.style(cx)
|
||||
.text
|
||||
.line_height_in_pixels(window.rem_size())
|
||||
});
|
||||
let window = cx.window;
|
||||
cx.simulate_window_resize(window, size(px(1000.), 4. * line_height));
|
||||
cx.set_state("ˇ0\n1\n2\n3\n4\n5\n6\n7\n8\n9");
|
||||
cx.update_editor(|editor, _, _| {
|
||||
assert_eq!(editor.visible_line_count(), Some(4.));
|
||||
});
|
||||
|
||||
// Row 2 is already visible, so moving there must not reserve space below it.
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
editor.move_down_by_lines(&MoveDownByLines { lines: 2 }, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
assert_cursor_row_and_scroll_top(&mut cx, 2, 0.);
|
||||
|
||||
// Row 4 is just outside the viewport: scroll only far enough to reveal it.
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
editor.move_down_by_lines(&MoveDownByLines { lines: 2 }, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
assert_cursor_row_and_scroll_top(&mut cx, 4, 1.);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_cursor_animation_remains_active_during_keyboard_autoscroll(cx: &mut TestAppContext) {
|
||||
init_test(cx, |_| {});
|
||||
@@ -43705,6 +43814,95 @@ async fn test_no_duplicated_sticky_headers(cx: &mut TestAppContext) {
|
||||
assert_eq!(sticky_headers(5.0), vec![]);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_autoscroll_margin_reserves_sticky_header_space(cx: &mut TestAppContext) {
|
||||
init_test(cx, |_| {});
|
||||
update_test_editor_settings(cx, &|settings| {
|
||||
settings.vertical_scroll_margin = Some(3.0);
|
||||
settings.sticky_scroll = Some(settings::StickyScrollContent {
|
||||
enabled: Some(true),
|
||||
});
|
||||
});
|
||||
|
||||
let mut cx = EditorTestContext::new(cx).await;
|
||||
cx.set_state(indoc! {"
|
||||
fn demo() {
|
||||
let value_01 = 1;
|
||||
let value_02 = 2;
|
||||
let value_03 = 3;
|
||||
let value_04 = 4;
|
||||
let value_05 = 5;
|
||||
let value_06 = 6;
|
||||
let value_07 = 7;
|
||||
let value_08 = 8;
|
||||
let value_09 = 9;
|
||||
let value_10 = 10;
|
||||
ˇlet value_11 = 11;
|
||||
let value_12 = 12;
|
||||
let value_13 = 13;
|
||||
}
|
||||
"});
|
||||
|
||||
// Use a parsed scope so the test also exercises sticky-header discovery.
|
||||
cx.update_buffer(|buffer, cx| buffer.set_language(Some(rust_lang()), cx));
|
||||
cx.run_until_parked();
|
||||
|
||||
let line_height = cx.update_editor(|editor, window, cx| {
|
||||
editor
|
||||
.style(cx)
|
||||
.text
|
||||
.line_height_in_pixels(window.rem_size())
|
||||
});
|
||||
let window = cx.window;
|
||||
|
||||
for visible_lines in [2., 2.5] {
|
||||
cx.simulate_window_resize(window, size(px(1000.), visible_lines * line_height));
|
||||
|
||||
// Center explicitly so the starting viewport does not depend on the
|
||||
// Fit calculation being tested.
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
editor.change_selections(SelectionEffects::no_scroll(), window, cx, |selections| {
|
||||
selections.select_ranges([Point::new(11, 4)..Point::new(11, 4)]);
|
||||
});
|
||||
editor.scroll_cursor_center(&ScrollCursorCenter, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
let snapshot = editor.snapshot(window, cx);
|
||||
assert_eq!(snapshot.scroll_position().y, 10.);
|
||||
assert_eq!(EditorElement::sticky_headers(editor, &snapshot).len(), 1);
|
||||
|
||||
// Row 10 is covered by the header; moving there must scroll to expose it.
|
||||
editor.move_up(&MoveUp, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
cx.update_editor(|editor, window, cx| {
|
||||
let snapshot = editor.snapshot(window, cx);
|
||||
let cursor_row = editor
|
||||
.selections
|
||||
.newest_display(&snapshot.display_snapshot)
|
||||
.head()
|
||||
.row();
|
||||
let sticky_header_count = EditorElement::sticky_headers(editor, &snapshot).len();
|
||||
|
||||
assert_eq!(cursor_row, DisplayRow(10));
|
||||
assert_eq!(snapshot.scroll_position().y, 9.);
|
||||
assert_eq!(sticky_header_count, 1);
|
||||
|
||||
// A scroll alone is insufficient: the full cursor row must be unobscured.
|
||||
assert!(
|
||||
cursor_row.as_f64() >= snapshot.scroll_position().y + sticky_header_count as f64
|
||||
);
|
||||
assert!(
|
||||
cursor_row.next_row().as_f64()
|
||||
<= snapshot.scroll_position().y + visible_lines as f64
|
||||
);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_autoscroll_keeps_cursor_visible_below_sticky_headers(cx: &mut TestAppContext) {
|
||||
init_test(cx, |_| {});
|
||||
|
||||
@@ -235,9 +235,14 @@ impl Editor {
|
||||
|
||||
let was_autoscrolled = match strategy {
|
||||
AutoscrollStrategy::Fit | AutoscrollStrategy::Newest => {
|
||||
let margin = margin.min(self.scroll_manager.vertical_scroll_margin);
|
||||
let target_top = (target_top - margin - visible_sticky_headers as f64).max(0.0);
|
||||
let target_bottom = target_bottom + margin;
|
||||
let (margin_top, margin_bottom) = self.fit_autoscroll_margins(
|
||||
visible_lines,
|
||||
target_bottom - target_top,
|
||||
visible_sticky_headers,
|
||||
);
|
||||
let target_top = (target_top - margin_top - visible_sticky_headers as f64).max(0.0);
|
||||
let target_bottom = target_bottom + margin_bottom;
|
||||
|
||||
let start_row = scroll_position.y;
|
||||
let end_row = start_row + visible_lines;
|
||||
|
||||
@@ -294,6 +299,28 @@ impl Editor {
|
||||
(NeedsHorizontalAutoscroll(true), was_scrolled)
|
||||
}
|
||||
|
||||
fn fit_autoscroll_margins(
|
||||
&self,
|
||||
visible_lines: ScrollOffset,
|
||||
target_height: ScrollOffset,
|
||||
visible_sticky_headers: usize,
|
||||
) -> (ScrollOffset, ScrollOffset) {
|
||||
if matches!(self.mode, EditorMode::AutoHeight { .. }) {
|
||||
return (0., 0.);
|
||||
}
|
||||
|
||||
let available_margin_space =
|
||||
(visible_lines - target_height - visible_sticky_headers as f64).max(0.);
|
||||
let top_space = (available_margin_space / 2.).floor();
|
||||
|
||||
// Keep the rounding remainder below the target so large margins leave no
|
||||
// slack for the cursor to move without scrolling.
|
||||
(
|
||||
top_space.min(self.scroll_manager.vertical_scroll_margin),
|
||||
(available_margin_space - top_space).min(self.scroll_manager.vertical_scroll_margin),
|
||||
)
|
||||
}
|
||||
|
||||
pub(crate) fn visible_sticky_header_count_for_point(
|
||||
&self,
|
||||
display_map: &DisplaySnapshot,
|
||||
|
||||
Reference in new issue
Block a user