From b2d7e34e80d8a7c06db7b399f7110b11cf097bc7 Mon Sep 17 00:00:00 2001 From: Paul Sadauskas Date: Tue, 9 Sep 2025 12:13:20 -0600 Subject: [PATCH] Update Editor::select_larger_syntax_node (#36971) When the cursor was sitting on a syntactically insignificant character, like a `{` or `,`, this function was selecting only that character, when what the user likely wanted was to select the next larger syntax node. Those punctuation characters all seemed to be not "named", in tree-sitter terminology, so I updated the function to walk up the node tree until it found a node where `is_named()` is true. Closes #4555 Also, while writing the tests, the output of a failing test with the wrong thing selected was harder to read than it needed to be. It used to output a diff of ranges, like this: image I leveraged the existing `generate_marked_text` helper function and updated the assertion to output a diff of the text with the selection markers: image Happy to make that a separate PR, if needed. Release Notes: - Fixed Editor select_larger_syntax_node to be smart about punctuation. --- crates/editor/src/editor.rs | 8 +- crates/editor/src/editor_tests.rs | 186 +++++++++++++++++++++++++++- crates/editor/src/test.rs | 13 +- crates/util/src/test/marked_text.rs | 11 +- 4 files changed, 204 insertions(+), 14 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index b54c2c11b1b0553328e4a1a27f380198b20ce55f..2991c47856a06098bbff0e56d67922a765d35efe 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -14946,9 +14946,13 @@ impl Editor { } let mut new_range = old_range.clone(); - while let Some((_node, containing_range)) = - buffer.syntax_ancestor(new_range.clone()) + while let Some((node, containing_range)) = buffer.syntax_ancestor(new_range.clone()) { + if !node.is_named() { + new_range = node.start_byte()..node.end_byte(); + continue; + } + new_range = match containing_range { MultiOrSingleBufferOffsetRange::Single(_) => break, MultiOrSingleBufferOffsetRange::Multi(range) => range, diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 9f578b8b9df374d44c9e9b58e9522489dca66cef..567018baaf9e717c2d968644aabb6a594478d572 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -8591,10 +8591,10 @@ async fn test_select_larger_smaller_syntax_node(cx: &mut TestAppContext) { assert_text_with_selections( editor, indoc! {r#" - use mod1::mod2::{mod3, mo«ˇ»d4}; + use mod1::mod2::{mod3, moˇd4}; fn fn_1(para«ˇm1: bool, pa»ram2: &str) { - let var1 = "te«ˇ»xt"; + let var1 = "teˇxt"; } "#}, cx, @@ -8609,10 +8609,10 @@ async fn test_select_larger_smaller_syntax_node(cx: &mut TestAppContext) { assert_text_with_selections( editor, indoc! {r#" - use mod1::mod2::{mod3, mo«ˇ»d4}; + use mod1::mod2::{mod3, moˇd4}; fn fn_1(para«ˇm1: bool, pa»ram2: &str) { - let var1 = "te«ˇ»xt"; + let var1 = "teˇxt"; } "#}, cx, @@ -8716,6 +8716,184 @@ async fn test_select_larger_syntax_node_for_cursor_at_end(cx: &mut TestAppContex }); } +#[gpui::test] +async fn test_select_larger_syntax_node_for_cursor_at_symbol(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + + let language = Arc::new(Language::new( + LanguageConfig { + name: "JavaScript".into(), + ..Default::default() + }, + Some(tree_sitter_typescript::LANGUAGE_TSX.into()), + )); + + let text = r#" + let a = { + key: "value", + }; + "# + .unindent(); + + let buffer = cx.new(|cx| Buffer::local(text, cx).with_language(language, cx)); + let buffer = cx.new(|cx| MultiBuffer::singleton(buffer, cx)); + let (editor, cx) = cx.add_window_view(|window, cx| build_editor(buffer, window, cx)); + + editor + .condition::(cx, |editor, cx| !editor.buffer.read(cx).is_parsing(cx)) + .await; + + // Test case 1: Cursor after '{' + editor.update_in(cx, |editor, window, cx| { + editor.change_selections(SelectionEffects::no_scroll(), window, cx, |s| { + s.select_display_ranges([ + DisplayPoint::new(DisplayRow(0), 9)..DisplayPoint::new(DisplayRow(0), 9) + ]); + }); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + let a = {ˇ + key: "value", + }; + "#}, + cx, + ); + }); + editor.update_in(cx, |editor, window, cx| { + editor.select_larger_syntax_node(&SelectLargerSyntaxNode, window, cx); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + let a = «ˇ{ + key: "value", + }»; + "#}, + cx, + ); + }); + + // Test case 2: Cursor after ':' + editor.update_in(cx, |editor, window, cx| { + editor.change_selections(SelectionEffects::no_scroll(), window, cx, |s| { + s.select_display_ranges([ + DisplayPoint::new(DisplayRow(1), 8)..DisplayPoint::new(DisplayRow(1), 8) + ]); + }); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + let a = { + key:ˇ "value", + }; + "#}, + cx, + ); + }); + editor.update_in(cx, |editor, window, cx| { + editor.select_larger_syntax_node(&SelectLargerSyntaxNode, window, cx); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + let a = { + «ˇkey: "value"», + }; + "#}, + cx, + ); + }); + editor.update_in(cx, |editor, window, cx| { + editor.select_larger_syntax_node(&SelectLargerSyntaxNode, window, cx); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + let a = «ˇ{ + key: "value", + }»; + "#}, + cx, + ); + }); + + // Test case 3: Cursor after ',' + editor.update_in(cx, |editor, window, cx| { + editor.change_selections(SelectionEffects::no_scroll(), window, cx, |s| { + s.select_display_ranges([ + DisplayPoint::new(DisplayRow(1), 17)..DisplayPoint::new(DisplayRow(1), 17) + ]); + }); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + let a = { + key: "value",ˇ + }; + "#}, + cx, + ); + }); + editor.update_in(cx, |editor, window, cx| { + editor.select_larger_syntax_node(&SelectLargerSyntaxNode, window, cx); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + let a = «ˇ{ + key: "value", + }»; + "#}, + cx, + ); + }); + + // Test case 4: Cursor after ';' + editor.update_in(cx, |editor, window, cx| { + editor.change_selections(SelectionEffects::no_scroll(), window, cx, |s| { + s.select_display_ranges([ + DisplayPoint::new(DisplayRow(2), 2)..DisplayPoint::new(DisplayRow(2), 2) + ]); + }); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + let a = { + key: "value", + };ˇ + "#}, + cx, + ); + }); + editor.update_in(cx, |editor, window, cx| { + editor.select_larger_syntax_node(&SelectLargerSyntaxNode, window, cx); + }); + editor.update(cx, |editor, cx| { + assert_text_with_selections( + editor, + indoc! {r#" + «ˇlet a = { + key: "value", + }; + »"#}, + cx, + ); + }); +} + #[gpui::test] async fn test_select_larger_smaller_syntax_node_for_string(cx: &mut TestAppContext) { init_test(cx, |_| {}); diff --git a/crates/editor/src/test.rs b/crates/editor/src/test.rs index 960fecf59a8ae80d168f5ab82c74f32dfa7d4745..03e99b9fff9a89fcac28605fe6bf7a08b23f8f02 100644 --- a/crates/editor/src/test.rs +++ b/crates/editor/src/test.rs @@ -20,7 +20,7 @@ use multi_buffer::ToPoint; use pretty_assertions::assert_eq; use project::{Project, project_settings::DiagnosticSeverity}; use ui::{App, BorrowAppContext, px}; -use util::test::{marked_text_offsets, marked_text_ranges}; +use util::test::{generate_marked_text, marked_text_offsets, marked_text_ranges}; #[cfg(test)] #[ctor::ctor] @@ -104,13 +104,14 @@ pub fn assert_text_with_selections( marked_text: &str, cx: &mut Context, ) { - let (unmarked_text, text_ranges) = marked_text_ranges(marked_text, true); + let (unmarked_text, _text_ranges) = marked_text_ranges(marked_text, true); assert_eq!(editor.text(cx), unmarked_text, "text doesn't match"); - assert_eq!( - editor.selections.ranges(cx), - text_ranges, - "selections don't match", + let actual = generate_marked_text( + &editor.text(cx), + &editor.selections.ranges(cx), + marked_text.contains("«"), ); + assert_eq!(actual, marked_text, "Selections don't match"); } // RA thinks this is dead code even though it is used in a whole lot of tests diff --git a/crates/util/src/test/marked_text.rs b/crates/util/src/test/marked_text.rs index d885e5920fc2e44efed354d1d0bc24bf971cff51..282a4779350f9d7a0266a06bd840e934a474e2d2 100644 --- a/crates/util/src/test/marked_text.rs +++ b/crates/util/src/test/marked_text.rs @@ -214,8 +214,15 @@ pub fn generate_marked_text( } } } else { - marked_text.insert(range.end, '»'); - marked_text.insert(range.start, '«'); + match range.start.cmp(&range.end) { + Ordering::Equal => { + marked_text.insert(range.start, 'ˇ'); + } + _ => { + marked_text.insert(range.end, '»'); + marked_text.insert(range.start, '«'); + } + } } } marked_text