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

Continue comments when opening a new line #1937

Closed
wants to merge 7 commits into from

Conversation

antoyo
Copy link
Contributor

@antoyo antoyo commented Apr 3, 2022

Fix #1730.

We probably want to be able to configure whether we want this or not. If so, what option name do you propose?

This does not support continuing block comment, i.e. adding a * when a comment starts with /*. I propose we do this in a future PR since this would require some more thought regarding the block comment token (i.e. do we want to add it to languages.toml?).

Comment on lines 2297 to 2300
let token = doc
.language_config()
.and_then(|lc| lc.comment_token.as_ref())
.map(|tc| tc.as_ref());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might want to move this to a new method since this code is duplicated 3 times. Where do you suggest we put it?

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving it to a new separate helper function residing in the same module. You could make it return an Option<()> to get rid of all the and_then and maps.

Comment on lines 2297 to 2300
let token = doc
.language_config()
.and_then(|lc| lc.comment_token.as_ref())
.map(|tc| tc.as_ref());
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving it to a new separate helper function residing in the same module. You could make it return an Option<()> to get rid of all the and_then and maps.

@@ -94,6 +96,24 @@ pub fn toggle_line_comments(doc: &Rope, selection: &Selection, token: Option<&st
Transaction::change(doc, changes.into_iter())
}

/// Return token if the current line is commented.
/// Otherwise, return None.
pub fn continue_comment<'a>(doc: &Rope, line: usize, token: Option<&'a str>) -> Option<&'a str> {
Copy link
Member

Choose a reason for hiding this comment

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

It makes more sense for this to return a bool since we already have the token in the calling code and continue_comment doesn't change it in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would require to unwrap() the token, though. Do you still want me to do this change?

Comment on lines 106 to 111
.map(|pos| {
let len = line_slice.len_chars();
// line can be shorter than pos + token len
let fragment = Cow::from(line_slice.slice(pos..std::cmp::min(pos + token.len(), len)));
fragment == token
})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.map(|pos| {
let len = line_slice.len_chars();
// line can be shorter than pos + token len
let fragment = Cow::from(line_slice.slice(pos..std::cmp::min(pos + token.len(), len)));
fragment == token
})
.map(|pos| Cow::from(line_slice.slice(pos..)).starts_with(token))

@antoyo antoyo marked this pull request as draft April 3, 2022 12:28
@antoyo
Copy link
Contributor Author

antoyo commented Apr 3, 2022

I converted to a draft because that doesn't support doc-comment. Any idea on how we want to do this? Do we want to add the doc-comment syntax in languages.toml?

That would require a list as there can be /// and //! for Rust, for instance.
Do we need some support for /* vs /*!?

@archseer
Copy link
Member

archseer commented Apr 6, 2022

I converted to a draft because that doesn't support doc-comment. Any idea on how we want to do this? Do we want to add the doc-comment syntax in languages.toml?

We could extend comment-token to be a list: comment-token: ["///", "//"].

Actually, an easier way would be: find a comment (//), then scan forward until you reach whitespace. Then scan to the end of whitespace:

//*  test

would produce //* which would retain any symbols at the start of the line (this would also cover the extra slash from the doc comment) plus indentation from the previous level.

Block comments

It's okay to solve block comments later since none of the functions have support for this yet.

Some bits and ideas:

VSCode's config seems to have two separate blocks for line & block comments:

"comments": {
    "lineComment": "//",
    "blockComment": ["/*", "*/"]
},

and here's the algorithms in codemirror: https://github.com/codemirror/comment/blob/269b9af21d68b1a71fa40bbf6c1e41c665e6e7d0/src/comment.ts#L48-L161=

The initial implementation for line comments in helix was based on the functions in ^

@antoyo
Copy link
Contributor Author

antoyo commented Apr 6, 2022

Actually, an easier way would be: find a comment (//), then scan forward until you reach whitespace. Then scan to the end of whitespace:

I thought about this, but that wouldn't work: you could have a shebang in bash and you don't want to have #!/bin/bash repeated on the next line. Also some users could not put a space after //. I believe I had another reason, but I forgot.

Edit: for the shebang, vim does not continue the comment. Any way we could do the same?

If I go with comment-token: ["///", "//"], do I have to add it manually for all languages or is there any automated tool that is used to generate this file?

@archseer
Copy link
Member

Actually, an easier way would be: find a comment (//), then scan forward until you reach whitespace. Then scan to the end of whitespace:

I thought about this, but that wouldn't work: you could have a shebang in bash and you don't want to have #!/bin/bash repeated on the next line. Also some users could not put a space after //. I believe I had another reason, but I forgot.

Edit: for the shebang, vim does not continue the comment. Any way we could do the same?

If I go with comment-token: ["///", "//"], do I have to add it manually for all languages or is there any automated tool that is used to generate this file?

Yeah let's go with the simple approach for now and only resume exact matches?

If I go with comment-token: ["///", "//"], do I have to add it manually for all languages or is there any automated tool that is used to generate this file?

You'll need to manually update the file, it's user-maintained. I'd also order the // first so we can use the first item in the list as the comment token when commenting lines.

@antoyo
Copy link
Contributor Author

antoyo commented Apr 25, 2022

Since I have to edit the languages.toml file, I thought I should be adding block comment as well at the same time.

The problem I have is I don't know if there's a way to distinguish between a block-comment vs a line-comment. Do we have a way to know whether a cursor is within a block-comment?

@archseer
Copy link
Member

archseer commented May 2, 2022

The problem I have is I don't know if there's a way to distinguish between a block-comment vs a line-comment. Do we have a way to know whether a cursor is within a block-comment?

You could match this via tree-sitter queries, usually line comments vs block comments use different node types. For a more generic system we'd need to immitate https://github.com/codemirror/comment/blob/269b9af21d68b1a71fa40bbf6c1e41c665e6e7d0/src/comment.ts#L88-L120= then fallback to line comments

@antoyo
Copy link
Contributor Author

antoyo commented May 2, 2022

You could match this via tree-sitter queries, usually line comments vs block comments use different node types.

I'm not familiar with tree-sitter. It seems to use strings to identify node: is this correct?
If so, where can I find the documentation for the different node types?

My attempt at using block_comment instead of comment didn't work.

@the-mikedavis
Copy link
Member

Using tree-sitter node names might actually not work out very well since different grammars use different names for the comment nodes (line_comment/block_comment/comment/etc). Instead using a capture like @comment could be more general but there are also captures like @comment.line/@comment.block/@comment.docs where you'd want to chop off the suffix.

@antoyo
Copy link
Contributor Author

antoyo commented May 2, 2022

Instead using a capture like @comment could be more general but there are also captures like @comment.line/@comment.block/@comment.docs where you'd want to chop off the suffix.

By that, do you mean that I should add a line like:

(block_comment) @comment.block.around

in runtime/queries/rust/textobjects.scm and all other languages?

Or should those captures already exist?

@the-mikedavis
Copy link
Member

I think something like that should work. It would be on a per-language basis though and I'm not sure how many languages have comment captures. I think it's ok if there's fallback behavior (even it the fallback behavior is just to not continue comments) for languages that don't have textobjects queries.

@antoyo antoyo force-pushed the feature/continue-comment branch from 99f66c6 to a6d1db4 Compare May 3, 2022 02:55
@antoyo
Copy link
Contributor Author

antoyo commented May 3, 2022

So, I started adding the missing tree-sitter objects and config for some languages and I wanted to have some feedback before I continue doing it for all the other languages.

Also, I have an issue: it seems like tree-sitter doesn't consider the start of a comment to be a non-terminating comment, which means I cannot continue them completely.
My attempt here only continues the first line, but it won't work on the following lines.
Any idea what I could do for this?

Thanks for your help!

@@ -33,3 +33,5 @@
(line_comment)+ @comment.around

(block_comment) @comment.around

(block_comment) @comment.block.around
Copy link
Contributor

@EpocSquadron EpocSquadron May 3, 2022

Choose a reason for hiding this comment

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

The textobject captures don't distinguish block vs line or other types of comments currently. Adding a .block. in the middle could be good, but existing code to handle textobject selection may need to be adjusted. @sudormrfbin what do you think?

Actually wondering if we add this to indents.scm instead, since it's kind of related functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second look at indents.scm, I don't think it'll work there easily. Textobjects is probably the best place unless we want a comments.scm specifically for this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but existing code to handle textobject selection may need to be adjusted

Why would it need to be adjusted? It continues using @comment.around and that still works.
I'm not sure I understand everything this file does, but I was under the impression that I only added a new capture named @comment.block.around.

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed the fact that you added a new capture instead of changing it, my bad! That is effectively adding a whole new capture, which is a good idea. I would maybe drop the 'around' and make up a new one like 'continue'. I'm wondering again in this light of this belongs in the indents.scm queries, since it would be a whole new capture name and wouldn't affect either existing implementations (indent or textobject).

Copy link
Member

Choose a reason for hiding this comment

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

In this particular case that second block_comment one overrides the first. You could have both captures like

(block_comment) @comment.around @comment.block.around

@antoyo
Copy link
Contributor Author

antoyo commented May 15, 2022

So, for the case of continuing start of comment, I had the following idea:
we could add a setting to auto-close block comments, so we should always be in block comments and never in start of comments.
What do you think?

@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 18, 2022
@the-mikedavis
Copy link
Member

I think it would be good to have an auto-pairs functionality for the block comment tokens, like HTML <!-- would auto-pair a -->. It wouldn't be reliable for detecting a block comment via tree-sitter though because you could disable auto-pair.

@antoyo
Copy link
Contributor Author

antoyo commented May 18, 2022

It wouldn't be reliable for detecting a block comment via tree-sitter though because you could disable auto-pair.

Do you suggest that it's OK that it's not reliable or would you prefer a workaround?
If you would want a workaround, do you have any idea that would be as fast as tree-sitter?

@the-mikedavis
Copy link
Member

It would probably be ok in most cases but it would be prone to edge-cases. I would prefer a workaround since I don't use auto-pairs myself but I don't have any good ideas for doing it efficiently.

In any case, I think block comments should be handled in a separate PR as you say in the initial description. There are other block comment features like surrounding a selection with a block comment that might inform the design of continuing block comments. Line comment continuation would be a great increment on its own (although I'm biased because I mostly use languages that only have line comments).

@antoyo
Copy link
Contributor Author

antoyo commented May 18, 2022

In any case, I think block comments should be handled in a separate PR as you say in the initial description.

Are you suggesting we keep the block comment as is in this PR and I'll fix them in a later PR?
Or do you want that I split that in another PR right away?

There are other block comment features like surrounding a selection with a block comment that might inform the design of continuing block comments.

I disagree because we probably don't want a surrounding feature to add a * (or other character) at the beginning of every line because removing them could actually break code.
So these two features should be separate in my opinion.

@the-mikedavis
Copy link
Member

I would prefer splitting the block comments entirely to another PR if possible since that implementation needs some design.

@kirawi kirawi added A-helix-term Area: Helix term improvements S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. A-core Area: Helix core improvements and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-helix-term Area: Helix term improvements labels Sep 13, 2022
@LeoniePhiline
Copy link
Contributor

Is this PR abandoned?

@antoyo
Copy link
Contributor Author

antoyo commented Feb 10, 2023

No, I just haven't got the time to clean it up.

I'll probably have time to do that in a couple months.

@antoyo
Copy link
Contributor Author

antoyo commented Jul 24, 2023

I switched back to neovim, so I won't be updating this PR anymore. Feel free to take the changes and make another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Helix core improvements S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continue comments
7 participants