vim: Fix visual block Shift-I jumping to original cursor after Ctrl-D scroll (#50822)

Xiaobo Liu and dino created 3 weeks ago

Ensure that the `vim::visual::Vim::visual_block_motion` method is called
when `scroll_editor` is called while in Visual Block mode. This fixes an
issue where, after getting into `Visual Block` mode, if the user used
`ctrl-d` or `ctrl-u` to scroll half of the page, the wrong selection
would be made and, if `shift-i` was used to start inserting at the start
of the line, a single cursor would be placed at the cursor position
where the selection was started, instead of one cursor per each of the
selected lines.

These changes ensure that we now match Neovim's behavior for the same
flow.

---------

Signed-off-by: Xiaobo Liu <[e-mail]>
Co-authored-by: dino <[e-mail]>

Change summary

crates/vim/src/normal/scroll.rs                                        | 164 
crates/vim/src/visual.rs                                               |  32 
crates/vim/test_data/test_visual_block_insert_after_ctrl_d_scroll.json |  10 
3 files changed, 130 insertions(+), 76 deletions(-)

Detailed changes

crates/vim/src/normal/scroll.rs

@@ -88,82 +88,74 @@ pub fn register(editor: &mut Editor, cx: &mut Context<Vim>) {
 impl Vim {
     fn scroll(
         &mut self,
-        move_cursor: bool,
+        preserve_cursor_position: bool,
         window: &mut Window,
         cx: &mut Context<Self>,
         by: fn(c: Option<f32>) -> ScrollAmount,
     ) {
         let amount = by(Vim::take_count(cx).map(|c| c as f32));
-        let mode = self.mode;
         Vim::take_forced_motion(cx);
         self.exit_temporary_normal(window, cx);
-        self.update_editor(cx, |_, editor, cx| {
-            scroll_editor(editor, mode, move_cursor, amount, window, cx)
-        });
+        self.scroll_editor(preserve_cursor_position, amount, window, cx);
     }
-}
 
-fn scroll_editor(
-    editor: &mut Editor,
-    mode: Mode,
-    preserve_cursor_position: bool,
-    amount: ScrollAmount,
-    window: &mut Window,
-    cx: &mut Context<Editor>,
-) {
-    let should_move_cursor = editor.newest_selection_on_screen(cx).is_eq();
-    let display_snapshot = editor.display_map.update(cx, |map, cx| map.snapshot(cx));
-    let old_top = editor
-        .scroll_manager
-        .scroll_top_display_point(&display_snapshot, cx);
-
-    if editor.scroll_hover(amount, window, cx) {
-        return;
-    }
+    fn scroll_editor(
+        &mut self,
+        preserve_cursor_position: bool,
+        amount: ScrollAmount,
+        window: &mut Window,
+        cx: &mut Context<Vim>,
+    ) {
+        self.update_editor(cx, |vim, editor, cx| {
+            let should_move_cursor = editor.newest_selection_on_screen(cx).is_eq();
+            let display_snapshot = editor.display_map.update(cx, |map, cx| map.snapshot(cx));
+            let old_top = editor
+                .scroll_manager
+                .scroll_top_display_point(&display_snapshot, cx);
+
+            if editor.scroll_hover(amount, window, cx) {
+                return;
+            }
 
-    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) as f32)
-            } else {
-                ScrollAmount::Line((amount.lines(visible_line_count) - 1.0) as f32)
+            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) as f32)
+                    } else {
+                        ScrollAmount::Line((amount.lines(visible_line_count) - 1.0) as f32)
+                    }
+                }
+                _ => amount,
+            };
+
+            editor.scroll_screen(&amount, window, cx);
+            if !should_move_cursor {
+                return;
             }
-        }
-        _ => amount,
-    };
 
-    editor.scroll_screen(&amount, window, cx);
-    if !should_move_cursor {
-        return;
-    }
+            let Some(visible_line_count) = editor.visible_line_count() else {
+                return;
+            };
 
-    let Some(visible_line_count) = editor.visible_line_count() else {
-        return;
-    };
+            let Some(visible_column_count) = editor.visible_column_count() else {
+                return;
+            };
 
-    let Some(visible_column_count) = editor.visible_column_count() else {
-        return;
-    };
+            let display_snapshot = editor.display_map.update(cx, |map, cx| map.snapshot(cx));
+            let top = editor
+                .scroll_manager
+                .scroll_top_display_point(&display_snapshot, cx);
+            let vertical_scroll_margin = EditorSettings::get_global(cx).vertical_scroll_margin;
 
-    let display_snapshot = editor.display_map.update(cx, |map, cx| map.snapshot(cx));
-    let top = editor
-        .scroll_manager
-        .scroll_top_display_point(&display_snapshot, cx);
-    let vertical_scroll_margin = EditorSettings::get_global(cx).vertical_scroll_margin;
-
-    editor.change_selections(
-        SelectionEffects::no_scroll().nav_history(false),
-        window,
-        cx,
-        |s| {
-            s.move_with(&mut |map, selection| {
+            let mut move_cursor = |map: &editor::display_map::DisplaySnapshot,
+                                   mut head: DisplayPoint,
+                                   goal: SelectionGoal| {
                 // TODO: Improve the logic and function calls below to be dependent on
                 // the `amount`. If the amount is vertical, we don't care about
                 // columns, while if it's horizontal, we don't care about rows,
                 // so we don't need to calculate both and deal with logic for
                 // both.
-                let mut head = selection.head();
                 let max_point = map.max_point();
                 let starting_column = head.column();
 
@@ -171,17 +163,18 @@ fn scroll_editor(
                     (vertical_scroll_margin as u32).min(visible_line_count as u32 / 2);
 
                 if preserve_cursor_position {
-                    let new_row = if old_top.row() == top.row() {
-                        DisplayRow(
-                            head.row()
-                                .0
-                                .saturating_add_signed(amount.lines(visible_line_count) as i32),
-                        )
-                    } else {
-                        DisplayRow(top.row().0.saturating_add_signed(
-                            selection.head().row().0 as i32 - old_top.row().0 as i32,
-                        ))
-                    };
+                    let new_row =
+                        if old_top.row() == top.row() {
+                            DisplayRow(
+                                head.row()
+                                    .0
+                                    .saturating_add_signed(amount.lines(visible_line_count) as i32),
+                            )
+                        } else {
+                            DisplayRow(top.row().0.saturating_add_signed(
+                                head.row().0 as i32 - old_top.row().0 as i32,
+                            ))
+                        };
                     head = map.clip_point(DisplayPoint::new(new_row, head.column()), Bias::Left)
                 }
 
@@ -259,17 +252,36 @@ fn scroll_editor(
                 let new_head = map.clip_point(DisplayPoint::new(new_row, new_column), Bias::Left);
                 let goal = match amount {
                     ScrollAmount::Column(_) | ScrollAmount::PageWidth(_) => SelectionGoal::None,
-                    _ => selection.goal,
+                    _ => goal,
                 };
 
-                if selection.is_empty() || !mode.is_visual() {
-                    selection.collapse_to(new_head, goal)
-                } else {
-                    selection.set_head(new_head, goal)
-                };
-            })
-        },
-    );
+                Some((new_head, goal))
+            };
+
+            if vim.mode == Mode::VisualBlock {
+                vim.visual_block_motion(true, editor, window, cx, &mut move_cursor);
+            } else {
+                editor.change_selections(
+                    SelectionEffects::no_scroll().nav_history(false),
+                    window,
+                    cx,
+                    |s| {
+                        s.move_with(&mut |map, selection| {
+                            if let Some((new_head, goal)) =
+                                move_cursor(map, selection.head(), selection.goal)
+                            {
+                                if selection.is_empty() || !vim.mode.is_visual() {
+                                    selection.collapse_to(new_head, goal)
+                                } else {
+                                    selection.set_head(new_head, goal)
+                                }
+                            }
+                        })
+                    },
+                );
+            }
+        });
+    }
 }
 
 #[cfg(test)]

crates/vim/src/visual.rs

@@ -1561,6 +1561,38 @@ mod test {
         });
     }
 
+    #[gpui::test]
+    async fn test_visual_block_insert_after_ctrl_d_scroll(cx: &mut gpui::TestAppContext) {
+        let mut cx = NeovimBackedTestContext::new(cx).await;
+        let shared_state_lines = (1..=10)
+            .map(|line_number| format!("{line_number:02}"))
+            .collect::<Vec<_>>()
+            .join("\n");
+        let shared_state = format!("ˇ{shared_state_lines}\n");
+
+        cx.set_scroll_height(5).await;
+        cx.set_shared_state(&shared_state).await;
+
+        cx.simulate_shared_keystrokes("ctrl-v ctrl-d").await;
+        cx.shared_state().await.assert_matches();
+
+        cx.simulate_shared_keystrokes("shift-i x escape").await;
+        cx.shared_state().await.assert_eq(indoc! {
+            "
+            ˇx01
+            x02
+            x03
+            x04
+            x05
+            06
+            07
+            08
+            09
+            10
+            "
+        });
+    }
+
     #[gpui::test]
     async fn test_visual_block_wrapping_selection(cx: &mut gpui::TestAppContext) {
         let mut cx = NeovimBackedTestContext::new(cx).await;

crates/vim/test_data/test_visual_block_insert_after_ctrl_d_scroll.json

@@ -0,0 +1,10 @@
+{"SetOption":{"value":"scrolloff=3"}}
+{"SetOption":{"value":"lines=7"}}
+{"Put":{"state":"ˇ01\n02\n03\n04\n05\n06\n07\n08\n09\n10\n"}}
+{"Key":"ctrl-v"}
+{"Key":"ctrl-d"}
+{"Get":{"state":"«0ˇ»1\n«0ˇ»2\n«0ˇ»3\n«0ˇ»4\n«0ˇ»5\n06\n07\n08\n09\n10\n","mode":"VisualBlock"}}
+{"Key":"shift-i"}
+{"Key":"x"}
+{"Key":"escape"}
+{"Get":{"state":"ˇx01\nx02\nx03\nx04\nx05\n06\n07\n08\n09\n10\n","mode":"Normal"}}