[CLOSED] Toggle Block Comment

[core-ai-bot]

Issue by TomMalbran
Thursday Nov 08, 2012 at 08:52 GMT
Originally opened as adobe/brackets#2080

---

This pull would add a new prefix/suffix type of block-comment feature that works similar to sublime text 2.

Block-comment/uncomment depends on: If the selection is inside a block-comment or one block-comment is inside or partially inside the selection we uncomment; otherwise we comment out. Commenting out adds the prefix before the selection and the suffix after. Uncommenting removes them.

A third possibility comes in languages that support both line and block comments. In this cases if the start or end of the selection is inside a line-comment it will try to do a line uncomment if all the lines in the selection are line-commented and will do nothing if at least one line is not line-commented. It might be missing the alternative, using a line-comment command to uncomment a block-comment when possible, but wasn't sure if this was wanted.

Note that some block comment could not actually create a comment, like commenting inside a string or a line comment (in cases the first and last lines are commented but one in the middle isn't), and in other cases it could break other comments. But I believe that it was better to show a result and let the user rollback than do nothing and letting the user suspect that something is broken.

There is still a problem that a uncommenting a multi-line block comment requires 2 undos. For commenting was easily fixed, but for uncommenting it because re-selecting the previous selection had several possibilities to do something about it. I read that in CodeMirror v3, this would be easily fixable, so I left it like that.

---

TomMalbran included the following code: https://github.com/adobe/brackets/pull/2080/commits

[core-ai-bot]

Comment by TomMalbran
Friday Nov 09, 2012 at 04:33 GMT

---

I used getTokenAt a lot for this and as I couldn't find it in Editor I just used it directly from _codeMirror, but I just found that all the functions needed are implemented in TokenUtils, so I am now updating the code to use them.

[core-ai-bot]

Comment by redmunds
Sunday Nov 11, 2012 at 18:23 GMT

---

Hey Tom, thanks for this contribution!

I noticed a problem with undo. If I create a block comment, then hit undo, it's undone as expected. But, if I use this command to remove a comment, I have to execute undo twice to put the comment back. Comment should come back with a single undo.

[core-ai-bot]

Comment by redmunds
Sunday Nov 11, 2012 at 18:36 GMT

---

I tested many cases where a partial block comment is selected, and most work as expected, but I found 1 case where the resulting code is invalid. Add this code to a page:

/*

*/

Select the line before the comment and the line with the opening comment delimiter ("/*"). Execute Toggle Block Comment. The resulting code is a nested block comment, which is invalid:

/*
/*
*/
*/

You get similar results if you also select the line inside the block comment.

[core-ai-bot]

Comment by redmunds
Sunday Nov 11, 2012 at 20:14 GMT

---

Another case that results in invalid JS. Start with this code:

// inline com|ment followed by non-empty line
if (something) | {

Select a range that starts after "//" in inline comment, and ends anywhere in non-empty line and execute Toggle Block Comment. Result is:

// inline com/*ment followed by non-empty line
if (something) */ {

[core-ai-bot]

Comment by TomMalbran
Sunday Nov 11, 2012 at 20:32 GMT

---

That is an expected result, since the actual initial selection is wrong for a block-comment. Removing the line-comment wouldn't be the expected result, since the selection goes beyond that, and doing nothing might bring the idea that something is wrong with the block-comment. So doing something, even if is wrong, and letting the user undo it seems better. Sublime does the same thing in this case.

But the first one isn't and i will fix it.

About the undo, I knew about the problem, but since for uncommenting the selection can be in several different place it was harder to restore it after replacing all block comment with the content inside of it. In adobe/brackets#1986 Peter mentioned that the batching logic is changing in CodeMirror v3, which might just fix this is at it is. Do you think I should still try to fix it now? Or maybe waiting and till v3 to fix or leave it if it gets fixed is ok?

[core-ai-bot]

Comment by redmunds
Sunday Nov 11, 2012 at 20:53 GMT

---

I agree that it's an exception case, so it's not a high priority. I'm not sure about where this is in the logic, but preceding comment with a newline might make sense:

// inline com
/*ment followed by non-empty line
if (something) */ {

For the multiple-undo issue, we have a CodeMirror v3 branch in progress if you'd like to try it there. Otherwise, we'll have to file an issue so that this gets tracked.

[core-ai-bot]

Comment by TomMalbran
Sunday Nov 11, 2012 at 21:07 GMT

---

That does work, but the selection should start after the inline comment, or before and have the block comment also include the inline comment.

Ok, I will try to see if it works in that branch and fix it if it doesn't.

[core-ai-bot]

Comment by redmunds
Sunday Nov 11, 2012 at 21:56 GMT

---

Done with initial code review.

[core-ai-bot]

Comment by TomMalbran
Sunday Nov 11, 2012 at 23:31 GMT

---

Thanks for the review.

I fixed all mentioned on the comments and the first case commented here.
I added the case of a line uncomment when the selection starts before the line comment.

Now on a complete line selection comment, the prefix and suffix are added on new lines at the ch 0. Then when uncommenting if both the prefix and suffix are at the ch 0 and are the only thing in the line, I removed the new lines on both prefix and suffix. It seems like it doesn't get unwanted cases doing it like this.

How can I get the cmv3 branch to test without cloning it?

[core-ai-bot]

Comment by redmunds
Monday Nov 12, 2012 at 00:49 GMT

---

Since you've already cloned brackets, I think you can checkout the cmv3 branch:

git checkout cmv3

[core-ai-bot]

Comment by redmunds
Monday Nov 12, 2012 at 01:19 GMT

---

Here's one more case where the resulting code is invalid. Start with this very simplified case:

/*

*/

/*

*/

Select the line with the comment "suffix" of the first comment, the line between the comments, and the comment "prefix" of the second comment. Execute Toggle Block Comment. The result is:

/*

/*
*/

/*
*/

*/

Anyone that purposefully does this probably doesn't understand what the command is for, or maybe they hit the command by accident, but it still shouldn't generate invalid code. You're already checking to see if the selection contains a comment prefix and suffix, so I think you just need to check what order they occur. Also, there could be multiple prefixes and suffixes.

Note that I think it's OK if your command does nothing in some cases such as this. The only reason I can think that someone would do this is maybe to try to join 2 comments, but that's a wild guess.

[core-ai-bot]

Comment by TomMalbran
Monday Nov 12, 2012 at 01:43 GMT

---

Cases where there are more than 2 block-comments in the selection are taken into account, but like the selection being part inside a inline-comment and part not, makes invalid selection.

So is better to just do nothing in cases like that?
That was my first idea, but then I did like Sublime, and just added new block-comments.

I tried the git checkout cmv3 and it doesn't work, saying that git doesn't know this branch

[core-ai-bot]

Comment by redmunds
Monday Nov 12, 2012 at 16:10 GMT

---

Looks good. Merging.