Skip to content

Highlight matching brackets#1421

Merged
dzhou121 merged 18 commits intolapce:masterfrom
zarathir:match-brackets
Oct 31, 2022
Merged

Highlight matching brackets#1421
dzhou121 merged 18 commits intolapce:masterfrom
zarathir:match-brackets

Conversation

@zarathir
Copy link
Copy Markdown
Contributor

@zarathir zarathir commented Oct 1, 2022

This PR adds matching brackets highlighting to Lapce.

image

This is a WIP, but I got the basic functionality working so I thought I'd share this as a draft.
The basic idea is to check if a bracket is at the cursor, looking for the closing one and highlighting them.

I'm not quite happy with the search of the matching bracket, but am currently out of ideas/inspiration for a different implementation. Maybe someone has a better idea, especially how to handle multiple scopes.

Feel free to discuss and comment.

Closes #974

@dzhou121
Copy link
Copy Markdown
Collaborator

dzhou121 commented Oct 1, 2022

There should be some existing code for finding matching brackets that you can have a look/reuse.

@nheuillet
Copy link
Copy Markdown
Contributor

I'm not an expert, maybe @dzhou121 would know better than me, but I'm pretty sure Tree sitter can give us enough information for all scope (bracket, curly brackets, parenthesis, etc.)
All we'd have to do in theory would be to query the tree, then colorize

@dzhou121
Copy link
Copy Markdown
Collaborator

dzhou121 commented Oct 1, 2022

There should be some existing code for finding matching brackets that you can have a look/reuse.

e.g. here

https://github.com/lapce/lapce/blob/master/lapce-data/src/document.rs#L2515

@zarathir
Copy link
Copy Markdown
Contributor Author

zarathir commented Oct 2, 2022

Thanks for the pointer @dzhou121. I got it working.

matching_brackets.mp4

Should I add more pairs to match? Looks like Helix is also matching on <, >, ', ". I think matching on < and > would be nice, but am not sure about the other symbols.

@dzhou121
Copy link
Copy Markdown
Collaborator

dzhou121 commented Oct 2, 2022

Looking good! Thanks for your good work.

I think let's keep the current pairs for this PR, and maybe explore other pairs in future PRs?

@nheuillet
Copy link
Copy Markdown
Contributor

any chance this PR can be expanded to colorized matching pairs, and maybe scope line? I understand if you'd instead leave that for later, but I figured I might suggest it just in case

@zarathir
Copy link
Copy Markdown
Contributor Author

zarathir commented Oct 3, 2022

I think let's keep the current pairs for this PR, and maybe explore other pairs in future PRs?

Sure 👍.

any chance this PR can be expanded to colorized matching pairs, and maybe scope line? I understand if you'd instead leave that for later, but I figured I might suggest it just in case

I thought about this and I'll leave the PR as is and probably try implementing the colorized brackets/scopes next. It's going to take me some time to research how to best implement this and I think the bracket matching is already a nice to have feature. Hope that's okay.

@MinusGix MinusGix added C-feature Category: New feature or request A-ui Area: UI rendering and interactions labels Oct 3, 2022
@nheuillet
Copy link
Copy Markdown
Contributor

I thought about this and I'll leave the PR as is and probably try implementing the colorized brackets/scopes next. It's going to take me some time to research how to best implement this and I think the bracket matching is already a nice to have feature. Hope that's okay.

Fair enough! Bracket matching is already amazing. Can't wait to see the next steps :)

@zarathir zarathir marked this pull request as ready for review October 3, 2022 16:54
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 3, 2022

Codecov Report

Attention: Patch coverage is 26.38037% with 120 lines in your changes missing coverage. Please review.

Project coverage is 6.91%. Comparing base (6cc6fc0) to head (2d72735).
Report is 1205 commits behind head on master.

Files with missing lines Patch % Lines
lapce-ui/src/editor.rs 0.00% 81 Missing ⚠️
lapce-core/src/syntax/mod.rs 0.00% 21 Missing ⚠️
lapce-data/src/document.rs 0.00% 17 Missing ⚠️
lapce-core/src/word.rs 97.72% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #1421      +/-   ##
=========================================
+ Coverage    6.84%   6.91%   +0.06%     
=========================================
  Files         126     126              
  Lines       52705   52868     +163     
=========================================
+ Hits         3609    3654      +45     
- Misses      49096   49214     +118     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JustForFun88
Copy link
Copy Markdown
Contributor

Should I add more pairs to match? Looks like Helix is also matching on <, > ... I think matching on < and > would be nice ...

It would be very helpful!!! VSCode doesn't highlight them, and it's really painful for fairly complex types, even at the second nesting level. I don't want to use Helix either, because it doesn't show inferred types. Having both inferred types and <, > highlighting would be great!

@zarathir
Copy link
Copy Markdown
Contributor Author

zarathir commented Oct 4, 2022

It would be very helpful!!! VSCode doesn't highlight them, and it's really painful for fairly complex types, even at the second nesting level. I don't want to use Helix either, because it doesn't show inferred types. Having both inferred types and <, > highlighting would be great!

I opened an issue to keep track of stuff to add (#1439), because I think I'll try to get this merged first and keep track of the rest for later.

@zarathir zarathir marked this pull request as draft October 16, 2022 11:37
@zarathir zarathir marked this pull request as ready for review October 28, 2022 15:45
@zarathir zarathir requested a review from dzhou121 October 31, 2022 18:20
@dzhou121
Copy link
Copy Markdown
Collaborator

LGTM. Thanks for your amazing work!

@dzhou121 dzhou121 merged commit e51ffc9 into lapce:master Oct 31, 2022
@zarathir zarathir deleted the match-brackets branch November 11, 2022 17:47
@panekj panekj added this to the v0.2.2 milestone Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ui Area: UI rendering and interactions C-feature Category: New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Highlight Matching Braces

9 participants