Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): add plaintext matching fallback to tree-sitter matching #4288

Merged
merged 15 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 112 additions & 9 deletions helix-core/src/match_brackets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ use tree_sitter::Node;

use crate::{Rope, Syntax};

const MAX_PLAINTEXT_SCAN: usize = 10000;

// Limit matching pairs to only ( ) { } [ ] < > ' ' " "
const PAIRS: &[(char, char)] = &[
('(', ')'),
('{', '}'),
Expand All @@ -11,15 +14,15 @@ const PAIRS: &[(char, char)] = &[
('\"', '\"'),
];

// limit matching pairs to only ( ) { } [ ] < > ' ' " "

// Returns the position of the matching bracket under cursor.
//
// If the cursor is one the opening bracket, the position of
// the closing bracket is returned. If the cursor in the closing
// bracket, the position of the opening bracket is returned.
//
// If the cursor is not on a bracket, `None` is returned.
/// Returns the position of the matching bracket under cursor.
///
/// If the cursor is on the opening bracket, the position of
/// the closing bracket is returned. If the cursor on the closing
/// bracket, the position of the opening bracket is returned.
///
/// If the cursor is not on a bracket, `None` is returned.
///
/// If no matching bracket is found, `None` is returned.
#[must_use]
pub fn find_matching_bracket(syntax: &Syntax, doc: &Rope, pos: usize) -> Option<usize> {
if pos >= doc.len_chars() || !is_valid_bracket(doc.char(pos)) {
Expand Down Expand Up @@ -70,10 +73,77 @@ fn find_pair(syntax: &Syntax, doc: &Rope, pos: usize, traverse_parents: bool) ->
}
}

/// Returns the position of the matching bracket under cursor, the search
/// is limited to the current line. This function works on plain text, it
/// ignores tree-sitter grammar.
///
/// If the cursor is on the opening bracket, the position of
/// the closing bracket is returned. If the cursor on the closing
/// bracket, the position of the opening bracket is returned.
///
/// If the cursor is not on a bracket, `None` is returned.
///
/// If no matching bracket is found, `None` is returned.
#[must_use]
pub fn find_matching_bracket_current_line_plaintext(
doc: &Rope,
cursor_pos: usize,
) -> Option<usize> {
// Don't do anything when the cursor is not on top of a bracket.
let bracket = doc.char(cursor_pos);
if !is_valid_bracket(bracket) {
return None;
}

// Determine the direction of the matching.
let is_fwd = is_forward_bracket(bracket);
let chars_iter = if is_fwd {
doc.chars_at(cursor_pos + 1)
} else {
doc.chars_at(cursor_pos).reversed()
};

let mut open_cnt = 1;

for (i, candidate) in chars_iter.take(MAX_PLAINTEXT_SCAN).enumerate() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing I'm noticing is that this doesn't limit the search to the current line only, as it advertises; it simply limits the scan to a max number of chars. I actually think this is fine, because limiting matching to the current line only seems unnecessarily restrictive to me, but we should probably update the function name and doc comment.

Copy link
Member

@pascalkuthe pascalkuthe Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it was changed to a fixed limit (from only matching on the current line) after feedback from @archseer and me. I missed that the doc comment wasn't updated good catch

Copy link
Contributor Author

@alevinval alevinval Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you folks use something to embed a constant inside a Doc? I wanted to do this:

diff --git a/helix-core/src/match_brackets.rs b/helix-core/src/match_brackets.rs
index 0ec040e5..07eb261a 100644
--- a/helix-core/src/match_brackets.rs
+++ b/helix-core/src/match_brackets.rs
@@ -73,9 +73,9 @@ fn find_pair(syntax: &Syntax, doc: &Rope, pos: usize, traverse_parents: bool) ->
     }
 }
 
-/// Returns the position of the matching bracket under cursor, the search
-/// is limited to the current line. This function works on plain text, it
-/// ignores tree-sitter grammar.
+/// Returns the position of the matching bracket under cursor.
+/// This function works on plain text and ignores tree-sitter grammar.
+/// The search is limited to [`MAX_PLAINTEXT_SCAN`] characters.
 ///
 /// If the cursor is on the opening bracket, the position of
 /// the closing bracket is returned. If the cursor on the closing

But that would require making the constant public for the docs to link to it correctly. If you are okay with that anyway (it works, but does not link) then I can push this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems fine to me 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove the angle brackets SL that cargo xtask docgen doesn't complain. The link doesn't matter much. The docs are not build anyway.

if candidate == bracket {
open_cnt += 1;
} else if is_valid_pair(
doc,
if is_fwd {
cursor_pos
} else {
cursor_pos - i - 1
},
if is_fwd {
cursor_pos + i + 1
} else {
cursor_pos
},
) {
// Return when all pending brackets have been closed.
if open_cnt == 1 {
return Some(if is_fwd {
cursor_pos + i + 1
} else {
cursor_pos - i - 1
});
}
open_cnt -= 1;
}
}

None
}

fn is_valid_bracket(c: char) -> bool {
PAIRS.iter().any(|(l, r)| *l == c || *r == c)
}

fn is_forward_bracket(c: char) -> bool {
PAIRS.iter().any(|(l, _)| *l == c)
}

fn is_valid_pair(doc: &Rope, start_char: usize, end_char: usize) -> bool {
PAIRS.contains(&(doc.char(start_char), doc.char(end_char)))
}
Expand All @@ -90,3 +160,36 @@ fn surrounding_bytes(doc: &Rope, node: &Node) -> Option<(usize, usize)> {

Some((start_byte, end_byte))
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_find_matching_bracket_current_line_plaintext() {
let assert = |input: &str, pos, expected| {
let input = &Rope::from(input);
let actual = find_matching_bracket_current_line_plaintext(input, pos);
assert_eq!(expected, actual.unwrap());

let actual = find_matching_bracket_current_line_plaintext(input, expected);
assert_eq!(pos, actual.unwrap(), "expected symmetrical behaviour");
};

assert("(hello)", 0, 6);
assert("((hello))", 0, 8);
assert("((hello))", 1, 7);
assert("(((hello)))", 2, 8);

assert("key: ${value}", 6, 12);
assert("key: ${value} # (some comment)", 16, 29);

assert("(paren (paren {bracket}))", 0, 24);
assert("(paren (paren {bracket}))", 7, 23);
assert("(paren (paren {bracket}))", 14, 22);

assert("(prev line\n ) (middle) ( \n next line)", 0, 12);
assert("(prev line\n ) (middle) ( \n next line)", 14, 21);
assert("(prev line\n ) (middle) ( \n next line)", 23, 36);
}
}
30 changes: 17 additions & 13 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4528,20 +4528,24 @@ fn select_prev_sibling(cx: &mut Context) {

fn match_brackets(cx: &mut Context) {
let (view, doc) = current!(cx.editor);
let is_select = cx.editor.mode == Mode::Select;
let text = doc.text();
let text_slice = text.slice(..);

if let Some(syntax) = doc.syntax() {
let text = doc.text().slice(..);
let selection = doc.selection(view.id).clone().transform(|range| {
if let Some(pos) =
match_brackets::find_matching_bracket_fuzzy(syntax, doc.text(), range.cursor(text))
{
range.put_cursor(text, pos, cx.editor.mode == Mode::Select)
} else {
range
}
});
doc.set_selection(view.id, selection);
}
let selection = doc.selection(view.id).clone().transform(|range| {
let pos = range.cursor(text_slice);
if let Some(matched_pos) = doc
.syntax()
.and_then(|syntax| match_brackets::find_matching_bracket_fuzzy(syntax, text, pos))
.or_else(|| match_brackets::find_matching_bracket_current_line_plaintext(text, pos))
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
{
range.put_cursor(text_slice, matched_pos, is_select)
} else {
range
}
});

doc.set_selection(view.id, selection);
}

//
Expand Down