From 96409965e428ec54f3c994e5a6b7d6d7a561ed40 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Tue, 24 Jun 2025 23:18:35 -0600 Subject: [PATCH] Cleanup handling of surrounding word logic, fixing crash in editor::SelectAllMatches (#33353) This reduces code complexity and avoids unnecessary roundtripping through `DisplayPoint`. Hopefully this doesn't cause behavior changes, but has one known behavior improvement: `clip_at_line_ends` logic caused `is_inside_word` to return false when on a word at the end of the line. In vim mode, this caused `select_all_matches` to not select words at the end of lines, and in some cases crashes due to not finding any selections. Closes #29823 Release Notes: - N/A --- crates/editor/src/editor.rs | 118 ++++++++++-------------- crates/editor/src/editor_tests.rs | 9 ++ crates/editor/src/movement.rs | 58 +----------- crates/multi_buffer/src/multi_buffer.rs | 13 +++ 4 files changed, 71 insertions(+), 127 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 6e9a9be0fe..770ad7fa70 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3388,9 +3388,12 @@ impl Editor { auto_scroll = true; } 2 => { - let range = movement::surrounding_word(&display_map, position); - start = buffer.anchor_before(range.start.to_point(&display_map)); - end = buffer.anchor_before(range.end.to_point(&display_map)); + let position = display_map + .clip_point(position, Bias::Left) + .to_offset(&display_map, Bias::Left); + let (range, _) = buffer.surrounding_word(position, false); + start = buffer.anchor_before(range.start); + end = buffer.anchor_before(range.end); mode = SelectMode::Word(start..end); auto_scroll = true; } @@ -3523,37 +3526,39 @@ impl Editor { if self.columnar_selection_state.is_some() { self.select_columns(position, goal_column, &display_map, window, cx); } else if let Some(mut pending) = self.selections.pending_anchor() { - let buffer = self.buffer.read(cx).snapshot(cx); + let buffer = &display_map.buffer_snapshot; let head; let tail; let mode = self.selections.pending_mode().unwrap(); match &mode { SelectMode::Character => { head = position.to_point(&display_map); - tail = pending.tail().to_point(&buffer); + tail = pending.tail().to_point(buffer); } SelectMode::Word(original_range) => { - let original_display_range = original_range.start.to_display_point(&display_map) - ..original_range.end.to_display_point(&display_map); - let original_buffer_range = original_display_range.start.to_point(&display_map) - ..original_display_range.end.to_point(&display_map); - if movement::is_inside_word(&display_map, position) - || original_display_range.contains(&position) + let offset = display_map + .clip_point(position, Bias::Left) + .to_offset(&display_map, Bias::Left); + let original_range = original_range.to_offset(buffer); + + let head_offset = if buffer.is_inside_word(offset, false) + || original_range.contains(&offset) { - let word_range = movement::surrounding_word(&display_map, position); - if word_range.start < original_display_range.start { - head = word_range.start.to_point(&display_map); + let (word_range, _) = buffer.surrounding_word(offset, false); + if word_range.start < original_range.start { + word_range.start } else { - head = word_range.end.to_point(&display_map); + word_range.end } } else { - head = position.to_point(&display_map); - } + offset + }; - if head <= original_buffer_range.start { - tail = original_buffer_range.end; + head = head_offset.to_point(buffer); + if head_offset <= original_range.start { + tail = original_range.end.to_point(buffer); } else { - tail = original_buffer_range.start; + tail = original_range.start.to_point(buffer); } } SelectMode::Line(original_range) => { @@ -10794,7 +10799,6 @@ impl Editor { where Fn: FnMut(&str) -> String, { - let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx)); let buffer = self.buffer.read(cx).snapshot(cx); let mut new_selections = Vec::new(); @@ -10805,13 +10809,8 @@ impl Editor { let selection_is_empty = selection.is_empty(); let (start, end) = if selection_is_empty { - let word_range = movement::surrounding_word( - &display_map, - selection.start.to_display_point(&display_map), - ); - let start = word_range.start.to_offset(&display_map, Bias::Left); - let end = word_range.end.to_offset(&display_map, Bias::Left); - (start, end) + let (word_range, _) = buffer.surrounding_word(selection.start, false); + (word_range.start, word_range.end) } else { (selection.start, selection.end) }; @@ -13255,12 +13254,10 @@ impl Editor { let query_match = query_match.unwrap(); // can only fail due to I/O let offset_range = start_offset + query_match.start()..start_offset + query_match.end(); - let display_range = offset_range.start.to_display_point(display_map) - ..offset_range.end.to_display_point(display_map); if !select_next_state.wordwise - || (!movement::is_inside_word(display_map, display_range.start) - && !movement::is_inside_word(display_map, display_range.end)) + || (!buffer.is_inside_word(offset_range.start, false) + && !buffer.is_inside_word(offset_range.end, false)) { // TODO: This is n^2, because we might check all the selections if !selections @@ -13324,12 +13321,9 @@ impl Editor { if only_carets { for selection in &mut selections { - let word_range = movement::surrounding_word( - display_map, - selection.start.to_display_point(display_map), - ); - selection.start = word_range.start.to_offset(display_map, Bias::Left); - selection.end = word_range.end.to_offset(display_map, Bias::Left); + let (word_range, _) = buffer.surrounding_word(selection.start, false); + selection.start = word_range.start; + selection.end = word_range.end; selection.goal = SelectionGoal::None; selection.reversed = false; self.select_match_ranges( @@ -13410,18 +13404,22 @@ impl Editor { } else { query_match.start()..query_match.end() }; - let display_range = offset_range.start.to_display_point(&display_map) - ..offset_range.end.to_display_point(&display_map); if !select_next_state.wordwise - || (!movement::is_inside_word(&display_map, display_range.start) - && !movement::is_inside_word(&display_map, display_range.end)) + || (!buffer.is_inside_word(offset_range.start, false) + && !buffer.is_inside_word(offset_range.end, false)) { new_selections.push(offset_range.start..offset_range.end); } } select_next_state.done = true; + + if new_selections.is_empty() { + log::error!("bug: new_selections is empty in select_all_matches"); + return Ok(()); + } + self.unfold_ranges(&new_selections.clone(), false, false, cx); self.change_selections(None, window, cx, |selections| { selections.select_ranges(new_selections) @@ -13481,12 +13479,10 @@ impl Editor { let query_match = query_match.unwrap(); // can only fail due to I/O let offset_range = end_offset - query_match.end()..end_offset - query_match.start(); - let display_range = offset_range.start.to_display_point(&display_map) - ..offset_range.end.to_display_point(&display_map); if !select_prev_state.wordwise - || (!movement::is_inside_word(&display_map, display_range.start) - && !movement::is_inside_word(&display_map, display_range.end)) + || (!buffer.is_inside_word(offset_range.start, false) + && !buffer.is_inside_word(offset_range.end, false)) { next_selected_range = Some(offset_range); break; @@ -13544,12 +13540,9 @@ impl Editor { if only_carets { for selection in &mut selections { - let word_range = movement::surrounding_word( - &display_map, - selection.start.to_display_point(&display_map), - ); - selection.start = word_range.start.to_offset(&display_map, Bias::Left); - selection.end = word_range.end.to_offset(&display_map, Bias::Left); + let (word_range, _) = buffer.surrounding_word(selection.start, false); + selection.start = word_range.start; + selection.end = word_range.end; selection.goal = SelectionGoal::None; selection.reversed = false; self.select_match_ranges( @@ -14024,26 +14017,11 @@ impl Editor { if let Some((node, _)) = buffer.syntax_ancestor(old_range.clone()) { // manually select word at selection if ["string_content", "inline"].contains(&node.kind()) { - let word_range = { - let display_point = buffer - .offset_to_point(old_range.start) - .to_display_point(&display_map); - let Range { start, end } = - movement::surrounding_word(&display_map, display_point); - start.to_point(&display_map).to_offset(&buffer) - ..end.to_point(&display_map).to_offset(&buffer) - }; + let (word_range, _) = buffer.surrounding_word(old_range.start, false); // ignore if word is already selected if !word_range.is_empty() && old_range != word_range { - let last_word_range = { - let display_point = buffer - .offset_to_point(old_range.end) - .to_display_point(&display_map); - let Range { start, end } = - movement::surrounding_word(&display_map, display_point); - start.to_point(&display_map).to_offset(&buffer) - ..end.to_point(&display_map).to_offset(&buffer) - }; + let (last_word_range, _) = + buffer.surrounding_word(old_range.end, false); // only select word if start and end point belongs to same word if word_range == last_word_range { selected_larger_node = true; diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 7f4e19e7d4..6a579cb1cd 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -6667,6 +6667,15 @@ async fn test_select_all_matches(cx: &mut TestAppContext) { cx.update_editor(|e, window, cx| e.select_all_matches(&SelectAllMatches, window, cx)) .unwrap(); cx.assert_editor_state("abc\n« ˇ»abc\nabc"); + + // Test with a single word and clip_at_line_ends=true (#29823) + cx.set_state("aˇbc"); + cx.update_editor(|e, window, cx| { + e.set_clip_at_line_ends(true, cx); + e.select_all_matches(&SelectAllMatches, window, cx).unwrap(); + e.set_clip_at_line_ends(false, cx); + }); + cx.assert_editor_state("«abcˇ»"); } #[gpui::test] diff --git a/crates/editor/src/movement.rs b/crates/editor/src/movement.rs index e4167ee68e..b9b7cb2e58 100644 --- a/crates/editor/src/movement.rs +++ b/crates/editor/src/movement.rs @@ -2,7 +2,7 @@ //! in editor given a given motion (e.g. it handles converting a "move left" command into coordinates in editor). It is exposed mostly for use by vim crate. use super::{Bias, DisplayPoint, DisplaySnapshot, SelectionGoal, ToDisplayPoint}; -use crate::{CharKind, DisplayRow, EditorStyle, ToOffset, ToPoint, scroll::ScrollAnchor}; +use crate::{DisplayRow, EditorStyle, ToOffset, ToPoint, scroll::ScrollAnchor}; use gpui::{Pixels, WindowTextSystem}; use language::Point; use multi_buffer::{MultiBufferRow, MultiBufferSnapshot}; @@ -721,38 +721,6 @@ pub fn chars_before( }) } -pub(crate) fn is_inside_word(map: &DisplaySnapshot, point: DisplayPoint) -> bool { - let raw_point = point.to_point(map); - let classifier = map.buffer_snapshot.char_classifier_at(raw_point); - let ix = map.clip_point(point, Bias::Left).to_offset(map, Bias::Left); - let text = &map.buffer_snapshot; - let next_char_kind = text.chars_at(ix).next().map(|c| classifier.kind(c)); - let prev_char_kind = text - .reversed_chars_at(ix) - .next() - .map(|c| classifier.kind(c)); - prev_char_kind.zip(next_char_kind) == Some((CharKind::Word, CharKind::Word)) -} - -pub(crate) fn surrounding_word( - map: &DisplaySnapshot, - position: DisplayPoint, -) -> Range { - let position = map - .clip_point(position, Bias::Left) - .to_offset(map, Bias::Left); - let (range, _) = map.buffer_snapshot.surrounding_word(position, false); - let start = range - .start - .to_point(&map.buffer_snapshot) - .to_display_point(map); - let end = range - .end - .to_point(&map.buffer_snapshot) - .to_display_point(map); - start..end -} - /// Returns a list of lines (represented as a [`DisplayPoint`] range) contained /// within a passed range. /// @@ -1091,30 +1059,6 @@ mod tests { }); } - #[gpui::test] - fn test_surrounding_word(cx: &mut gpui::App) { - init_test(cx); - - fn assert(marked_text: &str, cx: &mut gpui::App) { - let (snapshot, display_points) = marked_display_snapshot(marked_text, cx); - assert_eq!( - surrounding_word(&snapshot, display_points[1]), - display_points[0]..display_points[2], - "{}", - marked_text - ); - } - - assert("ˇˇloremˇ ipsum", cx); - assert("ˇloˇremˇ ipsum", cx); - assert("ˇloremˇˇ ipsum", cx); - assert("loremˇ ˇ ˇipsum", cx); - assert("lorem\nˇˇˇ\nipsum", cx); - assert("lorem\nˇˇipsumˇ", cx); - assert("loremˇ,ˇˇ ipsum", cx); - assert("ˇloremˇˇ, ipsum", cx); - } - #[gpui::test] async fn test_move_up_and_down_with_excerpts(cx: &mut gpui::TestAppContext) { cx.update(|cx| { diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 955b17d523..e22fdb1ed5 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -4214,6 +4214,19 @@ impl MultiBufferSnapshot { self.diffs.values().any(|diff| !diff.is_empty()) } + pub fn is_inside_word(&self, position: T, for_completion: bool) -> bool { + let position = position.to_offset(self); + let classifier = self + .char_classifier_at(position) + .for_completion(for_completion); + let next_char_kind = self.chars_at(position).next().map(|c| classifier.kind(c)); + let prev_char_kind = self + .reversed_chars_at(position) + .next() + .map(|c| classifier.kind(c)); + prev_char_kind.zip(next_char_kind) == Some((CharKind::Word, CharKind::Word)) + } + pub fn surrounding_word( &self, start: T,