Update Editor::select_larger_syntax_node (#36971)

Paul Sadauskas created 2 months ago

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:

<img width="217" height="111" alt="image"
src="https://github.com/user-attachments/assets/00de53a8-8776-47aa-8101-5a5b5bc3fa5e"
/>

I leveraged the existing `generate_marked_text` helper function and
updated the assertion to output a diff of the text with the selection
markers:

<img width="211" height="116" alt="image"
src="https://github.com/user-attachments/assets/53b2b882-2676-4c70-8718-e2e2ba6f254e"
/>

Happy to make that a separate PR, if needed.

Release Notes:

- Fixed Editor select_larger_syntax_node to be smart about punctuation.

Change summary

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(-)

Detailed changes

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,

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::<crate::EditorEvent>(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, |_| {});

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<Editor>,
 ) {
-    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

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