[CLOSED] Fixing several Block and Line Comment issues

core-ai-bot commented 3 years ago

Issue by TomMalbran Wednesday Dec 12, 2012 at 19:47 GMT Originally opened as https://github.com/adobe/brackets/pull/2342

This pull request fixes both issues mentioned in adobe/brackets#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 adobe/brackets#2337 since that really wasn't an intended behavior.

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

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Dec 13, 2012 at 19:22 GMT

A few issues I noticed in testing:

Case 3 in #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 #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).

core-ai-bot commented 3 years ago

Comment by TomMalbran Thursday Dec 13, 2012 at 20:41 GMT

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.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Dec 14, 2012 at 02:52 GMT

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:

Checkout my branch from upstream (creating a local tracking branch)
Merge your branch into it
Run the unit tests & modify your code as needed
git stash save
Checkout your branch
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)

core-ai-bot commented 3 years ago

Comment by TomMalbran Friday Dec 14, 2012 at 05:04 GMT

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

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Dec 14, 2012 at 22:37 GMT

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

core-ai-bot commented 3 years ago

Comment by TomMalbran Friday Dec 14, 2012 at 23:04 GMT

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.

core-ai-bot commented 3 years ago

Comment by TomMalbran Friday Dec 14, 2012 at 23:26 GMT

Changes done.

core-ai-bot commented 3 years ago

Comment by peterflynn Saturday Dec 15, 2012 at 00:01 GMT

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

brackets-archive / bracketsIssues

[CLOSED] Fixing several Block and Line Comment issues #2238