From 9a2b7ef372021e5bcad759a2dc871e0743b602c4 Mon Sep 17 00:00:00 2001 From: fantacell Date: Thu, 14 Aug 2025 19:04:07 +0200 Subject: [PATCH] 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 | 13 ++ crates/vim/src/helix.rs | 290 ++++++++++++++++++----------------- crates/vim/src/motion.rs | 3 +- 3 files changed, 162 insertions(+), 144 deletions(-) diff --git a/crates/text/src/selection.rs b/crates/text/src/selection.rs index 18b82dbb6a..d3c280bde8 100644 --- a/crates/text/src/selection.rs +++ b/crates/text/src/selection.rs @@ -104,6 +104,19 @@ impl Selection { 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; diff --git a/crates/vim/src/helix.rs b/crates/vim/src/helix.rs index 29633ddef9..0c8c06d8ab 100644 --- a/crates/vim/src/helix.rs +++ b/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, + 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, @@ -65,49 +96,30 @@ impl Vim { cx: &mut Context, mut is_boundary: impl FnMut(char, char, &CharClassifier) -> bool, ) { - self.update_editor(cx, |_, editor, cx| { - editor.change_selections(Default::default(), window, cx, |s| { - s.move_with(|map, selection| { - let times = times.unwrap_or(1); - let new_goal = SelectionGoal::None; - let mut head = selection.head(); - let mut tail = selection.tail(); + let times = times.unwrap_or(1); + self.helix_new_selections(window, cx, |cursor, map| { + let mut head = movement::right(map, cursor); + let mut tail = cursor; + let classifier = map.buffer_snapshot.char_classifier_at(head.to_point(map)); + if head == map.max_point() { + 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() { - return; - } + if next_head == head && maybe_next_tail.unwrap_or(next_head) == tail { + break; + } - // collapse to block cursor - if tail < head { - tail = movement::left(map, head); - } else { - tail = head; - head = movement::right(map, head); - } - - // 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); - }); - }); + head = next_head; + if let Some(next_tail) = maybe_next_tail { + tail = next_tail; + } + } + Some((head, tail)) }); } @@ -118,56 +130,33 @@ impl Vim { cx: &mut Context, mut is_boundary: impl FnMut(char, char, &CharClassifier) -> bool, ) { - self.update_editor(cx, |_, editor, cx| { - editor.change_selections(Default::default(), window, cx, |s| { - s.move_with(|map, selection| { - let times = times.unwrap_or(1); - let new_goal = SelectionGoal::None; - let mut head = selection.head(); - let mut tail = selection.tail(); + let times = times.unwrap_or(1); + self.helix_new_selections(window, cx, |cursor, map| { + let mut head = cursor; + // The original cursor was one character wide, + // but the search starts from the left side of it, + // so to include that space the selection must end one character to the right. + 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() { - return; - } + if next_head == head && maybe_next_tail.unwrap_or(next_head) == tail { + break; + } - // collapse to block cursor - if tail < head { - tail = movement::left(map, head); - } else { - tail = head; - head = movement::right(map, head); - } - - 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); - }); - }) + head = next_head; + if let Some(next_tail) = maybe_next_tail { + tail = next_tail; + } + } + Some((head, tail)) }); } @@ -255,58 +244,53 @@ impl Vim { found }) } - Motion::FindForward { .. } => { - self.update_editor(cx, |_, editor, cx| { - let text_layout_details = editor.text_layout_details(window); - editor.change_selections(Default::default(), window, cx, |s| { - s.move_with(|map, selection| { - let goal = selection.goal; - let cursor = if selection.is_empty() || selection.reversed { - selection.head() - } else { - movement::left(map, selection.head()) - }; - - let (point, goal) = motion - .move_point( - map, - cursor, - selection.goal, - times, - &text_layout_details, - ) - .unwrap_or((cursor, goal)); - selection.set_tail(selection.head(), goal); - selection.set_head(movement::right(map, point), goal); - }) - }); + Motion::FindForward { + before, + char, + mode, + smartcase, + } => { + self.helix_new_selections(window, cx, |cursor, map| { + let start = cursor; + let mut last_boundary = start; + for _ in 0..times.unwrap_or(1) { + last_boundary = movement::find_boundary( + map, + movement::right(map, last_boundary), + mode, + |left, right| { + let current_char = if before { right } else { left }; + motion::is_character_match(char, current_char, smartcase) + }, + ); + } + Some((last_boundary, start)) }); } - Motion::FindBackward { .. } => { - self.update_editor(cx, |_, editor, cx| { - let text_layout_details = editor.text_layout_details(window); - editor.change_selections(Default::default(), window, cx, |s| { - s.move_with(|map, selection| { - let goal = selection.goal; - let cursor = if selection.is_empty() || selection.reversed { - selection.head() - } else { - movement::left(map, selection.head()) - }; - - let (point, goal) = motion - .move_point( - map, - cursor, - selection.goal, - times, - &text_layout_details, - ) - .unwrap_or((cursor, goal)); - selection.set_tail(selection.head(), goal); - selection.set_head(point, goal); - }) - }); + Motion::FindBackward { + after, + char, + mode, + smartcase, + } => { + self.helix_new_selections(window, cx, |cursor, map| { + let start = cursor; + let mut last_boundary = start; + for _ in 0..times.unwrap_or(1) { + last_boundary = movement::find_preceding_boundary_display_point( + map, + last_boundary, + mode, + |left, right| { + let current_char = if after { left } else { right }; + motion::is_character_match(char, current_char, smartcase) + }, + ); + } + // The original cursor was one character wide, + // but the search started from the left side of it, + // 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 - fox jumps over - the laz»y dog."}, + The quick brown + fox jumps ov«ˇer + 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, ); } diff --git a/crates/vim/src/motion.rs b/crates/vim/src/motion.rs index 7ef883f406..a6a07e7b2f 100644 --- a/crates/vim/src/motion.rs +++ b/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