Language switcher: connect to prefs & clean up some code #8444
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,8 +38,10 @@ define(function (require, exports, module) { | |
| DropdownButton = require("widgets/DropdownButton").DropdownButton, | ||
| EditorManager = require("editor/EditorManager"), | ||
| Editor = require("editor/Editor").Editor, | ||
| FileUtils = require("file/FileUtils"), | ||
| KeyEvent = require("utils/KeyEvent"), | ||
| LanguageManager = require("language/LanguageManager"), | ||
| PreferencesManager = require("preferences/PreferencesManager"), | ||
| StatusBar = require("widgets/StatusBar"), | ||
| Strings = require("strings"), | ||
| StringUtils = require("utils/StringUtils"); | ||
|
|
@@ -53,6 +55,9 @@ define(function (require, exports, module) { | |
| $indentWidthInput, | ||
| $statusOverwrite; | ||
|
|
||
| /** Special list item for the 'set as default' gesture in language switcher dropdown */ | ||
| var LANGUAGE_SET_AS_DEFAULT = {}; | ||
|
|
||
|
|
||
| /** | ||
| * Determine string based on count | ||
|
|
@@ -300,6 +305,9 @@ define(function (require, exports, module) { | |
|
|
||
| languageSelect.items = languages; | ||
|
|
||
| // Add option to top of menu for persisting the override | ||
| languageSelect.items.unshift("---"); | ||
| languageSelect.items.unshift(LANGUAGE_SET_AS_DEFAULT); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -316,8 +324,14 @@ define(function (require, exports, module) { | |
|
|
||
| languageSelect = new DropdownButton("", [], function (item, index) { | ||
| var document = EditorManager.getActiveEditor().document, | ||
| defaultLang = LanguageManager.getLanguageForPath(document.file.fullPath, true); | ||
|
|
||
| if (item === LANGUAGE_SET_AS_DEFAULT) { | ||
| var label = _.escape(StringUtils.format(Strings.STATUSBAR_SET_DEFAULT_LANG, FileUtils.getSmartFileExtension(document.file.fullPath))); | ||
| return { html: label, enabled: document.getLanguage() !== defaultLang }; | ||
| } | ||
|
|
||
| var html = _.escape(item.getName()); | ||
|
|
||
| // Show indicators for currently selected & default languages for the current file | ||
| if (item === defaultLang) { | ||
|
|
@@ -361,13 +375,22 @@ define(function (require, exports, module) { | |
| $indentWidthInput.focus(function () { $indentWidthInput.select(); }); | ||
|
|
||
| // Language select change handler | ||
| $(languageSelect).on("select", function (e, lang) { | ||
| var document = EditorManager.getActiveEditor().document, | ||
| fullPath = document.file.fullPath; | ||
|
|
||
| if (lang === LANGUAGE_SET_AS_DEFAULT) { | ||
| // Set file's current language in preferences as a file extension override (only enabled if not default already) | ||
| var fileExtensionMap = PreferencesManager.get("language.fileExtensions"); | ||
|
Contributor
There was a problem hiding this comment. This is why I can't get this to work for me. You need to handle Uncaught TypeError: Cannot set property 'jst' of undefined EditorStatusBar.js:385
Member
Author
There was a problem hiding this comment. Yikes, good catch indeed, @dangoor The docs for LanguageManager.definePreference() make it appear as if the
Contributor
There was a problem hiding this comment. You're right. What LanguageManager does is less than ideal because var newMapping = PreferencesManager.get(pref) || {},rather than just having a default set via |
||
| fileExtensionMap[FileUtils.getSmartFileExtension(fullPath)] = document.getLanguage().getId(); | ||
| PreferencesManager.set("language.fileExtensions", fileExtensionMap); | ||
|
Contributor
There was a problem hiding this comment. "language.fileExtensions" was a declared constant
Contributor
There was a problem hiding this comment. or actually, why isn't this functionality just an API on language manager?
Member
Author
There was a problem hiding this comment. I guess that's an interesting architecture philosophy question here (CC @dangoor :-) ...should prefs be considered an encapsulated, private detail of a module (hidden behind programmatic APIs)? Or are they a public API in themselves, since any user can touch them too? Personally, I'd lean toward the latter. Preferences have to be treated much like a frozen API, because we document them publicly and we need backwards compatibility with existing user .json files. Also, prefs definers need to listen to changes in the underlying prefs since users can edit the file at any time -- and making programmatic changes all go through an API might subtly encourage forgetting to handle the external-change case properly.
Contributor
There was a problem hiding this comment. that sounds reasonable. It's just interesting the design of languageManger encapsulates so much of the language management features -- even the prefs seem to be defined for internal use only. The constants are even declared as "private" as if we shouldn't use them outside of the module.
Member
Author
There was a problem hiding this comment. The semi-private consts LanguageManager uses for those two prefs keys do seem a little odd -- almost everywhere else, our code just uses string literals...
Contributor
There was a problem hiding this comment. Yeah, this makes sense philosophically: prefs are fundamentally public. I vaguely remember creating some pref or view state value that had a leading underscore, but otherwise they are values that need to have some level of compatibility guarantee just like a public API. |
||
|
|
||
| } else { | ||
| // Set selected language as a path override for just this one file (not persisted) | ||
| var defaultLang = LanguageManager.getLanguageForPath(fullPath, true); | ||
| // if default language selected, pass null to clear the override | ||
| LanguageManager.setLanguageOverrideForPath(fullPath, lang === defaultLang ? null : lang); | ||
| } | ||
| }); | ||
|
|
||
| $statusOverwrite.on("click", _updateEditorOverwriteMode); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original Language Switcher pr went through and we didn't update the events in Document.js -- specifically the
LanguageChangedevent (which is a past-tense event name and we normally use current-tense event names)Actually it appears that there are NO events documented for document objects for some reason. is there a reason why we aren't documenting them? Are they documented somewhere else? I couldn't find them documented anywhere.
We also should notify lint extension authors that they should update their extensions to listen to the "languageChanged" event since most of them leave cruft behind after changing the language of a javascript file to "text". We might want to change it to "languageChange" to match the rest of the events if no one is using it yet as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
"languageChanged"event was added much earlier than the 0.42 language switcher PR -- it dates back to Sprint 21 (c00bfb1), so I'd rather not change it as part of this diff.There are some events documented for Document (see docs on the constructor at the top of Document.js), but not this one. I'll add it.