editor: Refactor cursor_offset_on_selection field in favor of VimModeSettings (#44889)

Dino created

In a previous Pull Request, a new field was added to `editor::Editor`,
namely `cursor_offset_on_selection`, in order to control whether the
cursor representing the head of a selection should be positioned in the
last selected character, as we have on Vim mode, or after, like we have
when Vim mode is disabled.

This field would then be set by the `vim` crate, depending on the
current vim mode. However, it was noted that
`vim_mode_setting::VimModeSetting` already exsits and allows other
crates to determine whether Vim mode is enabled or not. Since we're
already checking `!range.is_empty()` in
`editor::element::SelectionLayout::new` we can then rely on simply
determining whether Vim mode is enabled to decide whether tho shift the
cursor one position to the left when making a selection.

As such, this commit removes the `cursor_offset_on_selection` field, as
well as any related methods in favor of a new `Editor.vim_mode_enabled`
method, which can be used to achieve the same behavior.

Relates to #42837 

Release Notes:

- N/A

Change summary

crates/editor/src/editor.rs  | 31 +++++++++++++------------------
crates/editor/src/element.rs | 27 ++++++++++++++++-----------
crates/vim/src/vim.rs        |  1 -
3 files changed, 29 insertions(+), 30 deletions(-)

Detailed changes

crates/editor/src/editor.rs 🔗

@@ -202,6 +202,7 @@ use ui::{
     IconSize, Indicator, Key, Tooltip, h_flex, prelude::*, scrollbars::ScrollbarAutoHide,
 };
 use util::{RangeExt, ResultExt, TryFutureExt, maybe, post_inc};
+use vim_mode_setting::VimModeSetting;
 use workspace::{
     CollaboratorId, Item as WorkspaceItem, ItemId, ItemNavHistory, OpenInTerminal, OpenTerminal,
     RestoreOnStartupBehavior, SERIALIZATION_THROTTLE_TIME, SplitDirection, TabBarSettings, Toast,
@@ -1110,9 +1111,6 @@ pub struct Editor {
     pending_rename: Option<RenameState>,
     searchable: bool,
     cursor_shape: CursorShape,
-    /// Whether the cursor is offset one character to the left when something is
-    /// selected (needed for vim visual mode)
-    cursor_offset_on_selection: bool,
     current_line_highlight: Option<CurrentLineHighlight>,
     pub collapse_matches: bool,
     autoindent_mode: Option<AutoindentMode>,
@@ -2288,7 +2286,6 @@ impl Editor {
             cursor_shape: EditorSettings::get_global(cx)
                 .cursor_shape
                 .unwrap_or_default(),
-            cursor_offset_on_selection: false,
             current_line_highlight: None,
             autoindent_mode: Some(AutoindentMode::EachLine),
             collapse_matches: false,
@@ -2475,10 +2472,7 @@ impl Editor {
                     }
                 }
                 EditorEvent::Edited { .. } => {
-                    let vim_mode = vim_mode_setting::VimModeSetting::try_get(cx)
-                        .map(|vim_mode| vim_mode.0)
-                        .unwrap_or(false);
-                    if !vim_mode {
+                    if !editor.is_vim_mode_enabled(cx) {
                         let display_map = editor.display_snapshot(cx);
                         let selections = editor.selections.all_adjusted_display(&display_map);
                         let pop_state = editor
@@ -3107,10 +3101,6 @@ impl Editor {
         self.cursor_shape
     }
 
-    pub fn set_cursor_offset_on_selection(&mut self, set_cursor_offset_on_selection: bool) {
-        self.cursor_offset_on_selection = set_cursor_offset_on_selection;
-    }
-
     pub fn set_current_line_highlight(
         &mut self,
         current_line_highlight: Option<CurrentLineHighlight>,
@@ -22607,10 +22597,7 @@ impl Editor {
             .and_then(|e| e.to_str())
             .map(|a| a.to_string()));
 
-        let vim_mode = vim_mode_setting::VimModeSetting::try_get(cx)
-            .map(|vim_mode| vim_mode.0)
-            .unwrap_or(false);
-
+        let vim_mode_enabled = self.is_vim_mode_enabled(cx);
         let edit_predictions_provider = all_language_settings(file, cx).edit_predictions.provider;
         let copilot_enabled = edit_predictions_provider
             == language::language_settings::EditPredictionProvider::Copilot;
@@ -22628,7 +22615,7 @@ impl Editor {
                 event_type,
                 type = if auto_saved {"autosave"} else {"manual"},
                 file_extension,
-                vim_mode,
+                vim_mode_enabled,
                 copilot_enabled,
                 copilot_enabled_for_language,
                 edit_predictions_provider,
@@ -22638,7 +22625,7 @@ impl Editor {
             telemetry::event!(
                 event_type,
                 file_extension,
-                vim_mode,
+                vim_mode_enabled,
                 copilot_enabled,
                 copilot_enabled_for_language,
                 edit_predictions_provider,
@@ -23253,6 +23240,14 @@ impl Editor {
             show_underlines: self.diagnostics_enabled(),
         }
     }
+
+    /// Returns the value of the `vim_mode` setting, defaulting `false` if the
+    /// setting is not set.
+    pub(crate) fn is_vim_mode_enabled(&self, cx: &App) -> bool {
+        VimModeSetting::try_get(cx)
+            .map(|vim_mode| vim_mode.0)
+            .unwrap_or(false)
+    }
 }
 
 fn edit_for_markdown_paste<'a>(

crates/editor/src/element.rs 🔗

@@ -133,7 +133,7 @@ impl SelectionLayout {
     fn new<T: ToPoint + ToDisplayPoint + Clone>(
         selection: Selection<T>,
         line_mode: bool,
-        cursor_offset: bool,
+        vim_mode_enabled: bool,
         cursor_shape: CursorShape,
         map: &DisplaySnapshot,
         is_newest: bool,
@@ -154,7 +154,7 @@ impl SelectionLayout {
         }
 
         // any vim visual mode (including line mode)
-        if cursor_offset && !range.is_empty() && !selection.reversed {
+        if vim_mode_enabled && !range.is_empty() && !selection.reversed {
             if head.column() > 0 {
                 head = map.clip_point(DisplayPoint::new(head.row(), head.column() - 1), Bias::Left);
             } else if head.row().0 > 0 && head != map.max_point() {
@@ -1463,7 +1463,7 @@ impl EditorElement {
                     let layout = SelectionLayout::new(
                         selection,
                         editor.selections.line_mode(),
-                        editor.cursor_offset_on_selection,
+                        editor.is_vim_mode_enabled(cx),
                         editor.cursor_shape,
                         &snapshot.display_snapshot,
                         is_newest,
@@ -1510,7 +1510,7 @@ impl EditorElement {
                     let drag_cursor_layout = SelectionLayout::new(
                         drop_cursor.clone(),
                         false,
-                        editor.cursor_offset_on_selection,
+                        editor.is_vim_mode_enabled(cx),
                         CursorShape::Bar,
                         &snapshot.display_snapshot,
                         false,
@@ -1574,7 +1574,7 @@ impl EditorElement {
                         .push(SelectionLayout::new(
                             selection.selection,
                             selection.line_mode,
-                            editor.cursor_offset_on_selection,
+                            editor.is_vim_mode_enabled(cx),
                             selection.cursor_shape,
                             &snapshot.display_snapshot,
                             false,
@@ -1585,8 +1585,7 @@ impl EditorElement {
 
                 selections.extend(remote_selections.into_values());
             } else if !editor.is_focused(window) && editor.show_cursor_when_unfocused {
-                let cursor_offset_on_selection = editor.cursor_offset_on_selection;
-
+                let player = editor.current_user_player_color(cx);
                 let layouts = snapshot
                     .buffer_snapshot()
                     .selections_in_range(&(start_anchor..end_anchor), true)
@@ -1594,7 +1593,7 @@ impl EditorElement {
                         SelectionLayout::new(
                             selection,
                             line_mode,
-                            cursor_offset_on_selection,
+                            editor.is_vim_mode_enabled(cx),
                             cursor_shape,
                             &snapshot.display_snapshot,
                             false,
@@ -1603,7 +1602,7 @@ impl EditorElement {
                         )
                     })
                     .collect::<Vec<_>>();
-                let player = editor.current_user_player_color(cx);
+
                 selections.push((player, layouts));
             }
         });
@@ -3318,7 +3317,7 @@ impl EditorElement {
                 SelectionLayout::new(
                     newest,
                     editor.selections.line_mode(),
-                    editor.cursor_offset_on_selection,
+                    editor.is_vim_mode_enabled(cx),
                     editor.cursor_shape,
                     &snapshot.display_snapshot,
                     true,
@@ -11549,6 +11548,7 @@ mod tests {
     use log::info;
     use std::num::NonZeroU32;
     use util::test::sample_text;
+    use vim_mode_setting::VimModeSetting;
 
     #[gpui::test]
     async fn test_soft_wrap_editor_width_auto_height_editor(cx: &mut TestAppContext) {
@@ -11893,6 +11893,12 @@ mod tests {
     async fn test_vim_visual_selections(cx: &mut TestAppContext) {
         init_test(cx, |_| {});
 
+        // Enable `vim_mode` setting so the logic that checks whether this is
+        // enabled can work as expected.
+        cx.update(|cx| {
+            VimModeSetting::override_global(VimModeSetting(true), cx);
+        });
+
         let window = cx.add_window(|window, cx| {
             let buffer = MultiBuffer::build_simple(&(sample_text(6, 6, 'a') + "\n"), cx);
             Editor::new(EditorMode::full(), buffer, None, window, cx)
@@ -11903,7 +11909,6 @@ mod tests {
 
         window
             .update(cx, |editor, window, cx| {
-                editor.cursor_offset_on_selection = true;
                 editor.change_selections(SelectionEffects::no_scroll(), window, cx, |s| {
                     s.select_ranges([
                         Point::new(0, 0)..Point::new(1, 0),

crates/vim/src/vim.rs 🔗

@@ -1943,7 +1943,6 @@ impl Vim {
             editor.set_collapse_matches(collapse_matches);
             editor.set_input_enabled(vim.editor_input_enabled());
             editor.set_autoindent(vim.should_autoindent());
-            editor.set_cursor_offset_on_selection(vim.mode.is_visual());
             editor
                 .selections
                 .set_line_mode(matches!(vim.mode, Mode::VisualLine));