Merge pull request #2176 from zed-industries/better-move-to-brackets

Make jump to matching bracket action more predictable

crates/editor/src/editor.rs

@@ -4754,27 +4754,52 @@ impl Editor {
         _: &MoveToEnclosingBracket,
         cx: &mut ViewContext<Self>,
     ) {
-        let buffer = self.buffer.read(cx).snapshot(cx);
-        let mut selections = self.selections.all::<usize>(cx);
-        for selection in &mut selections {
-            if let Some((open_range, close_range)) =
-                buffer.enclosing_bracket_ranges(selection.start..selection.end)
-            {
-                let close_range = close_range.to_inclusive();
-                let destination = if close_range.contains(&selection.start)
-                    && close_range.contains(&selection.end)
-                {
-                    open_range.end
-                } else {
-                    *close_range.start()
-                };
-                selection.start = destination;
-                selection.end = destination;
-            }
-        }
-
         self.change_selections(Some(Autoscroll::fit()), cx, |s| {
-            s.select(selections);
+            s.move_offsets_with(|snapshot, selection| {
+                let Some(enclosing_bracket_ranges) = snapshot.enclosing_bracket_ranges(selection.start..selection.end) else { return; };
+                
+                let mut best_length = usize::MAX;
+                let mut best_inside = false;
+                let mut best_in_bracket_range = false;
+                let mut best_destination = None;
+                for (open, close) in enclosing_bracket_ranges {
+                    let close = close.to_inclusive();
+                    let length = close.end() - open.start;
+                    let inside = selection.start >= open.end && selection.end <= *close.start();
+                    let in_bracket_range = open.to_inclusive().contains(&selection.head()) || close.contains(&selection.head());
+                    
+                    // If best is next to a bracket and current isn't, skip
+                    if !in_bracket_range && best_in_bracket_range {
+                        continue;
+                    }
+                    
+                    // Prefer smaller lengths unless best is inside and current isn't
+                    if length > best_length && (best_inside || !inside) {
+                        continue;
+                    }
+                    
+                    best_length = length;
+                    best_inside = inside;
+                    best_in_bracket_range = in_bracket_range;
+                    best_destination = Some(if close.contains(&selection.start) && close.contains(&selection.end) {
+                        if inside {
+                            open.end
+                        } else {
+                            open.start
+                        }
+                    } else {
+                        if inside {
+                            *close.start()
+                        } else {
+                            *close.end()
+                        }
+                    });
+                }
+                
+                if let Some(destination) = best_destination {
+                    selection.collapse_to(destination, SelectionGoal::None);
+                }
+            })
         });
     }
 

crates/editor/src/editor_tests.rs

@@ -5528,6 +5528,54 @@ fn test_split_words() {
     assert_eq!(split("helloworld"), &["helloworld"]);
 }
 
+#[gpui::test]
+async fn test_move_to_enclosing_bracket(cx: &mut gpui::TestAppContext) {
+    let mut cx = EditorLspTestContext::new_typescript(Default::default(), cx).await;
+    let mut assert = |before, after| {
+        let _state_context = cx.set_state(before);
+        cx.update_editor(|editor, cx| {
+            editor.move_to_enclosing_bracket(&MoveToEnclosingBracket, cx)
+        });
+        cx.assert_editor_state(after);
+    };
+
+    // Outside bracket jumps to outside of matching bracket
+    assert("console.logˇ(var);", "console.log(var)ˇ;");
+    assert("console.log(var)ˇ;", "console.logˇ(var);");
+
+    // Inside bracket jumps to inside of matching bracket
+    assert("console.log(ˇvar);", "console.log(varˇ);");
+    assert("console.log(varˇ);", "console.log(ˇvar);");
+
+    // When outside a bracket and inside, favor jumping to the inside bracket
+    assert(
+        "console.log('foo', [1, 2, 3]ˇ);",
+        "console.log(ˇ'foo', [1, 2, 3]);",
+    );
+    assert(
+        "console.log(ˇ'foo', [1, 2, 3]);",
+        "console.log('foo', [1, 2, 3]ˇ);",
+    );
+
+    // Bias forward if two options are equally likely
+    assert(
+        "let result = curried_fun()ˇ();",
+        "let result = curried_fun()()ˇ;",
+    );
+
+    // If directly adjacent to a smaller pair but inside a larger (not adjacent), pick the smaller
+    assert(
+        indoc! {"
+            function test() {
+                console.log('test')ˇ
+            }"},
+        indoc! {"
+            function test() {
+                console.logˇ('test')
+            }"},
+    );
+}
+
 fn empty_range(row: usize, column: usize) -> Range<DisplayPoint> {
     let point = DisplayPoint::new(row as u32, column as u32);
     point..point

crates/editor/src/highlight_matching_bracket.rs

@@ -17,7 +17,7 @@ pub fn refresh_matching_bracket_highlights(editor: &mut Editor, cx: &mut ViewCon
     let snapshot = editor.snapshot(cx);
     if let Some((opening_range, closing_range)) = snapshot
         .buffer_snapshot
-        .enclosing_bracket_ranges(head..head)
+        .innermost_enclosing_bracket_ranges(head..head)
     {
         editor.highlight_background::<MatchingBracketHighlight>(
             vec![

crates/editor/src/multi_buffer.rs

@@ -2621,56 +2621,72 @@ impl MultiBufferSnapshot {
         self.parse_count
     }
 
-    pub fn enclosing_bracket_ranges<T: ToOffset>(
+    pub fn innermost_enclosing_bracket_ranges<T: ToOffset>(
         &self,
         range: Range<T>,
     ) -> Option<(Range<usize>, Range<usize>)> {
         let range = range.start.to_offset(self)..range.end.to_offset(self);
 
-        let mut cursor = self.excerpts.cursor::<usize>();
-        cursor.seek(&range.start, Bias::Right, &());
-        let start_excerpt = cursor.item();
+        // Get the ranges of the innermost pair of brackets.
+        let mut result: Option<(Range<usize>, Range<usize>)> = None;
 
-        cursor.seek(&range.end, Bias::Right, &());
-        let end_excerpt = cursor.item();
+        let Some(enclosing_bracket_ranges) = self.enclosing_bracket_ranges(range.clone()) else { return None; };
 
-        start_excerpt
-            .zip(end_excerpt)
-            .and_then(|(start_excerpt, end_excerpt)| {
-                if start_excerpt.id != end_excerpt.id {
-                    return None;
+        for (open, close) in enclosing_bracket_ranges {
+            let len = close.end - open.start;
+
+            if let Some((existing_open, existing_close)) = &result {
+                let existing_len = existing_close.end - existing_open.start;
+                if len > existing_len {
+                    continue;
                 }
+            }
 
-                let excerpt_buffer_start = start_excerpt
-                    .range
-                    .context
-                    .start
-                    .to_offset(&start_excerpt.buffer);
-                let excerpt_buffer_end = excerpt_buffer_start + start_excerpt.text_summary.len;
+            result = Some((open, close));
+        }
+
+        result
+    }
+
+    /// Returns enclosinng bracket ranges containing the given range or returns None if the range is
+    /// not contained in a single excerpt
+    pub fn enclosing_bracket_ranges<'a, T: ToOffset>(
+        &'a self,
+        range: Range<T>,
+    ) -> Option<impl Iterator<Item = (Range<usize>, Range<usize>)> + 'a> {
+        let range = range.start.to_offset(self)..range.end.to_offset(self);
+        self.excerpt_containing(range.clone())
+            .map(|(excerpt, excerpt_offset)| {
+                let excerpt_buffer_start = excerpt.range.context.start.to_offset(&excerpt.buffer);
+                let excerpt_buffer_end = excerpt_buffer_start + excerpt.text_summary.len;
 
                 let start_in_buffer =
-                    excerpt_buffer_start + range.start.saturating_sub(*cursor.start());
-                let end_in_buffer =
-                    excerpt_buffer_start + range.end.saturating_sub(*cursor.start());
-                let (mut start_bracket_range, mut end_bracket_range) = start_excerpt
-                    .buffer
-                    .enclosing_bracket_ranges(start_in_buffer..end_in_buffer)?;
+                    excerpt_buffer_start + range.start.saturating_sub(excerpt_offset);
+                let end_in_buffer = excerpt_buffer_start + range.end.saturating_sub(excerpt_offset);
 
-                if start_bracket_range.start >= excerpt_buffer_start
-                    && end_bracket_range.end <= excerpt_buffer_end
-                {
-                    start_bracket_range.start =
-                        cursor.start() + (start_bracket_range.start - excerpt_buffer_start);
-                    start_bracket_range.end =
-                        cursor.start() + (start_bracket_range.end - excerpt_buffer_start);
-                    end_bracket_range.start =
-                        cursor.start() + (end_bracket_range.start - excerpt_buffer_start);
-                    end_bracket_range.end =
-                        cursor.start() + (end_bracket_range.end - excerpt_buffer_start);
-                    Some((start_bracket_range, end_bracket_range))
-                } else {
-                    None
-                }
+                excerpt
+                    .buffer
+                    .enclosing_bracket_ranges(start_in_buffer..end_in_buffer)
+                    .filter_map(move |(start_bracket_range, end_bracket_range)| {
+                        if start_bracket_range.start < excerpt_buffer_start
+                            || end_bracket_range.end > excerpt_buffer_end
+                        {
+                            return None;
+                        }
+
+                        let mut start_bracket_range = start_bracket_range.clone();
+                        start_bracket_range.start =
+                            excerpt_offset + (start_bracket_range.start - excerpt_buffer_start);
+                        start_bracket_range.end =
+                            excerpt_offset + (start_bracket_range.end - excerpt_buffer_start);
+
+                        let mut end_bracket_range = end_bracket_range.clone();
+                        end_bracket_range.start =
+                            excerpt_offset + (end_bracket_range.start - excerpt_buffer_start);
+                        end_bracket_range.end =
+                            excerpt_offset + (end_bracket_range.end - excerpt_buffer_start);
+                        Some((start_bracket_range, end_bracket_range))
+                    })
             })
     }
 
@@ -2812,40 +2828,23 @@ impl MultiBufferSnapshot {
     pub fn range_for_syntax_ancestor<T: ToOffset>(&self, range: Range<T>) -> Option<Range<usize>> {
         let range = range.start.to_offset(self)..range.end.to_offset(self);
 
-        let mut cursor = self.excerpts.cursor::<usize>();
-        cursor.seek(&range.start, Bias::Right, &());
-        let start_excerpt = cursor.item();
-
-        cursor.seek(&range.end, Bias::Right, &());
-        let end_excerpt = cursor.item();
-
-        start_excerpt
-            .zip(end_excerpt)
-            .and_then(|(start_excerpt, end_excerpt)| {
-                if start_excerpt.id != end_excerpt.id {
-                    return None;
-                }
-
-                let excerpt_buffer_start = start_excerpt
-                    .range
-                    .context
-                    .start
-                    .to_offset(&start_excerpt.buffer);
-                let excerpt_buffer_end = excerpt_buffer_start + start_excerpt.text_summary.len;
+        self.excerpt_containing(range.clone())
+            .and_then(|(excerpt, excerpt_offset)| {
+                let excerpt_buffer_start = excerpt.range.context.start.to_offset(&excerpt.buffer);
+                let excerpt_buffer_end = excerpt_buffer_start + excerpt.text_summary.len;
 
                 let start_in_buffer =
-                    excerpt_buffer_start + range.start.saturating_sub(*cursor.start());
-                let end_in_buffer =
-                    excerpt_buffer_start + range.end.saturating_sub(*cursor.start());
-                let mut ancestor_buffer_range = start_excerpt
+                    excerpt_buffer_start + range.start.saturating_sub(excerpt_offset);
+                let end_in_buffer = excerpt_buffer_start + range.end.saturating_sub(excerpt_offset);
+                let mut ancestor_buffer_range = excerpt
                     .buffer
                     .range_for_syntax_ancestor(start_in_buffer..end_in_buffer)?;
                 ancestor_buffer_range.start =
                     cmp::max(ancestor_buffer_range.start, excerpt_buffer_start);
                 ancestor_buffer_range.end = cmp::min(ancestor_buffer_range.end, excerpt_buffer_end);
 
-                let start = cursor.start() + (ancestor_buffer_range.start - excerpt_buffer_start);
-                let end = cursor.start() + (ancestor_buffer_range.end - excerpt_buffer_start);
+                let start = excerpt_offset + (ancestor_buffer_range.start - excerpt_buffer_start);
+                let end = excerpt_offset + (ancestor_buffer_range.end - excerpt_buffer_start);
                 Some(start..end)
             })
     }
@@ -2929,6 +2928,31 @@ impl MultiBufferSnapshot {
         None
     }
 
+    /// Returns the excerpt containing range and its offset start within the multibuffer or none if `range` spans multiple excerpts
+    fn excerpt_containing<'a, T: ToOffset>(
+        &'a self,
+        range: Range<T>,
+    ) -> Option<(&'a Excerpt, usize)> {
+        let range = range.start.to_offset(self)..range.end.to_offset(self);
+
+        let mut cursor = self.excerpts.cursor::<usize>();
+        cursor.seek(&range.start, Bias::Right, &());
+        let start_excerpt = cursor.item();
+
+        cursor.seek(&range.end, Bias::Right, &());
+        let end_excerpt = cursor.item();
+
+        start_excerpt
+            .zip(end_excerpt)
+            .and_then(|(start_excerpt, end_excerpt)| {
+                if start_excerpt.id != end_excerpt.id {
+                    return None;
+                }
+
+                Some((start_excerpt, *cursor.start()))
+            })
+    }
+
     pub fn remote_selections_in_range<'a>(
         &'a self,
         range: &'a Range<Anchor>,

crates/editor/src/selections_collection.rs

@@ -659,6 +659,31 @@ impl<'a> MutableSelectionsCollection<'a> {
         }
     }
 
+    pub fn move_offsets_with(
+        &mut self,
+        mut move_selection: impl FnMut(&MultiBufferSnapshot, &mut Selection<usize>),
+    ) {
+        let mut changed = false;
+        let snapshot = self.buffer().clone();
+        let selections = self
+            .all::<usize>(self.cx)
+            .into_iter()
+            .map(|selection| {
+                let mut moved_selection = selection.clone();
+                move_selection(&snapshot, &mut moved_selection);
+                if selection != moved_selection {
+                    changed = true;
+                }
+                moved_selection
+            })
+            .collect();
+        drop(snapshot);
+
+        if changed {
+            self.select(selections)
+        }
+    }
+
     pub fn move_heads_with(
         &mut self,
         mut update_head: impl FnMut(

crates/editor/src/test/editor_lsp_test_context.rs

@@ -1,4 +1,5 @@
 use std::{
+    borrow::Cow,
     ops::{Deref, DerefMut, Range},
     sync::Arc,
 };
@@ -7,7 +8,8 @@ use anyhow::Result;
 
 use futures::Future;
 use gpui::{json, ViewContext, ViewHandle};
-use language::{point_to_lsp, FakeLspAdapter, Language, LanguageConfig};
+use indoc::indoc;
+use language::{point_to_lsp, FakeLspAdapter, Language, LanguageConfig, LanguageQueries};
 use lsp::{notification, request};
 use project::Project;
 use smol::stream::StreamExt;
@@ -125,6 +127,32 @@ impl<'a> EditorLspTestContext<'a> {
         Self::new(language, capabilities, cx).await
     }
 
+    pub async fn new_typescript(
+        capabilities: lsp::ServerCapabilities,
+        cx: &'a mut gpui::TestAppContext,
+    ) -> EditorLspTestContext<'a> {
+        let language = Language::new(
+            LanguageConfig {
+                name: "Typescript".into(),
+                path_suffixes: vec!["ts".to_string()],
+                ..Default::default()
+            },
+            Some(tree_sitter_typescript::language_typescript()),
+        )
+        .with_queries(LanguageQueries {
+            brackets: Some(Cow::from(indoc! {r#"
+                ("(" @open ")" @close)
+                ("[" @open "]" @close)
+                ("{" @open "}" @close)
+                ("<" @open ">" @close)
+                ("\"" @open "\"" @close)"#})),
+            ..Default::default()
+        })
+        .expect("Could not parse brackets");
+
+        Self::new(language, capabilities, cx).await
+    }
+
     // Constructs lsp range using a marked string with '[', ']' range delimiters
     pub fn lsp_range(&mut self, marked_text: &str) -> lsp::Range {
         let ranges = self.ranges(marked_text);

crates/editor/src/test/editor_test_context.rs

@@ -162,10 +162,13 @@ impl<'a> EditorTestContext<'a> {
     /// embedded range markers that represent the ranges and directions of
     /// each selection.
     ///
+    /// Returns a context handle so that assertion failures can print what
+    /// editor state was needed to cause the failure.
+    ///
     /// See the `util::test::marked_text_ranges` function for more information.
     pub fn set_state(&mut self, marked_text: &str) -> ContextHandle {
         let _state_context = self.add_assertion_context(format!(
-            "Editor State: \"{}\"",
+            "Initial Editor State: \"{}\"",
             marked_text.escape_debug().to_string()
         ));
         let (unmarked_text, selection_ranges) = marked_text_ranges(marked_text, true);