vim: Fix scrolling (#2647)

After #2641 we noticed that scrolling didn't take a count parameter, and
a few other issues with the way that we calculated the distance to
scroll.
 
Release Notes:

- Improved distance calculations for page-up/page-down
- vim: Allow counts to work with scrolling shortcuts.

assets/keymaps/vim.json

@@ -58,10 +58,6 @@
         }
       ],
       "%": "vim::Matching",
-      "ctrl-y": [
-        "vim::Scroll",
-        "LineUp"
-      ],
       "f": [
         "vim::PushOperator",
         {
@@ -197,26 +193,14 @@
           "focus": true
         }
       ],
-      "ctrl-f": [
+      "ctrl-f": "vim::PageDown",
-        "vim::Scroll",
+      "pagedown": "vim::PageDown",
-        "PageDown"
+      "ctrl-b": "vim::PageUp",
-      ],
+      "pageup": "vim::PageUp",
-      "ctrl-b": [
+      "ctrl-d": "vim::ScrollDown",
-        "vim::Scroll",
+      "ctrl-u": "vim::ScrollUp",
-        "PageUp"
+      "ctrl-e": "vim::LineDown",
-      ],
+      "ctrl-y": "vim::LineUp",
-      "ctrl-d": [
-        "vim::Scroll",
-        "HalfPageDown"
-      ],
-      "ctrl-u": [
-        "vim::Scroll",
-        "HalfPageUp"
-      ],
-      "ctrl-e": [
-        "vim::Scroll",
-        "LineDown"
-      ],
       "r": [
         "vim::PushOperator",
         "Replace"

crates/editor/src/editor_tests.rs

@@ -1,5 +1,6 @@
 use super::*;
 use crate::{
+    scroll::scroll_amount::ScrollAmount,
     test::{
         assert_text_with_selections, build_editor, editor_lsp_test_context::EditorLspTestContext,
         editor_test_context::EditorTestContext, select_ranges,
@@ -1359,6 +1360,43 @@ async fn test_move_start_of_paragraph_end_of_paragraph(cx: &mut gpui::TestAppCon
     );
 }
 
+#[gpui::test]
+async fn test_scroll_page_up_page_down(cx: &mut gpui::TestAppContext) {
+    init_test(cx, |_| {});
+    let mut cx = EditorTestContext::new(cx).await;
+    let line_height = cx.editor(|editor, cx| editor.style(cx).text.line_height(cx.font_cache()));
+    cx.simulate_window_resize(cx.window_id, vec2f(1000., 4. * line_height + 0.5));
+
+    cx.set_state(
+        &r#"ˇone
+        two
+        three
+        four
+        five
+        six
+        seven
+        eight
+        nine
+        ten
+        "#,
+    );
+
+    cx.update_editor(|editor, cx| {
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 0.));
+        editor.scroll_screen(&ScrollAmount::Page(1.), cx);
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 3.));
+        editor.scroll_screen(&ScrollAmount::Page(1.), cx);
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 6.));
+        editor.scroll_screen(&ScrollAmount::Page(-1.), cx);
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 3.));
+
+        editor.scroll_screen(&ScrollAmount::Page(-0.5), cx);
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 2.));
+        editor.scroll_screen(&ScrollAmount::Page(0.5), cx);
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 3.));
+    });
+}
+
 #[gpui::test]
 async fn test_move_page_up_page_down(cx: &mut gpui::TestAppContext) {
     init_test(cx, |_| {});

crates/editor/src/scroll.rs

@@ -368,7 +368,7 @@ impl Editor {
         }
 
         let cur_position = self.scroll_position(cx);
-        let new_pos = cur_position + vec2f(0., amount.lines(self) - 1.);
+        let new_pos = cur_position + vec2f(0., amount.lines(self));
         self.set_scroll_position(new_pos, cx);
     }
 

crates/editor/src/scroll/actions.rs

@@ -27,22 +27,22 @@ pub fn init(cx: &mut AppContext) {
     cx.add_action(Editor::scroll_cursor_center);
     cx.add_action(Editor::scroll_cursor_bottom);
     cx.add_action(|this: &mut Editor, _: &LineDown, cx| {
-        this.scroll_screen(&ScrollAmount::LineDown, cx)
+        this.scroll_screen(&ScrollAmount::Line(1.), cx)
     });
     cx.add_action(|this: &mut Editor, _: &LineUp, cx| {
-        this.scroll_screen(&ScrollAmount::LineUp, cx)
+        this.scroll_screen(&ScrollAmount::Line(-1.), cx)
     });
     cx.add_action(|this: &mut Editor, _: &HalfPageDown, cx| {
-        this.scroll_screen(&ScrollAmount::HalfPageDown, cx)
+        this.scroll_screen(&ScrollAmount::Page(0.5), cx)
     });
     cx.add_action(|this: &mut Editor, _: &HalfPageUp, cx| {
-        this.scroll_screen(&ScrollAmount::HalfPageUp, cx)
+        this.scroll_screen(&ScrollAmount::Page(-0.5), cx)
     });
     cx.add_action(|this: &mut Editor, _: &PageDown, cx| {
-        this.scroll_screen(&ScrollAmount::PageDown, cx)
+        this.scroll_screen(&ScrollAmount::Page(1.), cx)
     });
     cx.add_action(|this: &mut Editor, _: &PageUp, cx| {
-        this.scroll_screen(&ScrollAmount::PageUp, cx)
+        this.scroll_screen(&ScrollAmount::Page(-1.), cx)
     });
 }
 

crates/editor/src/scroll/scroll_amount.rs

@@ -6,12 +6,10 @@ use crate::Editor;
 
 #[derive(Clone, PartialEq, Deserialize)]
 pub enum ScrollAmount {
-    LineUp,
+    // Scroll N lines (positive is towards the end of the document)
-    LineDown,
+    Line(f32),
-    HalfPageUp,
+    // Scroll N pages (positive is towards the end of the document)
-    HalfPageDown,
+    Page(f32),
-    PageUp,
-    PageDown,
 }
 
 impl ScrollAmount {
@@ -24,10 +22,10 @@ impl ScrollAmount {
             let context_menu = editor.context_menu.as_mut()?;
 
             match self {
-                Self::LineDown | Self::HalfPageDown => context_menu.select_next(cx),
+                Self::Line(c) if *c > 0. => context_menu.select_next(cx),
-                Self::LineUp | Self::HalfPageUp => context_menu.select_prev(cx),
+                Self::Line(_) => context_menu.select_prev(cx),
-                Self::PageDown => context_menu.select_last(cx),
+                Self::Page(c) if *c > 0. => context_menu.select_last(cx),
-                Self::PageUp => context_menu.select_first(cx),
+                Self::Page(_) => context_menu.select_first(cx),
             }
             .then_some(())
         })
@@ -36,13 +34,13 @@ impl ScrollAmount {
 
     pub fn lines(&self, editor: &mut Editor) -> f32 {
         match self {
-            Self::LineDown => 1.,
+            Self::Line(count) => *count,
-            Self::LineUp => -1.,
+            Self::Page(count) => editor
-            Self::HalfPageDown => editor.visible_line_count().map(|l| l / 2.).unwrap_or(1.),
+                .visible_line_count()
-            Self::HalfPageUp => -editor.visible_line_count().map(|l| l / 2.).unwrap_or(1.),
+                // subtract one to leave an anchor line
-            // Minus 1. here so that there is a pivot line that stays on the screen
+                // round towards zero (so page-up and page-down are symmetric)
-            Self::PageDown => editor.visible_line_count().unwrap_or(1.) - 1.,
+                .map(|l| ((l - 1.) * count).trunc())
-            Self::PageUp => -editor.visible_line_count().unwrap_or(1.) - 1.,
+                .unwrap_or(0.),
         }
     }
 }

crates/vim/src/normal.rs

@@ -1,9 +1,10 @@
 mod change;
 mod delete;
+mod scroll;
 mod substitute;
 mod yank;
 
-use std::{borrow::Cow, cmp::Ordering, sync::Arc};
+use std::{borrow::Cow, sync::Arc};
 
 use crate::{
     motion::Motion,
@@ -13,14 +14,12 @@ use crate::{
 };
 use collections::{HashMap, HashSet};
 use editor::{
-    display_map::ToDisplayPoint,
+    display_map::ToDisplayPoint, scroll::autoscroll::Autoscroll, Anchor, Bias, ClipboardSelection,
-    scroll::{autoscroll::Autoscroll, scroll_amount::ScrollAmount},
+    DisplayPoint,
-    Anchor, Bias, ClipboardSelection, DisplayPoint, Editor,
 };
-use gpui::{actions, impl_actions, AppContext, ViewContext, WindowContext};
+use gpui::{actions, AppContext, ViewContext, WindowContext};
 use language::{AutoindentMode, Point, SelectionGoal};
 use log::error;
-use serde::Deserialize;
 use workspace::Workspace;
 
 use self::{
@@ -30,9 +29,6 @@ use self::{
     yank::{yank_motion, yank_object},
 };
 
-#[derive(Clone, PartialEq, Deserialize)]
-struct Scroll(ScrollAmount);
-
 actions!(
     vim,
     [
@@ -51,8 +47,6 @@ actions!(
     ]
 );
 
-impl_actions!(vim, [Scroll]);
-
 pub fn init(cx: &mut AppContext) {
     cx.add_action(insert_after);
     cx.add_action(insert_first_non_whitespace);
@@ -90,13 +84,8 @@ pub fn init(cx: &mut AppContext) {
         })
     });
     cx.add_action(paste);
-    cx.add_action(|_: &mut Workspace, Scroll(amount): &Scroll, cx| {
+
-        Vim::update(cx, |vim, cx| {
+    scroll::init(cx);
-            vim.update_active_editor(cx, |editor, cx| {
-                scroll(editor, amount, cx);
-            })
-        })
-    });
 }
 
 pub fn normal_motion(
@@ -393,46 +382,6 @@ fn paste(_: &mut Workspace, _: &Paste, cx: &mut ViewContext<Workspace>) {
     });
 }
 
-fn scroll(editor: &mut Editor, amount: &ScrollAmount, cx: &mut ViewContext<Editor>) {
-    let should_move_cursor = editor.newest_selection_on_screen(cx).is_eq();
-    editor.scroll_screen(amount, cx);
-    if should_move_cursor {
-        let selection_ordering = editor.newest_selection_on_screen(cx);
-        if selection_ordering.is_eq() {
-            return;
-        }
-
-        let visible_rows = if let Some(visible_rows) = editor.visible_line_count() {
-            visible_rows as u32
-        } else {
-            return;
-        };
-
-        let scroll_margin_rows = editor.vertical_scroll_margin() as u32;
-        let top_anchor = editor.scroll_manager.anchor().anchor;
-
-        editor.change_selections(None, cx, |s| {
-            s.replace_cursors_with(|snapshot| {
-                let mut new_point = top_anchor.to_display_point(&snapshot);
-
-                match selection_ordering {
-                    Ordering::Less => {
-                        *new_point.row_mut() += scroll_margin_rows;
-                        new_point = snapshot.clip_point(new_point, Bias::Right);
-                    }
-                    Ordering::Greater => {
-                        *new_point.row_mut() += visible_rows - scroll_margin_rows as u32;
-                        new_point = snapshot.clip_point(new_point, Bias::Left);
-                    }
-                    Ordering::Equal => unreachable!(),
-                }
-
-                vec![new_point]
-            })
-        });
-    }
-}
-
 pub(crate) fn normal_replace(text: Arc<str>, cx: &mut WindowContext) {
     Vim::update(cx, |vim, cx| {
         vim.update_active_editor(cx, |editor, cx| {

crates/vim/src/normal/scroll.rs

@@ -0,0 +1,120 @@
+use std::cmp::Ordering;
+
+use crate::Vim;
+use editor::{display_map::ToDisplayPoint, scroll::scroll_amount::ScrollAmount, Editor};
+use gpui::{actions, AppContext, ViewContext};
+use language::Bias;
+use workspace::Workspace;
+
+actions!(
+    vim,
+    [LineUp, LineDown, ScrollUp, ScrollDown, PageUp, PageDown,]
+);
+
+pub fn init(cx: &mut AppContext) {
+    cx.add_action(|_: &mut Workspace, _: &LineDown, cx| {
+        scroll(cx, |c| ScrollAmount::Line(c.unwrap_or(1.)))
+    });
+    cx.add_action(|_: &mut Workspace, _: &LineUp, cx| {
+        scroll(cx, |c| ScrollAmount::Line(-c.unwrap_or(1.)))
+    });
+    cx.add_action(|_: &mut Workspace, _: &PageDown, cx| {
+        scroll(cx, |c| ScrollAmount::Page(c.unwrap_or(1.)))
+    });
+    cx.add_action(|_: &mut Workspace, _: &PageUp, cx| {
+        scroll(cx, |c| ScrollAmount::Page(-c.unwrap_or(1.)))
+    });
+    cx.add_action(|_: &mut Workspace, _: &ScrollDown, cx| {
+        scroll(cx, |c| {
+            if let Some(c) = c {
+                ScrollAmount::Line(c)
+            } else {
+                ScrollAmount::Page(0.5)
+            }
+        })
+    });
+    cx.add_action(|_: &mut Workspace, _: &ScrollUp, cx| {
+        scroll(cx, |c| {
+            if let Some(c) = c {
+                ScrollAmount::Line(-c)
+            } else {
+                ScrollAmount::Page(-0.5)
+            }
+        })
+    });
+}
+
+fn scroll(cx: &mut ViewContext<Workspace>, by: fn(c: Option<f32>) -> ScrollAmount) {
+    Vim::update(cx, |vim, cx| {
+        let amount = by(vim.pop_number_operator(cx).map(|c| c as f32));
+        vim.update_active_editor(cx, |editor, cx| scroll_editor(editor, &amount, cx));
+    })
+}
+
+fn scroll_editor(editor: &mut Editor, amount: &ScrollAmount, cx: &mut ViewContext<Editor>) {
+    let should_move_cursor = editor.newest_selection_on_screen(cx).is_eq();
+    editor.scroll_screen(amount, cx);
+    if should_move_cursor {
+        let selection_ordering = editor.newest_selection_on_screen(cx);
+        if selection_ordering.is_eq() {
+            return;
+        }
+
+        let visible_rows = if let Some(visible_rows) = editor.visible_line_count() {
+            visible_rows as u32
+        } else {
+            return;
+        };
+
+        let top_anchor = editor.scroll_manager.anchor().anchor;
+
+        editor.change_selections(None, cx, |s| {
+            s.replace_cursors_with(|snapshot| {
+                let mut new_point = top_anchor.to_display_point(&snapshot);
+
+                match selection_ordering {
+                    Ordering::Less => {
+                        new_point = snapshot.clip_point(new_point, Bias::Right);
+                    }
+                    Ordering::Greater => {
+                        *new_point.row_mut() += visible_rows - 1;
+                        new_point = snapshot.clip_point(new_point, Bias::Left);
+                    }
+                    Ordering::Equal => unreachable!(),
+                }
+
+                vec![new_point]
+            })
+        });
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use crate::{state::Mode, test::VimTestContext};
+    use gpui::geometry::vector::vec2f;
+    use indoc::indoc;
+
+    #[gpui::test]
+    async fn test_scroll(cx: &mut gpui::TestAppContext) {
+        let mut cx = VimTestContext::new(cx, true).await;
+
+        cx.set_state(indoc! {"ˇa\nb\nc\nd\ne\n"}, Mode::Normal);
+
+        cx.update_editor(|editor, cx| {
+            assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 0.))
+        });
+        cx.simulate_keystrokes(["ctrl-e"]);
+        cx.update_editor(|editor, cx| {
+            assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 1.))
+        });
+        cx.simulate_keystrokes(["2", "ctrl-e"]);
+        cx.update_editor(|editor, cx| {
+            assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 3.))
+        });
+        cx.simulate_keystrokes(["ctrl-y"]);
+        cx.update_editor(|editor, cx| {
+            assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 2.))
+        });
+    }
+}