Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix #3057: Toggle Block Comment doesn't work if the open/close delimiters are the same #3060

Closed
wants to merge 13 commits into from
Closed

Fix #3057: Toggle Block Comment doesn't work if the open/close delimiters are the same #3060

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2d6039c
Fixing issues when open and close delimiters are the same
TomMalbran Mar 7, 2013
5fa487c
Merging to master
TomMalbran Mar 19, 2013
29d68d0
Improving the Block and Line comment fix
TomMalbran Mar 19, 2013
06e42b6
Fix Language test
TomMalbran Mar 19, 2013
e6c2abf
Adding new tests for languages like CoffeeScript
TomMalbran Mar 20, 2013
a283618
Fixing tests and docs
TomMalbran Mar 20, 2013
4b9cc7e
Fixes after second review
TomMalbran Mar 30, 2013
bedec43
Fix JSHint
TomMalbran Mar 30, 2013
1038904
Merge remote-tracking branch 'upstream/master' into tom/fix-issue-3057
TomMalbran Mar 30, 2013
cbf893a
Big code refactor
TomMalbran Apr 1, 2013
8c11ca7
Simplify the first step
TomMalbran Apr 1, 2013
3ec3cfa
Merge remote-tracking branch 'upstream/master' into tom/fix-issue-3057
TomMalbran Apr 13, 2013
851cfea
Updates and minor refactor after CodeMirror update
TomMalbran Apr 14, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions src/editor/EditorCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,24 @@ define(function (require, exports, module) {
* @param {!{editor:{CodeMirror}, pos:{ch:{string}, line:{number}}, token:{object}}} ctx - token context
* @param {!RegExp} suffixExp - a valid regular expression
* @param {!number} suffixLen - length of the suffix
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to update this JSDocs. Will do it on the next commit

* @param {?{line: number, ch: number}} prefixPos - the recently found prefix position
* @return {?{line: number, ch: number}}
*/
function _findCommentEnd(ctx, suffixExp, suffixLen) {
function _findCommentEnd(ctx, suffixExp, suffixLen, prefixPos) {
var result = true;

while (result && !ctx.token.string.match(suffixExp)) {
result = TokenUtils.moveSkippingWhitespace(TokenUtils.moveNextToken, ctx);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a bit of a problem with this loop in the case where the start & end tokens are the same. There's some ambiguity when we return from _findCommentStart() -- since the start/end tokens look the same, it may have actually found an end instead. (We don't really know until we look at the first non-ws token after it).

That causes the loop here to misbehave: if the "start token" is actually the end of a block comment, the loop will happily scan past lots of other tokens until it finds the beginning of the next block command, and misinterprets that as the "end."

We could fix that by having the loop here break if it hits a token where classType isn't null/"comment", but I worry a little bit that the general assumption -- 'hey, I found a "###", we must have found a start token' -- could bite us in other spots too. I wonder if we could fix _findCommentStart() to read ahead to verify that there's a comment token afterward. (Seems like there's a little wrinkle that, since this munges the ctx arg, it could push ctx too far forward if anyone else, like _findCommentEnd(), is expecting to reuse that context obj... but hopefully that's easy to address).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, the testcase where I was seeing this loop misbehave is:

###
math =
  root:   Math.sqrt
###
asda
a
adsadsadas
###
math =
  root:   Math.sqrt
###

Select from the start of the 2nd "###" to the start of the line after the 3rd "###". Toggle Block Comment strips the two "###"s out of the selection, whereas in a similar situation with JS block comments it no-ops.

return result ? {line: ctx.pos.line, ch: ctx.token.end - suffixLen} : null;
var pos = {line: ctx.pos.line, ch: ctx.token.end - suffixLen};

// If the position found is the same as the prefix one, start again from the next token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to note why/when this can happen: when the block-comment prefix syntax === the suffix.

if (result && prefixPos && prefixPos.line === pos.line && prefixPos.ch === pos.ch) {
result = TokenUtils.moveSkippingWhitespace(TokenUtils.moveNextToken, ctx);
return result ? _findCommentEnd(ctx, suffixExp, suffixLen) : null;
} else {
return result ? pos : null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repeated checks for result make the logic a tiny bit harder to follow -- maybe wrap this all in an if (result) { ... } else { return null; } to simplify? (Then you can remove the ternary operators, move the pos decl inside the if body, etc.)

}

/**
Expand Down Expand Up @@ -233,7 +242,8 @@ define(function (require, exports, module) {
}

// Check if we should just do a line uncomment (if all lines in the selection are commented).
if (lineExp && (ctx.token.string.match(lineExp) || endCtx.token.string.match(lineExp))) {
if (lineExp && (ctx.token.string.match(lineExp) || endCtx.token.string.match(lineExp)) &&
!ctx.token.string.match(prefixExp)) {
var startCtxIndex = editor.indexFromPos({line: ctx.pos.line, ch: ctx.token.start});
var endCtxIndex = editor.indexFromPos({line: endCtx.pos.line, ch: endCtx.token.start + endCtx.token.string.length});

Expand Down Expand Up @@ -264,7 +274,7 @@ define(function (require, exports, module) {
}
} else {
prefixPos = _findCommentStart(startCtx, prefixExp);
suffixPos = _findCommentEnd(startCtx, suffixExp, suffix.length);
suffixPos = _findCommentEnd(startCtx, suffixExp, suffix.length, prefixPos);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the code already worked like this, but... I'm finding it a little confusing that both lines here mutate startCtx (if I'm reading _findCommentEnd() correctly). So by the end of the 2nd line here startCtx is actually pointing to the end of the block comment, no? (Which is also potentially confusable with endCtx, i.e. the end of the selection). Seems maybe worth leaving some comments up where these are declared, at least, indicating what they'll mean (or whether they're just scratch vars).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I believe it should always use the ctx, since this is the one used at the end to check if there might be another block comment, making startCtx and endCtx stay always at the start and end of the selection. I did the change and all the tests pass.

}

// If we are in a selection starting and ending in invalid tokens and with no content (not considering spaces),
Expand All @@ -276,9 +286,9 @@ define(function (require, exports, module) {
// We found a comment, find the start and end and check if the selection is inside the block-comment.
if (startCtx.token.className === "comment") {
prefixPos = _findCommentStart(startCtx, prefixExp);
suffixPos = _findCommentEnd(startCtx, suffixExp, suffix.length);
suffixPos = _findCommentEnd(startCtx, suffixExp, suffix.length, prefixPos);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see four copies of this fix... is it possible to centralize this pair of lines into a shared function? (The last of the four occurrences isn't quite paired up as cleanly but I bet we could still get it to share code with the others, given that the else case above it is a dupe of the other line in the pair...)


if (prefixPos !== null && suffix !== null && !editor.posWithinRange(sel.start, prefixPos, suffixPos)) {
if (prefixPos !== null && suffixPos !== null && !editor.posWithinRange(sel.start, prefixPos, suffixPos)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was a typo in the original code, but I'm wondering... in what case is the suffixPos check actually needed? That is, when would we get prefixPos !== null but suffixPos === null?

canComment = true;
}
} else {
Expand All @@ -288,7 +298,7 @@ define(function (require, exports, module) {
// If the start is inside a comment, find the prefix and suffix positions.
} else if (ctx.token.className === "comment") {
prefixPos = _findCommentStart(ctx, prefixExp);
suffixPos = _findCommentEnd(ctx, suffixExp, suffix.length);
suffixPos = _findCommentEnd(ctx, suffixExp, suffix.length, prefixPos);

// If not try to find the first comment inside the selection.
} else {
Expand All @@ -303,7 +313,7 @@ define(function (require, exports, module) {
} else {
prefixPos = {line: ctx.pos.line, ch: ctx.token.start};
}
suffixPos = _findCommentEnd(ctx, suffixExp, suffix.length);
suffixPos = _findCommentEnd(ctx, suffixExp, suffix.length, prefixPos);
}
}

Expand Down