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

add toggle-block-comment #771

Merged
merged 6 commits into from Jan 19, 2022

Conversation

takase1121
Copy link
Member

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.

@Guldoman
Copy link
Member

I haven't looked at the code yet, but do you think we could make doc:toggle-line-comments fallback to doc:toggle-block-comments when the user wants to comment a single line, but the language only supports multiline comments?

@takase1121
Copy link
Member Author

I renamed the key to block_comment because its more suitable.

Copy link
Member

@Guldoman Guldoman left a 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?

@adamharrison
Copy link
Member

I really like this. It's good stuff.

When there is a selection, only the selected part is commented. Is this intended?

I can only assume this is intended. This is how I'd expect it to work.

The comment always starts at the beginning of the line, while toggle-line-comments starts from the position of the text.

In the version I just built, here's the behaviour I'm seeing:

Command Selection Result
toggle-line-comments No Selection Insert beginning of line comment.
toggle-line-comments Single Line Selection Insert beginning of line comment.
toggle-line-comments Multiple Line Selection Insert beginning of each line comment.
toggle-block-comments No Selection Wraps entire line.
toggle-block-comments Single Line Selection Wraps selection.
toggle-block-comments Multiple Line Selection Wraps selection.

All of these behaviours make sense to me.

Also, do you think we can improve this functionality to respect nested syntaxes or would that be too complex?

Yeah; that's a good point. Currently, toggle-line-comments doesn't do this, so this would be a general enhancement anyway.

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 ).

@Guldoman
Copy link
Member

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.
In the end it's fine like this too, but I do prefer the other way.

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 ).

Yeah, it works so LGTM.

@adamharrison
Copy link
Member

The behavior is different from atom and vscode, as both of them start the block comment after the indentation.

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.

They also wrap the entire line, not just the selection.

Hrm. I booted up atom, but don't seem to have an obvious "block comment" function; I only have "Editor: Toggle Line Comments". What's the command for that in atom?

For vscode; I checked, and it does seem to be block commenting from the selection, not the entire line. What language did you try? I tried HTML and JavaScript.

@Guldoman
Copy link
Member

Guldoman commented Dec 29, 2021

That's currently also a problem with line comments again, so yeah; I think we should have another PR to fix up that behaviour.

Line comments start correctly after the indentation for me. Both with single line and multi-line selections.

Hrm. I booted up atom, but don't seem to have an obvious "block comment" function; I only have "Editor: Toggle Line Comments". What's the command for that in atom?

I used that command in an HTML file, it does the block comment (but it might be a plugin, I don't know).

For vscode; I checked, and it does seem to be block commenting from the selection, not the entire line. What language did you try? I tried HTML and JavaScript.

This is how it works for me:

  • Atom
    2021-12-29_19-16_1
  • VSCodium
    2021-12-29_19-16

Edit: it looks like VSCodium does comment only the selection, but only with the block comment command, not the normal line comment.

@adamharrison
Copy link
Member

adamharrison commented Dec 29, 2021

Line comments start correctly after the indentation for me. Both with single line and multi-line selections.

Ah; sorry, yeah. This PR changes that; master performs correctly. So that's actually a regression that should be fixed.
UPD: Ok, now wait a minute. This seems to change depending on syntax. Let me look into this a little more.
UPD2: OK, I can't actually reproduce this now. Weird. Maybe I just got confused. But yes; block comments should start at the beginning of indents if no selection, as line comments do, 100% agreed.
UPD3: OK, actually, found it. It's because in HTML, there are no non-block comments, so it's running through the block logic. So it's actually that first issue you mention; that block comments just don't indent properly. So yeah. That should be fixed, then this is all good.

I used that command in an HTML file, it does the block comment (but it might be a plugin, I don't know).

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.

Edit: it looks like VSCodium does comment only the selection, but only with the block comment command, not the normal line comment.

Right. I think that's reasonable (and I think that's what this PR currently does, yeah?).

@Guldoman
Copy link
Member

Line comments start correctly after the indentation for me. Both with single line and multi-line selections.

Ah; sorry, yeah. This PR changes that; master performs correctly. So that's actually a regression that should be fixed.

This PR regresses the single line comment command only when it needs to call the block comment command.

I used that command in an HTML file, it does the block comment (but it might be a plugin, I don't know).

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.

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:

  • Single line comments: comment the entire line, starting after the indentation; if no simple line comment exists for that language, the behavior remains unchanged and instead uses the block comment syntax.
  • Block comments: comment only the selection.

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.

@adamharrison
Copy link
Member

adamharrison commented Dec 29, 2021

This PR regresses the single line comment command only when it needs to call the block comment command.

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.

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.

Agreed. Functionally that can be achieved by setting your XDG_CONFIG_HOME to an empty folder, but we probably want a less esoteric way to do it. (Command line options?)

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).

So it seems that in the end the behaviors are:

Single line comments: comment the entire line, starting after the indentation; if no simple line comment exists for that language, the behavior remains unchanged and instead uses the block comment syntax.
Block comments: comment only the selection.

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).

Yeah. Given the behaviour in vscode, that if you do toggle-line-comments, it should always follow the line conventions, and if you toggle-block-comments, it should follow the block conventions; regardless of whether or not the syntax defines a line or a block comment. Agreed.

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.

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.

@Guldoman
Copy link
Member

Agreed. Functionally that can be achieved by setting your XDG_CONFIG_HOME to an empty folder, but we probably want a less esoteric way to do it. (Command line options?)

Command line parsing would be great, yes.

Yeah. Given the behaviour in vscode, that if you do toggle-line-comments, it should always follow the line conventions, and if you toggle-block-comments, it should follow the block conventions; regardless of whether or not the syntax defines a line or a block comment. Agreed.

Yep, I'm OK with this behavior.

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.

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.

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.

@takase1121
Copy link
Member Author

The comment always starts at the beginning of the line, while toggle-line-comments starts from the position of the text.

Not sure how I would do this.

@adamharrison
Copy link
Member

adamharrison commented Dec 30, 2021

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

      <hr/>

And you comment it with a line comment, it becomes

<!--      <hr/> -->

The suggestion is that instead it becomes:

      <!-- <hr/> -->

@takase1121
Copy link
Member Author

takase1121 commented Jan 2, 2022

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

    hello
    world    |

(| marks EOL)

would work.

@adamharrison
Copy link
Member

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).

@Guldoman
Copy link
Member

Guldoman commented Jan 5, 2022

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.

@takase1121
Copy link
Member Author

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).

I can fix that.

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.

I understand your meaning.
if we have selected the entire line,
image

If I press ctrl+/, it should be
image

Instead of
image

Right?

@Guldoman
Copy link
Member

Guldoman commented Jan 5, 2022

Basically if there is a single line comment defined for the language, calling toggle-line-comments:

Selection Result
No Selection Insert line comment at text start.
Single Line Selection Insert line comment at text start.
Multiple Line Selection Insert line comment at text start for each line.

If there is not:

Selection Result
No Selection Insert block comment from line text start, to line end.
Single Line Selection Insert block comment from line text start, to line end.
Multiple Line Selection Insert block comment from first line text start, to last line end.

Calling toggle-block-comments is fine as it is now, but I haven't tested what happens if the language doesn't use block comments.

@takase1121
Copy link
Member Author

I think I adressed it with my latest commit.

@adamharrison
Copy link
Member

Looking good; one last thing. Toggling line comments in this situation:

image

Leads to this selection, which is kinda weird.

image

Should probably select the whole line. Same thing happens in C.

Block commenting works perfectly, as far as I can tell.

@Guldoman
Copy link
Member

Guldoman commented Jan 8, 2022

Yep, this seems fine.
The behavior of line comment on multiple lines selection is a bit different than what I expected, but it's still fine (I'd even say it's more logical the way it's implemented with a new block comment for each line).

This behavior shows a little "problem": it inserts the line comment at each line "text start", while the normal line insert keeps the smallest indent.

Indent on the same row: Indent on "text start":
2022-01-08_19-50 2022-01-08_19-50_1

But I think we could just merge this as it is.

@adamharrison
Copy link
Member

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.

@adamharrison adamharrison merged commit d3e1636 into lite-xl:master Jan 19, 2022
adamharrison added a commit to adamharrison/lite-xl that referenced this pull request Jan 19, 2022
adamharrison added a commit to adamharrison/lite-xl that referenced this pull request Jan 19, 2022
adamharrison added a commit to adamharrison/lite-xl that referenced this pull request Jan 19, 2022
adamharrison added a commit to adamharrison/lite-xl that referenced this pull request Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants