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

Avoid searching binary files; tweaks to language mappings; APIs to unregister file extension mapping #7290

Merged
merged 4 commits into from
Mar 28, 2014
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
88 changes: 79 additions & 9 deletions src/language/LanguageManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -466,11 +466,17 @@ define(function (require, exports, module) {
};

/**
* Adds a file extension to this language.
* @param {!string} extension A file extension used by this language
* @return {boolean} Whether adding the file extension was successful or not
* Adds one or more file extensions to this language.
* @param {!string|Array.<string>>} extension A file extension (or array thereof) used by this language
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor Nit: extra >

*/
Language.prototype.addFileExtension = function (extension) {
if (Array.isArray(extension)) {
extension.forEach(this._addFileExtension.bind(this));
} else {
this._addFileExtension(extension);
}
};
Language.prototype._addFileExtension = function (extension) {
// Remove a leading dot if present
if (extension.charAt(0) === ".") {
extension = extension.substr(1);
Expand All @@ -494,11 +500,47 @@ define(function (require, exports, module) {
};

/**
* Adds a file name to the language which is used to match files that don't have extensions like "Makefile" for example.
* @param {!string} extension An extensionless file name used by this language
* @return {boolean} Whether adding the file name was successful or not
* Unregisters one or more file extensions from this language.
* @param {!string|Array.<string>} extension File extension (or array thereof) to stop using for this language
*/
Language.prototype.removeFileExtension = function (extension) {
if (Array.isArray(extension)) {
extension.forEach(this._removeFileExtension.bind(this));
} else {
this._removeFileExtension(extension);
}
};
Language.prototype._removeFileExtension = function (extension) {
// Remove a leading dot if present
if (extension.charAt(0) === ".") {
extension = extension.substr(1);
}

// Make checks below case-INsensitive
extension = extension.toLowerCase();

var index = this._fileExtensions.indexOf(extension);
if (index !== -1) {
this._fileExtensions.splice(index, 1);

delete _fileExtensionToLanguageMap[extension];

this._wasModified();
}
};

/**
* Adds one or more file names to the language which is used to match files that don't have extensions like "Makefile" for example.
* @param {!string|Array.<string>} extension An extensionless file name (or array thereof) used by this language
*/
Language.prototype.addFileName = function (name) {
if (Array.isArray(name)) {
name.forEach(this._addFileName.bind(this));
} else {
this._addFileName(name);
}
};
Language.prototype._addFileName = function (name) {
// Make checks below case-INsensitive
name = name.toLowerCase();

Expand All @@ -514,7 +556,31 @@ define(function (require, exports, module) {

this._wasModified();
}
return true;
};

/**
* Unregisters one or more file names from this language.
* @param {!string|Array.<string>} extension An extensionless file name (or array thereof) used by this language
*/
Language.prototype.removeFileName = function (name) {
if (Array.isArray(name)) {
name.forEach(this._removeFileName.bind(this));
} else {
this._removeFileName(name);
}
};
Language.prototype._removeFileName = function (name) {
// Make checks below case-INsensitive
name = name.toLowerCase();

var index = this._fileNames.indexOf(name);
if (index !== -1) {
this._fileNames.splice(index, 1);

delete _fileNameToLanguageMap[name];

this._wasModified();
}
};

/**
Expand Down Expand Up @@ -770,11 +836,11 @@ define(function (require, exports, module) {
_patchCodeMirror();

// Define a custom MIME mode here instead of putting it directly into languages.json
// because JSON files must not contain regular expressions. Also, all other modes so
// because JSON files can't contain regular expressions. Also, all other modes so
// far were strings, so we spare us the trouble of allowing more complex mode values.
CodeMirror.defineMIME("text/x-brackets-html", {
"name": "htmlmixed",
"scriptTypes": [{"matches": /\/x-handlebars-template|\/x-mustache/i,
"scriptTypes": [{"matches": /\/x-handlebars|\/x-mustache|^text\/html$/i,
"mode": null}]
});

Expand All @@ -796,6 +862,10 @@ define(function (require, exports, module) {
// But for now, we need to associate this madeup "html" mode with our HTML language object.
_setLanguageForMode("html", html);

// Similarly, the php mode uses clike internally for the PHP parts
var php = getLanguage("php");
php._setLanguageForMode("clike", php);

// The fallback language for unknown modes and file extensions
_fallbackLanguage = getLanguage("unknown");
});
Expand Down
22 changes: 13 additions & 9 deletions src/language/languages.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@
"php": {
"name": "PHP",
"mode": "php",
"fileExtensions": ["php", "php3", "php4", "php5", "phtm", "phtml", "ctp"]
"fileExtensions": ["php", "php3", "php4", "php5", "phtm", "phtml", "ctp"],
"blockComment": ["/*", "*/"],
"lineComment": ["//", "#"]
},

"c": {
Expand All @@ -120,13 +122,6 @@
"lineComment": ["//"]
},

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

"java": {
"name": "Java",
"mode": ["clike", "text/x-java"],
Expand All @@ -138,7 +133,9 @@
"scala": {
"name": "Scala",
"mode": ["clike", "text/x-scala"],
"fileExtensions": ["scala", "sbt"]
"fileExtensions": ["scala", "sbt"],
"blockComment": ["/*", "*/"],
"lineComment": ["//"]
},

"coffeescript": {
Expand Down Expand Up @@ -242,5 +239,12 @@
"name": "Audio",
"fileExtensions": ["mp3", "wav", "aif", "aiff", "ogg"],
"isBinary": true
},

"binary": {
"name": "Other Binary",
"fileExtensions": ["svgz", "jsz", "zip", "gz", "htmz", "htmlz", "rar", "tar", "exe", "bin", "dll", "pdb", "lib", "obj", "so", "a", "dylib",
"pdf", "psd", "ai", "tif", "tiff", "mpg", "mpeg", "avi", "flv", "mp4", "ttf", "otf", "woff"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this break extensions like mine that creates a custom viewer for fonts since now some of the extensions are in this unused language? https://github.com/TomMalbran/brackets-fonts-viewer

My extension defines a font language, and uses it to register a custom font viewer. We could have other extensions doing the same for pdf, zip files and others.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good point - so we should hold off on merging this part until #6873 is implemented

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could at least split them and give them better languages and not put them all in the same bag. But we could still run into the same issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should think about grouping them a bit more in order to make some more custom viewers available. Maybe think of these:

  • video
  • audio
  • fonts

And why don't we add .tiff or such to the image group? The image viewer should still work, so there are no problems, are they?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don`t think that we need separate languages, I would just put them all together. A custom viewer extension can just remove the file extensions from this list and create the language. Also, we can't even open audio or video files in Brackets, so doesn't make sense to make languages that we are not using.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think if we're not doing any special handling of them we should just lump them all together for now. As @TomMalbran says, if an extension wants to provide support for a bunch of font type, or video, or whatnot, it can register a new language and take over those file extensions itself.

I'm not sure why we even break out audio separately -- but just in case extensions are relying on it, I'd like to leave it as-is for now.

@SAplayer The reason .tiff isn't listed under images is because Brackets doesn't support it -- our image-preview UI can't render it, and it's not a format used when coding CSS/HTML (for the same reason).

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we can break extensions that use any of the new file extensions added here, we could just also break extensions that use the audio language. I don't think that there should be any with it since you can't do much with it. Can you check that?

"isBinary": true
}
}
4 changes: 3 additions & 1 deletion src/search/FindInFiles.js
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,9 @@ define(function (require, exports, module) {
var inWorkingSet = DocumentManager.getWorkingSet().some(function (wsFile) {
return wsFile.fullPath === file.fullPath;
});
return inWorkingSet;
if (!inWorkingSet) {
return false;
}
}
}
// In the initial search, this is passed as a getAllFiles() filter
Expand Down
82 changes: 82 additions & 0 deletions test/spec/LanguageManager-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,88 @@ define(function (require, exports, module) {
expect(LanguageManager.getLanguageForPath("cakefile.doesNotExist")).toBe(unknown);
expect(LanguageManager.getLanguageForPath("Something.cakefile")).toBe(unknown);
});

it("should remove file extensions and add to new languages", function () {
var html = LanguageManager.getLanguage("html"),
ruby = LanguageManager.getLanguage("ruby"),
unknown = LanguageManager.getLanguage("unknown");

expect(LanguageManager.getLanguageForPath("test.html")).toBe(html);

html.removeFileExtension("html");
expect(LanguageManager.getLanguageForPath("test.html")).toBe(unknown);

ruby.addFileExtension("html");
expect(LanguageManager.getLanguageForPath("test.html")).toBe(ruby);
});

it("should remove file names and add to new languages", function () {
var coffee = LanguageManager.getLanguage("coffeescript"),
html = LanguageManager.getLanguage("html"),
unknown = LanguageManager.getLanguage("unknown");

expect(LanguageManager.getLanguageForPath("Cakefile")).toBe(coffee);

coffee.removeFileName("Cakefile");
expect(LanguageManager.getLanguageForPath("Cakefile")).toBe(unknown);

html.addFileName("Cakefile");
expect(LanguageManager.getLanguageForPath("Cakefile")).toBe(html);
});

it("should add multiple file extensions to languages", function () {
var ruby = LanguageManager.getLanguage("ruby"),
unknown = LanguageManager.getLanguage("unknown");

expect(LanguageManager.getLanguageForPath("foo.1")).toBe(unknown);
expect(LanguageManager.getLanguageForPath("foo.2")).toBe(unknown);

ruby.addFileExtension(["1", "2"]);

expect(LanguageManager.getLanguageForPath("foo.1")).toBe(ruby);
expect(LanguageManager.getLanguageForPath("foo.2")).toBe(ruby);
});

it("should remove multiple file extensions from languages", function () {
var ruby = LanguageManager.getLanguage("ruby"),
unknown = LanguageManager.getLanguage("unknown");

// Assumes test above already ran (tests in this suite are not isolated)
expect(LanguageManager.getLanguageForPath("foo.1")).toBe(ruby);
expect(LanguageManager.getLanguageForPath("foo.2")).toBe(ruby);

ruby.removeFileExtension(["1", "2"]);

expect(LanguageManager.getLanguageForPath("foo.1")).toBe(unknown);
expect(LanguageManager.getLanguageForPath("foo.2")).toBe(unknown);
});

it("should add multiple file names to languages", function () {
var ruby = LanguageManager.getLanguage("ruby"),
unknown = LanguageManager.getLanguage("unknown");

expect(LanguageManager.getLanguageForPath("rubyFile1")).toBe(unknown);
expect(LanguageManager.getLanguageForPath("rubyFile2")).toBe(unknown);

ruby.addFileName(["rubyFile1", "rubyFile2"]);

expect(LanguageManager.getLanguageForPath("rubyFile1")).toBe(ruby);
expect(LanguageManager.getLanguageForPath("rubyFile2")).toBe(ruby);
});

it("should remove multiple file names from languages", function () {
var ruby = LanguageManager.getLanguage("ruby"),
unknown = LanguageManager.getLanguage("unknown");

// Assumes test above already ran (tests in this suite are not isolated)
expect(LanguageManager.getLanguageForPath("rubyFile1")).toBe(ruby);
expect(LanguageManager.getLanguageForPath("rubyFile2")).toBe(ruby);

ruby.removeFileName(["rubyFile1", "rubyFile2"]);

expect(LanguageManager.getLanguageForPath("rubyFile1")).toBe(unknown);
expect(LanguageManager.getLanguageForPath("rubyFile2")).toBe(unknown);
});
});

describe("defineLanguage", function () {
Expand Down