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

Fix behaviour of matching when tree-sitter grammar is available #7238

Closed
wants to merge 2 commits into from

Conversation

alevinval
Copy link
Contributor

@alevinval alevinval commented Jun 5, 2023

If the current character is a valid bracket, resort to plaintext matching. Otherwise, use regular tree-sitter node traversal.
This is a pragmatic solution that I propose, as addressing the tree-sitter grammars seems way more complex and tedious.
Why is believe this is acceptable? If the user is not on top of a valid bracket, regular operation will take place. It simply deals with the edge cases where the user is on top of a bracket, which brings the unexpected behaviour that users complain about (see #3357, #5177)

If the current character is a valid bracket, resort to plaintext matching.
Otherwise, use regular tree-sitter node traversal.
doc: &Rope,
cursor_pos: usize,
) -> Option<usize> {
pub fn find_matching_bracket_plaintext(doc: &Rope, cursor_pos: usize) -> Option<usize> {
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.

😓 even after having so much looks into it, and fixing the docs, I missed that the function name was left saying it worked on "current_line". So I'm addressing it here, removing mention of only working on the current line.

… we are

not on top of the boundary of the node *and* we have a valid pair in the node
start/ending bytes.
@alevinval
Copy link
Contributor Author

alevinval commented Jun 5, 2023

The original proposed solution is quite simple, just look at the first commit: d0eae33

It could be argued that this simple solution is wasteful in case that we are on top of a valid bracket, and we are right on the boundary of the node, and this node boundaries are valid bracket pairs. Under such case, we would be resorting to plaintext matching, when we could achieve the matching much faster by looking at the node positions. So... I have pushed a commit that would address those concerns: 1fb75bb

Either option works, and I have no strong feelings reverting the second commit, if you think the optimisation is an overkill.

@pascalkuthe
Copy link
Member

This is actually the opposite direction of what I was looking to do. TS is generally preferable to text matching because it handles edge cases much better. I want to move miX and maX to TS too, see #6893 (review). The TS matching just needed some improvements. I had these in mind but never got around to it but I nave now implemented them in #7242. That PR also contains an (intentionally) very limited plaintext fallgack to handle... Mostly horrible plaintext macros #3357 (comment)... I might undo that tough we could handle those cases by injecting C into C preprocessor tokens the way nvim does

@alevinval
Copy link
Contributor Author

@pascalkuthe oh, and you have it implemented already. Let me close this one, yours is looking much better and keeping in on TS.

@alevinval alevinval closed this Jun 6, 2023
@alevinval alevinval deleted the issue-3357 branch June 6, 2023 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants