Toggle Block Comment #2080

TomMalbran · 2012-11-08T08:52:50Z

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 · 2012-11-09T04:33:09Z

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.

redmunds · 2012-11-11T18:23:59Z

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.

redmunds · 2012-11-11T18:36:30Z

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.

redmunds · 2012-11-11T18:41:44Z

src/command/Menus.js

@@ -918,6 +918,8 @@ define(function (require, exports, module) {
                                                          platform: "mac"}]);
        menu.addMenuDivider();
        menu.addMenuItem(Commands.EDIT_LINE_COMMENT,    "Ctrl-/");
+        menu.addMenuItem(Commands.EDIT_BLOCK_COMMENT,   [{key: "Ctrl-Shift-/", platform: "win"},
+                                                         {key: "Ctrl-Alt-/",   platform: "mac"}]);


I don't see any conflicts with this shortcut, so why did you choose a different shortcut for mac and win? We should be consistent wherever possible.

This was the only thing I took from the previous implementation of Block-comment. I'll use Ctrl-Shift-/ for both

redmunds · 2012-11-11T20:14:41Z

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) */ {

redmunds · 2012-11-11T20:25:01Z

src/editor/EditorCommandHandlers.js

+     * @param {!RegExp} prefixExp - a valid regular expression
+     * @return {{line: number, ch: number}}
+     */
+    function _findCommentStart(ctx, prefixExp) {


It would be more consistent if _findCommentStart() returned null if not found the same as _findCommentEnd().

The idea is that that case should never happen, if the ctx starts inside a block-comment. But for commentEnd it can since a block comment could be valid without the */ in the case of starting it and ending at the end of the document.

But I will change it.

Thanks. Yes, I understand that it won't happen as your code is currently written. But, eventually someone else may come along and change your code, or use that function for some other purpose.

TomMalbran · 2012-11-11T20:32:57Z

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 #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?

redmunds · 2012-11-11T20:53:31Z

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.

TomMalbran · 2012-11-11T21:07:34Z

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.

redmunds · 2012-11-11T21:18:16Z

src/editor/EditorCommandHandlers.js

+            }
+
+            // Check if we should just do a line uncomment (if all lines in the selection are commented)
+            if (slashComment && (startCtx.token.string.match(/^\/\//) || endCtx.token.string.match(/^\/\//))) {


This RegExp is used multiple times, so it should be stored in a variable.

redmunds · 2012-11-11T21:56:45Z

Done with initial code review.

TomMalbran · 2012-11-11T23:31:59Z

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?

redmunds · 2012-11-12T00:49:33Z

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

git checkout cmv3

redmunds · 2012-11-12T00:59:52Z

src/editor/EditorCommandHandlers.js

+        while (result && !ctx.token.string.match(prefixExp)) {
+            result = TokenUtils.moveSkippingWhitespace(TokenUtils.movePrevToken, ctx);
+        }
+        return !result ? null : {line: ctx.pos.line, ch: ctx.token.start};


Sorry for not mentioning this on the first pass, but I think it's an unnecessary level of indirection to reverse the logic here (i.e. "!result"). Please use:

return result ? {line: ctx.pos.line, ch: ctx.token.start} : null;

Same in _findCommentEnd().

redmunds · 2012-11-12T01:19:08Z

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.

TomMalbran · 2012-11-12T01:43:32Z

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

redmunds · 2012-11-12T16:10:01Z

Looks good. Merging.

Toggle Block Comment

TomMalbran added 5 commits November 8, 2012 05:30

Adding block-commenting

1c1d5cd

Description fix

32c88f3

Description fix

b1e356c

Description fix

f68324d

Merge remote-tracking branch 'upstream/master' into tom/block-comment

af27a6f

TomMalbran added 3 commits November 9, 2012 05:12

Code change to use TokenUtils

2c2df20

Fixing a particular case

7c926c4

More bugs fixed

6abb018

ghost assigned redmunds Nov 11, 2012

redmunds reviewed Nov 11, 2012
View reviewed changes

Fixes after initial review

cd2f5c5

redmunds reviewed Nov 12, 2012
View reviewed changes

Typo and return fixed

9bbf542

Do nothing on invalid selections

8928049

redmunds added a commit that referenced this pull request Nov 12, 2012

Merge pull request #2080 from TomMalbran/tom/block-comment

d0cc399

Toggle Block Comment

redmunds merged commit d0cc399 into adobe:master Nov 12, 2012

core-ai-bot mentioned this pull request Aug 29, 2021

[CLOSED] Toggle Block Comment brackets-archive/bracketsIssues#2011

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toggle Block Comment #2080

Toggle Block Comment #2080

TomMalbran commented Nov 8, 2012

TomMalbran commented Nov 9, 2012

redmunds commented Nov 11, 2012

redmunds commented Nov 11, 2012

redmunds Nov 11, 2012

TomMalbran Nov 11, 2012

redmunds commented Nov 11, 2012

redmunds Nov 11, 2012

TomMalbran Nov 11, 2012

redmunds Nov 11, 2012

TomMalbran commented Nov 11, 2012

redmunds commented Nov 11, 2012

TomMalbran commented Nov 11, 2012

redmunds Nov 11, 2012

redmunds commented Nov 11, 2012

TomMalbran commented Nov 11, 2012

redmunds commented Nov 12, 2012

redmunds Nov 12, 2012

redmunds commented Nov 12, 2012

TomMalbran commented Nov 12, 2012

redmunds commented Nov 12, 2012

Toggle Block Comment #2080

Toggle Block Comment #2080

Conversation

TomMalbran commented Nov 8, 2012

TomMalbran commented Nov 9, 2012

redmunds commented Nov 11, 2012

redmunds commented Nov 11, 2012

redmunds Nov 11, 2012

Choose a reason for hiding this comment

TomMalbran Nov 11, 2012

Choose a reason for hiding this comment

redmunds commented Nov 11, 2012

redmunds Nov 11, 2012

Choose a reason for hiding this comment

TomMalbran Nov 11, 2012

Choose a reason for hiding this comment

redmunds Nov 11, 2012

Choose a reason for hiding this comment

TomMalbran commented Nov 11, 2012

redmunds commented Nov 11, 2012

TomMalbran commented Nov 11, 2012

redmunds Nov 11, 2012

Choose a reason for hiding this comment

redmunds commented Nov 11, 2012

TomMalbran commented Nov 11, 2012

redmunds commented Nov 12, 2012

redmunds Nov 12, 2012

Choose a reason for hiding this comment

redmunds commented Nov 12, 2012

TomMalbran commented Nov 12, 2012

redmunds commented Nov 12, 2012