-
Notifications
You must be signed in to change notification settings - Fork 236
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
add toggle-block-comment #771
add toggle-block-comment #771
Conversation
I haven't looked at the code yet, but do you think we could make |
I renamed the key to |
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.
A couple of problems I encountered:
- When there is a selection, only the selected part is commented. Is this intended?
- The comment always starts at the beginning of the line, while
toggle-line-comments
starts from the position of the text.
Also, do you think we can improve this functionality to respect nested syntaxes or would that be too complex?
I really like this. It's good stuff.
I can only assume this is intended. This is how I'd expect it to work.
In the version I just built, here's the behaviour I'm seeing:
All of these behaviours make sense to me.
Yeah; that's a good point. Currently, My vote is, let's merge this (if @Guldoman you're cool with the behaviour in that table, and that's what you're seeing as well), and we can add in support for the commenting functions hitting subsyntaxes in another PR (unless you wanna just slip that in @takase1121 ). |
The behavior is different from atom and vscode, as both of them start the block comment after the indentation. They also wrap the entire line, not just the selection.
Yeah, it works so LGTM. |
Aha; OK, sorry, I misunderstood what you meant when you said "beginning of the text". Gotcha. In that case, yeah; I think we should probably follow that convention, then. That's currently also a problem with line comments again, so yeah; I think we should have another PR to fix up that behaviour.
Hrm. I booted up For |
Line comments start correctly after the indentation for me. Both with single line and multi-line selections.
I used that command in an HTML file, it does the block comment (but it might be a plugin, I don't know).
This is how it works for me: Edit: it looks like VSCodium does comment only the selection, but only with the block comment command, not the normal line comment. |
Ah; sorry, yeah.
Well, there is no line comment in HTML; so the block comment makes sense, but given a bit of googling, it doesn't actually appear that atom has a block comment command out of the box; so I say we discount atom's behaviour for now.
Right. I think that's reasonable (and I think that's what this PR currently does, yeah?). |
This PR regresses the single line comment command only when it needs to call the block comment command.
Just tested Atom in safe mode (I think we need a safe mode too), and it has the same behavior, so it wasn't a plugin. So it seems that in the end the behaviors are:
So the only thing this PR is "missing" is the possibility of commenting the entire line, starting after the indentation, when calling the single line comment (and no simple line comment is available). Also, testing more, VSCodium seems to not wrap the entire line when there isn't a selection, but it just adds the block comment where the caret is. I don't know how to feel about this. |
Yeah, you're right. It's even less a regression, and more an expansion of functionality (because you can't even comment line comment HTML in master, for example) that just was not as expected.
Agreed. Functionally that can be achieved by setting your But yeah; sorry, I don't mean that the behaviour was a plugin, I mean that it doesn't have an explicit command for block comments, so regardless of which behaviour it chooses, I don't think we should use it as a model for block-style commenting, because there's no explicit ask to perform a particular style of commenting; there's only the ask for line-style commenting, which is semantically different from block-style commenting. In Atom, it's basically only ever doing line-style commenting (even if it's using block comments).
Yeah. Given the behaviour in vscode, that if you do
I am not a huge fan of that either. If no one objects, I'd be happy if we made that act like a line comment. |
Command line parsing would be great, yes.
Yep, I'm OK with this behavior.
My vote goes for full line comment too, as I don't think adding the block comment in the middle of a line is used much, but I could be wrong. |
Not sure how I would do this. |
I think Guldo just means that we should just mirror the behaviour of toggle-line-comments in the positioning of the block comment when you have nothing selected, and the syntax only supports block comments. UPD: I messed this up. HTML is a better example: i.e. if you have
And you comment it with a line comment, it becomes
The suggestion is that instead it becomes:
|
Alright, I've refactored the code and also make it whitespace aware. The cols are normalised to word start and end before anything is done, so cases like
( would work. |
Looks good. I'm happy to merge this now; just one last nitpick. When I toggle a line comment, it selects the whole commented line. Can we change it so the cursor just stays where it is? (this is the behaviour in VScode and atom). |
This still doesn't distinguish between line and block comments when dealing with languages that only have one or the other. What I mean is that the line comment command should always comment the whole line/s, as opposed to block comment that should comment only what's selected. So now in HTML for example, calling the line comment command with multiple selected lines, it comments only the selection, while it should comment from the beginning of the first line, to the end of the last, in a single comment block. In the end the current behavior is ok, so we could just merge this in, and iron out the corner cases later. |
Basically if there is a single line comment defined for the language, calling
If there is not:
Calling |
I think I adressed it with my latest commit. |
This mostly works; so with @takase1121 's blessing, I'm merging this, and we'll fix up those last little issues in another PR. It's still much better than what we've got currently. |
This is requested from Discord. The support is rather rudimentary - no support for comment nesting, etc.
It might not work well with unicode characters. I'll need guidance to make them work.