-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat: Determine comment tokens from injection layers #12759
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
base: master
Are you sure you want to change the base?
feat: Determine comment tokens from injection layers #12759
Conversation
63e26be to
3281c81
Compare
019fe67 to
c585fca
Compare
c160b1e to
dd5d4e8
Compare
1afd4c5 to
8f9fcc1
Compare
|
Thank you for this PR! I really hope it lands ASAP and its gonna be finally truly possible to work with It uses incorrect comment tokens if there are whitespaces in beginning or end? Not sure. css.example.mp4when.line.wraps.incorrect.tokens.mp4 |
Thanks for checking out the PR! When you select the full line like that, what actually happens is that your line has 2 injections in it: the CSS (inner) and the HTML (outer) That's because the CSS injection doesnt start at the beginning of the line. It starts after a few spaces (Its weird, but it isnt a bug) Here, the PR chooses comment tokens based upon the most tightly encompassing injection range. Because the CSS injection isnt fully encompassing the selection, it chooses the HTML comment tokens because that layer is fully encompassing |
|
You can verify this with |
Yeah, I checked that, indeed. or maybe they comment correctly with the help of their LSP which is more sophisticated? (Im just guessing, I have no idea) yo.mp4 |
Co-authored-by: the-mikedavis <[email protected]>
|
Update Now that helix-editor/tree-house#9 is merged (required for this PR) and the PR has been rebased on the latest master (which notably includes the tree-house changes) Once tree-house 0.1.1 releases which will include helix-editor/tree-house#9 I'll update the PR to use tree-house 0.1.1 It should then be ready for merge Edit: it is now ready |
This would fit in a different PR.
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn test_join_selections_comment() -> anyhow::Result<()> { | ||
| test(( | ||
| indoc! {"\ | ||
| /// #[a|]#bc | ||
| /// def | ||
| "}, | ||
| ":lang rust<ret>J", | ||
| indoc! {"\ | ||
| /// #[a|]#bc def | ||
| "}, | ||
| )) | ||
| .await?; | ||
|
|
||
| // Only join if the comment token matches the previous line. | ||
| test(( | ||
| indoc! {"\ | ||
| #[| // a | ||
| // b | ||
| /// c | ||
| /// d | ||
| e | ||
| /// f | ||
| // g]# | ||
| "}, | ||
| ":lang rust<ret>J", | ||
| indoc! {"\ | ||
| #[| // a b /// c d e f // g]# | ||
| "}, | ||
| )) | ||
| .await?; | ||
|
|
||
| test(( | ||
| "#[|\t// Join comments | ||
| \t// with indent]#", | ||
| ":lang go<ret>J", | ||
| "#[|\t// Join comments with indent]#", | ||
| )) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: these tests were moved into comment.rs
781a3fe to
76687f5
Compare
Co-authored-by: Michael Davis <[email protected]>
Co-authored-by: Michael Davis <[email protected]>
Co-authored-by: Michael Davis <[email protected]>
93f5d9c to
aca1b0e
Compare
Co-authored-by: Michael Davis <[email protected]>
|
Oh, thank you so much for this feature! It's been my biggest pain when working with Vue projects in Helix. I'm not familiar with Rust or the helix codebase to help with a review, but I've tested your branch on some Vue files and it works as expected. I found one weird issue with one markdown file, however, I couldn't reproduce it in any way, it's affecting one README.md file I've got in a project (I cannot even record the demo as the contents are private). When I copy this file or a chunk of it to another directory, the issue disappears 😅 |
|
Ok, I anonymized the README and managed to reproduce the issue in an empty folder with just this one file. Here's the demo: So, on a first opening of the file, I quickly search for the heading of "Development server" and navigate towards a bash script injection. The comment command EDIT: I see that on the screencast, the final bit lagged a little, but I simply navigated to the injection and the commenting worked as expected immediately. On the second run, I navigate to the section by simply scrolling through the file. The comment inside the bash injection is applied as expected this time. I wonder if this is not an issue with the tree sitter rather than your addition. |
Hi @souserge, this seems to be the same as: #12759 (comment) It's technically not a bug because the injection for the bash file doesn't include the newline at the end of the markdown file. So, the newline character's injection is markdown but the character right before that is bash. Since these 2 characters are on the same line, the behaviour seems off What could be done to help with this if modify the markdown grammar so that it considers the final newline a part of the code block |
the-mikedavis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also an opportunity to handle nested comments like in
Line 166 in 1941f0b
| /// // 14 is the byte index of the pirate flag's starting cluster boundary. |
Ideally we should add the /// for the outer doc comment and another // after a space for the inner comment. This can probably be a follow-up though
| added_chars: &mut usize, | ||
| removed_chars: &mut usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pass these in C++ style? Returning these values in a tuple seems more natural
| command_line, comment, | ||
| command_line, | ||
| comment::{self}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be reset as it's the same import
| let continue_comment_token = continue_comment_tokens | ||
| .and_then(|tokens| comment::get_comment_token(text, tokens, current_line)); | ||
| let continue_comment_token = comment::get_line_comment_token( | ||
| &cx.editor.syn_loader.load(), | ||
| syntax, | ||
| text, | ||
| doc_default_comment_token, | ||
| current_line, | ||
| ) | ||
| .filter(|_| config.continue_comments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I mentioned this above but I can't find the comment. We should avoid doing the work in comment::get_line_comment_token when we know that config.continue_comments is false rather than filtering afterwards. I think we can also skip this work when doc_default_comment_token is None, so the pattern from the old code to treat both as None would make more sense
| doc_line_token: Option<&str>, | ||
| doc_block_tokens: Option<&[BlockCommentToken]>, | ||
| syntax: Option<&Syntax>, | ||
| loader: &syntax::Loader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With #12275 I think this became ambiguous. Here we can use helix_core::syntax::Loader instead and remove the use of helix_core::syntax::self
|
I think Astro can be added to the list of languages. |
|
@smellydelli AFAIK this feature works with tree-sitter injections, there's no list of languages for which it's enabled, it simply checks whether the selection to be commented is inside an injection. |
|
This changes everything |
|
I did not review your code yet, but I have a question maybe not related at all : now there is the new match feature (inside/around) "x" (html tags) and it's not working in .svelte files. Do you think is it related to? |
Previously, if you had a file like this:
Pressing
Space + c(toggle comment) on the JavaScript comment would've used the HTML comment token:This PR fixes that. Now, the comment token is properly recognized:
It also works for continue comment functionality (when pressing
oor adding a newline, for example)Languages Tested
I've added a lot of tests, each of which actually helped me fix a bug or two while I was implementing this. So I ended up extracting all comment-relating integration tests into a separate module
The core functionality works. What's left is to make sure that we do all we can to have individual languages work as well. For example, Svelte's comment tokens had to be adjusted in this PR for the best experience.
I've tested these languages manually as well. If you have a language which relies on injections, be sure to test it and comment here, I'll add it to the list.
JSX and TSX
These languages don't use tree-sitter injections:
Has just this injection:
Which means this PR won't affect them
You can use
Space + Cto comment JSX at the moment. It'd be nice if you could useSpace + cto make line comments that use the tokens{/*and*/}though, as it uses//right now.But to do this we could have
.tsxand.jsxlanguages usetsandjsas the file's "main" language, and then when we injecttsxandjsxlanguages when we encounter a tag.Not exactly sure how to accomplish this though, and it's not really related to this PR (can be done as a follow up if I or someone figures out how to do this)
Implementation
Before:
Selection'sRangeto aChangeTransactionfrom those changesNow
RangeinSelectionget its comment tokensRangeto aVec<Change>Transactionfrom those changesCloses #7364
Closes #11647
Related to to #9425