Fixed issue with enabling and disabling vim mode dynamically

Also added indoc and marked text utility to vim tests to improve readability

Cargo.lock

@@ -2527,6 +2527,15 @@ dependencies = [
  "hashbrown 0.9.1",
 ]
 
+[[package]]
+name = "indoc"
+version = "1.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e7906a9fababaeacb774f72410e497a1d18de916322e33797bb2cd29baa23c9e"
+dependencies = [
+ "unindent",
+]
+
 [[package]]
 name = "infer"
 version = "0.2.3"
@@ -5553,9 +5562,9 @@ checksum = "39ec24b3121d976906ece63c9daad25b85969647682eee313cb5779fdd69e14e"
 
 [[package]]
 name = "unindent"
-version = "0.1.7"
+version = "0.1.8"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f14ee04d9415b52b3aeab06258a3f07093182b88ba0f9b8d203f211a7a7d41c7"
+checksum = "514672a55d7380da379785a4d70ca8386c8883ff7eaae877be4d2081cebe73d8"
 
 [[package]]
 name = "universal-hash"
@@ -5680,9 +5689,11 @@ dependencies = [
  "collections",
  "editor",
  "gpui",
+ "indoc",
  "language",
  "log",
  "project",
+ "util",
  "workspace",
 ]
 

crates/editor/src/display_map.rs

@@ -498,17 +498,14 @@ impl ToDisplayPoint for Anchor {
 #[cfg(test)]
 pub mod tests {
     use super::*;
-    use crate::{
+    use crate::{movement, test::marked_display_snapshot};
-        movement,
-        test::{marked_display_snapshot, marked_text_ranges},
-    };
     use gpui::{color::Color, elements::*, test::observe, MutableAppContext};
     use language::{Buffer, Language, LanguageConfig, RandomCharIter, SelectionGoal};
     use rand::{prelude::*, Rng};
     use smol::stream::StreamExt;
     use std::{env, sync::Arc};
     use theme::SyntaxTheme;
-    use util::test::sample_text;
+    use util::test::{marked_text_ranges, sample_text};
     use Bias::*;
 
     #[gpui::test(iterations = 100)]

crates/editor/src/editor.rs

@@ -6167,7 +6167,6 @@ pub fn styled_runs_for_code_label<'a>(
 
 #[cfg(test)]
 mod tests {
-    use crate::test::marked_text_by;
 
     use super::*;
     use gpui::{
@@ -6181,7 +6180,7 @@ mod tests {
     use std::{cell::RefCell, rc::Rc, time::Instant};
     use text::Point;
     use unindent::Unindent;
-    use util::test::sample_text;
+    use util::test::{marked_text_by, sample_text};
     use workspace::FollowableItem;
 
     #[gpui::test]

crates/editor/src/test.rs

@@ -1,6 +1,4 @@
-use std::ops::Range;
+use util::test::marked_text;
-
-use collections::HashMap;
 
 use crate::{
     display_map::{DisplayMap, DisplaySnapshot, ToDisplayPoint},
@@ -15,51 +13,6 @@ fn init_logger() {
     }
 }
 
-pub fn marked_text_by(
-    marked_text: &str,
-    markers: Vec<char>,
-) -> (String, HashMap<char, Vec<usize>>) {
-    let mut extracted_markers: HashMap<char, Vec<usize>> = Default::default();
-    let mut unmarked_text = String::new();
-
-    for char in marked_text.chars() {
-        if markers.contains(&char) {
-            let char_offsets = extracted_markers.entry(char).or_insert(Vec::new());
-            char_offsets.push(unmarked_text.len());
-        } else {
-            unmarked_text.push(char);
-        }
-    }
-
-    (unmarked_text, extracted_markers)
-}
-
-pub fn marked_text(marked_text: &str) -> (String, Vec<usize>) {
-    let (unmarked_text, mut markers) = marked_text_by(marked_text, vec!['|']);
-    (unmarked_text, markers.remove(&'|').unwrap_or_else(Vec::new))
-}
-
-pub fn marked_text_ranges(
-    marked_text: &str,
-    range_markers: Vec<(char, char)>,
-) -> (String, Vec<Range<usize>>) {
-    let mut marker_chars = Vec::new();
-    for (start, end) in range_markers.iter() {
-        marker_chars.push(*start);
-        marker_chars.push(*end);
-    }
-    let (unmarked_text, markers) = marked_text_by(marked_text, marker_chars);
-    let ranges = range_markers
-        .iter()
-        .map(|(start_marker, end_marker)| {
-            let start = markers.get(start_marker).unwrap()[0];
-            let end = markers.get(end_marker).unwrap()[0];
-            start..end
-        })
-        .collect();
-    (unmarked_text, ranges)
-}
-
 // Returns a snapshot from text containing '|' character markers with the markers removed, and DisplayPoints for each one.
 pub fn marked_display_snapshot(
     text: &str,

crates/util/src/test.rs

@@ -1,4 +1,8 @@
-use std::path::{Path, PathBuf};
+use std::{
+    collections::HashMap,
+    ops::Range,
+    path::{Path, PathBuf},
+};
 use tempdir::TempDir;
 
 pub fn temp_tree(tree: serde_json::Value) -> TempDir {
@@ -48,3 +52,48 @@ pub fn sample_text(rows: usize, cols: usize, start_char: char) -> String {
     }
     text
 }
+
+pub fn marked_text_by(
+    marked_text: &str,
+    markers: Vec<char>,
+) -> (String, HashMap<char, Vec<usize>>) {
+    let mut extracted_markers: HashMap<char, Vec<usize>> = Default::default();
+    let mut unmarked_text = String::new();
+
+    for char in marked_text.chars() {
+        if markers.contains(&char) {
+            let char_offsets = extracted_markers.entry(char).or_insert(Vec::new());
+            char_offsets.push(unmarked_text.len());
+        } else {
+            unmarked_text.push(char);
+        }
+    }
+
+    (unmarked_text, extracted_markers)
+}
+
+pub fn marked_text(marked_text: &str) -> (String, Vec<usize>) {
+    let (unmarked_text, mut markers) = marked_text_by(marked_text, vec!['|']);
+    (unmarked_text, markers.remove(&'|').unwrap_or_else(Vec::new))
+}
+
+pub fn marked_text_ranges(
+    marked_text: &str,
+    range_markers: Vec<(char, char)>,
+) -> (String, Vec<Range<usize>>) {
+    let mut marker_chars = Vec::new();
+    for (start, end) in range_markers.iter() {
+        marker_chars.push(*start);
+        marker_chars.push(*end);
+    }
+    let (unmarked_text, markers) = marked_text_by(marked_text, marker_chars);
+    let ranges = range_markers
+        .iter()
+        .map(|(start_marker, end_marker)| {
+            let start = markers.get(start_marker).unwrap()[0];
+            let end = markers.get(end_marker).unwrap()[0];
+            start..end
+        })
+        .collect();
+    (unmarked_text, ranges)
+}

crates/vim/Cargo.toml

@@ -16,8 +16,10 @@ workspace = { path = "../workspace" }
 log = "0.4"
 
 [dev-dependencies]
+indoc = "1.0.4"
 editor = { path = "../editor", features = ["test-support"] }
 gpui = { path = "../gpui", features = ["test-support"] }
 project = { path = "../project", features = ["test-support"] }
 language = { path = "../language", features = ["test-support"] }
+util = { path = "../util", features = ["test-support"] }
 workspace = { path = "../workspace", features = ["test-support"] }

crates/vim/src/editor_events.rs

@@ -13,9 +13,7 @@ pub fn init(cx: &mut MutableAppContext) {
 fn editor_created(EditorCreated(editor): &EditorCreated, cx: &mut MutableAppContext) {
     cx.update_default_global(|vim_state: &mut VimState, cx| {
         vim_state.editors.insert(editor.id(), editor.downgrade());
-        if vim_state.enabled {
+        VimState::sync_editor_options(cx);
-            VimState::update_cursor_shapes(cx);
-        }
     })
 }
 
@@ -40,9 +38,7 @@ fn editor_blurred(EditorBlurred(editor): &EditorBlurred, cx: &mut MutableAppCont
             }
         }
     });
-    editor.update(cx, |editor, _| {
+    VimState::sync_editor_options(cx);
-        editor.remove_keymap_context_layer::<VimState>();
-    })
 }
 
 fn editor_released(EditorReleased(editor): &EditorReleased, cx: &mut MutableAppContext) {

crates/vim/src/editor_utils.rs

@@ -1,97 +0,0 @@
-use editor::{display_map::DisplaySnapshot, Bias, DisplayPoint, Editor};
-use gpui::ViewContext;
-use language::{Selection, SelectionGoal};
-
-pub trait VimEditorExt {
-    fn clip_selections(self: &mut Self, cx: &mut ViewContext<Self>);
-    fn clipped_move_selections(
-        self: &mut Self,
-        cx: &mut ViewContext<Self>,
-        move_selection: impl Fn(&DisplaySnapshot, &mut Selection<DisplayPoint>),
-    );
-    fn clipped_move_selection_heads(
-        &mut self,
-        cx: &mut ViewContext<Self>,
-        update_head: impl Fn(
-            &DisplaySnapshot,
-            DisplayPoint,
-            SelectionGoal,
-        ) -> (DisplayPoint, SelectionGoal),
-    );
-    fn clipped_move_cursors(
-        self: &mut Self,
-        cx: &mut ViewContext<Self>,
-        update_cursor_position: impl Fn(
-            &DisplaySnapshot,
-            DisplayPoint,
-            SelectionGoal,
-        ) -> (DisplayPoint, SelectionGoal),
-    );
-}
-
-pub fn clip_display_point(map: &DisplaySnapshot, mut display_point: DisplayPoint) -> DisplayPoint {
-    let next_char = map.chars_at(display_point).next();
-    if next_char == Some('\n') || next_char == None {
-        *display_point.column_mut() = display_point.column().saturating_sub(1);
-        display_point = map.clip_point(display_point, Bias::Left);
-    }
-    display_point
-}
-
-impl VimEditorExt for Editor {
-    fn clip_selections(self: &mut Self, cx: &mut ViewContext<Self>) {
-        self.move_selections(cx, |map, selection| {
-            if selection.is_empty() {
-                let adjusted_cursor = clip_display_point(map, selection.start);
-                selection.collapse_to(adjusted_cursor, selection.goal);
-            } else {
-                let adjusted_head = clip_display_point(map, selection.head());
-                selection.set_head(adjusted_head, selection.goal);
-            }
-        })
-    }
-
-    fn clipped_move_selections(
-        self: &mut Self,
-        cx: &mut ViewContext<Self>,
-        move_selection: impl Fn(&DisplaySnapshot, &mut Selection<DisplayPoint>),
-    ) {
-        self.move_selections(cx, |map, selection| {
-            move_selection(map, selection);
-            let adjusted_head = clip_display_point(map, selection.head());
-            selection.set_head(adjusted_head, selection.goal);
-        })
-    }
-
-    fn clipped_move_selection_heads(
-        &mut self,
-        cx: &mut ViewContext<Self>,
-        update_head: impl Fn(
-            &DisplaySnapshot,
-            DisplayPoint,
-            SelectionGoal,
-        ) -> (DisplayPoint, SelectionGoal),
-    ) {
-        self.clipped_move_selections(cx, |map, selection| {
-            let (new_head, new_goal) = update_head(map, selection.head(), selection.goal);
-            let adjusted_head = clip_display_point(map, new_head);
-            selection.set_head(adjusted_head, new_goal);
-        });
-    }
-
-    fn clipped_move_cursors(
-        self: &mut Self,
-        cx: &mut ViewContext<Self>,
-        update_cursor_position: impl Fn(
-            &DisplaySnapshot,
-            DisplayPoint,
-            SelectionGoal,
-        ) -> (DisplayPoint, SelectionGoal),
-    ) {
-        self.move_selections(cx, |map, selection| {
-            let (cursor, new_goal) = update_cursor_position(map, selection.head(), selection.goal);
-            let adjusted_cursor = clip_display_point(map, cursor);
-            selection.collapse_to(adjusted_cursor, new_goal);
-        });
-    }
-}

crates/vim/src/insert.rs

@@ -3,7 +3,7 @@ use gpui::{action, keymap::Binding, MutableAppContext, ViewContext};
 use language::SelectionGoal;
 use workspace::Workspace;
 
-use crate::{editor_utils::VimEditorExt, mode::Mode, SwitchMode, VimState};
+use crate::{mode::Mode, SwitchMode, VimState};
 
 action!(NormalBefore);
 
@@ -18,11 +18,11 @@ pub fn init(cx: &mut MutableAppContext) {
 }
 
 fn normal_before(_: &mut Workspace, _: &NormalBefore, cx: &mut ViewContext<Workspace>) {
-    VimState::switch_mode(&SwitchMode(Mode::Normal), cx);
     VimState::update_active_editor(cx, |editor, cx| {
-        editor.clipped_move_cursors(cx, |map, mut cursor, _| {
+        editor.move_cursors(cx, |map, mut cursor, _| {
             *cursor.column_mut() = cursor.column().saturating_sub(1);
             (map.clip_point(cursor, Bias::Left), SelectionGoal::None)
         });
     });
+    VimState::switch_mode(&SwitchMode(Mode::Normal), cx);
 }

crates/vim/src/vim.rs

@@ -1,5 +1,4 @@
 mod editor_events;
-mod editor_utils;
 mod insert;
 mod mode;
 mod normal;
@@ -8,7 +7,6 @@ mod vim_tests;
 
 use collections::HashMap;
 use editor::{CursorShape, Editor};
-use editor_utils::VimEditorExt;
 use gpui::{action, MutableAppContext, ViewContext, WeakViewHandle};
 
 use mode::Mode;
@@ -49,21 +47,11 @@ impl VimState {
     }
 
     fn switch_mode(SwitchMode(mode): &SwitchMode, cx: &mut MutableAppContext) {
-        let active_editor = cx.update_default_global(|this: &mut Self, _| {
+        cx.update_default_global(|this: &mut Self, _| {
             this.mode = *mode;
-            this.active_editor.clone()
         });
 
-        if let Some(active_editor) = active_editor.and_then(|e| e.upgrade(cx)) {
+        VimState::sync_editor_options(cx);
-            active_editor.update(cx, |active_editor, cx| {
-                active_editor.set_keymap_context_layer::<Self>(mode.keymap_context_layer());
-                active_editor.set_input_enabled(*mode == Mode::Insert);
-                if *mode != Mode::Insert {
-                    active_editor.clip_selections(cx);
-                }
-            });
-        }
-        VimState::update_cursor_shapes(cx);
     }
 
     fn settings_changed(cx: &mut MutableAppContext) {
@@ -76,20 +64,29 @@ impl VimState {
                 } else {
                     Mode::Insert
                 };
-                Self::update_cursor_shapes(cx);
+                Self::sync_editor_options(cx);
             }
         });
     }
 
-    fn update_cursor_shapes(cx: &mut MutableAppContext) {
+    fn sync_editor_options(cx: &mut MutableAppContext) {
         cx.defer(move |cx| {
             cx.update_default_global(|this: &mut VimState, cx| {
-                let cursor_shape = this.mode.cursor_shape();
+                let mode = this.mode;
+                let cursor_shape = mode.cursor_shape();
+                let keymap_layer_active = this.enabled;
                 for editor in this.editors.values() {
                     if let Some(editor) = editor.upgrade(cx) {
                         editor.update(cx, |editor, cx| {
                             editor.set_cursor_shape(cursor_shape, cx);
                             editor.set_clip_at_line_ends(cursor_shape == CursorShape::Block, cx);
+                            editor.set_input_enabled(mode == Mode::Insert);
+                            if keymap_layer_active {
+                                let context_layer = mode.keymap_context_layer();
+                                editor.set_keymap_context_layer::<Self>(context_layer);
+                            } else {
+                                editor.remove_keymap_context_layer::<Self>();
+                            }
                         });
                     }
                 }

crates/vim/src/vim_tests.rs

@@ -1,8 +1,10 @@
+use indoc::indoc;
 use std::ops::Deref;
 
 use editor::{display_map::ToDisplayPoint, DisplayPoint};
 use gpui::{json::json, keymap::Keystroke, ViewHandle};
 use language::{Point, Selection};
+use util::test::marked_text;
 use workspace::{WorkspaceHandle, WorkspaceParams};
 
 use crate::*;
@@ -10,46 +12,98 @@ use crate::*;
 #[gpui::test]
 async fn test_insert_mode(cx: &mut gpui::TestAppContext) {
     let mut cx = VimTestAppContext::new(cx, "").await;
-    assert_eq!(cx.mode(), Mode::Normal);
     cx.simulate_keystroke("i");
     assert_eq!(cx.mode(), Mode::Insert);
     cx.simulate_keystrokes(&["T", "e", "s", "t"]);
-    assert_eq!(cx.editor_text(), "Test".to_owned());
+    cx.assert_newest_selection_head("Test|");
     cx.simulate_keystroke("escape");
     assert_eq!(cx.mode(), Mode::Normal);
+    cx.assert_newest_selection_head("Tes|t");
 }
 
 #[gpui::test]
 async fn test_normal_hjkl(cx: &mut gpui::TestAppContext) {
     let mut cx = VimTestAppContext::new(cx, "Test\nTestTest\nTest").await;
-    assert_eq!(cx.mode(), Mode::Normal);
     cx.simulate_keystroke("l");
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(0, 1));
+    cx.assert_newest_selection_head(indoc! {"
+        T|est
+        TestTest
+        Test"});
     cx.simulate_keystroke("h");
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(0, 0));
+    cx.assert_newest_selection_head(indoc! {"
+        |Test
+        TestTest
+        Test"});
     cx.simulate_keystroke("j");
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(1, 0));
+    cx.assert_newest_selection_head(indoc! {"
+        Test
+        |TestTest
+        Test"});
     cx.simulate_keystroke("k");
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(0, 0));
+    cx.assert_newest_selection_head(indoc! {"
-
+        |Test
+        TestTest
+        Test"});
     cx.simulate_keystroke("j");
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(1, 0));
+    cx.assert_newest_selection_head(indoc! {"
+        Test
+        |TestTest
+        Test"});
 
     // When moving left, cursor does not wrap to the previous line
     cx.simulate_keystroke("h");
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(1, 0));
+    cx.assert_newest_selection_head(indoc! {"
+        Test
+        |TestTest
+        Test"});
 
     // When moving right, cursor does not reach the line end or wrap to the next line
     for _ in 0..9 {
         cx.simulate_keystroke("l");
     }
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(1, 7));
+    cx.assert_newest_selection_head(indoc! {"
+        Test
+        TestTes|t
+        Test"});
 
     // Goal column respects the inability to reach the end of the line
     cx.simulate_keystroke("k");
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(0, 3));
+    cx.assert_newest_selection_head(indoc! {"
+        Tes|t
+        TestTest
+        Test"});
     cx.simulate_keystroke("j");
-    assert_eq!(cx.newest_selection().head(), DisplayPoint::new(1, 7));
+    cx.assert_newest_selection_head(indoc! {"
+        Test
+        TestTes|t
+        Test"});
+}
+
+#[gpui::test]
+async fn test_toggle_through_settings(cx: &mut gpui::TestAppContext) {
+    let mut cx = VimTestAppContext::new(cx, "").await;
+
+    // Editor acts as though vim is disabled
+    cx.disable_vim();
+    assert_eq!(cx.mode(), Mode::Insert);
+    cx.simulate_keystrokes(&["h", "j", "k", "l"]);
+    cx.assert_newest_selection_head("hjkl|");
+
+    // Enabling dynamically sets vim mode again
+    cx.enable_vim();
+    assert_eq!(cx.mode(), Mode::Normal);
+    cx.simulate_keystrokes(&["h", "h", "h", "l"]);
+    assert_eq!(cx.editor_text(), "hjkl".to_owned());
+    cx.assert_newest_selection_head("hj|kl");
+    cx.simulate_keystrokes(&["i", "T", "e", "s", "t"]);
+    cx.assert_newest_selection_head("hjTest|kl");
+
+    // Disabling and enabling resets to normal mode
+    assert_eq!(cx.mode(), Mode::Insert);
+    cx.disable_vim();
+    assert_eq!(cx.mode(), Mode::Insert);
+    cx.enable_vim();
+    assert_eq!(cx.mode(), Mode::Normal);
 }
 
 struct VimTestAppContext<'a> {
@@ -68,6 +122,13 @@ impl<'a> VimTestAppContext<'a> {
             crate::init(cx);
         });
         let params = cx.update(WorkspaceParams::test);
+
+        cx.update(|cx| {
+            cx.update_global(|settings: &mut Settings, _| {
+                settings.vim_mode = true;
+            });
+        });
+
         params
             .fs
             .as_fake()
@@ -107,6 +168,22 @@ impl<'a> VimTestAppContext<'a> {
         }
     }
 
+    fn enable_vim(&mut self) {
+        self.cx.update(|cx| {
+            cx.update_global(|settings: &mut Settings, _| {
+                settings.vim_mode = true;
+            });
+        })
+    }
+
+    fn disable_vim(&mut self) {
+        self.cx.update(|cx| {
+            cx.update_global(|settings: &mut Settings, _| {
+                settings.vim_mode = false;
+            });
+        })
+    }
+
     fn newest_selection(&mut self) -> Selection<DisplayPoint> {
         self.editor.update(self.cx, |editor, cx| {
             let snapshot = editor.snapshot(cx);
@@ -141,6 +218,20 @@ impl<'a> VimTestAppContext<'a> {
             self.simulate_keystroke(keystroke_text);
         }
     }
+
+    fn assert_newest_selection_head(&mut self, text: &str) {
+        let (unmarked_text, markers) = marked_text(&text);
+        assert_eq!(
+            self.editor_text(),
+            unmarked_text,
+            "Unmarked text doesn't match editor text"
+        );
+        let newest_selection = self.newest_selection();
+        let expected_head = self.editor.update(self.cx, |editor, cx| {
+            markers[0].to_display_point(&editor.snapshot(cx))
+        });
+        assert_eq!(newest_selection.head(), expected_head)
+    }
 }
 
 impl<'a> Deref for VimTestAppContext<'a> {