Fix vim editor focus selection issues, cancel vim operators on escape and unbound keys

Keith Simmons created

Change summary

Cargo.lock                         |   1 
assets/keymaps/vim.json            |  22 ++----
crates/search/src/buffer_search.rs |   2 
crates/vim/Cargo.toml              |   1 
crates/vim/src/editor_events.rs    |  13 +++
crates/vim/src/normal.rs           |   2 
crates/vim/src/normal/delete.rs    |  40 ++++++++++++
crates/vim/src/state.rs            |  27 +++++---
crates/vim/src/vim.rs              | 104 ++++++++++++++++++++++++++++----
crates/vim/src/vim_test_context.rs |  26 +++++++
crates/zed/src/zed.rs              |   1 
styles/package-lock.json           |   1 
12 files changed, 198 insertions(+), 42 deletions(-)

Detailed changes

Cargo.lock 🔗

@@ -5768,6 +5768,7 @@ dependencies = [
  "language",
  "log",
  "project",
+ "search",
  "serde",
  "settings",
  "util",

assets/keymaps/vim.json 🔗

@@ -37,16 +37,12 @@
                     "ignorePunctuation": true
                 }
             ],
-            "escape": [
-                "vim::SwitchMode",
-                "Normal"
-            ]
+            "escape": "editor::Cancel"
         }
     },
     {
-        "context": "Editor && vim_mode == normal",
+        "context": "Editor && vim_mode == normal && vim_operator == none",
         "bindings": {
-            "escape": "editor::Cancel",
             "c": [
                 "vim::PushOperator",
                 "Change"
@@ -92,7 +88,13 @@
             "p": "vim::Paste",
             "u": "editor::Undo",
             "ctrl-r": "editor::Redo",
-            "ctrl-o": "pane::GoBack"
+            "ctrl-o": "pane::GoBack",
+            "/": [
+                "buffer_search::Deploy",
+                {
+                    "focus": true
+                }
+            ]
         }
     },
     {
@@ -146,11 +148,5 @@
             "escape": "vim::NormalBefore",
             "ctrl-c": "vim::NormalBefore"
         }
-    },
-    {
-        "context": "Editor && mode == singleline",
-        "bindings": {
-            "escape": "editor::Cancel"
-        }
     }
 ]

crates/search/src/buffer_search.rs 🔗

@@ -58,7 +58,7 @@ fn add_toggle_option_action<A: Action>(option: SearchOption, cx: &mut MutableApp
 }
 
 pub struct BufferSearchBar {
-    query_editor: ViewHandle<Editor>,
+    pub query_editor: ViewHandle<Editor>,
     active_editor: Option<ViewHandle<Editor>>,
     active_match_index: Option<usize>,
     active_editor_subscription: Option<Subscription>,

crates/vim/Cargo.toml 🔗

@@ -14,6 +14,7 @@ command_palette = { path = "../command_palette" }
 editor = { path = "../editor" }
 gpui = { path = "../gpui" }
 language = { path = "../language" }
+search = { path = "../search" }
 serde = { version = "1.0", features = ["derive", "rc"] }
 settings = { path = "../settings" }
 workspace = { path = "../workspace" }

crates/vim/src/editor_events.rs 🔗

@@ -29,8 +29,17 @@ fn editor_focused(EditorFocused(editor): &EditorFocused, cx: &mut MutableAppCont
             }
         }));
 
-        if editor.read(cx).mode() != EditorMode::Full {
-            vim.switch_mode(Mode::Insert, cx);
+        if !vim.enabled {
+            return;
+        }
+
+        let editor = editor.read(cx);
+        if editor.selections.newest::<usize>(cx).is_empty() {
+            if editor.mode() != EditorMode::Full {
+                vim.switch_mode(Mode::Insert, cx);
+            }
+        } else {
+            vim.switch_mode(Mode::Visual { line: false }, cx);
         }
     });
 }

crates/vim/src/normal.rs 🔗

@@ -1165,7 +1165,7 @@ mod test {
                 The quick brown
                 fox [jump}s over
                 the lazy dog"},
-            Mode::Normal,
+            Mode::Visual { line: false },
         );
         cx.simulate_keystroke("y");
         cx.set_state(

crates/vim/src/normal/delete.rs 🔗

@@ -40,7 +40,7 @@ pub fn delete_over(vim: &mut Vim, motion: Motion, cx: &mut MutableAppContext) {
 mod test {
     use indoc::indoc;
 
-    use crate::vim_test_context::VimTestContext;
+    use crate::{state::Mode, vim_test_context::VimTestContext};
 
     #[gpui::test]
     async fn test_delete_h(cx: &mut gpui::TestAppContext) {
@@ -390,4 +390,42 @@ mod test {
                 the lazy"},
         );
     }
+
+    #[gpui::test]
+    async fn test_cancel_delete_operator(cx: &mut gpui::TestAppContext) {
+        let mut cx = VimTestContext::new(cx, true).await;
+        cx.set_state(
+            indoc! {"
+                The quick brown
+                fox ju|mps over
+                the lazy dog"},
+            Mode::Normal,
+        );
+
+        // Canceling operator twice reverts to normal mode with no active operator
+        cx.simulate_keystrokes(["d", "escape", "k"]);
+        assert_eq!(cx.active_operator(), None);
+        assert_eq!(cx.mode(), Mode::Normal);
+        cx.assert_editor_state(indoc! {"
+            The qu|ick brown
+            fox jumps over
+            the lazy dog"});
+    }
+
+    #[gpui::test]
+    async fn test_unbound_command_cancels_pending_operator(cx: &mut gpui::TestAppContext) {
+        let mut cx = VimTestContext::new(cx, true).await;
+        cx.set_state(
+            indoc! {"
+                The quick brown
+                fox ju|mps over
+                the lazy dog"},
+            Mode::Normal,
+        );
+
+        // Canceling operator twice reverts to normal mode with no active operator
+        cx.simulate_keystrokes(["d", "y"]);
+        assert_eq!(cx.active_operator(), None);
+        assert_eq!(cx.mode(), Mode::Normal);
+    }
 }

crates/vim/src/state.rs 🔗

@@ -37,7 +37,14 @@ pub struct VimState {
 impl VimState {
     pub fn cursor_shape(&self) -> CursorShape {
         match self.mode {
-            Mode::Normal | Mode::Visual { .. } => CursorShape::Block,
+            Mode::Normal => {
+                if self.operator_stack.is_empty() {
+                    CursorShape::Block
+                } else {
+                    CursorShape::Underscore
+                }
+            }
+            Mode::Visual { .. } => CursorShape::Block,
             Mode::Insert => CursorShape::Bar,
         }
     }
@@ -73,20 +80,20 @@ impl VimState {
             context.set.insert("VimControl".to_string());
         }
 
-        if let Some(operator) = &self.operator_stack.last() {
-            operator.set_context(&mut context);
-        }
+        Operator::set_context(self.operator_stack.last(), &mut context);
+
         context
     }
 }
 
 impl Operator {
-    pub fn set_context(&self, context: &mut Context) {
-        let operator_context = match self {
-            Operator::Namespace(Namespace::G) => "g",
-            Operator::Change => "c",
-            Operator::Delete => "d",
-            Operator::Yank => "y",
+    pub fn set_context(operator: Option<&Operator>, context: &mut Context) {
+        let operator_context = match operator {
+            Some(Operator::Namespace(Namespace::G)) => "g",
+            Some(Operator::Change) => "c",
+            Some(Operator::Delete) => "d",
+            Some(Operator::Yank) => "y",
+            None => "none",
         }
         .to_owned();
 

crates/vim/src/vim.rs 🔗

@@ -11,7 +11,7 @@ mod visual;
 
 use collections::HashMap;
 use command_palette::CommandPaletteFilter;
-use editor::{Bias, CursorShape, Editor, Input};
+use editor::{Bias, Cancel, CursorShape, Editor, Input};
 use gpui::{impl_actions, MutableAppContext, Subscription, ViewContext, WeakViewHandle};
 use serde::Deserialize;
 
@@ -34,6 +34,7 @@ pub fn init(cx: &mut MutableAppContext) {
     insert::init(cx);
     motion::init(cx);
 
+    // Vim Actions
     cx.add_action(|_: &mut Workspace, &SwitchMode(mode): &SwitchMode, cx| {
         Vim::update(cx, |vim, cx| vim.switch_mode(mode, cx))
     });
@@ -42,7 +43,11 @@ pub fn init(cx: &mut MutableAppContext) {
             Vim::update(cx, |vim, cx| vim.push_operator(operator, cx))
         },
     );
+
+    // Editor Actions
     cx.add_action(|_: &mut Editor, _: &Input, cx| {
+        // If we have an unbound input with an active operator, cancel that operator. Otherwise forward
+        // the input to the editor
         if Vim::read(cx).active_operator().is_some() {
             // Defer without updating editor
             MutableAppContext::defer(cx, |cx| Vim::update(cx, |vim, cx| vim.clear_operator(cx)))
@@ -50,6 +55,20 @@ pub fn init(cx: &mut MutableAppContext) {
             cx.propagate_action()
         }
     });
+    cx.add_action(|_: &mut Editor, _: &Cancel, cx| {
+        // If we are in a non normal mode or have an active operator, swap to normal mode
+        // Otherwise forward cancel on to the editor
+        let vim = Vim::read(cx);
+        if vim.state.mode != Mode::Normal || vim.active_operator().is_some() {
+            MutableAppContext::defer(cx, |cx| {
+                Vim::update(cx, |state, cx| {
+                    state.switch_mode(Mode::Normal, cx);
+                });
+            });
+        } else {
+            cx.propagate_action();
+        }
+    });
 
     // Sync initial settings with the rest of the app
     Vim::update(cx, |state, cx| state.sync_vim_settings(cx));
@@ -97,9 +116,46 @@ impl Vim {
     }
 
     fn switch_mode(&mut self, mode: Mode, cx: &mut MutableAppContext) {
+        let previous_mode = self.state.mode;
         self.state.mode = mode;
         self.state.operator_stack.clear();
+
+        // Sync editor settings like clip mode
         self.sync_vim_settings(cx);
+
+        // Adjust selections
+        for editor in self.editors.values() {
+            if let Some(editor) = editor.upgrade(cx) {
+                editor.update(cx, |editor, cx| {
+                    editor.change_selections(None, cx, |s| {
+                        s.move_with(|map, selection| {
+                            // If empty selections
+                            if self.state.empty_selections_only() {
+                                let new_head = map.clip_point(selection.head(), Bias::Left);
+                                selection.collapse_to(new_head, selection.goal)
+                            } else {
+                                if matches!(mode, Mode::Visual { line: false })
+                                    && !matches!(previous_mode, Mode::Visual { .. })
+                                    && !selection.reversed
+                                    && !selection.is_empty()
+                                {
+                                    // Mode wasn't visual mode before, but is now. We need to move the end
+                                    // back by one character so that the region to be modifed stays the same
+                                    *selection.end.column_mut() =
+                                        selection.end.column().saturating_sub(1);
+                                    selection.end = map.clip_point(selection.end, Bias::Left);
+                                }
+
+                                selection.set_head(
+                                    map.clip_point(selection.head(), Bias::Left),
+                                    selection.goal,
+                                );
+                            }
+                        });
+                    })
+                })
+            }
+        }
     }
 
     fn push_operator(&mut self, operator: Operator, cx: &mut MutableAppContext) {
@@ -127,7 +183,7 @@ impl Vim {
             self.enabled = enabled;
             self.state = Default::default();
             if enabled {
-                self.state.mode = Mode::Normal;
+                self.switch_mode(Mode::Normal, cx);
             }
             self.sync_vim_settings(cx);
         }
@@ -156,17 +212,6 @@ impl Vim {
                             matches!(state.mode, Mode::Visual { line: true });
                         let context_layer = state.keymap_context_layer();
                         editor.set_keymap_context_layer::<Self>(context_layer);
-                        editor.change_selections(None, cx, |s| {
-                            s.move_with(|map, selection| {
-                                selection.set_head(
-                                    map.clip_point(selection.head(), Bias::Left),
-                                    selection.goal,
-                                );
-                                if state.empty_selections_only() {
-                                    selection.collapse_to(selection.head(), selection.goal)
-                                }
-                            });
-                        })
                     } else {
                         editor.set_cursor_shape(CursorShape::Bar, cx);
                         editor.set_clip_at_line_ends(false, cx);
@@ -182,6 +227,9 @@ impl Vim {
 
 #[cfg(test)]
 mod test {
+    use indoc::indoc;
+    use search::BufferSearchBar;
+
     use crate::{state::Mode, vim_test_context::VimTestContext};
 
     #[gpui::test]
@@ -226,4 +274,34 @@ mod test {
         cx.enable_vim();
         assert_eq!(cx.mode(), Mode::Normal);
     }
+
+    #[gpui::test]
+    async fn test_buffer_search_switches_mode(cx: &mut gpui::TestAppContext) {
+        let mut cx = VimTestContext::new(cx, true).await;
+
+        cx.set_state(
+            indoc! {"
+            The quick brown
+            fox ju|mps over
+            the lazy dog"},
+            Mode::Normal,
+        );
+        cx.simulate_keystroke("/");
+
+        assert_eq!(cx.mode(), Mode::Visual { line: false });
+
+        let search_bar = cx.workspace(|workspace, cx| {
+            workspace
+                .active_pane()
+                .read(cx)
+                .toolbar()
+                .read(cx)
+                .item_of_type::<BufferSearchBar>()
+                .expect("Buffer search bar should be deployed")
+        });
+
+        search_bar.read_with(cx.cx, |bar, cx| {
+            assert_eq!(bar.query_editor.read(cx).text(cx), "jumps");
+        })
+    }
 }

crates/vim/src/vim_test_context.rs 🔗

@@ -1,14 +1,16 @@
 use std::ops::{Deref, DerefMut};
 
 use editor::test::EditorTestContext;
-use gpui::json::json;
+use gpui::{json::json, AppContext, ViewHandle};
 use project::Project;
+use search::{BufferSearchBar, ProjectSearchBar};
 use workspace::{pane, AppState, WorkspaceHandle};
 
 use crate::{state::Operator, *};
 
 pub struct VimTestContext<'a> {
     cx: EditorTestContext<'a>,
+    workspace: ViewHandle<Workspace>,
 }
 
 impl<'a> VimTestContext<'a> {
@@ -16,6 +18,7 @@ impl<'a> VimTestContext<'a> {
         cx.update(|cx| {
             editor::init(cx);
             pane::init(cx);
+            search::init(cx);
             crate::init(cx);
 
             settings::KeymapFileContent::load("keymaps/vim.json", cx).unwrap();
@@ -37,6 +40,19 @@ impl<'a> VimTestContext<'a> {
             .await;
 
         let (window_id, workspace) = cx.add_window(|cx| Workspace::new(project.clone(), cx));
+
+        // Setup search toolbars
+        workspace.update(cx, |workspace, cx| {
+            workspace.active_pane().update(cx, |pane, cx| {
+                pane.toolbar().update(cx, |toolbar, cx| {
+                    let buffer_search_bar = cx.add_view(|cx| BufferSearchBar::new(cx));
+                    toolbar.add_item(buffer_search_bar, cx);
+                    let project_search_bar = cx.add_view(|_| ProjectSearchBar::new());
+                    toolbar.add_item(project_search_bar, cx);
+                })
+            });
+        });
+
         project
             .update(cx, |project, cx| {
                 project.find_or_create_local_worktree("/root", true, cx)
@@ -64,9 +80,17 @@ impl<'a> VimTestContext<'a> {
                 window_id,
                 editor,
             },
+            workspace,
         }
     }
 
+    pub fn workspace<F, T>(&mut self, read: F) -> T
+    where
+        F: FnOnce(&Workspace, &AppContext) -> T,
+    {
+        self.workspace.read_with(self.cx.cx, read)
+    }
+
     pub fn enable_vim(&mut self) {
         self.cx.update(|cx| {
             cx.update_global(|settings: &mut Settings, _| {

crates/zed/src/zed.rs 🔗

@@ -97,6 +97,7 @@ pub fn init(app_state: &Arc<AppState>, cx: &mut gpui::MutableAppContext) {
     cx.add_action({
         let app_state = app_state.clone();
         move |_: &mut Workspace, _: &OpenSettings, cx: &mut ViewContext<Workspace>| {
+            println!("open settings");
             open_config_file(&SETTINGS_PATH, app_state.clone(), cx);
         }
     });

styles/package-lock.json 🔗

@@ -5,6 +5,7 @@
     "requires": true,
     "packages": {
         "": {
+            "name": "styles",
             "version": "1.0.0",
             "license": "ISC",
             "dependencies": {