Add clip_to_line_end to display_map/snapshot and set it to ensure vim positioning in normal mode

Keith Simmons and Nathan Sobo created

Co-authored-by: Nathan Sobo <nathan@zed.dev>

Change summary

crates/editor/src/display_map.rs | 110 +++++++++++++++++++++------------
crates/editor/src/editor.rs      |   5 +
crates/editor/src/element.rs     |   2 
crates/editor/src/movement.rs    |  43 ++----------
crates/editor/src/test.rs        |  32 +++++++++
crates/vim/src/editor_utils.rs   |  33 ++++-----
crates/vim/src/insert.rs         |   2 
crates/vim/src/normal.rs         |  10 +-
crates/vim/src/vim.rs            |   5 
crates/vim/src/vim_tests.rs      |   2 
10 files changed, 141 insertions(+), 103 deletions(-)

Detailed changes

crates/editor/src/display_map.rs 🔗

@@ -36,6 +36,7 @@ pub struct DisplayMap {
     wrap_map: ModelHandle<WrapMap>,
     block_map: BlockMap,
     text_highlights: TextHighlights,
+    pub clip_at_line_ends: bool,
 }
 
 impl Entity for DisplayMap {
@@ -67,6 +68,7 @@ impl DisplayMap {
             wrap_map,
             block_map,
             text_highlights: Default::default(),
+            clip_at_line_ends: false,
         }
     }
 
@@ -87,6 +89,7 @@ impl DisplayMap {
             wraps_snapshot,
             blocks_snapshot,
             text_highlights: self.text_highlights.clone(),
+            clip_at_line_ends: self.clip_at_line_ends,
         }
     }
 
@@ -204,6 +207,7 @@ pub struct DisplaySnapshot {
     wraps_snapshot: wrap_map::WrapSnapshot,
     blocks_snapshot: block_map::BlockSnapshot,
     text_highlights: TextHighlights,
+    clip_at_line_ends: bool,
 }
 
 impl DisplaySnapshot {
@@ -331,7 +335,12 @@ impl DisplaySnapshot {
     }
 
     pub fn clip_point(&self, point: DisplayPoint, bias: Bias) -> DisplayPoint {
-        DisplayPoint(self.blocks_snapshot.clip_point(point.0, bias))
+        let mut clipped = self.blocks_snapshot.clip_point(point.0, bias);
+        if self.clip_at_line_ends && clipped.column == self.line_len(clipped.row) {
+            clipped.column = clipped.column.saturating_sub(1);
+            clipped = self.blocks_snapshot.clip_point(clipped, Bias::Left);
+        }
+        DisplayPoint(clipped)
     }
 
     pub fn folds_in_range<'a, T>(
@@ -487,11 +496,11 @@ impl ToDisplayPoint for Anchor {
 }
 
 #[cfg(test)]
-mod tests {
+pub mod tests {
     use super::*;
     use crate::{
         movement,
-        test::{marked_text_ranges},
+        test::{marked_display_snapshot, marked_text_ranges},
     };
     use gpui::{color::Color, elements::*, test::observe, MutableAppContext};
     use language::{Buffer, Language, LanguageConfig, RandomCharIter, SelectionGoal};
@@ -1132,49 +1141,70 @@ mod tests {
 
     #[gpui::test]
     fn test_clip_point(cx: &mut gpui::MutableAppContext) {
-        use Bias::{Left, Right};
+        fn assert(text: &str, shift_right: bool, bias: Bias, cx: &mut gpui::MutableAppContext) {
+            let (unmarked_snapshot, mut markers) = marked_display_snapshot(text, cx);
 
-        let text = "\n'a', 'α',\t'✋',\t'❎', '🍐'\n";
-        let display_text = "\n'a', 'α',   '✋',    '❎', '🍐'\n";
-        let buffer = MultiBuffer::build_simple(text, cx);
+            match bias {
+                Bias::Left => {
+                    if shift_right {
+                        *markers[1].column_mut() += 1;
+                    }
 
-        let tab_size = 4;
-        let font_cache = cx.font_cache();
-        let family_id = font_cache.load_family(&["Helvetica"]).unwrap();
-        let font_id = font_cache
-            .select_font(family_id, &Default::default())
-            .unwrap();
-        let font_size = 14.0;
-        let map = cx.add_model(|cx| {
-            DisplayMap::new(buffer.clone(), tab_size, font_id, font_size, None, 1, 1, cx)
-        });
-        let map = map.update(cx, |map, cx| map.snapshot(cx));
+                    assert_eq!(unmarked_snapshot.clip_point(markers[1], bias), markers[0])
+                }
+                Bias::Right => {
+                    if shift_right {
+                        *markers[0].column_mut() += 1;
+                    }
 
-        assert_eq!(map.text(), display_text);
-        for (input_column, bias, output_column) in vec![
-            ("'a', '".len(), Left, "'a', '".len()),
-            ("'a', '".len() + 1, Left, "'a', '".len()),
-            ("'a', '".len() + 1, Right, "'a', 'α".len()),
-            ("'a', 'α', ".len(), Left, "'a', 'α',".len()),
-            ("'a', 'α', ".len(), Right, "'a', 'α',   ".len()),
-            ("'a', 'α',   '".len() + 1, Left, "'a', 'α',   '".len()),
-            ("'a', 'α',   '".len() + 1, Right, "'a', 'α',   '✋".len()),
-            ("'a', 'α',   '✋',".len(), Right, "'a', 'α',   '✋',".len()),
-            ("'a', 'α',   '✋', ".len(), Left, "'a', 'α',   '✋',".len()),
-            (
-                "'a', 'α',   '✋', ".len(),
-                Right,
-                "'a', 'α',   '✋',    ".len(),
-            ),
-        ] {
+                    assert_eq!(
+                        unmarked_snapshot.clip_point(dbg!(markers[0]), bias),
+                        markers[1]
+                    )
+                }
+            };
+        }
+
+        use Bias::{Left, Right};
+        assert("||α", false, Left, cx);
+        assert("||α", true, Left, cx);
+        assert("||α", false, Right, cx);
+        assert("|α|", true, Right, cx);
+        assert("||✋", false, Left, cx);
+        assert("||✋", true, Left, cx);
+        assert("||✋", false, Right, cx);
+        assert("|✋|", true, Right, cx);
+        assert("||🍐", false, Left, cx);
+        assert("||🍐", true, Left, cx);
+        assert("||🍐", false, Right, cx);
+        assert("|🍐|", true, Right, cx);
+        assert("||\t", false, Left, cx);
+        assert("||\t", true, Left, cx);
+        assert("||\t", false, Right, cx);
+        assert("|\t|", true, Right, cx);
+        assert(" ||\t", false, Left, cx);
+        assert(" ||\t", true, Left, cx);
+        assert(" ||\t", false, Right, cx);
+        assert(" |\t|", true, Right, cx);
+        assert("   ||\t", false, Left, cx);
+        assert("   ||\t", false, Right, cx);
+    }
+
+    #[gpui::test]
+    fn test_clip_at_line_ends(cx: &mut gpui::MutableAppContext) {
+        fn assert(text: &str, cx: &mut gpui::MutableAppContext) {
+            let (mut unmarked_snapshot, markers) = marked_display_snapshot(text, cx);
+            unmarked_snapshot.clip_at_line_ends = true;
             assert_eq!(
-                map.clip_point(DisplayPoint::new(1, input_column as u32), bias),
-                DisplayPoint::new(1, output_column as u32),
-                "clip_point(({}, {}))",
-                1,
-                input_column,
+                unmarked_snapshot.clip_point(markers[1], Bias::Left),
+                markers[0]
             );
         }
+
+        assert("||", cx);
+        assert("|a|", cx);
+        assert("a|b|", cx);
+        assert("a|α|", cx);
     }
 
     #[gpui::test]

crates/editor/src/editor.rs 🔗

@@ -1071,6 +1071,11 @@ impl Editor {
         cx.notify();
     }
 
+    pub fn set_clip_at_line_ends(&mut self, clip: bool, cx: &mut ViewContext<Self>) {
+        self.display_map
+            .update(cx, |map, _| map.clip_at_line_ends = clip);
+    }
+
     pub fn set_keymap_context_layer<Tag: 'static>(&mut self, context: gpui::keymap::Context) {
         self.keymap_context_layers
             .insert(TypeId::of::<Tag>(), context);

crates/editor/src/element.rs 🔗

@@ -1292,7 +1292,7 @@ impl PaintState {
     }
 }
 
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, PartialEq, Eq)]
 pub enum CursorShape {
     Bar,
     Block,

crates/editor/src/movement.rs 🔗

@@ -266,13 +266,13 @@ pub fn surrounding_word(map: &DisplaySnapshot, position: DisplayPoint) -> Range<
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::{test::marked_text, Buffer, DisplayMap, MultiBuffer};
+    use crate::{test::marked_display_snapshot, Buffer, DisplayMap, MultiBuffer};
     use language::Point;
 
     #[gpui::test]
     fn test_previous_word_start(cx: &mut gpui::MutableAppContext) {
         fn assert(marked_text: &str, cx: &mut gpui::MutableAppContext) {
-            let (snapshot, display_points) = marked_snapshot(marked_text, cx);
+            let (snapshot, display_points) = marked_display_snapshot(marked_text, cx);
             assert_eq!(
                 previous_word_start(&snapshot, display_points[1]),
                 display_points[0]
@@ -298,7 +298,7 @@ mod tests {
     #[gpui::test]
     fn test_previous_subword_start(cx: &mut gpui::MutableAppContext) {
         fn assert(marked_text: &str, cx: &mut gpui::MutableAppContext) {
-            let (snapshot, display_points) = marked_snapshot(marked_text, cx);
+            let (snapshot, display_points) = marked_display_snapshot(marked_text, cx);
             assert_eq!(
                 previous_subword_start(&snapshot, display_points[1]),
                 display_points[0]
@@ -335,7 +335,7 @@ mod tests {
             cx: &mut gpui::MutableAppContext,
             is_boundary: impl FnMut(char, char) -> bool,
         ) {
-            let (snapshot, display_points) = marked_snapshot(marked_text, cx);
+            let (snapshot, display_points) = marked_display_snapshot(marked_text, cx);
             assert_eq!(
                 find_preceding_boundary(&snapshot, display_points[1], is_boundary),
                 display_points[0]
@@ -362,7 +362,7 @@ mod tests {
     #[gpui::test]
     fn test_next_word_end(cx: &mut gpui::MutableAppContext) {
         fn assert(marked_text: &str, cx: &mut gpui::MutableAppContext) {
-            let (snapshot, display_points) = marked_snapshot(marked_text, cx);
+            let (snapshot, display_points) = marked_display_snapshot(marked_text, cx);
             assert_eq!(
                 next_word_end(&snapshot, display_points[0]),
                 display_points[1]
@@ -385,7 +385,7 @@ mod tests {
     #[gpui::test]
     fn test_next_subword_end(cx: &mut gpui::MutableAppContext) {
         fn assert(marked_text: &str, cx: &mut gpui::MutableAppContext) {
-            let (snapshot, display_points) = marked_snapshot(marked_text, cx);
+            let (snapshot, display_points) = marked_display_snapshot(marked_text, cx);
             assert_eq!(
                 next_subword_end(&snapshot, display_points[0]),
                 display_points[1]
@@ -421,7 +421,7 @@ mod tests {
             cx: &mut gpui::MutableAppContext,
             is_boundary: impl FnMut(char, char) -> bool,
         ) {
-            let (snapshot, display_points) = marked_snapshot(marked_text, cx);
+            let (snapshot, display_points) = marked_display_snapshot(marked_text, cx);
             assert_eq!(
                 find_boundary(&snapshot, display_points[0], is_boundary),
                 display_points[1]
@@ -448,7 +448,7 @@ mod tests {
     #[gpui::test]
     fn test_surrounding_word(cx: &mut gpui::MutableAppContext) {
         fn assert(marked_text: &str, cx: &mut gpui::MutableAppContext) {
-            let (snapshot, display_points) = marked_snapshot(marked_text, cx);
+            let (snapshot, display_points) = marked_display_snapshot(marked_text, cx);
             assert_eq!(
                 surrounding_word(&snapshot, display_points[1]),
                 display_points[0]..display_points[2]
@@ -532,31 +532,4 @@ mod tests {
             (DisplayPoint::new(7, 2), SelectionGoal::Column(2)),
         );
     }
-
-    // Returns a snapshot from text containing '|' character markers with the markers removed, and DisplayPoints for each one.
-    fn marked_snapshot(
-        text: &str,
-        cx: &mut gpui::MutableAppContext,
-    ) -> (DisplaySnapshot, Vec<DisplayPoint>) {
-        let (unmarked_text, markers) = marked_text(text);
-
-        let tab_size = 4;
-        let family_id = cx.font_cache().load_family(&["Helvetica"]).unwrap();
-        let font_id = cx
-            .font_cache()
-            .select_font(family_id, &Default::default())
-            .unwrap();
-        let font_size = 14.0;
-
-        let buffer = MultiBuffer::build_simple(&unmarked_text, cx);
-        let display_map = cx
-            .add_model(|cx| DisplayMap::new(buffer, tab_size, font_id, font_size, None, 1, 1, cx));
-        let snapshot = display_map.update(cx, |map, cx| map.snapshot(cx));
-        let markers = markers
-            .into_iter()
-            .map(|offset| offset.to_display_point(&snapshot))
-            .collect();
-
-        (snapshot, markers)
-    }
 }

crates/editor/src/test.rs 🔗

@@ -2,6 +2,11 @@ use std::ops::Range;
 
 use collections::HashMap;
 
+use crate::{
+    display_map::{DisplayMap, DisplaySnapshot, ToDisplayPoint},
+    DisplayPoint, MultiBuffer,
+};
+
 #[cfg(test)]
 #[ctor::ctor]
 fn init_logger() {
@@ -54,3 +59,30 @@ pub fn marked_text_ranges(
         .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,
+    cx: &mut gpui::MutableAppContext,
+) -> (DisplaySnapshot, Vec<DisplayPoint>) {
+    let (unmarked_text, markers) = marked_text(text);
+
+    let tab_size = 4;
+    let family_id = cx.font_cache().load_family(&["Helvetica"]).unwrap();
+    let font_id = cx
+        .font_cache()
+        .select_font(family_id, &Default::default())
+        .unwrap();
+    let font_size = 14.0;
+
+    let buffer = MultiBuffer::build_simple(&unmarked_text, cx);
+    let display_map =
+        cx.add_model(|cx| DisplayMap::new(buffer, tab_size, font_id, font_size, None, 1, 1, cx));
+    let snapshot = display_map.update(cx, |map, cx| map.snapshot(cx));
+    let markers = markers
+        .into_iter()
+        .map(|offset| offset.to_display_point(&snapshot))
+        .collect();
+
+    (snapshot, markers)
+}

crates/vim/src/editor_utils.rs 🔗

@@ -3,13 +3,13 @@ use gpui::ViewContext;
 use language::{Selection, SelectionGoal};
 
 pub trait VimEditorExt {
-    fn adjust_selections(self: &mut Self, cx: &mut ViewContext<Self>);
-    fn adjusted_move_selections(
+    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 adjusted_move_selection_heads(
+    fn clipped_move_selection_heads(
         &mut self,
         cx: &mut ViewContext<Self>,
         update_head: impl Fn(
@@ -18,7 +18,7 @@ pub trait VimEditorExt {
             SelectionGoal,
         ) -> (DisplayPoint, SelectionGoal),
     );
-    fn adjusted_move_cursors(
+    fn clipped_move_cursors(
         self: &mut Self,
         cx: &mut ViewContext<Self>,
         update_cursor_position: impl Fn(
@@ -29,10 +29,7 @@ pub trait VimEditorExt {
     );
 }
 
-pub fn adjust_display_point(
-    map: &DisplaySnapshot,
-    mut display_point: DisplayPoint,
-) -> DisplayPoint {
+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);
@@ -42,31 +39,31 @@ pub fn adjust_display_point(
 }
 
 impl VimEditorExt for Editor {
-    fn adjust_selections(self: &mut Self, cx: &mut ViewContext<Self>) {
+    fn clip_selections(self: &mut Self, cx: &mut ViewContext<Self>) {
         self.move_selections(cx, |map, selection| {
             if selection.is_empty() {
-                let adjusted_cursor = adjust_display_point(map, selection.start);
+                let adjusted_cursor = clip_display_point(map, selection.start);
                 selection.collapse_to(adjusted_cursor, selection.goal);
             } else {
-                let adjusted_head = adjust_display_point(map, selection.head());
+                let adjusted_head = clip_display_point(map, selection.head());
                 selection.set_head(adjusted_head, selection.goal);
             }
         })
     }
 
-    fn adjusted_move_selections(
+    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 = adjust_display_point(map, selection.head());
+            let adjusted_head = clip_display_point(map, selection.head());
             selection.set_head(adjusted_head, selection.goal);
         })
     }
 
-    fn adjusted_move_selection_heads(
+    fn clipped_move_selection_heads(
         &mut self,
         cx: &mut ViewContext<Self>,
         update_head: impl Fn(
@@ -75,14 +72,14 @@ impl VimEditorExt for Editor {
             SelectionGoal,
         ) -> (DisplayPoint, SelectionGoal),
     ) {
-        self.adjusted_move_selections(cx, |map, selection| {
+        self.clipped_move_selections(cx, |map, selection| {
             let (new_head, new_goal) = update_head(map, selection.head(), selection.goal);
-            let adjusted_head = adjust_display_point(map, new_head);
+            let adjusted_head = clip_display_point(map, new_head);
             selection.set_head(adjusted_head, new_goal);
         });
     }
 
-    fn adjusted_move_cursors(
+    fn clipped_move_cursors(
         self: &mut Self,
         cx: &mut ViewContext<Self>,
         update_cursor_position: impl Fn(
@@ -93,7 +90,7 @@ impl VimEditorExt for Editor {
     ) {
         self.move_selections(cx, |map, selection| {
             let (cursor, new_goal) = update_cursor_position(map, selection.head(), selection.goal);
-            let adjusted_cursor = adjust_display_point(map, cursor);
+            let adjusted_cursor = clip_display_point(map, cursor);
             selection.collapse_to(adjusted_cursor, new_goal);
         });
     }

crates/vim/src/insert.rs 🔗

@@ -20,7 +20,7 @@ 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.adjusted_move_cursors(cx, |map, mut cursor, _| {
+        editor.clipped_move_cursors(cx, |map, mut cursor, _| {
             *cursor.column_mut() = cursor.column().saturating_sub(1);
             (map.clip_point(cursor, Bias::Left), SelectionGoal::None)
         });

crates/vim/src/normal.rs 🔗

@@ -3,7 +3,7 @@ use gpui::{action, keymap::Binding, MutableAppContext, ViewContext};
 use language::SelectionGoal;
 use workspace::Workspace;
 
-use crate::{editor_utils::VimEditorExt, Mode, SwitchMode, VimState};
+use crate::{Mode, SwitchMode, VimState};
 
 action!(InsertBefore);
 action!(MoveLeft);
@@ -29,7 +29,7 @@ pub fn init(cx: &mut MutableAppContext) {
 
 fn move_left(_: &mut Workspace, _: &MoveLeft, cx: &mut ViewContext<Workspace>) {
     VimState::update_active_editor(cx, |editor, cx| {
-        editor.adjusted_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)
         });
@@ -38,19 +38,19 @@ fn move_left(_: &mut Workspace, _: &MoveLeft, cx: &mut ViewContext<Workspace>) {
 
 fn move_down(_: &mut Workspace, _: &MoveDown, cx: &mut ViewContext<Workspace>) {
     VimState::update_active_editor(cx, |editor, cx| {
-        editor.adjusted_move_cursors(cx, movement::down);
+        editor.move_cursors(cx, movement::down);
     });
 }
 
 fn move_up(_: &mut Workspace, _: &MoveUp, cx: &mut ViewContext<Workspace>) {
     VimState::update_active_editor(cx, |editor, cx| {
-        editor.adjusted_move_cursors(cx, movement::up);
+        editor.move_cursors(cx, movement::up);
     });
 }
 
 fn move_right(_: &mut Workspace, _: &MoveRight, cx: &mut ViewContext<Workspace>) {
     VimState::update_active_editor(cx, |editor, cx| {
-        editor.adjusted_move_cursors(cx, |map, mut cursor, _| {
+        editor.move_cursors(cx, |map, mut cursor, _| {
             *cursor.column_mut() += 1;
             (map.clip_point(cursor, Bias::Right), SelectionGoal::None)
         });

crates/vim/src/vim.rs 🔗

@@ -7,7 +7,7 @@ mod normal;
 mod vim_tests;
 
 use collections::HashMap;
-use editor::Editor;
+use editor::{CursorShape, Editor};
 use editor_utils::VimEditorExt;
 use gpui::{action, MutableAppContext, ViewContext, WeakViewHandle};
 
@@ -59,7 +59,7 @@ impl VimState {
                 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.adjust_selections(cx);
+                    active_editor.clip_selections(cx);
                 }
             });
         }
@@ -89,6 +89,7 @@ impl VimState {
                     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);
                         });
                     }
                 }

crates/vim/src/vim_tests.rs 🔗

@@ -1,7 +1,7 @@
 use std::ops::Deref;
 
 use editor::{display_map::ToDisplayPoint, DisplayPoint};
-use gpui::{json::json, keymap::Keystroke, AppContext, ViewHandle};
+use gpui::{json::json, keymap::Keystroke, ViewHandle};
 use language::{Point, Selection};
 use workspace::{WorkspaceHandle, WorkspaceParams};