Improve Visual Mode Selection and Command Consistency (#867)

* fix Vi mode change: restore normal mode after cut operations in visual mode

* fix Cut/Change/Delete under visual-mode: Align Vim standards

* fix ESC behavior under visual mode: clear selection when pressing ESC

* fix ESC behavior under visual mode: follow-up: Implement ReedlineEvent::ResetSelection

* fix Vi mode change: follow-up: Change/Delete + Incomplete motion could yield mode change from visual to insert/normal

* fix: Cut/Change/Delete under visual-mode (follow-up): selection range considers UTF8

---------

Co-authored-by: WHOWHOWHOWHOWHOWHOWHOWHOWHOWHO <who@users.noreply.github.com>

Author: deephbz

src/core_editor/editor.rs

@@ -564,10 +564,19 @@ impl Editor {
     /// The range is guaranteed to be ascending.
     pub fn get_selection(&self) -> Option<(usize, usize)> {
         self.selection_anchor.map(|selection_anchor| {
+            let buffer_len = self.line_buffer.len();
             if self.insertion_point() > selection_anchor {
-                (selection_anchor, self.insertion_point())
+                (
+                    selection_anchor,
+                    self.line_buffer.grapheme_right_index().min(buffer_len),
+                )
             } else {
-                (self.insertion_point(), selection_anchor)
+                (
+                    self.insertion_point(),
+                    self.line_buffer
+                        .grapheme_right_index_from_pos(selection_anchor)
+                        .min(buffer_len),
+                )
             }
         })
     }
@@ -648,6 +657,10 @@ impl Editor {
         self.delete_selection();
         insert_clipboard_content_before(&mut self.line_buffer, self.cut_buffer.deref_mut());
     }
+
+    pub(crate) fn reset_selection(&mut self) {
+        self.selection_anchor = None;
+    }
 }
 
 fn insert_clipboard_content_before(line_buffer: &mut LineBuffer, clipboard: &mut dyn Clipboard) {

src/core_editor/line_buffer.rs

@@ -168,6 +168,15 @@ impl LineBuffer {
             .unwrap_or(0)
     }
 
+    /// Cursor position *behind* the next unicode grapheme to the right from the given position
+    pub fn grapheme_right_index_from_pos(&self, pos: usize) -> usize {
+        self.lines[pos..]
+            .grapheme_indices(true)
+            .nth(1)
+            .map(|(i, _)| pos + i)
+            .unwrap_or_else(|| self.lines.len())
+    }
+
     /// Cursor position *behind* the next word to the right
     pub fn word_right_index(&self) -> usize {
         self.lines[self.insertion_point..]
@@ -1597,4 +1606,26 @@ mod test {
 
         assert_eq!(index, expected);
     }
+
+    #[rstest]
+    #[case("abc", 0, 1)] // Basic ASCII
+    #[case("abc", 1, 2)] // From middle position
+    #[case("abc", 2, 3)] // From last char
+    #[case("abc", 3, 3)] // From end of string
+    #[case("🦀rust", 0, 4)] // Unicode emoji
+    #[case("🦀rust", 4, 5)] // After emoji
+    #[case("é́", 0, 4)] // Combining characters
+    fn test_grapheme_right_index_from_pos(
+        #[case] input: &str,
+        #[case] position: usize,
+        #[case] expected: usize,
+    ) {
+        let mut line = LineBuffer::new();
+        line.insert_str(input);
+        assert_eq!(
+            line.grapheme_right_index_from_pos(position),
+            expected,
+            "input: {input:?}, pos: {position}"
+        );
+    }
 }

src/edit_mode/vi/command.rs

@@ -1,4 +1,4 @@
-use super::{motion::Motion, motion::ViCharSearch, parser::ReedlineOption};
+use super::{motion::Motion, motion::ViCharSearch, parser::ReedlineOption, ViMode};
 use crate::{EditCommand, ReedlineEvent, Vi};
 use std::iter::Peekable;
 
@@ -166,11 +166,23 @@ impl Command {
                 select: false,
             })],
             Self::RewriteCurrentLine => vec![ReedlineOption::Edit(EditCommand::CutCurrentLine)],
-            Self::DeleteChar => vec![ReedlineOption::Edit(EditCommand::CutChar)],
+            Self::DeleteChar => {
+                if vi_state.mode == ViMode::Visual {
+                    vec![ReedlineOption::Edit(EditCommand::CutSelection)]
+                } else {
+                    vec![ReedlineOption::Edit(EditCommand::CutChar)]
+                }
+            }
             Self::ReplaceChar(c) => {
                 vec![ReedlineOption::Edit(EditCommand::ReplaceChar(*c))]
             }
-            Self::SubstituteCharWithInsert => vec![ReedlineOption::Edit(EditCommand::CutChar)],
+            Self::SubstituteCharWithInsert => {
+                if vi_state.mode == ViMode::Visual {
+                    vec![ReedlineOption::Edit(EditCommand::CutSelection)]
+                } else {
+                    vec![ReedlineOption::Edit(EditCommand::CutChar)]
+                }
+            }
             Self::HistorySearch => vec![ReedlineOption::Event(ReedlineEvent::SearchHistory)],
             Self::Switchcase => vec![ReedlineOption::Edit(EditCommand::SwitchcaseChar)],
             // Whenever a motion is required to finish the command we must be in visual mode

src/edit_mode/vi/mod.rs

@@ -89,11 +89,10 @@ impl EditMode for Vi {
                             self.cache.clear();
                             ReedlineEvent::None
                         } else if res.is_complete(self.mode) {
-                            if let Some(mode) = res.changes_mode() {
+                            let event = res.to_reedline_event(self);
+                            if let Some(mode) = res.changes_mode(self.mode) {
                                 self.mode = mode;
                             }
-
-                            let event = res.to_reedline_event(self);
                             self.cache.clear();
                             event
                         } else {
@@ -143,7 +142,11 @@ impl EditMode for Vi {
                 (_, KeyModifiers::NONE, KeyCode::Esc) => {
                     self.cache.clear();
                     self.mode = ViMode::Normal;
-                    ReedlineEvent::Multiple(vec![ReedlineEvent::Esc, ReedlineEvent::Repaint])
+                    ReedlineEvent::Multiple(vec![
+                        ReedlineEvent::ResetSelection,
+                        ReedlineEvent::Esc,
+                        ReedlineEvent::Repaint,
+                    ])
                 }
                 (_, KeyModifiers::NONE, KeyCode::Enter) => {
                     self.mode = ViMode::Insert;
@@ -192,7 +195,11 @@ mod test {
 
         assert_eq!(
             result,
-            ReedlineEvent::Multiple(vec![ReedlineEvent::Esc, ReedlineEvent::Repaint])
+            ReedlineEvent::Multiple(vec![
+                ReedlineEvent::ResetSelection,
+                ReedlineEvent::Esc,
+                ReedlineEvent::Repaint
+            ])
         );
         assert!(matches!(vi.mode, ViMode::Normal));
     }

src/edit_mode/vi/parser.rs

@@ -98,7 +98,7 @@ impl ParsedViSequence {
         }
     }
 
-    pub fn changes_mode(&self) -> Option<ViMode> {
+    pub fn changes_mode(&self, mode: ViMode) -> Option<ViMode> {
         match (&self.command, &self.motion) {
             (Some(Command::EnterViInsert), ParseResult::Incomplete)
             | (Some(Command::EnterViAppend), ParseResult::Incomplete)
@@ -109,12 +109,17 @@ impl ParsedViSequence {
             | (Some(Command::SubstituteCharWithInsert), ParseResult::Incomplete)
             | (Some(Command::HistorySearch), ParseResult::Incomplete)
             | (Some(Command::Change), ParseResult::Valid(_)) => Some(ViMode::Insert),
-            (Some(Command::ChangeInside(char)), ParseResult::Incomplete)
+            (Some(Command::Change), ParseResult::Incomplete) if mode == ViMode::Visual => {
+                Some(ViMode::Insert)
+            }
+            (Some(Command::Delete), ParseResult::Incomplete) if mode == ViMode::Visual => {
+                Some(ViMode::Normal)
+            }
+            (Some(Command::ChangeInside(char)), ParseResult::Valid(_))
                 if is_valid_change_inside_left(char) || is_valid_change_inside_right(char) =>
             {
                 Some(ViMode::Insert)
             }
-            (Some(Command::Delete), ParseResult::Incomplete) => Some(ViMode::Normal),
             _ => None,
         }
     }

src/engine.rs

@@ -909,6 +909,10 @@ impl Reedline {
                 self.input_mode = InputMode::Regular;
                 Ok(EventStatus::Handled)
             }
+            ReedlineEvent::ResetSelection => {
+                self.editor.reset_selection();
+                Ok(EventStatus::Handled)
+            }
             // TODO: Check if events should be handled
             ReedlineEvent::Right
             | ReedlineEvent::Left
@@ -1197,6 +1201,10 @@ impl Reedline {
                 Ok(EventStatus::Handled)
             }
             ReedlineEvent::OpenEditor => self.open_editor().map(|_| EventStatus::Handled),
+            ReedlineEvent::ResetSelection => {
+                self.editor.reset_selection();
+                Ok(EventStatus::Handled)
+            }
             ReedlineEvent::Resize(width, height) => {
                 self.painter.handle_resize(width, height);
                 Ok(EventStatus::Handled)

src/enums.rs

@@ -644,6 +644,9 @@ pub enum ReedlineEvent {
 
     /// Open text editor
     OpenEditor,
+
+    /// Reset the current text selection
+    ResetSelection,
 }
 
 impl Display for ReedlineEvent {
@@ -687,6 +690,7 @@ impl Display for ReedlineEvent {
             ReedlineEvent::MenuPagePrevious => write!(f, "MenuPagePrevious"),
             ReedlineEvent::ExecuteHostCommand(_) => write!(f, "ExecuteHostCommand"),
             ReedlineEvent::OpenEditor => write!(f, "OpenEditor"),
+            ReedlineEvent::ResetSelection => write!(f, "ResetSelection"),
         }
     }
 }