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

Adds preferences that map extensions and filenames to languages. #7588

Merged
merged 2 commits into from
Apr 24, 2014
Merged

Adds preferences that map extensions and filenames to languages. #7588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99159de
Adds preferences that map extensions and filenames to languages.
dangoor Apr 21, 2014
3669cfd
Address review comments
dangoor Apr 23, 2014
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
149 changes: 141 additions & 8 deletions src/language/LanguageManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, $ */
/*unittests: LanguageManager*/

/**
* LanguageManager provides access to the languages supported by Brackets
Expand Down Expand Up @@ -117,18 +118,46 @@ define(function (require, exports, module) {
var CodeMirror = require("thirdparty/CodeMirror2/lib/codemirror"),
Async = require("utils/Async"),
FileUtils = require("file/FileUtils"),
_defaultLanguagesJSON = require("text!language/languages.json");

_defaultLanguagesJSON = require("text!language/languages.json"),
_ = require("thirdparty/lodash"),

// PreferencesManager is loaded near the end of the file
PreferencesManager;

// State
var _fallbackLanguage = null,
_pendingLanguages = {},
_languages = {},
_fileExtensionToLanguageMap = {},
_fileNameToLanguageMap = {},
_modeToLanguageMap = {},
var _fallbackLanguage = null,
_pendingLanguages = {},
_languages = {},
_baseFileExtensionToLanguageMap = {},
_fileExtensionToLanguageMap = Object.create(_baseFileExtensionToLanguageMap),
_fileNameToLanguageMap = {},
_modeToLanguageMap = {},
_ready;

// Constants

var _EXTENSION_MAP_PREF = "language.fileExtensions",
_NAME_MAP_PREF = "language.fileNames";

// Tracking for changes to mappings made by preferences
var _prefState = {};

_prefState[_EXTENSION_MAP_PREF] = {
last: {},
overridden: {},
add: "addFileExtension",
remove: "removeFileExtension",
get: "getLanguageForExtension"
};

_prefState[_NAME_MAP_PREF] = {
last: {},
overridden: {},
add: "addFileName",
remove: "removeFileName",
get: "getLanguageForPath"
};

// Helper functions

/**
Expand Down Expand Up @@ -840,6 +869,90 @@ define(function (require, exports, module) {
return result.promise();
}

/**
* @private
*
* If a default file extension or name was overridden by a pref, restore it.
*
* @param {string} name Extension or filename that should be restored
* @param {{overridden: string, add: string}} prefState object for the pref that is currently being updated
*/
function _restoreOverriddenDefault(name, state) {
if (state.overridden[name]) {
var language = getLanguage(state.overridden[name]);
language[state.add](name);
delete state.overridden[name];
}
}

/**
* @private
*
* Updates extension and filename mappings from languages based on the current preferences values.
*
* The preferences look like this in a prefs file:
*
* Map *.foo to javascript, *.vm to html
* "language.fileExtensions": {
* "foo": "javascript",
* "vm": "html"
* }
*
* Map "Gemfile" to ruby:
* "language.fileNames": {
* "Gemfile": "ruby"
* }
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

need to document the structure of pref (preference entry for either language.fileNames or language.fileExtensions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

function _updateFromPrefs(pref) {
var newMapping = PreferencesManager.get(pref) || {},
newNames = Object.keys(newMapping),
state = _prefState[pref],
last = state.last,
overridden = state.overridden;

// Look for added and changed names (extensions or filenames)
newNames.forEach(function (name) {
var language;
if (newMapping[name] !== last[name]) {
if (last[name]) {
language = getLanguage(last[name]);
if (language) {
language[state.remove](name);

// If this name that was previously mapped was overriding a default
// restore it now.
_restoreOverriddenDefault(name, state);
}
}

language = exports[state.get](name);
if (language) {
language[state.remove](name);

// We're removing a name that was defined in Brackets or an extension,
// so keep track of how it used to be mapped.
if (!overridden[name]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It will cause to fall back to the first override if it was a second override which the default might have been undefined. If it is important, perhaps null value should be used to indicate a lack of default.

Edit: if the file extension was previously unknown to Brackets, I mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't because it tracks the extensions added by prefs. The flow is something like this:

  1. pref maps .foo to javacript
  2. that goes through this code under newNames
  3. the last state now contains that foo -> javascript mapping
  4. pref changes to map .foo to php
  5. the newMapping of php doesn't match the last mapping of javascript
  6. the default mapping for .foo is restored (via that _restoreOveriddenDefault call)
  7. that default mapping is undefined in the case of .foo, so no override is saved
  8. then the new mapping of foo -> php is created

So, the only overrides that are saved are actually the default mappings since we always restore the default before creating a new mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I missed that the previous mapping is restored before.

overridden[name] = language.getId();
}
}
language = getLanguage(newMapping[name]);
if (language) {
language[state.add](name);
}
}
});

// Look for removed names (extensions or filenames)
_.difference(Object.keys(last), newNames).forEach(function (name) {
var language = getLanguage(last[name]);
if (language) {
language[state.remove](name);
_restoreOverriddenDefault(name, state);
}
});
state.last = newMapping;
}


// Prevent modes from being overwritten by extensions
_patchCodeMirror();
Expand Down Expand Up @@ -881,8 +994,28 @@ define(function (require, exports, module) {

// The fallback language for unknown modes and file extensions
_fallbackLanguage = getLanguage("unknown");

// There is a circular dependency between FileUtils and LanguageManager which
// was introduced in 254b01e2f2eebea4416026d0f40d017b8ca6dbc9
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not nice to have the circular dependency between those. FileUtils should not be aware of LanguageManager. Perhaps we should have a dictionary of known extensions (created and maintained by LanguageManager) which would be used by FileUtils. That would help to decouple these two. I don't know if it is worth fixing. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dangoor @busykai @zaggino Oops -- I was the reviewer on that change.

Another way to solve this is to move the getBaseName() function (and a few other file path utilities) out of FileUtils to remove the FileUtils dependency from LanguageManager. They could be moved to StringUtils or start a new FilePathUtils module.

Let me know if you think that's a good solution and I can make that change as part of the feature work I am currently working on for https://trello.com/c/Sji5hLvW/1219-1s-automatically-ignore-exclude-binary-files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree that FileUtils shouldn't really depend on LanguageManager (and the circular dependency is not good). I'm not sure what the cleanest fix would be, which is why I didn't attempt the fix.

@zaggino any opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, there's a typo in that line. Should be LanguageManager 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a tough one and I didn't really realize that there'll be circular dependency here. But I think that FilePathUtils module would be the best way to go here since those methods doesn't really deal with files but only with their paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dangoor @busykai If I don't hear any disagreement, I'll create a new FilePathUtils module for file path utilities and keep utilities for actual files in FileUtils. It sounds like this might also resolve problems for these comments in #6409.

cc @peterflynn @zaggino

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened a code cleanup issue for this: #7631

// and may be preventing us from importing PreferencesManager (which also
// depends on FileUtils) here. Using the async form of require fixes this.
require(["preferences/PreferencesManager"], function (pm) {
PreferencesManager = pm;
_updateFromPrefs(_EXTENSION_MAP_PREF);
_updateFromPrefs(_NAME_MAP_PREF);
pm.definePreference(_EXTENSION_MAP_PREF, "object").on("change", function () {
_updateFromPrefs(_EXTENSION_MAP_PREF);
});
pm.definePreference(_NAME_MAP_PREF, "object").on("change", function () {
_updateFromPrefs(_NAME_MAP_PREF);
});
});
});

// Private for unit tests
exports._EXTENSION_MAP_PREF = _EXTENSION_MAP_PREF;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: making indent of "=" at the same level as public exports will probably make it more readable.

exports._NAME_MAP_PREF = _NAME_MAP_PREF;

// Public methods
exports.ready = _ready;
exports.defineLanguage = defineLanguage;
Expand Down
74 changes: 68 additions & 6 deletions test/spec/LanguageManager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,19 @@

/*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, $, describe, jasmine, beforeEach, afterEach, it, runs, waitsFor, expect, waitsForDone, waitsForFail, spyOn */
/*unittests: LanguageManager */

define(function (require, exports, module) {
'use strict';

// Load dependent modules
var CodeMirror = require("thirdparty/CodeMirror2/lib/codemirror"),
LanguageManager = require("language/LanguageManager"),
DocumentManager = require("document/DocumentManager"),
PathUtils = require("thirdparty/path-utils/path-utils.min"),
SpecRunnerUtils = require("spec/SpecRunnerUtils"),
FileSystem = require("filesystem/FileSystem");
var CodeMirror = require("thirdparty/CodeMirror2/lib/codemirror"),
LanguageManager = require("language/LanguageManager"),
DocumentManager = require("document/DocumentManager"),
PathUtils = require("thirdparty/path-utils/path-utils.min"),
SpecRunnerUtils = require("spec/SpecRunnerUtils"),
PreferencesManager = require("preferences/PreferencesManager"),
FileSystem = require("filesystem/FileSystem");

describe("LanguageManager", function () {

Expand Down Expand Up @@ -669,5 +671,65 @@ define(function (require, exports, module) {
doc.releaseRef();
});
});

describe("Preferences", function () {
it("should be able to add extension mappings via a preference", function () {
var language = LanguageManager.getLanguageForExtension("foo");
expect(language).toBeUndefined();
PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, {
foo: "javascript"
});
language = LanguageManager.getLanguageForExtension("foo");
expect(language.getId()).toBe("javascript");
PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, { });
language = LanguageManager.getLanguageForExtension("foo");
expect(language).toBeUndefined();
});

it("should manage overridden default extensions", function () {
PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, {
js: "html"
});
var language = LanguageManager.getLanguageForExtension("js");
expect(language.getId()).toBe("html");
PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, {
js: "php"
});
language = LanguageManager.getLanguageForExtension("js");
expect(language.getId()).toBe("php");
PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, { });
language = LanguageManager.getLanguageForExtension("js");
expect(language.getId()).toBe("javascript");
});

it("should be able to manage file name mappings via a preference", function () {
var language = LanguageManager.getLanguageForPath("/bar/Foofile");
expect(language.getId()).toBe("unknown");
PreferencesManager.set(LanguageManager._NAME_MAP_PREF, {
"Foofile": "javascript"
});
language = LanguageManager.getLanguageForPath("/bar/Foofile");
expect(language.getId()).toBe("javascript");
PreferencesManager.set(LanguageManager._NAME_MAP_PREF, { });
language = LanguageManager.getLanguageForPath("/bar/Foofile");
expect(language.getId()).toBe("unknown");
});

it("should manage overridden default file names", function () {
PreferencesManager.set(LanguageManager._NAME_MAP_PREF, {
Gemfile: "python"
});
var language = LanguageManager.getLanguageForPath("Gemfile");
expect(language.getId()).toBe("python");
PreferencesManager.set(LanguageManager._NAME_MAP_PREF, {
Gemfile: "php"
});
language = LanguageManager.getLanguageForPath("Gemfile");
expect(language.getId()).toBe("php");
PreferencesManager.set(LanguageManager._NAME_MAP_PREF, { });
language = LanguageManager.getLanguageForPath("Gemfile");
expect(language.getId()).toBe("ruby");
});
});
});
});