Vi mode: avoid placing cursor beyond last character

Today fish_cursor_selection_mode controls whether selection mode includes
the cursor. Since it's by default only used for Vi mode, perhaps use it to
also decide whether it should be allowed to select one-past the last character.

Not allowing to select to select one-past the last character is much nicer
in Vi mode.  Unfortunately Vi mode sometimes needs to temporarily select
past end (using forward-single-char and such), so reset fish_cursor_selection_mode
for the duration of the binding.

Also fix other things like cursor placement after yank/yank-pop.

Closes #10286
Closes #3299

share/functions/fish_vi_key_bindings.fish

@@ -65,12 +65,12 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
     bind -s --preset -M default l forward-char
     bind -s --preset -m insert \n execute
     bind -s --preset -m insert \r execute
-    bind -s --preset -m insert o insert-line-under repaint-mode
+    bind -s --preset -m insert o 'set fish_cursor_end_mode exclusive' insert-line-under repaint-mode
-    bind -s --preset -m insert O insert-line-over repaint-mode
+    bind -s --preset -m insert O 'set fish_cursor_end_modefish_cursor_end_modeexclusive' insert-line-over repaint-mode
     bind -s --preset -m insert i repaint-mode
     bind -s --preset -m insert I beginning-of-line repaint-mode
-    bind -s --preset -m insert a forward-single-char repaint-mode
+    bind -s --preset -m insert a 'set fish_cursor_end_mode exclusive' forward-single-char repaint-mode
-    bind -s --preset -m insert A end-of-line repaint-mode
+    bind -s --preset -m insert A 'set fish_cursor_end_mode exclusive' end-of-line repaint-mode
     bind -s --preset -m visual v begin-selection repaint-mode
 
     bind -s --preset gg beginning-of-buffer
@@ -98,8 +98,8 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
     bind -s --preset gE backward-bigword
     bind -s --preset w forward-word forward-single-char
     bind -s --preset W forward-bigword forward-single-char
-    bind -s --preset e forward-single-char forward-word backward-char
+    bind -s --preset e 'set fish_cursor_end_mode exclusive' forward-single-char forward-word backward-char 'set fish_cursor_end_mode inclusive'
-    bind -s --preset E forward-single-char forward-bigword backward-char
+    bind -s --preset E 'set fish_cursor_end_mode exclusive' forward-single-char forward-bigword backward-char 'set fish_cursor_end_mode inclusive'
 
     bind -s --preset -M insert \cn accept-autosuggestion
 
@@ -112,10 +112,10 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
     # Vi moves the cursor back if, after deleting, it is at EOL.
     # To emulate that, move forward, then backward, which will be a NOP
     # if there is something to move forward to.
-    bind -s --preset -M default x delete-char forward-single-char backward-char
+    bind -s --preset -M default x delete-char 'set fish_cursor_end_mode exclusive' forward-single-char backward-char 'set fish_cursor_end_mode inclusive'
     bind -s --preset -M default X backward-delete-char
     bind -s --preset -M insert -k dc delete-char forward-single-char backward-char
-    bind -s --preset -M default -k dc delete-char forward-single-char backward-char
+    bind -s --preset -M default -k dc delete-char 'set fish_cursor_end_mode exclusive' forward-single-char backward-char 'set fish_cursor_end_mode inclusive'
 
     # Backspace deletes a char in insert mode, but not in normal/default mode.
     bind -s --preset -M insert -k backspace backward-delete-char
@@ -225,13 +225,13 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
     # in vim p means paste *after* current character, so go forward a char before pasting
     # also in vim, P means paste *at* current position (like at '|' with cursor = line),
     # \ so there's no need to go back a char, just paste it without moving
-    bind -s --preset p forward-char yank
+    bind -s --preset p 'set -g fish_cursor_end_mode exclusive' forward-char 'set -g fish_cursor_end_modefish_cursor_end_modeinclusive' yank
     bind -s --preset P yank
     bind -s --preset gp yank-pop
 
     # same vim 'pasting' note as upper
-    bind -s --preset '"*p' forward-char "commandline -i ( xsel -p; echo )[1]"
+    bind -s --preset '"*p' 'set -g fish_cursor_end_mode exclusive' forward-char 'set -g fish_cursor_end_mode inclusive' fish_clipboard_paste
-    bind -s --preset '"*P' "commandline -i ( xsel -p; echo )[1]"
+    bind -s --preset '"*P' fish_clipboard_paste
 
     #
     # Lowercase r, enters replace_one mode
@@ -267,8 +267,8 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
     bind -s --preset -M visual gE backward-bigword
     bind -s --preset -M visual w forward-word
     bind -s --preset -M visual W forward-bigword
-    bind -s --preset -M visual e forward-word
+    bind -s --preset -M visual e 'set fish_cursor_end_mode exclusive' forward-single-char forward-word backward-char 'set fish_cursor_end_mode inclusive'
-    bind -s --preset -M visual E forward-bigword
+    bind -s --preset -M visual E 'set fish_cursor_end_mode exclusive' forward-single-char forward-bigword backward-char 'set fish_cursor_end_mode inclusive'
     bind -s --preset -M visual o swap-selection-start-stop repaint-mode
 
     bind -s --preset -M visual f forward-jump
@@ -306,7 +306,22 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
     # Therefore it needs to be before setting fish_bind_mode.
     fish_vi_cursor
     set -g fish_cursor_selection_mode inclusive
+    function __fish_vi_key_bindings_on_mode_change --on-variable fish_bind_mode
+        switch $fish_bind_mode
+            case insert
+                set -g fish_cursor_end_mode exclusive
+            case '*'
+                set -g fish_cursor_end_mode inclusive
+        end
+    end
+    function __fish_vi_key_bindings_remove_handlers --on-variable __fish_active_key_bindings
+        functions --erase __fish_vi_key_bindings_remove_handlers
+        functions --erase __fish_vi_key_bindings_on_mode_change
+        functions --erase fish_vi_cursor_handle
+        functions --erase fish_vi_cursor_handle_preexec
+        set -e -g fish_cursor_end_mode
+        set -e -g fish_cursor_selection_mode
+    end
 
     set fish_bind_mode $init_mode
-
 end

src/editable_line.rs

@@ -123,6 +123,7 @@ impl EditableLine {
         self.position
     }
     pub fn set_position(&mut self, position: usize) {
+        assert!(position <= self.len());
         self.position = position;
     }
 

src/env_dispatch.rs

@@ -9,8 +9,8 @@ use crate::input_common::{update_wait_on_escape_ms, update_wait_on_sequence_key_
 use crate::output::ColorSupport;
 use crate::proc::is_interactive_session;
 use crate::reader::{
-    reader_change_cursor_selection_mode, reader_change_history, reader_schedule_prompt_repaint,
+    reader_change_cursor_end_mode, reader_change_cursor_selection_mode, reader_change_history,
-    reader_set_autosuggestion_enabled,
+    reader_schedule_prompt_repaint, reader_set_autosuggestion_enabled,
 };
 use crate::screen::screen_set_midnight_commander_hack;
 use crate::screen::LAYOUT_CACHE_SHARED;
@@ -85,6 +85,10 @@ static VAR_DISPATCH_TABLE: once_cell::sync::Lazy<VarDispatchTable> =
             L!("fish_cursor_selection_mode"),
             handle_fish_cursor_selection_mode_change,
         );
+        table.add_anon(
+            L!("fish_cursor_end_mode"),
+            handle_fish_cursor_end_mode_change,
+        );
 
         table
     });
@@ -259,6 +263,24 @@ fn handle_fish_cursor_selection_mode_change(vars: &EnvStack) {
     reader_change_cursor_selection_mode(mode);
 }
 
+fn handle_fish_cursor_end_mode_change(vars: &EnvStack) {
+    use crate::reader::CursorEndMode;
+
+    let inclusive = vars
+        .get(L!("fish_cursor_end_mode"))
+        .as_ref()
+        .map(|v| v.as_string())
+        .map(|v| v == "inclusive")
+        .unwrap_or(false);
+    let mode = if inclusive {
+        CursorEndMode::Inclusive
+    } else {
+        CursorEndMode::Exclusive
+    };
+
+    reader_change_cursor_end_mode(mode);
+}
+
 fn handle_autosuggestion_change(vars: &EnvStack) {
     reader_set_autosuggestion_enabled(vars);
 }

src/reader.rs

@@ -338,6 +338,12 @@ pub enum CursorSelectionMode {
     Inclusive,
 }
 
+#[derive(Eq, PartialEq)]
+pub enum CursorEndMode {
+    Exclusive,
+    Inclusive,
+}
+
 /// A mode for calling the reader_kill function.
 enum Kill {
     /// In this mode, the new string is appended to the current contents of the kill buffer.
@@ -500,6 +506,7 @@ pub struct ReaderData {
 
     /// The cursor selection mode.
     cursor_selection_mode: CursorSelectionMode,
+    cursor_end_mode: CursorEndMode,
 
     /// The selection data. If this is not none, then we have an active selection.
     selection: Option<SelectionData>,
@@ -814,7 +821,28 @@ pub fn reader_change_history(name: &wstr) {
 pub fn reader_change_cursor_selection_mode(selection_mode: CursorSelectionMode) {
     // We don't need to _change_ if we're not initialized yet.
     if let Some(data) = current_data() {
+        if data.cursor_selection_mode == selection_mode {
+            return;
+        }
+        let invalidates_selection = data.selection.is_some();
         data.cursor_selection_mode = selection_mode;
+        if invalidates_selection {
+            data.update_buff_pos(EditableLineTag::Commandline, None);
+        }
+    }
+}
+
+pub fn reader_change_cursor_end_mode(end_mode: CursorEndMode) {
+    // We don't need to _change_ if we're not initialized yet.
+    if let Some(data) = current_data() {
+        if data.cursor_end_mode == end_mode {
+            return;
+        }
+        let invalidates_end = data.selection.is_some();
+        data.cursor_end_mode = end_mode;
+        if invalidates_end {
+            data.update_buff_pos(EditableLineTag::Commandline, None);
+        }
     }
 }
 
@@ -1027,6 +1055,7 @@ impl ReaderData {
             history_pager_history_index_start: usize::MAX,
             history_pager_history_index_end: usize::MAX,
             cursor_selection_mode: CursorSelectionMode::Exclusive,
+            cursor_end_mode: CursorEndMode::Exclusive,
             selection: Default::default(),
             left_prompt_buff: Default::default(),
             mode_prompt_buff: Default::default(),
@@ -1159,13 +1188,24 @@ impl ReaderData {
     }
 
     /// Update the cursor position.
-    fn update_buff_pos(&mut self, elt: EditableLineTag, new_pos: Option<usize>) {
+    fn update_buff_pos(&mut self, elt: EditableLineTag, mut new_pos: Option<usize>) -> bool {
+        if self.cursor_end_mode == CursorEndMode::Inclusive {
+            let el = self.edit_line(elt);
+            let mut pos = new_pos.unwrap_or(el.position());
+            if !el.is_empty() && pos == el.len() {
+                pos = el.len() - 1;
+                if el.position() == pos {
+                    return false;
+                }
+                new_pos = Some(pos);
+            }
+        }
         if let Some(pos) = new_pos {
             self.edit_line_mut(elt).set_position(pos);
         }
 
         if elt != EditableLineTag::Commandline {
-            return;
+            return true;
         }
         let buff_pos = self.command_line.position();
         let target_char = if self.cursor_selection_mode == CursorSelectionMode::Inclusive {
@@ -1174,7 +1214,7 @@ impl ReaderData {
             0
         };
         let Some(selection) = self.selection.as_mut() else {
-            return;
+            return true;
         };
         if selection.begin <= buff_pos {
             selection.start = selection.begin;
@@ -1183,6 +1223,7 @@ impl ReaderData {
             selection.start = buff_pos;
             selection.stop = selection.begin + target_char;
         }
+        true
     }
 }
 
@@ -2025,7 +2066,7 @@ impl ReaderData {
             }
             rl::EndOfLine => {
                 let (_elt, el) = self.active_edit_line();
-                if el.position() == el.len() {
+                if self.is_at_end(el) {
                     self.accept_autosuggestion(true, false, MoveWordStyle::Punctuation);
                 } else {
                     loop {
@@ -2040,7 +2081,9 @@ impl ReaderData {
                             }
                             position
                         };
-                        self.update_buff_pos(self.active_edit_line_tag(), Some(position + 1));
+                        if !self.update_buff_pos(self.active_edit_line_tag(), Some(position + 1)) {
+                            break;
+                        }
                     }
                 }
             }
@@ -2255,17 +2298,24 @@ impl ReaderData {
                 let yank_str = kill_yank();
                 self.insert_string(self.active_edit_line_tag(), &yank_str);
                 rls.yank_len = yank_str.len();
+                if self.cursor_end_mode == CursorEndMode::Inclusive {
+                    let (_elt, el) = self.active_edit_line();
+                    self.update_buff_pos(self.active_edit_line_tag(), Some(el.position() - 1));
+                }
             }
             rl::YankPop => {
                 if rls.yank_len != 0 {
                     let (elt, el) = self.active_edit_line();
                     let yank_str = kill_yank_rotate();
                     let new_yank_len = yank_str.len();
-                    self.replace_substring(
+                    let bias = if self.cursor_end_mode == CursorEndMode::Inclusive {
-                        elt,
+                        1
-                        el.position() - rls.yank_len..el.position(),
+                    } else {
-                        yank_str,
+                        0
-                    );
+                    };
+                    let begin = el.position() + bias - rls.yank_len;
+                    let end = el.position() + bias;
+                    self.replace_substring(elt, begin..end, yank_str);
                     self.update_buff_pos(elt, None);
                     rls.yank_len = new_yank_len;
                     self.suppress_autosuggestion = true;
@@ -2448,14 +2498,14 @@ impl ReaderData {
                 let (elt, el) = self.active_edit_line();
                 if self.is_navigating_pager_contents() {
                     self.select_completion_in_direction(SelectionMotion::East, false);
-                } else if el.position() != el.len() {
+                } else if self.is_at_end(el) {
-                    self.update_buff_pos(elt, Some(el.position() + 1));
-                } else {
                     self.accept_autosuggestion(
                         /*full=*/ c != rl::ForwardSingleChar,
                         /*single=*/ c == rl::ForwardSingleChar,
                         MoveWordStyle::Punctuation,
                     );
+                } else {
+                    self.update_buff_pos(elt, Some(el.position() + 1));
                 }
             }
             rl::BackwardKillWord | rl::BackwardKillPathComponent | rl::BackwardKillBigword => {
@@ -2539,10 +2589,10 @@ impl ReaderData {
                     MoveWordStyle::Whitespace
                 };
                 let (elt, el) = self.active_edit_line();
-                if el.position() != el.len() {
+                if self.is_at_end(el) {
-                    self.move_word(elt, MoveWordDir::Right, /*erase=*/ false, style, false);
-                } else {
                     self.accept_autosuggestion(false, false, style);
+                } else {
+                    self.move_word(elt, MoveWordDir::Right, /*erase=*/ false, style, false);
                 }
             }
             rl::BeginningOfHistory | rl::EndOfHistory => {
@@ -2836,7 +2886,9 @@ impl ReaderData {
                     if el.position() == 0 || el.text().as_char_slice()[el.position() - 1] == '\n' {
                         break elt;
                     }
-                    self.update_buff_pos(elt, Some(el.position() - 1));
+                    if !self.update_buff_pos(elt, Some(el.position() - 1)) {
+                        break elt;
+                    }
                 };
                 self.insert_char(elt, '\n');
                 let (elt, el) = self.active_edit_line();
@@ -2849,7 +2901,9 @@ impl ReaderData {
                     {
                         break elt;
                     }
-                    self.update_buff_pos(elt, Some(el.position() + 1));
+                    if !self.update_buff_pos(elt, Some(el.position() + 1)) {
+                        break elt;
+                    }
                 };
                 self.insert_char(elt, '\n');
             }
@@ -3907,6 +3961,13 @@ impl ReaderData {
         debounce_autosuggestions().perform_with_completion(performer, completion);
     }
 
+    fn is_at_end(&self, el: &EditableLine) -> bool {
+        match self.cursor_end_mode {
+            CursorEndMode::Exclusive => el.position() == el.len(),
+            CursorEndMode::Inclusive => el.position() + 1 >= el.len(),
+        }
+    }
+
     // Accept any autosuggestion by replacing the command line with it. If full is true, take the whole
     // thing; if it's false, then respect the passed in style.
     fn accept_autosuggestion(