Fix switching between helix and vim modes to not require returning to non-modal in between (#51706)

Finn Eitreim and dino created 2 weeks ago

* Update `Vim::register` to correctly re-activate when either `vim_mode`
  or `helix_mode` are enabled, when they were previously disabled.
* Update `settings_ui::page_data::keymap_page` such that, when enabling
  or disabling `vim_mode` or `helix_mode`, the other setting is
  disabled, in case it is set. This ensures that only one mode is
  enabled at a time, as it should be.
  
Closes #51704 

Release Notes:

- Fixed switching between vim and helix mode needing multiple calls to
  have an effect while an editor is already open
- Update Settings UI such that, enabling vim or helix mode, now disables
  the other if it was previously active

---------

Co-authored-by: dino <[e-mail]>

Change summary

crates/settings_ui/src/page_data.rs | 74 +++++++++++++++++++++++++++---
crates/vim/src/vim.rs               | 17 +++++--
2 files changed, 78 insertions(+), 13 deletions(-)

Detailed changes

crates/settings_ui/src/page_data.rs

@@ -1294,17 +1294,13 @@ fn keymap_page() -> SettingsPage {
     fn modal_editing_section() -> [SettingsPageItem; 3] {
         [
             SettingsPageItem::SectionHeader("Modal Editing"),
-            // todo(settings_ui): Vim/Helix Mode should be apart of one type because it's undefined
-            // behavior to have them both enabled at the same time
             SettingsPageItem::SettingItem(SettingItem {
                 title: "Vim Mode",
                 description: "Enable Vim mode and key bindings.",
                 field: Box::new(SettingField {
                     json_path: Some("vim_mode"),
                     pick: |settings_content| settings_content.vim_mode.as_ref(),
-                    write: |settings_content, value| {
-                        settings_content.vim_mode = value;
-                    },
+                    write: write_vim_mode,
                 }),
                 metadata: None,
                 files: USER,
@@ -1315,9 +1311,7 @@ fn keymap_page() -> SettingsPage {
                 field: Box::new(SettingField {
                     json_path: Some("helix_mode"),
                     pick: |settings_content| settings_content.helix_mode.as_ref(),
-                    write: |settings_content, value| {
-                        settings_content.helix_mode = value;
-                    },
+                    write: write_helix_mode,
                 }),
                 metadata: None,
                 files: USER,
@@ -9194,3 +9188,67 @@ where
 {
     <<T as strum::IntoDiscriminant>::Discriminant as strum::VariantArray>::VARIANTS
 }
+
+/// Updates the `vim_mode` setting, disabling `helix_mode` if present and
+/// `vim_mode` is being enabled.
+fn write_vim_mode(settings: &mut SettingsContent, value: Option<bool>) {
+    if value == Some(true) && settings.helix_mode == Some(true) {
+        settings.helix_mode = Some(false);
+    }
+    settings.vim_mode = value;
+}
+
+/// Updates the `helix_mode` setting, disabling `vim_mode` if present and
+/// `helix_mode` is being enabled.
+fn write_helix_mode(settings: &mut SettingsContent, value: Option<bool>) {
+    if value == Some(true) && settings.vim_mode == Some(true) {
+        settings.vim_mode = Some(false);
+    }
+    settings.helix_mode = value;
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_write_vim_helix_mode() {
+        // Enabling vim mode while `vim_mode` and `helix_mode` are not yet set
+        // should only update the `vim_mode` setting.
+        let mut settings = SettingsContent::default();
+        write_vim_mode(&mut settings, Some(true));
+        assert_eq!(settings.vim_mode, Some(true));
+        assert_eq!(settings.helix_mode, None);
+
+        // Enabling helix mode while `vim_mode` and `helix_mode` are not yet set
+        // should only update the `helix_mode` setting.
+        let mut settings = SettingsContent::default();
+        write_helix_mode(&mut settings, Some(true));
+        assert_eq!(settings.helix_mode, Some(true));
+        assert_eq!(settings.vim_mode, None);
+
+        // Disabling helix mode should only touch `helix_mode` setting when
+        // `vim_mode` is not set.
+        write_helix_mode(&mut settings, Some(false));
+        assert_eq!(settings.helix_mode, Some(false));
+        assert_eq!(settings.vim_mode, None);
+
+        // Enabling vim mode should update `vim_mode` but leave `helix_mode`
+        // untouched.
+        write_vim_mode(&mut settings, Some(true));
+        assert_eq!(settings.vim_mode, Some(true));
+        assert_eq!(settings.helix_mode, Some(false));
+
+        // Enabling helix mode should update `helix_mode` and disable
+        // `vim_mode`.
+        write_helix_mode(&mut settings, Some(true));
+        assert_eq!(settings.helix_mode, Some(true));
+        assert_eq!(settings.vim_mode, Some(false));
+
+        // Enabling vim mode should update `vim_mode` and disable
+        // `helix_mode`.
+        write_vim_mode(&mut settings, Some(true));
+        assert_eq!(settings.vim_mode, Some(true));
+        assert_eq!(settings.helix_mode, Some(false));
+    }
+}

crates/vim/src/vim.rs

@@ -601,9 +601,11 @@ impl Vim {
         }
 
         let mut was_enabled = Vim::enabled(cx);
+        let mut was_helix_enabled = HelixModeSetting::get_global(cx).0;
         let mut was_toggle = VimSettings::get_global(cx).toggle_relative_line_numbers;
         cx.observe_global_in::<SettingsStore>(window, move |editor, window, cx| {
             let enabled = Vim::enabled(cx);
+            let helix_enabled = HelixModeSetting::get_global(cx).0;
             let toggle = VimSettings::get_global(cx).toggle_relative_line_numbers;
             if enabled && was_enabled && (toggle != was_toggle) {
                 if toggle {
@@ -615,15 +617,20 @@ impl Vim {
                     editor.set_relative_line_number(None, cx)
                 }
             }
-            was_toggle = VimSettings::get_global(cx).toggle_relative_line_numbers;
-            if was_enabled == enabled {
+            let helix_changed = was_helix_enabled != helix_enabled;
+            was_toggle = toggle;
+            was_helix_enabled = helix_enabled;
+
+            let state_changed = (was_enabled != enabled) || (was_enabled && helix_changed);
+            if !state_changed {
                 return;
             }
+            if was_enabled {
+                Self::deactivate(editor, cx);
+            }
             was_enabled = enabled;
             if enabled {
-                Self::activate(editor, window, cx)
-            } else {
-                Self::deactivate(editor, cx)
+                Self::activate(editor, window, cx);
             }
         })
         .detach();