Fixing several Block and Line Comment issues

[TomMalbran]

This pull request fixes both issues mentioned in #2339, one of them being a line comment issue and the other a block comment issue and the 6/7 and 9/10 inconsistency mentioned in #2337 since that really wasn't an intended behavior.

[peterflynn]

A few issues I noticed in testing:

• Case 3 in Block comment command behaves unpredictably near line comments #2337: Invoking block comment when the cursor is in whitespace to the left of a line comment now inserts an empty block comment. It used to uncomment (as long as there's nothing but whitespace to the left of it). Is this intentional? (It still works if the cursor is in the comment).
• Cases 13/14 are still inconsistent: if I select some whitespace to the left plus some of the comment, it will uncomment unless my selection extends to the end of the line (i.e. select all of the comment), in which case it block comments instead. (This also feels inconsitent with case
• Similarly, if I select an entire comment (from the first "/" to end of line), it will block-comment instead of uncommenting. This seems inconsistent with cases 9/10 (i.e. I can select the whole comment except the first "/" and it will uncomment).
• If I select an entire comment (including "//") that has code to the left, it wraps it in a block comment. This seems a little inconsistent with cases 6/7 (i.e. I can select the whole comment except the first "/" and it will no-op instead).
• If I select the whole first line in Block comment command behaves unpredictably near line comments #2337, it block comments. But if I leave out the first or last char on the line, it no-ops instead. That feels wrong: if I'm allowed to block-comment a mix of code + line comment sometimes, I should be allowed to do it always (so that implies cases 11/12 should block-comment, not no-op).

[TomMalbran]

If you are allowed to block comment with any selection of a mix of code + line comment, even when doing a block comment might uncomment part of the line comments, it makes things easier to check. This should then work for all the cases mentioned above.

[peterflynn]

I've created a big suite of unit tests for line comment that I hope will be helpful: see pflynn/block-comment-unittests (it's not on master yet since they're not all passing).

It's still hitting two things that I think are bugs, but almost all the tests pass with your latest commit.

One problem seems to with looking at the char or token after the end of the selection:

• Failing: "should uncomment multi-line block comment selection, only end selected" - selection gets messed up
• But passing: "should uncomment multi-line block comment selection, only end selected, ends at EOF" - works fine if you're at the bottom of the file

The other is related to selections that end at ch:0. The behavior varies depending on what's on that line, when it probably shouldn't matter (because the selection doesn't include any chars from that line; just the CR ending the previous line).

• Failing: "should switch to line uncomment, all of line-comment line selected (following line is code)" & "should switch to line uncomment, all of line-comment line selected (following line is block comment)" - these block-comment instead
• But passing: "should switch to line uncomment, all of line-comment line selected (following line is whitespace)" & "should switch to line uncomment, all of line-comment line selected (following line is line comment)"

If you'd like to use my unit tests for testing your changes, you could use a procedure like this:

1. Checkout my branch from upstream (creating a local tracking branch)
2. Merge your branch into it
3. Run the unit tests & modify your code as needed
4. git stash save
5. Checkout your branch
6. git stash apply and then commit your changes

(This way you can use my tests before they've landed in master, without getting them stuck in your branch & thus your pull request)

[TomMalbran]

I fixed the code so that it passed the 3 failed unit tests and every other test.

[peterflynn]

I think this could be written more simply as just:

result = TokenUtils.moveSkippingWhitespace(TokenUtils.moveNextToken, ctx) &&
         _findNextBlockComment(ctx, sel.end, prefixExp);

...and then only check for result below. I'm pretty sure this is logically equivalent (short circuiting guarantees that _findNextBlockComment() won't run if the first moveSkippingWhitespace() returns false), and all the unit tests still pass if I change this. Although it's a little confusing that the first clause has side-effects on ctx which is then re-used in the 2nd clause, if you wanted to make that more explicit I'd suggest something like this:

result = TokenUtils.moveSkippingWhitespace(TokenUtils.moveNextToken, ctx);
result = result && _findNextBlockComment(ctx, sel.end, prefixExp);

or even more explicit:

result = TokenUtils.moveSkippingWhitespace(TokenUtils.moveNextToken, ctx);
if (result) {
    result = _findNextBlockComment(ctx, sel.end, prefixExp);
}

[peterflynn]

@TomMalbran: Done reviewing. Looks good except for those two nits about the boolean operation.

Btw -- unrelated to this pull request -- I was wondering if you could tell me more about the block of code that starts with else if (startCtx.token.className === null &&... I'm a little confused by it. Tokens with className == null aren't "invalid" or otherwise special (they're just uncolored text like operators, commas, semicolons, whitespace, etc.). It seems like we really only need that case to handle whitespace (because it has className == null, it's harder to tell if we're inside an existing block comment). Am I interpreting it correctly?

[TomMalbran]

Yes I am using that to check if the start of the selection is in a whitespace before the text. So then it can be used to uncomment line comments of multiple block comments, but when the start of the selection is after the first line of the block comment and before the start of the line, since here it still has className === null. Maybe a better check would be !startCtx.token.className && startCtx.token.string.trim().length === 0 to just check for white spaces, like is done in Token Utils.

[TomMalbran]

Changes done.

[peterflynn]

Looks great -- thanks again! Merging now (whew!)