From a07ba3c7183190dd05916f1e92aa84f088e542d0 Mon Sep 17 00:00:00 2001 From: Francisco Fernandes Date: Tue, 6 May 2025 07:37:58 +0100 Subject: [PATCH] editor: Fix inconsistent SelectPrevious behavior (#27695) When starting a selection from only carets, the action `editor::SelectPrevious` behaved in a manner inconsistent with `editor::SelectNext` as well as equivalent keybinds in editors such as VSCode, by selecting substrings of whole words matching the initially selected string on subsequent triggers. This fix brings the `select_previous` function in line with `select_next_internal`by calling `select_match_ranges` (previously an internal function of `select_next_internal`) in the same way it was previously used in the function that exhibited expected behavior. Furthermore, the relevant test was adapted to bring it in line with the equivalent test for the `editor::SelectNext` action Closes #24346 Release Notes: - Fixed inconsistent SelectPrevious behavior --- crates/editor/src/editor.rs | 87 ++++++++++++++----------------- crates/editor/src/editor_tests.rs | 6 +-- 2 files changed, 41 insertions(+), 52 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 02cc8a3306..d722f71d8b 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -12129,6 +12129,28 @@ impl Editor { } } + fn select_match_ranges( + &mut self, + range: Range, + reversed: bool, + replace_newest: bool, + auto_scroll: Option, + window: &mut Window, + cx: &mut Context, + ) { + self.unfold_ranges(&[range.clone()], false, auto_scroll.is_some(), cx); + self.change_selections(auto_scroll, window, cx, |s| { + if replace_newest { + s.delete(s.newest_anchor().id); + } + if reversed { + s.insert_range(range.end..range.start); + } else { + s.insert_range(range); + } + }); + } + pub fn select_next_match_internal( &mut self, display_map: &DisplaySnapshot, @@ -12137,28 +12159,6 @@ impl Editor { window: &mut Window, cx: &mut Context, ) -> Result<()> { - fn select_next_match_ranges( - this: &mut Editor, - range: Range, - reversed: bool, - replace_newest: bool, - auto_scroll: Option, - window: &mut Window, - cx: &mut Context, - ) { - this.unfold_ranges(&[range.clone()], false, auto_scroll.is_some(), cx); - this.change_selections(auto_scroll, window, cx, |s| { - if replace_newest { - s.delete(s.newest_anchor().id); - } - if reversed { - s.insert_range(range.end..range.start); - } else { - s.insert_range(range); - } - }); - } - let buffer = &display_map.buffer_snapshot; let mut selections = self.selections.all::(cx); if let Some(mut select_next_state) = self.select_next_state.take() { @@ -12203,8 +12203,7 @@ impl Editor { } if let Some(next_selected_range) = next_selected_range { - select_next_match_ranges( - self, + self.select_match_ranges( next_selected_range, last_selection.reversed, replace_newest, @@ -12262,8 +12261,7 @@ impl Editor { selection.end = word_range.end.to_offset(display_map, Bias::Left); selection.goal = SelectionGoal::None; selection.reversed = false; - select_next_match_ranges( - self, + self.select_match_ranges( selection.start..selection.end, selection.reversed, replace_newest, @@ -12428,17 +12426,14 @@ impl Editor { } if let Some(next_selected_range) = next_selected_range { - self.unfold_ranges(&[next_selected_range.clone()], false, true, cx); - self.change_selections(Some(Autoscroll::newest()), window, cx, |s| { - if action.replace_newest { - s.delete(s.newest_anchor().id); - } - if last_selection.reversed { - s.insert_range(next_selected_range.end..next_selected_range.start); - } else { - s.insert_range(next_selected_range); - } - }); + self.select_match_ranges( + next_selected_range, + last_selection.reversed, + action.replace_newest, + Some(Autoscroll::newest()), + window, + cx, + ); } else { select_prev_state.done = true; } @@ -12489,6 +12484,14 @@ impl Editor { selection.end = word_range.end.to_offset(&display_map, Bias::Left); selection.goal = SelectionGoal::None; selection.reversed = false; + self.select_match_ranges( + selection.start..selection.end, + selection.reversed, + action.replace_newest, + Some(Autoscroll::newest()), + window, + cx, + ); } if selections.len() == 1 { let selection = selections @@ -12507,16 +12510,6 @@ impl Editor { } else { self.select_prev_state = None; } - - self.unfold_ranges( - &selections.iter().map(|s| s.range()).collect::>(), - false, - true, - cx, - ); - self.change_selections(Some(Autoscroll::newest()), window, cx, |s| { - s.select(selections); - }); } else if let Some(selected_text) = selected_text { self.select_prev_state = Some(SelectNextState { query: AhoCorasick::new(&[selected_text.chars().rev().collect::()])?, diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 676233227b..b45675dda6 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -6130,11 +6130,7 @@ async fn test_select_previous_with_single_caret(cx: &mut TestAppContext) { cx.update_editor(|e, window, cx| e.select_previous(&SelectPrevious::default(), window, cx)) .unwrap(); - cx.assert_editor_state("«abcˇ»\n«abcˇ» abc\ndef«abcˇ»\n«abcˇ»"); - - cx.update_editor(|e, window, cx| e.select_previous(&SelectPrevious::default(), window, cx)) - .unwrap(); - cx.assert_editor_state("«abcˇ»\n«abcˇ» «abcˇ»\ndef«abcˇ»\n«abcˇ»"); + cx.assert_editor_state("«abcˇ»\n«abcˇ» «abcˇ»\ndefabc\n«abcˇ»"); } #[gpui::test]