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

Adding support for multiple lineComment prefixes #3121

Merged
redmunds merged 6 commits into from
Mar 16, 2013
Merged

Adding support for multiple lineComment prefixes #3121

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
36e60ac
Adding support for multiple lineComment prefixes
TomMalbran Mar 13, 2013
a07aff9
Fixing tests
TomMalbran Mar 13, 2013
a3352f2
Allowing setLineCommentSyntax to take a string or an array of strings…
TomMalbran Mar 13, 2013
b6a7769
Fixes after first review
TomMalbran Mar 15, 2013
25287ad
Fixes after second review and Editor Command Handlers tests
TomMalbran Mar 16, 2013
379af7b
Fixes after third review
TomMalbran Mar 16, 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
106 changes: 81 additions & 25 deletions src/editor/EditorCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,71 @@ define(function (require, exports, module) {
var DIRECTION_DOWN = +1;


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

/**
* @private
* Returns true if any regular expression matches the given string
* @param {!string} string - where to look
* @param {!Array.<RegExp>} expressions - what to look
* @return {boolean}
*/
function _matchExpressions(string, expressions) {
return expressions.some(function (exp) {
return string.match(exp);
});
}

/**
* @private
* Returns the line comment prefix that best matches the string. Since there might be line comment prefixes
* that are prefixes of other line comment prefixes, it searches throught all and returns the longest line
* comment prefix that matches the string.
* @param {!string} string - where to look
* @param {!Array.<RegExp>} expressions - the line comment regular expressions
* @param {!Array.<string>} prefixes - the line comment prefixes
* @return {string}
*/
function _getLinePrefix(string, expressions, prefixes) {
var result = null;
expressions.forEach(function (exp, index) {
Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 15, 2013

Choose a reason for hiding this comment

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

Is needed here to cover the entire array to cover cases where the line prefixes are prefixes of each other and find the best match. Peter mentioned a case in #3056 where in Clojure people use ; and ;; for line commenting, and this should cover that case.

if (string.match(exp) && ((result && result.length < prefixes[index].length) || !result)) {
result = prefixes[index];
}
Copy link
Copy Markdown
Contributor

@redmunds redmunds Mar 15, 2013

Choose a reason for hiding this comment

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

Please add a comment to the explaining that you need to test all expressions so the longest match is found.

Also, when I look in languages.json, I see an entry for clojure, but there are no lineComment prefixes specified. Should they be?

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 15, 2013

Choose a reason for hiding this comment

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

There could be. I never used Clojure before, so don't know much about it, but would still be possible to test and add both line comments prefixes since Peter mentioned that both are used. I did a dummy test using in JavaScript the line comment prefixes ["//", "////"] and it works great.

});
return result;
}

/**
* @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>} lineExp - an array of line comment prefixes regular expressions
* @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, lineExp) {
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/)) {
// A line is commented out if it starts with 0-N whitespace chars, then a line comment prefix
if (line.match(/\S/) && !_matchExpressions(line, lineExp)) {
containsUncommented = true;
Copy link
Copy Markdown
Contributor

@redmunds redmunds Mar 14, 2013

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,12 +125,16 @@ 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) {
var doc = editor.document;
var sel = editor.getSelection();
var startLine = sel.start.line;
var endLine = sel.end.line;
function lineCommentPrefix(editor, prefixes) {
var doc = editor.document,
sel = editor.getSelection(),
startLine = sel.start.line,
endLine = sel.end.line,
lineExp = _createLineExpressions(prefixes);

// Is a range of text selected? (vs just an insertion pt)
var hasSelection = (startLine !== endLine) || (sel.start.ch !== sel.end.ch);
Expand All @@ -95,31 +147,35 @@ 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 containsUncommented = _containsUncommented(editor, startLine, endLine, lineExp);
var i;
var line;
var prefix;
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 the prefix on each line (if any)
for (i = startLine; i <= endLine; i++) {
line = doc.getLine(i);
var commentI = line.indexOf(prefix);
if (commentI !== -1) {
line = doc.getLine(i);
prefix = _getLinePrefix(line, lineExp, prefixes);

if (prefix) {
commentI = line.indexOf(prefix);
doc.replaceRange("", {line: i, ch: commentI}, {line: i, ch: commentI + prefix.length});
}
}
Expand Down Expand Up @@ -206,9 +262,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 +273,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 && linePrefixes.length ? _createLineExpressions(linePrefixes) : null,
prefixPos = null,
suffixPos = null,
canComment = false,
Expand All @@ -233,13 +289,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 +311,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, lineExp)) {
lineUncomment = true;

// Block-comment in all the other cases
Expand Down Expand Up @@ -327,7 +383,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
View file
Open in desktop
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: ["//"]
});
});
38 changes: 25 additions & 13 deletions src/language/LanguageManager.js
View file
Open in desktop
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

@redmunds redmunds Mar 16, 2013

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

@redmunds redmunds Mar 14, 2013

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

@TomMalbran TomMalbran Mar 15, 2013

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

@redmunds redmunds Mar 15, 2013

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

@redmunds redmunds Mar 15, 2013

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

@DennisKehrig DennisKehrig Mar 19, 2013

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

@peterflynn peterflynn Mar 22, 2013

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,37 @@ 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

@redmunds redmunds Mar 16, 2013

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
* Returns an array of prefixes to use for line comments.
* @return {Array.<string>} The prefixes
Copy link
Copy Markdown
Contributor

@redmunds redmunds Mar 14, 2013

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

@redmunds redmunds Mar 16, 2013

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;
};

/**
* Sets the prefix to use for line comments in this language.
* @param {!string} prefix Prefix string to use for block comments (i.e. "//")
* Sets the prefixes to use for line comments in this language.
* @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];
var i;

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

@redmunds redmunds Mar 14, 2013

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

@TomMalbran TomMalbran Mar 15, 2013

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();
} else {
console.error("The prefix array should not be empty");
}
};

/**
Expand Down Expand Up @@ -564,7 +576,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
16 changes: 9 additions & 7 deletions src/language/languages.json
View file
Open in desktop
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 All @@ -92,7 +93,8 @@
"clojure": {
"name": "Clojure",
"mode": "clojure",
"fileExtensions": ["clj"]
"fileExtensions": ["clj"],
"lineComment": [";", ";;"]
},

"perl": {
Expand Down
Loading
⚡