helix: Change f and t motions (#35216)

In vim and zed (vim and helix modes) typing "tx" will jump before the
next `x`, but typing it again won't do anything. But in helix the cursor
just jumps before the `x` after that. I added that in helix mode.
This also solves another small issue where the selection doesn't include
the first `x` after typing "fx" twice. And similarly after typing "Fx"
or "Tx" the selection should include the character that the motion
startet on.

Release Notes:

- helix: Fixed inconsistencies in the "f" and "t" motions

crates/text/src/selection.rs

@@ -104,6 +104,19 @@ impl<T: Copy + Ord> Selection<T> {
         self.goal = new_goal;
     }
 
+    pub fn set_head_tail(&mut self, head: T, tail: T, new_goal: SelectionGoal) {
+        if head < tail {
+            self.reversed = true;
+            self.start = head;
+            self.end = tail;
+        } else {
+            self.reversed = false;
+            self.start = tail;
+            self.end = head;
+        }
+        self.goal = new_goal;
+    }
+
     pub fn swap_head_tail(&mut self) {
         if self.reversed {
             self.reversed = false;

crates/vim/src/helix.rs

@@ -1,9 +1,11 @@
+use editor::display_map::DisplaySnapshot;
 use editor::{DisplayPoint, Editor, SelectionEffects, ToOffset, ToPoint, movement};
 use gpui::{Action, actions};
 use gpui::{Context, Window};
 use language::{CharClassifier, CharKind};
 use text::{Bias, SelectionGoal};
 
+use crate::motion;
 use crate::{
     Vim,
     motion::{Motion, right},
@@ -58,6 +60,35 @@ impl Vim {
         self.helix_move_cursor(motion, times, window, cx);
     }
 
+    /// Updates all selections based on where the cursors are.
+    fn helix_new_selections(
+        &mut self,
+        window: &mut Window,
+        cx: &mut Context<Self>,
+        mut change: impl FnMut(
+            // the start of the cursor
+            DisplayPoint,
+            &DisplaySnapshot,
+        ) -> Option<(DisplayPoint, DisplayPoint)>,
+    ) {
+        self.update_editor(cx, |_, editor, cx| {
+            editor.change_selections(Default::default(), window, cx, |s| {
+                s.move_with(|map, selection| {
+                    let cursor_start = if selection.reversed || selection.is_empty() {
+                        selection.head()
+                    } else {
+                        movement::left(map, selection.head())
+                    };
+                    let Some((head, tail)) = change(cursor_start, map) else {
+                        return;
+                    };
+
+                    selection.set_head_tail(head, tail, SelectionGoal::None);
+                });
+            });
+        });
+    }
+
     fn helix_find_range_forward(
         &mut self,
         times: Option<usize>,
@@ -65,49 +96,30 @@ impl Vim {
         cx: &mut Context<Self>,
         mut is_boundary: impl FnMut(char, char, &CharClassifier) -> bool,
     ) {
-        self.update_editor(cx, |_, editor, cx| {
+        let times = times.unwrap_or(1);
-            editor.change_selections(Default::default(), window, cx, |s| {
+        self.helix_new_selections(window, cx, |cursor, map| {
-                s.move_with(|map, selection| {
+            let mut head = movement::right(map, cursor);
-                    let times = times.unwrap_or(1);
+            let mut tail = cursor;
-                    let new_goal = SelectionGoal::None;
+            let classifier = map.buffer_snapshot.char_classifier_at(head.to_point(map));
-                    let mut head = selection.head();
+            if head == map.max_point() {
-                    let mut tail = selection.tail();
+                return None;
+            }
+            for _ in 0..times {
+                let (maybe_next_tail, next_head) =
+                    movement::find_boundary_trail(map, head, |left, right| {
+                        is_boundary(left, right, &classifier)
+                    });
 
-                    if head == map.max_point() {
+                if next_head == head && maybe_next_tail.unwrap_or(next_head) == tail {
-                        return;
+                    break;
-                    }
+                }
 
-                    // collapse to block cursor
+                head = next_head;
-                    if tail < head {
+                if let Some(next_tail) = maybe_next_tail {
-                        tail = movement::left(map, head);
+                    tail = next_tail;
-                    } else {
+                }
-                        tail = head;
+            }
-                        head = movement::right(map, head);
+            Some((head, tail))
-                    }
-
-                    // create a classifier
-                    let classifier = map.buffer_snapshot.char_classifier_at(head.to_point(map));
-
-                    for _ in 0..times {
-                        let (maybe_next_tail, next_head) =
-                            movement::find_boundary_trail(map, head, |left, right| {
-                                is_boundary(left, right, &classifier)
-                            });
-
-                        if next_head == head && maybe_next_tail.unwrap_or(next_head) == tail {
-                            break;
-                        }
-
-                        head = next_head;
-                        if let Some(next_tail) = maybe_next_tail {
-                            tail = next_tail;
-                        }
-                    }
-
-                    selection.set_tail(tail, new_goal);
-                    selection.set_head(head, new_goal);
-                });
-            });
         });
     }
 
@@ -118,56 +130,33 @@ impl Vim {
         cx: &mut Context<Self>,
         mut is_boundary: impl FnMut(char, char, &CharClassifier) -> bool,
     ) {
-        self.update_editor(cx, |_, editor, cx| {
+        let times = times.unwrap_or(1);
-            editor.change_selections(Default::default(), window, cx, |s| {
+        self.helix_new_selections(window, cx, |cursor, map| {
-                s.move_with(|map, selection| {
+            let mut head = cursor;
-                    let times = times.unwrap_or(1);
+            // The original cursor was one character wide,
-                    let new_goal = SelectionGoal::None;
+            // but the search starts from the left side of it,
-                    let mut head = selection.head();
+            // so to include that space the selection must end one character to the right.
-                    let mut tail = selection.tail();
+            let mut tail = movement::right(map, cursor);
+            let classifier = map.buffer_snapshot.char_classifier_at(head.to_point(map));
+            if head == DisplayPoint::zero() {
+                return None;
+            }
+            for _ in 0..times {
+                let (maybe_next_tail, next_head) =
+                    movement::find_preceding_boundary_trail(map, head, |left, right| {
+                        is_boundary(left, right, &classifier)
+                    });
 
-                    if head == DisplayPoint::zero() {
+                if next_head == head && maybe_next_tail.unwrap_or(next_head) == tail {
-                        return;
+                    break;
-                    }
+                }
 
-                    // collapse to block cursor
+                head = next_head;
-                    if tail < head {
+                if let Some(next_tail) = maybe_next_tail {
-                        tail = movement::left(map, head);
+                    tail = next_tail;
-                    } else {
+                }
-                        tail = head;
+            }
-                        head = movement::right(map, head);
+            Some((head, tail))
-                    }
-
-                    selection.set_head(head, new_goal);
-                    selection.set_tail(tail, new_goal);
-                    // flip the selection
-                    selection.swap_head_tail();
-                    head = selection.head();
-                    tail = selection.tail();
-
-                    // create a classifier
-                    let classifier = map.buffer_snapshot.char_classifier_at(head.to_point(map));
-
-                    for _ in 0..times {
-                        let (maybe_next_tail, next_head) =
-                            movement::find_preceding_boundary_trail(map, head, |left, right| {
-                                is_boundary(left, right, &classifier)
-                            });
-
-                        if next_head == head && maybe_next_tail.unwrap_or(next_head) == tail {
-                            break;
-                        }
-
-                        head = next_head;
-                        if let Some(next_tail) = maybe_next_tail {
-                            tail = next_tail;
-                        }
-                    }
-
-                    selection.set_tail(tail, new_goal);
-                    selection.set_head(head, new_goal);
-                });
-            })
         });
     }
 
@@ -255,58 +244,53 @@ impl Vim {
                     found
                 })
             }
-            Motion::FindForward { .. } => {
+            Motion::FindForward {
-                self.update_editor(cx, |_, editor, cx| {
+                before,
-                    let text_layout_details = editor.text_layout_details(window);
+                char,
-                    editor.change_selections(Default::default(), window, cx, |s| {
+                mode,
-                        s.move_with(|map, selection| {
+                smartcase,
-                            let goal = selection.goal;
+            } => {
-                            let cursor = if selection.is_empty() || selection.reversed {
+                self.helix_new_selections(window, cx, |cursor, map| {
-                                selection.head()
+                    let start = cursor;
-                            } else {
+                    let mut last_boundary = start;
-                                movement::left(map, selection.head())
+                    for _ in 0..times.unwrap_or(1) {
-                            };
+                        last_boundary = movement::find_boundary(
-
+                            map,
-                            let (point, goal) = motion
+                            movement::right(map, last_boundary),
-                                .move_point(
+                            mode,
-                                    map,
+                            |left, right| {
-                                    cursor,
+                                let current_char = if before { right } else { left };
-                                    selection.goal,
+                                motion::is_character_match(char, current_char, smartcase)
-                                    times,
+                            },
-                                    &text_layout_details,
+                        );
-                                )
+                    }
-                                .unwrap_or((cursor, goal));
+                    Some((last_boundary, start))
-                            selection.set_tail(selection.head(), goal);
-                            selection.set_head(movement::right(map, point), goal);
-                        })
-                    });
                 });
             }
-            Motion::FindBackward { .. } => {
+            Motion::FindBackward {
-                self.update_editor(cx, |_, editor, cx| {
+                after,
-                    let text_layout_details = editor.text_layout_details(window);
+                char,
-                    editor.change_selections(Default::default(), window, cx, |s| {
+                mode,
-                        s.move_with(|map, selection| {
+                smartcase,
-                            let goal = selection.goal;
+            } => {
-                            let cursor = if selection.is_empty() || selection.reversed {
+                self.helix_new_selections(window, cx, |cursor, map| {
-                                selection.head()
+                    let start = cursor;
-                            } else {
+                    let mut last_boundary = start;
-                                movement::left(map, selection.head())
+                    for _ in 0..times.unwrap_or(1) {
-                            };
+                        last_boundary = movement::find_preceding_boundary_display_point(
-
+                            map,
-                            let (point, goal) = motion
+                            last_boundary,
-                                .move_point(
+                            mode,
-                                    map,
+                            |left, right| {
-                                    cursor,
+                                let current_char = if after { left } else { right };
-                                    selection.goal,
+                                motion::is_character_match(char, current_char, smartcase)
-                                    times,
+                            },
-                                    &text_layout_details,
+                        );
-                                )
+                    }
-                                .unwrap_or((cursor, goal));
+                    // The original cursor was one character wide,
-                            selection.set_tail(selection.head(), goal);
+                    // but the search started from the left side of it,
-                            selection.set_head(point, goal);
+                    // so to include that space the selection must end one character to the right.
-                        })
+                    Some((last_boundary, movement::right(map, start)))
-                    });
                 });
             }
             _ => self.helix_move_and_collapse(motion, times, window, cx),
@@ -630,13 +614,33 @@ mod test {
             Mode::HelixNormal,
         );
 
-        cx.simulate_keystrokes("2 T r");
+        cx.simulate_keystrokes("F e F e");
 
         cx.assert_state(
             indoc! {"
-                The quick br«ˇown
+                The quick brown
-                fox jumps over
+                fox jumps ov«ˇer
-                the laz»y dog."},
+                the» lazy dog."},
+            Mode::HelixNormal,
+        );
+
+        cx.simulate_keystrokes("e 2 F e");
+
+        cx.assert_state(
+            indoc! {"
+                Th«ˇe quick brown
+                fox jumps over»
+                the lazy dog."},
+            Mode::HelixNormal,
+        );
+
+        cx.simulate_keystrokes("t r t r");
+
+        cx.assert_state(
+            indoc! {"
+                The quick «brown
+                fox jumps oveˇ»r
+                the lazy dog."},
             Mode::HelixNormal,
         );
     }

crates/vim/src/motion.rs

@@ -2639,7 +2639,8 @@ fn find_backward(
     }
 }
 
-fn is_character_match(target: char, other: char, smartcase: bool) -> bool {
+/// Returns true if one char is equal to the other or its uppercase variant (if smartcase is true).
+pub fn is_character_match(target: char, other: char, smartcase: bool) -> bool {
     if smartcase {
         if target.is_uppercase() {
             target == other