From e3d54b22118f54d13488707bc3cb9f0e9143b1c8 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Mon, 16 Sep 2024 09:01:20 -0400 Subject: [PATCH] vim: Fix ctrl-b not moving the cursor (#17808) Closes #17687 Release Notes: - Fixed `ctrl-b` not moving the cursor. --------- Co-authored-by: Abdelhakim Qbaich Co-authored-by: Pete LeVasseur --- crates/editor/src/scroll/scroll_amount.rs | 29 ++++++- crates/vim/src/normal/scroll.rs | 92 +++++++++++++++++++++-- crates/vim/test_data/test_ctrl_f_b.json | 24 ++++++ 3 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 crates/vim/test_data/test_ctrl_f_b.json diff --git a/crates/editor/src/scroll/scroll_amount.rs b/crates/editor/src/scroll/scroll_amount.rs index d115be68a0d4d..ee80b3d86f7aa 100644 --- a/crates/editor/src/scroll/scroll_amount.rs +++ b/crates/editor/src/scroll/scroll_amount.rs @@ -1,6 +1,18 @@ use serde::Deserialize; use ui::{px, Pixels}; +#[derive(Debug)] +pub enum ScrollDirection { + Upwards, + Downwards, +} + +impl ScrollDirection { + pub fn is_upwards(&self) -> bool { + matches!(self, ScrollDirection::Upwards) + } +} + #[derive(Debug, Clone, PartialEq, Deserialize)] pub enum ScrollAmount { // Scroll N lines (positive is towards the end of the document) @@ -15,7 +27,7 @@ impl ScrollAmount { Self::Line(count) => *count, Self::Page(count) => { // for full pages subtract one to leave an anchor line - if count.abs() == 1.0 { + if self.is_full_page() { visible_line_count -= 1.0 } (visible_line_count * count).trunc() @@ -29,4 +41,19 @@ impl ScrollAmount { ScrollAmount::Page(x) => px(height.0 * x), } } + + pub fn is_full_page(&self) -> bool { + match self { + ScrollAmount::Page(count) if count.abs() == 1.0 => true, + _ => false, + } + } + + pub fn direction(&self) -> ScrollDirection { + match self { + Self::Line(amount) if amount.is_sign_positive() => ScrollDirection::Downwards, + Self::Page(amount) if amount.is_sign_positive() => ScrollDirection::Downwards, + _ => ScrollDirection::Upwards, + } + } } diff --git a/crates/vim/src/normal/scroll.rs b/crates/vim/src/normal/scroll.rs index f89faa3748372..8d1443e633902 100644 --- a/crates/vim/src/normal/scroll.rs +++ b/crates/vim/src/normal/scroll.rs @@ -73,14 +73,24 @@ fn scroll_editor( return; } - editor.scroll_screen(amount, cx); + let full_page_up = amount.is_full_page() && amount.direction().is_upwards(); + let amount = match (amount.is_full_page(), editor.visible_line_count()) { + (true, Some(visible_line_count)) => { + if amount.direction().is_upwards() { + ScrollAmount::Line(amount.lines(visible_line_count) + 1.0) + } else { + ScrollAmount::Line(amount.lines(visible_line_count) - 1.0) + } + } + _ => amount.clone(), + }; + + editor.scroll_screen(&amount, cx); if !should_move_cursor { return; } - let visible_line_count = if let Some(visible_line_count) = editor.visible_line_count() { - visible_line_count - } else { + let Some(visible_line_count) = editor.visible_line_count() else { return; }; @@ -115,11 +125,18 @@ fn scroll_editor( } else { DisplayRow(top.row().0 + vertical_scroll_margin) }; - let max_row = DisplayRow(map.max_point().row().0.max(top.row().0.saturating_add( - (visible_line_count as u32).saturating_sub(1 + vertical_scroll_margin), - ))); - let new_row = if head.row() < min_row { + let max_visible_row = top.row().0.saturating_add( + (visible_line_count as u32).saturating_sub(1 + vertical_scroll_margin), + ); + let max_row = DisplayRow(map.max_point().row().0.max(max_visible_row)); + + 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 @@ -251,6 +268,7 @@ mod test { ) }); } + #[gpui::test] async fn test_ctrl_d_u(cx: &mut gpui::TestAppContext) { let mut cx = NeovimBackedTestContext::new(cx).await; @@ -282,6 +300,64 @@ mod test { cx.shared_state().await.assert_matches(); } + #[gpui::test] + async fn test_ctrl_f_b(cx: &mut gpui::TestAppContext) { + let mut cx = NeovimBackedTestContext::new(cx).await; + + let visible_lines = 10; + cx.set_scroll_height(visible_lines).await; + + // First test without vertical scroll margin + cx.neovim.set_option(&format!("scrolloff={}", 0)).await; + cx.update_global(|store: &mut SettingsStore, cx| { + store.update_user_settings::(cx, |s| { + s.vertical_scroll_margin = Some(0.0) + }); + }); + + let content = "ˇ".to_owned() + &sample_text(26, 2, 'a'); + cx.set_shared_state(&content).await; + + // scroll down: ctrl-f + cx.simulate_shared_keystrokes("ctrl-f").await; + cx.shared_state().await.assert_matches(); + + cx.simulate_shared_keystrokes("ctrl-f").await; + cx.shared_state().await.assert_matches(); + + // scroll up: ctrl-b + cx.simulate_shared_keystrokes("ctrl-b").await; + cx.shared_state().await.assert_matches(); + + cx.simulate_shared_keystrokes("ctrl-b").await; + cx.shared_state().await.assert_matches(); + + // Now go back to start of file, and test with vertical scroll margin + cx.simulate_shared_keystrokes("g g").await; + cx.shared_state().await.assert_matches(); + + cx.neovim.set_option(&format!("scrolloff={}", 3)).await; + cx.update_global(|store: &mut SettingsStore, cx| { + store.update_user_settings::(cx, |s| { + s.vertical_scroll_margin = Some(3.0) + }); + }); + + // scroll down: ctrl-f + cx.simulate_shared_keystrokes("ctrl-f").await; + cx.shared_state().await.assert_matches(); + + cx.simulate_shared_keystrokes("ctrl-f").await; + cx.shared_state().await.assert_matches(); + + // scroll up: ctrl-b + cx.simulate_shared_keystrokes("ctrl-b").await; + cx.shared_state().await.assert_matches(); + + cx.simulate_shared_keystrokes("ctrl-b").await; + cx.shared_state().await.assert_matches(); + } + #[gpui::test] async fn test_scroll_beyond_last_line(cx: &mut gpui::TestAppContext) { let mut cx = NeovimBackedTestContext::new(cx).await; diff --git a/crates/vim/test_data/test_ctrl_f_b.json b/crates/vim/test_data/test_ctrl_f_b.json new file mode 100644 index 0000000000000..19c94d8b6e94b --- /dev/null +++ b/crates/vim/test_data/test_ctrl_f_b.json @@ -0,0 +1,24 @@ +{"SetOption":{"value":"scrolloff=3"}} +{"SetOption":{"value":"lines=12"}} +{"SetOption":{"value":"scrolloff=0"}} +{"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"}} +{"Key":"ctrl-f"} +{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\ngg\nhh\nˇii\njj\nkk\nll\nmm\nnn\noo\npp\nqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz","mode":"Normal"}} +{"Key":"ctrl-f"} +{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\njj\nkk\nll\nmm\nnn\noo\npp\nˇqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz","mode":"Normal"}} +{"Key":"ctrl-b"} +{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\njj\nkk\nll\nmm\nnn\noo\npp\nqq\nˇrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz","mode":"Normal"}} +{"Key":"ctrl-b"} +{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\nˇjj\nkk\nll\nmm\nnn\noo\npp\nqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz","mode":"Normal"}} +{"Key":"g"} +{"Key":"g"} +{"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","mode":"Normal"}} +{"SetOption":{"value":"scrolloff=3"}} +{"Key":"ctrl-f"} +{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\njj\nkk\nˇll\nmm\nnn\noo\npp\nqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz","mode":"Normal"}} +{"Key":"ctrl-f"} +{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\njj\nkk\nll\nmm\nnn\noo\npp\nqq\nrr\nss\nˇtt\nuu\nvv\nww\nxx\nyy\nzz","mode":"Normal"}} +{"Key":"ctrl-b"} +{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\ngg\nhh\nii\njj\nkk\nll\nmm\nnn\nˇoo\npp\nqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz","mode":"Normal"}} +{"Key":"ctrl-b"} +{"Get":{"state":"aa\nbb\ncc\ndd\nee\nff\nˇgg\nhh\nii\njj\nkk\nll\nmm\nnn\noo\npp\nqq\nrr\nss\ntt\nuu\nvv\nww\nxx\nyy\nzz","mode":"Normal"}}