Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
81 changes: 60 additions & 21 deletions src/editor/EditorCommandHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,53 @@ define(function (require, exports, module) {
var DIRECTION_DOWN = +1;


/**
* @private
* Creates regular expressions for multiple line comments prefixes
* @param {Array.<string>} prefixes - Description
* @return {Array.<RegExp>}
*/
function _createLineExpressions(prefixes) {
var lineExps = [];
prefixes.forEach(function (prefix) {
lineExps.push(new RegExp("^\\s*" + StringUtils.regexEscape(prefix)));
});
return lineExps;
}

/**
* @private
* Returns true if any regular expression matches the given string
* @param {string} string
* @param {Array.<RegExp>} expressions
* @return {boolean}
*/
function _matchExpressions(string, expressions) {
var matchAny = false;
expressions.forEach(function (exp) {
matchAny = matchAny || string.match(exp);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This loop searches for every expression no matter what. It should stop searching after a match is found.

});
return matchAny;
}

/**
* @private
* Searchs for an uncommented line between startLine and endLine
* @param {!Editor} editor
* @param {!number} startLine - valid line inside the document
* @param {!number} endLine - valid line inside the document
* @param {!Array.<string>} prefixes - Array of line comment prefixes
* @return {boolean} true if there is at least one uncommented line
*/
function _containsUncommented(editor, startLine, endLine, prefix) {
var lineExp = new RegExp("^\\s*" + StringUtils.regexEscape(prefix));
function _containsUncommented(editor, startLine, endLine, prefixes) {
var lineExp = _createLineExpressions(prefixes);
var containsUncommented = false;
var i;
var line;
for (i = startLine; i <= endLine; i++) {
line = editor.document.getLine(i);
// A line is commented out if it starts with 0-N whitespace chars, then "//"
if (!line.match(lineExp) && line.match(/\S/)) {
if (!_matchExpressions(line, lineExp) && line.match(/\S/)) {
containsUncommented = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The second condition will probably evaluate faster than the first condition, so the order should be reversed:

    if (line.match(/\S/) && !_matchExpressions(line, lineExp)) {

break;
}
Expand All @@ -77,8 +107,11 @@ define(function (require, exports, module) {
* If all non-whitespace lines are already commented out, then we uncomment; otherwise we comment
* out. Commenting out adds the prefix at column 0 of every line. Uncommenting removes the first prefix
* on each line (if any - empty lines might not have one).
*
* @param {!Editor} editor
* @param {!Array.<string>} prefixes, e.g. ["//"]
*/
function lineCommentPrefix(editor, prefix) {
function lineCommentPrefix(editor, prefixes) {
var doc = editor.document;
var sel = editor.getSelection();
var startLine = sel.start.line;
Expand All @@ -95,32 +128,38 @@ define(function (require, exports, module) {
// Decide if we're commenting vs. un-commenting
// Are there any non-blank lines that aren't commented out? (We ignore blank lines because
// some editors like Sublime don't comment them out)
var containsUncommented = _containsUncommented(editor, startLine, endLine, prefix);
var i;
var containsUncommented = _containsUncommented(editor, startLine, endLine, prefixes);
var i, j;
var line;
var trimmedLine;
var commentI;
var updateSelection = false;

// Make the edit
doc.batchOperation(function () {

if (containsUncommented) {
// Comment out - prepend "//" to each line
// Comment out - prepend the first prefix to each line
for (i = startLine; i <= endLine; i++) {
doc.replaceRange(prefix, {line: i, ch: 0});
doc.replaceRange(prefixes[0], {line: i, ch: 0});
}

// Make sure selection includes "//" that was added at start of range
// Make sure selection includes the prefix that was added at start of range
if (sel.start.ch === 0 && hasSelection) {
updateSelection = true;
}

} else {
// Uncomment - remove first "//" on each line (if any)
// Uncomment - remove first every prefix on each line (if any)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Improved wording: remove prefix on each line (if any)

for (i = startLine; i <= endLine; i++) {
line = doc.getLine(i);
var commentI = line.indexOf(prefix);
if (commentI !== -1) {
doc.replaceRange("", {line: i, ch: commentI}, {line: i, ch: commentI + prefix.length});
trimmedLine = line.trim();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The trim() method is unnecessarily expensive since it does actual string manipulation (allocates new string and copies data) but all you want to do is search the string. You should use the `_matchExpressions()' function here instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. I just needed to know if the prefix is at the start of the string, since if you comment with both prefixes, it could unprefix the wrong one. For example in #comment 1 //comment 2 it would remove the // instead of removing the # as wanted. Maybe just doing a match with the prefix expression wanted would be better.

for (j = 0; j < prefixes.length; j++) {
commentI = line.indexOf(prefixes[j]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's no need to search for commentI until you're inside the following if block.

if (trimmedLine.indexOf(prefixes[j]) === 0) {
doc.replaceRange("", {line: i, ch: commentI}, {line: i, ch: commentI + prefixes[j].length});
break;
}
}
}
}
Expand Down Expand Up @@ -206,9 +245,9 @@ define(function (require, exports, module) {
* @param {!Editor} editor
* @param {!string} prefix, e.g. "<!--"
* @param {!string} suffix, e.g. "-->"
* @param {?string} linePrefix, e.g. "//"
* @param {?Array.<string>} linePrefixes, e.g. ["//"]
*/
function blockCommentPrefixSuffix(editor, prefix, suffix, linePrefix) {
function blockCommentPrefixSuffix(editor, prefix, suffix, linePrefixes) {

var doc = editor.document,
sel = editor.getSelection(),
Expand All @@ -217,7 +256,7 @@ define(function (require, exports, module) {
endCtx = TokenUtils.getInitialContext(editor._codeMirror, {line: sel.end.line, ch: sel.end.ch}),
prefixExp = new RegExp("^" + StringUtils.regexEscape(prefix), "g"),
suffixExp = new RegExp(StringUtils.regexEscape(suffix) + "$", "g"),
lineExp = linePrefix ? new RegExp("^" + StringUtils.regexEscape(linePrefix)) : null,
lineExp = linePrefixes ? _createLineExpressions(linePrefixes) : null,
prefixPos = null,
suffixPos = null,
canComment = false,
Expand All @@ -233,13 +272,13 @@ 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 && (_matchExpressions(ctx.token.string, lineExp) || _matchExpressions(endCtx.token.string, lineExp))) {
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});

// Find if we aren't actually inside a block-comment
result = true;
while (result && ctx.token.string.match(lineExp)) {
while (result && _matchExpressions(ctx.token.string, lineExp)) {
result = TokenUtils.moveSkippingWhitespace(TokenUtils.movePrevToken, ctx);
}

Expand All @@ -255,7 +294,7 @@ define(function (require, exports, module) {
}

// Find if all the lines are line-commented.
if (!_containsUncommented(editor, sel.start.line, endLine, linePrefix)) {
if (!_containsUncommented(editor, sel.start.line, endLine, linePrefixes)) {
lineUncomment = true;

// Block-comment in all the other cases
Expand Down Expand Up @@ -327,7 +366,7 @@ define(function (require, exports, module) {
return;

} else if (lineUncomment) {
lineCommentPrefix(editor, linePrefix);
lineCommentPrefix(editor, linePrefixes);

} else {
doc.batchOperation(function () {
Expand Down
2 changes: 1 addition & 1 deletion src/extensions/default/LESSSupport/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,6 @@ define(function (require, exports, module) {
mode: "less",
fileExtensions: ["less"],
blockComment: ["/*", "*/"],
lineComment: "//"
lineComment: ["//"]
});
});
32 changes: 21 additions & 11 deletions src/language/LanguageManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ define(function (require, exports, module) {
this._fileExtensions = [];
this._fileNames = [];
this._modeToLanguageMap = {};
this._lineCommentSyntax = [];
}


Expand All @@ -284,12 +285,12 @@ define(function (require, exports, module) {
/** @type {Array.<string>} File names for extensionless files that use this language */
Language.prototype._fileNames = null;

/** @type {{ prefix: string }} Line comment syntax */
Language.prototype._lineCommentSyntax = null;

/** @type {Object.<string,Language>} Which language to use for what CodeMirror mode */
Language.prototype._modeToLanguageMap = null;

/** @type {Array.<string>} Line comment syntax */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this code back up where it originally was above Language.prototype._modeToLanguageMap so the diff is easier to read.

Language.prototype._lineCommentSyntax = null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is an array, so why isn't this (and many other similar data members) initialized as an empty array?

    Language.prototype._lineCommentSyntax = [];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually this doesn't really initialize anything, since prototype functions and arguments are shared by every instance of the object. It doesn't even make sense to have this here, besides documentation since they need to be different for every instance. The real initialization is done inside the constructor or the setters, where _lineCommentSyntax is initialized for that instance. If the constructor or any setter wouldn't initialize the properties, then it would return that null value from the prototype.

I was actually curious about the purpose of this properties since aren't really used. When accessing the members of an instance it first searches for the members set with this.property and then it searches through the prototypes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good question.

@DennisKehrig @peterflynn @jasonsanjose What's the purpose of the Language.prototype data members? For example:

    Language.prototype._lineCommentSyntax = null;

_lineCommentSyntax is redefined in the constructor, so it does not seem to be used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess this doesn't need to be solved here. My main concern was dereferencing a null object reference, but this code always uses this._lineCommentSyntax so it looks good.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@redmunds re: What's the purpose of the Language.prototype data members?

I think they are mainly there so we have a place to add documentation to.

They might also have a positive impact on performance as V8 has its own concept of classes and assigns objects with the same properties the same class. So if you take an empty object, and add properties to it one by one, its class changes every time. If all properties are known from the start, one class object is used for all instances, resulting in better optimization.
Note that this is just a wild guess based on something I remembered from reading about how to optimize JavaScript code, I am not sure that this works when the properties are defined in the prototype rather than the object itself.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fwiw, the main motivations in my mind are (a) a place to hang documentation off of (without cluttering up the constructor body code), and (b) having all 'class members' listed out in one centralized place. The V8 benefit is neat too though -- hadn't occurred to me, but that meshes with what I've read about V8 too.


/** @type {{ prefix: string, suffix: string }} Block comment syntax */
Language.prototype._blockCommentSyntax = null;

Expand Down Expand Up @@ -449,26 +450,35 @@ define(function (require, exports, module) {
* @return {boolean} Whether line comments are supported
*/
Language.prototype.hasLineCommentSyntax = function () {
return Boolean(this._lineCommentSyntax);
return Boolean(this._lineCommentSyntax.length);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The intent of the code would be more clear to return a boolean expression than to convert an integer to Boolean type:

    return (this._lineCommentSyntax.length > 0);

};

/**
* Returns the prefix to use for line comments.
* @return {string} The prefix
* @return {Array.<string>} The prefixes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this "Returns the prefix to use for line comments", then the return type is still {string}, right?

*/
Language.prototype.getLineCommentPrefix = function () {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be changed to should be changed to getLineCommentPrefixes now that there can be multiple.

return this._lineCommentSyntax && this._lineCommentSyntax.prefix;
return this._lineCommentSyntax.length && this._lineCommentSyntax;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should return the item at index 0 or an empty string:

    return (this._lineCommentSyntax.length > 0) ? this._lineCommentSyntax[0] : "";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The comment should now be, "Returns the prefixes used to line comments". Both line commenting and block commenting now handle multiple line comment prefixes, since they need to, so it easier if they always receive an array, even if it is of one element as in most cases. I don't think that there could be a use to have the first comment prefix, except when there is one, but to cover all cases an array is better. I also don't think that this is used by extensions yet and is only used in brackets. But if needed I could make 2 factions for backwards compatibility.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, but if it's returning the array, why does it need to check the length? Can it just be:

    return this._lineCommentSyntax;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should work after changing BlockComment to identify the empty array as null.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand what you mean by this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Block comment always receives as a third parameter getLineCommentPrefix() and checks if is defined or not when creating the line prefix, so it works with languages that don't have line comment prefixes. This was changed with the addition of the LanguageManager, since before it received a bool. But it can be changed to check for the length of the array too, or make it always receive an array and check for the length, to make it work for languages that don't have line prefixes.

};

/**
* Sets the prefix to use for line comments in this language.
* @param {!string} prefix Prefix string to use for block comments (i.e. "//")
* @param {!string|Array.<string>} prefix Prefix string or and array of prefix strings
* to use for line comments (i.e. "//" or ["//", "#"])
*/
Language.prototype.setLineCommentSyntax = function (prefix) {
_validateNonEmptyString(prefix, "prefix");
var prefixes = !Array.isArray(prefix) ? [prefix] : prefix;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It adds an extra level of abstraction to reverse the logic since it has to work either way. This is simpler:

    var prefixes = Array.isArray(prefix) ? prefix : [prefix];

var i;

this._lineCommentSyntax = { prefix: prefix };
this._wasModified();
if (prefixes.length) {
this._lineCommentSyntax = [];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reseting array to empty should be done outside the if (prefixes.length) block, so Array is also reset if empty.

this._wasModified() also needs to be moved outside of the if (prefixes.length) block.

UPDATED: I see that the code throws an exception if string passed in or any array item is empty, so maybe instead you should throw an exception if an empty array is passed in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. It shows a console error because of #2967, and I will make it work like #3034 once is merge. I updated the test to cover this case too.

for (i = 0; i < prefixes.length; i++) {
_validateNonEmptyString(String(prefixes[i]), "prefix");

this._lineCommentSyntax.push(prefixes[i]);
}
this._wasModified();
}
};

/**
Expand Down Expand Up @@ -564,7 +574,7 @@ define(function (require, exports, module) {
* @param {!string} definition.name Human-readable name of the language, as it's commonly referred to (i.e. "C++")
* @param {Array.<string>} definition.fileExtensions List of file extensions used by this language (i.e. ["php", "php3"])
* @param {Array.<string>} definition.blockComment Array with two entries defining the block comment prefix and suffix (i.e. ["<!--", "-->"])
* @param {string} definition.lineComment Line comment prefix (i.e. "//")
* @param {string|Array.<string>} definition.lineComment Line comment prefixes (i.e. "//" or ["//", "#"])
* @param {string|Array.<string>} definition.mode CodeMirror mode (i.e. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
* Unless the mode is located in thirdparty/CodeMirror2/mode/<name>/<name>.js, you need to first load it yourself.
*
Expand Down
13 changes: 7 additions & 6 deletions src/language/languages.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,14 @@
"mode": "javascript",
"fileExtensions": ["js", "jsx"],
"blockComment": ["/*", "*/"],
"lineComment": "//"
"lineComment": ["//"]
},

"json": {
"name": "JSON",
"mode": ["javascript", "application/json"],
"fileExtensions": ["json"]
"fileExtensions": ["json"],
"lineComment": ["//"]
},

"xml": {
Expand All @@ -50,30 +51,30 @@
"mode": ["clike", "text/x-csrc"],
"fileExtensions": ["c", "h", "i"],
"blockComment": ["/*", "*/"],
"lineComment": "//"
"lineComment": ["//"]
},

"cpp": {
"name": "C++",
"mode": ["clike", "text/x-c++src"],
"fileExtensions": ["cc", "cp", "cpp", "c++", "cxx", "hh", "hpp", "hxx", "h++", "ii"],
"blockComment": ["/*", "*/"],
"lineComment": "//"
"lineComment": ["//"]
},

"csharp": {
"name": "C#",
"mode": ["clike", "text/x-csharp"],
"fileExtensions": ["cs"],
"blockComment": ["/*", "*/"],
"lineComment": "//"
"lineComment": ["//"]
},

"clike": {
"name": "clike",
"mode": "clike",
"blockComment": ["/*", "*/"],
"lineComment": "//"
"lineComment": ["//", "#"]
},

"java": {
Expand Down
45 changes: 36 additions & 9 deletions test/spec/LanguageManager-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ define(function (require, exports, module) {
def.blockComment = [def.blockComment.prefix, def.blockComment.suffix];
}

if (def.lineComment) {
def.lineComment = def.lineComment.prefix;
}

return LanguageManager.defineLanguage(definition.id, def);
}

Expand All @@ -76,8 +72,9 @@ define(function (require, exports, module) {
}

if (expected.lineComment) {
var lineComment = Array.isArray(expected.lineComment) ? expected.lineComment : [expected.lineComment];
expect(actual.hasLineCommentSyntax()).toBe(true);
expect(actual.getLineCommentPrefix()).toBe(expected.lineComment.prefix);
expect(actual.getLineCommentPrefix().toString()).toBe(lineComment.toString());
} else {
expect(actual.hasLineCommentSyntax()).toBe(false);
}
Expand Down Expand Up @@ -259,21 +256,51 @@ define(function (require, exports, module) {
expect(function () { language.setBlockCommentSyntax("<!---", ""); }).toThrow(new Error("suffix must not be empty"));
expect(function () { language.setBlockCommentSyntax("", "--->"); }).toThrow(new Error("prefix must not be empty"));

def.lineComment = {
prefix: "//"
};
def.lineComment = "//";
def.blockComment = {
prefix: "<!---",
suffix: "--->"
};

language.setLineCommentSyntax(def.lineComment.prefix);
language.setLineCommentSyntax(def.lineComment);
language.setBlockCommentSyntax(def.blockComment.prefix, def.blockComment.suffix);

validateLanguage(def, language);
});
});

it("should validate multiple line comment prefixes", function () {
var def = { id: "php2", name: "PHP2", fileExtensions: ["php2"], mode: "php" },
language;

runs(function () {
defineLanguage(def).done(function (lang) {
language = lang;
});
});

waitsFor(function () {
return Boolean(language);
}, "The language should be resolved", 50);

runs(function () {
expect(function () { language.setLineCommentSyntax([""]); }).toThrow(new Error("prefix must not be empty"));
expect(function () { language.setLineCommentSyntax(["#", ""]); }).toThrow(new Error("prefix must not be empty"));

def.lineComment = ["#"];

language.setLineCommentSyntax(def.lineComment);
validateLanguage(def, language);
});

runs(function () {
def.lineComment = ["#", "//"];

language.setLineCommentSyntax(def.lineComment);
validateLanguage(def, language);
});
});

it("should load a built-in CodeMirror mode", function () {
var id = "erlang",
def = { id: id, name: "erlang", fileExtensions: ["erlang"], mode: "erlang" },
Expand Down