From cc4704350a2113a01ecccd3c999fe7f42d9825f6 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Thu, 17 Jul 2014 14:12:46 -0700 Subject: [PATCH 1/5] Cleanups to status bar language-switcher: * Make LanguageManager the official public API that is used to set per-file overrides, rather than proxying through Document objects - remove Document.setLanguageOverride() * Clear path overrides when switching projects, so it's more obvious that it's not something that gets persisted across sessions * Simplify how JS Code Hints listen for language updates: remove dependency on "currentDocumentLanguageChanged" event; remove unneeded extra 'if' tests --- src/document/Document.js | 14 +--- src/editor/EditorStatusBar.js | 6 +- .../JavaScriptCodeHints/ScopeManager.js | 8 +-- .../default/JavaScriptCodeHints/main.js | 20 +++--- src/language/LanguageManager.js | 64 +++++++++++-------- src/project/ProjectManager.js | 7 +- test/spec/LanguageManager-test.js | 14 ++-- 7 files changed, 63 insertions(+), 70 deletions(-) diff --git a/src/document/Document.js b/src/document/Document.js index 47e46dc30f7..9cae05e50c8 100644 --- a/src/document/Document.js +++ b/src/document/Document.js @@ -672,19 +672,7 @@ define(function (require, exports, module) { }; /** - * Overrides the default language of this document and sets it to the given - * language. This change is not persisted if the document is closed. - * @param {?Language} language The language to be set for this document; if - * null, the language will be set back to the default. - */ - Document.prototype.setLanguageOverride = function (language) { - LanguageManager._setLanguageOverrideForPath(this.file.fullPath, language); - this._updateLanguage(); - }; - - /** - * Updates the language according to the file extension. If the current - * language was forced (set manually by user), don't change it. + * Updates the language to match the current mapping given by LanguageManager */ Document.prototype._updateLanguage = function () { var oldLanguage = this.language; diff --git a/src/editor/EditorStatusBar.js b/src/editor/EditorStatusBar.js index 81ad39de5c9..c1b9eab2cce 100644 --- a/src/editor/EditorStatusBar.js +++ b/src/editor/EditorStatusBar.js @@ -365,9 +365,9 @@ define(function (require, exports, module) { var document = EditorManager.getActiveEditor().document, fullPath = document.file.fullPath, defaultLang = LanguageManager.getLanguageForPath(fullPath, true); - // if default language selected, don't "force" it - // (passing in null will reset the force flag) - document.setLanguageOverride(lang === defaultLang ? null : lang); + + // if default language selected, pass null to clear the override + LanguageManager.setLanguageOverrideForPath(fullPath, lang === defaultLang ? null : lang); }); $statusOverwrite.on("click", _updateEditorOverwriteMode); diff --git a/src/extensions/default/JavaScriptCodeHints/ScopeManager.js b/src/extensions/default/JavaScriptCodeHints/ScopeManager.js index 12716e7cc66..a9497620691 100644 --- a/src/extensions/default/JavaScriptCodeHints/ScopeManager.js +++ b/src/extensions/default/JavaScriptCodeHints/ScopeManager.js @@ -1037,8 +1037,8 @@ define(function (require, exports, module) { * Do the work to initialize a code hinting session. * * @param {Session} session - the active hinting session - * @param {Document} document - the document the editor has changed to - * @param {Document} previousDocument - the document the editor has changed from + * @param {!Document} document - the document the editor has changed to + * @param {?Document} previousDocument - the document the editor has changed from */ function doEditorChange(session, document, previousDocument) { var file = document.file, @@ -1138,7 +1138,7 @@ define(function (require, exports, module) { * * @param {Session} session - the active hinting session * @param {Document} document - the document of the editor that has changed - * @param {Document} previousDocument - the document of the editor is changing from + * @param {?Document} previousDocument - the document of the editor is changing from */ function handleEditorChange(session, document, previousDocument) { if (addFilesPromise === null) { @@ -1376,7 +1376,7 @@ define(function (require, exports, module) { * * @param {Session} session - the active hinting session * @param {Document} document - the document of the editor that has changed - * @param {Document} previousDocument - the document of the editor is changing from + * @param {?Document} previousDocument - the document of the editor is changing from */ function handleEditorChange(session, document, previousDocument) { diff --git a/src/extensions/default/JavaScriptCodeHints/main.js b/src/extensions/default/JavaScriptCodeHints/main.js index 1aea34e5f46..e3a6bd1fc24 100644 --- a/src/extensions/default/JavaScriptCodeHints/main.js +++ b/src/extensions/default/JavaScriptCodeHints/main.js @@ -564,7 +564,7 @@ define(function (require, exports, module) { * information, and reject any pending deferred requests. * * @param {Editor} editor - editor context to be initialized. - * @param {Editor} previousEditor - the previous editor. + * @param {?Editor} previousEditor - the previous editor. */ function initializeSession(editor, previousEditor) { session = new Session(editor); @@ -579,7 +579,7 @@ define(function (require, exports, module) { * * @param {Editor} editor - editor context on which to listen for * changes - * @param {Editor} previousEditor - the previous editor + * @param {?Editor} previousEditor - the previous editor */ function installEditorListeners(editor, previousEditor) { // always clean up cached scope and hint info @@ -624,18 +624,21 @@ define(function (require, exports, module) { * @param {Editor} previous - the previous editor context */ function handleActiveEditorChange(event, current, previous) { - // Uninstall "languageChanged" event listeners on the previous editor's document - if (previous && previous !== current) { + // Uninstall "languageChanged" event listeners on previous editor's document & put them on current editor's doc + if (previous) { $(previous.document) .off(HintUtils.eventName("languageChanged")); } - if (current && current.document !== DocumentManager.getCurrentDocument()) { + if (current) { $(current.document) .on(HintUtils.eventName("languageChanged"), function () { + // If current doc's language changed, reset our state by treating it as if the user switched to a + // different document altogether uninstallEditorListeners(current); installEditorListeners(current); }); } + uninstallEditorListeners(previous); installEditorListeners(current, previous); } @@ -803,13 +806,6 @@ define(function (require, exports, module) { .on(HintUtils.eventName("activeEditorChange"), handleActiveEditorChange); - $(DocumentManager) - .on("currentDocumentLanguageChanged", function (e) { - var activeEditor = EditorManager.getActiveEditor(); - uninstallEditorListeners(activeEditor); - installEditorListeners(activeEditor); - }); - $(ProjectManager).on("beforeProjectClose", function () { ScopeManager.handleProjectClose(); }); diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index ca66cdb05b3..d9aef8d0e69 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -227,28 +227,6 @@ define(function (require, exports, module) { _modeToLanguageMap[mode] = language; } - /** - * Adds a language mapping for the specified fullPath. If language is falsy (null or undefined), the mapping - * is removed. - * - * @param {!fullPath} fullPath absolute path of the file - * @param {?object} language language to associate the file with or falsy value to remove the existing mapping - */ - function _setLanguageOverrideForPath(fullPath, language) { - if (!language) { - delete _filePathToLanguageMap[fullPath]; - } else { - _filePathToLanguageMap[fullPath] = language; - } - } - - /** - * Resets all the language overrides for file paths. Used by unit tests only. - */ - function _resetLanguageOverrides() { - _filePathToLanguageMap = {}; - } - /** * Resolves a language ID to a Language object. * File names have a higher priority than file extensions. @@ -260,7 +238,9 @@ define(function (require, exports, module) { } /** - * Resolves a language to a file extension + * Resolves a file extension to a Language object. + * *Warning:* it is almost always better to use getLanguageForPath(), since Language can depend + * on file name and even full path. Use this API only if no relevant file/path exists. * @param {!string} extension Extension that language should be resolved for * @return {?Language} The language for the provided extension or null if none exists */ @@ -383,6 +363,37 @@ define(function (require, exports, module) { $(exports).triggerHandler("languageModified", [language]); } + /** + * Adds a language mapping for the specified fullPath. If language is falsy (null or undefined), the mapping + * is removed. The override is NOT persisted across Brackets sessions. + * + * @param {!fullPath} fullPath absolute path of the file + * @param {?object} language language to associate the file with or falsy value to remove any existing override + */ + function setLanguageOverrideForPath(fullPath, language) { + var oldLang = getLanguageForPath(fullPath); + if (!language) { + delete _filePathToLanguageMap[fullPath]; + } else { + _filePathToLanguageMap[fullPath] = language; + } + var newLang = getLanguageForPath(fullPath); + + // Old language changed since this path is no longer mapped to it + _triggerLanguageModified(oldLang); + // New language changed since a path is now mapped to it that wasn't before + _triggerLanguageModified(newLang); + } + + /** + * Resets all the language overrides for file paths. Used by unit tests only. + */ + function _resetPathLanguageOverrides() { + _filePathToLanguageMap = {}; + } + + + /** * Model for a language. @@ -1100,11 +1111,7 @@ define(function (require, exports, module) { // Private for unit tests exports._EXTENSION_MAP_PREF = _EXTENSION_MAP_PREF; exports._NAME_MAP_PREF = _NAME_MAP_PREF; - exports._resetLanguageOverrides = _resetLanguageOverrides; - // Internal use only - // _setLanguageOverrideForPath is used by Document to help LanguageManager keeping track of - // in-document language overrides - exports._setLanguageOverrideForPath = _setLanguageOverrideForPath; + exports._resetPathLanguageOverrides = _resetPathLanguageOverrides; // Public methods exports.ready = _ready; @@ -1113,4 +1120,5 @@ define(function (require, exports, module) { exports.getLanguageForExtension = getLanguageForExtension; exports.getLanguageForPath = getLanguageForPath; exports.getLanguages = getLanguages; + exports.setLanguageOverrideForPath = setLanguageOverrideForPath; }); diff --git a/src/project/ProjectManager.js b/src/project/ProjectManager.js index 97fba36e307..300c71ff400 100644 --- a/src/project/ProjectManager.js +++ b/src/project/ProjectManager.js @@ -1117,8 +1117,8 @@ define(function (require, exports, module) { } /** - * Loads the given folder as a project. Normally, you would call openProject() instead to let the - * user choose a folder. + * Loads the given folder as a project. Does NOT prompt about any unsaved changes - use openProject() + * instead to check for unsaved changes and (optionally) let the user choose the folder to open. * * @param {!string} rootPath Absolute path to the root folder of the project. * A trailing "/" on the path is optional (unlike many Brackets APIs that assume a trailing "/"). @@ -1155,8 +1155,9 @@ define(function (require, exports, module) { DocumentManager.closeAll(); _unwatchProjectRoot().always(function () { - // Done closing old project (if any) + // Finish closing old project (if any) if (_projectRoot) { + LanguageManager._resetPathLanguageOverrides(); PreferencesManager._reloadUserPrefs(_projectRoot); $(exports).triggerHandler("projectClose", _projectRoot); } diff --git a/test/spec/LanguageManager-test.js b/test/spec/LanguageManager-test.js index 7dc846a140f..0a89f90b5ea 100644 --- a/test/spec/LanguageManager-test.js +++ b/test/spec/LanguageManager-test.js @@ -47,7 +47,7 @@ define(function (require, exports, module) { }); afterEach(function () { - LanguageManager._resetLanguageOverrides(); + LanguageManager._resetPathLanguageOverrides(); }); function defineLanguage(definition) { @@ -544,7 +544,7 @@ define(function (require, exports, module) { var renameDeferred = $.Deferred(); runs(function () { - DocumentManager.setCurrentDocument(doc); + DocumentManager.setCurrentDocument(doc); javascript = LanguageManager.getLanguage("javascript"); // sanity check language @@ -705,7 +705,7 @@ define(function (require, exports, module) { // make active doc.addRef(); - doc.setLanguageOverride(phpLang); + LanguageManager.setLanguageOverrideForPath(doc.file.fullPath, phpLang); // language should change expect(doc.getLanguage()).toBe(phpLang); @@ -747,7 +747,7 @@ define(function (require, exports, module) { // make active doc.addRef(); - doc.setLanguageOverride(phpLang); + LanguageManager.setLanguageOverrideForPath(doc.file.fullPath, phpLang); // language should change expect(doc.getLanguage()).toBe(phpLang); @@ -756,7 +756,7 @@ define(function (require, exports, module) { expect(spy.mostRecentCall.args[2]).toBe(phpLang); expect(LanguageManager.getLanguageForPath(doc.file.fullPath)).toBe(phpLang); - doc.setLanguageOverride(null); + LanguageManager.setLanguageOverrideForPath(doc.file.fullPath, null); // language should revert expect(doc.getLanguage()).toBe(unknownLang); @@ -777,7 +777,7 @@ define(function (require, exports, module) { expect(LanguageManager.getLanguageForPath(doc.file.fullPath)).toBe(modifiedLanguage); // override again - doc.setLanguageOverride(phpLang); + LanguageManager.setLanguageOverrideForPath(doc.file.fullPath, phpLang); expect(doc.getLanguage()).toBe(phpLang); expect(spy.callCount).toBe(4); @@ -786,7 +786,7 @@ define(function (require, exports, module) { expect(LanguageManager.getLanguageForPath(doc.file.fullPath)).toBe(phpLang); // remove override, should restore to modifiedLanguage - doc.setLanguageOverride(null); + LanguageManager.setLanguageOverrideForPath(doc.file.fullPath, null); expect(doc.getLanguage()).toBe(modifiedLanguage); expect(spy.callCount).toBe(5); From 2f542eb572d24e6d95b13a161517b4c6ea10a5d3 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Thu, 17 Jul 2014 14:35:15 -0700 Subject: [PATCH 2/5] Add "Set as Default" menu item to status bar language switcher -- adds a permanent file extension mapping to preferences, based on the current file's path-specific language override (if any) and its file extension. --- src/editor/EditorStatusBar.js | 35 ++++++++++++++++++++++++++++++----- src/nls/root/strings.js | 1 + src/widgets/DropdownButton.js | 4 +++- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/editor/EditorStatusBar.js b/src/editor/EditorStatusBar.js index c1b9eab2cce..59a90fd33f3 100644 --- a/src/editor/EditorStatusBar.js +++ b/src/editor/EditorStatusBar.js @@ -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,13 @@ define(function (require, exports, module) { languageSelect = new DropdownButton("", [], function (item, index) { var document = EditorManager.getActiveEditor().document, - defaultLang = LanguageManager.getLanguageForPath(document.file.fullPath, true), - html = _.escape(item.getName()); + defaultLang = LanguageManager.getLanguageForPath(document.file.fullPath, true); + + if (item === LANGUAGE_SET_AS_DEFAULT) { + return _.escape(StringUtils.format(Strings.STATUSBAR_SET_DEFAULT_LANG, FileUtils.getSmartFileExtension(document.file.fullPath))); + } + + var html = _.escape(item.getName()); // Show indicators for currently selected & default languages for the current file if (item === defaultLang) { @@ -361,13 +374,25 @@ define(function (require, exports, module) { $indentWidthInput.focus(function () { $indentWidthInput.select(); }); // Language select change handler - $(languageSelect).on("select", function (e, lang, index) { + $(languageSelect).on("select", function (e, lang) { var document = EditorManager.getActiveEditor().document, fullPath = document.file.fullPath, defaultLang = LanguageManager.getLanguageForPath(fullPath, true); - // if default language selected, pass null to clear the override - LanguageManager.setLanguageOverrideForPath(fullPath, lang === defaultLang ? null : lang); + if (lang === LANGUAGE_SET_AS_DEFAULT) { + lang = document.getLanguage(); + if (lang !== defaultLang) { + // Set file's current language in preferences as a file extension override + var fileExtensionMap = PreferencesManager.get("language.fileExtensions"); + fileExtensionMap[FileUtils.getSmartFileExtension(fullPath)] = lang.getId(); + PreferencesManager.set("language.fileExtensions", fileExtensionMap); + } + + } else { + // Set selected language as a path override for just this one file (not persisted) + // if default language selected, pass null to clear the override + LanguageManager.setLanguageOverrideForPath(fullPath, lang === defaultLang ? null : lang); + } }); $statusOverwrite.on("click", _updateEditorOverwriteMode); diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 1d315472b08..131caf666a5 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -247,6 +247,7 @@ define({ "STATUSBAR_INSERT" : "INS", "STATUSBAR_OVERWRITE" : "OVR", "STATUSBAR_DEFAULT_LANG" : "(default)", + "STATUSBAR_SET_DEFAULT_LANG" : "Set as Default for .{0} Files", // CodeInspection: errors/warnings "ERRORS_PANEL_TITLE_MULTIPLE" : "{0} Problems", diff --git a/src/widgets/DropdownButton.js b/src/widgets/DropdownButton.js index d18904c6d71..817bbc02fe9 100644 --- a/src/widgets/DropdownButton.js +++ b/src/widgets/DropdownButton.js @@ -56,7 +56,9 @@ define(function (require, exports, module) { * - "select" - when an option in the dropdown is clicked. Passed item object and index. * * @param {!string} label Label to display on the button - * @param {!Array.<*>} items Items in the dropdown list + * @param {!Array.<*>} items Items in the dropdown list. It generally doesn't matter what type/value the + * items have, except that any item === "---" will be treated as a divider. Such items are not + * clickable and itemRenderer() will not be called for them. * @param {?function(*, number):!string} itemRenderer Optional function to convert a single item to HTML * (see itemRenderer() docs below). If not provided, items are assumed to be plain text strings. */ From 6e6c6189bd124ab38d617d240944bfc4f2111e80 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Thu, 17 Jul 2014 18:52:43 -0700 Subject: [PATCH 3/5] * Gray out status-bar language switcher's "Set as Default" item if the doc's current language is already the default (i.e. there's no path-specific override). DropdownButton changes: * Support disabled items * Fix bug #8433 (Page Up/Down work oddly) - Only wrap around to other end of list when using arrow keys. Page Up/Down 'stick' at start/end of list instead (so they will act like Home/End when there's no scrollbar). * Add Home/End key support --- src/editor/EditorStatusBar.js | 18 ++--- src/styles/brackets_patterns_override.less | 7 ++ src/utils/DropdownEventHandler.js | 94 ++++++++++++---------- src/widgets/DropdownButton.js | 17 ++-- 4 files changed, 79 insertions(+), 57 deletions(-) diff --git a/src/editor/EditorStatusBar.js b/src/editor/EditorStatusBar.js index 59a90fd33f3..fcbc447978e 100644 --- a/src/editor/EditorStatusBar.js +++ b/src/editor/EditorStatusBar.js @@ -327,7 +327,8 @@ define(function (require, exports, module) { defaultLang = LanguageManager.getLanguageForPath(document.file.fullPath, true); if (item === LANGUAGE_SET_AS_DEFAULT) { - return _.escape(StringUtils.format(Strings.STATUSBAR_SET_DEFAULT_LANG, FileUtils.getSmartFileExtension(document.file.fullPath))); + 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()); @@ -376,20 +377,17 @@ define(function (require, exports, module) { // Language select change handler $(languageSelect).on("select", function (e, lang) { var document = EditorManager.getActiveEditor().document, - fullPath = document.file.fullPath, - defaultLang = LanguageManager.getLanguageForPath(fullPath, true); + fullPath = document.file.fullPath; if (lang === LANGUAGE_SET_AS_DEFAULT) { - lang = document.getLanguage(); - if (lang !== defaultLang) { - // Set file's current language in preferences as a file extension override - var fileExtensionMap = PreferencesManager.get("language.fileExtensions"); - fileExtensionMap[FileUtils.getSmartFileExtension(fullPath)] = lang.getId(); - PreferencesManager.set("language.fileExtensions", fileExtensionMap); - } + // Set file's current language in preferences as a file extension override (only enabled if not default already) + var fileExtensionMap = PreferencesManager.get("language.fileExtensions"); + fileExtensionMap[FileUtils.getSmartFileExtension(fullPath)] = document.getLanguage().getId(); + PreferencesManager.set("language.fileExtensions", fileExtensionMap); } 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); } diff --git a/src/styles/brackets_patterns_override.less b/src/styles/brackets_patterns_override.less index 86ce5925ddd..52eaa46b168 100644 --- a/src/styles/brackets_patterns_override.less +++ b/src/styles/brackets_patterns_override.less @@ -479,6 +479,13 @@ a:focus { &.dropdown-menu li a { padding: 1px 15px 1px 15px; color: @bc-black; + + &.disabled { + color: @bc-gray; + &:hover { + background: @bc-white; + } + } } &.dropdown-menu .stylesheet-link { diff --git a/src/utils/DropdownEventHandler.js b/src/utils/DropdownEventHandler.js index a700ca52d11..eba62b687c4 100644 --- a/src/utils/DropdownEventHandler.js +++ b/src/utils/DropdownEventHandler.js @@ -48,6 +48,8 @@ define(function (require, exports, module) { * - Esc - dismiss list * - Up/Down - change selection * - PageUp/Down - change selection + * + * Items whose has the .disabled class do not respond to selection. * * @constructor * @param {jQueryObject} $list associated list object @@ -88,13 +90,23 @@ define(function (require, exports, module) { keyCode = event.keyCode; if (keyCode === KeyEvent.DOM_VK_UP) { - self._rotateSelection(-1); + // Move up one, wrapping at edges (if nothing selected, select the last item) + self._tryToSelect(self._selectedIndex === -1 ? -1 : self._selectedIndex - 1, -1); } else if (keyCode === KeyEvent.DOM_VK_DOWN) { - self._rotateSelection(1); + // Move down one, wrapping at edges (if nothing selected, select the first item) + self._tryToSelect(self._selectedIndex === -1 ? 0 : self._selectedIndex + 1, +1); } else if (keyCode === KeyEvent.DOM_VK_PAGE_UP) { - self._rotateSelection(-self._itemsPerPage()); + // Move up roughly one 'page', stopping at edges (not wrapping) (if nothing selected, selects the first item) + self._tryToSelect((self._selectedIndex || 0) - self._itemsPerPage(), -1, true); } else if (keyCode === KeyEvent.DOM_VK_PAGE_DOWN) { - self._rotateSelection(self._itemsPerPage()); + // Move down roughly one 'page', stopping at edges (not wrapping) (if nothing selected, selects the item one page down from the top) + self._tryToSelect((self._selectedIndex || 0) + self._itemsPerPage(), +1, true); + + } else if (keyCode === KeyEvent.DOM_VK_HOME) { + self._tryToSelect(0, +1); + } else if (keyCode === KeyEvent.DOM_VK_END) { + self._tryToSelect(self.$items.length - 1, -1); + } else if (self._selectedIndex !== -1 && (keyCode === KeyEvent.DOM_VK_RETURN)) { @@ -150,48 +162,42 @@ define(function (require, exports, module) { }; /** - * Change selection by specified amount. After selection reaches the last item - * in the rotation direction, then it wraps around to the start. - * - * @param {number} distance Number of items to move change selection where - * positive distance rotates down and negative distance rotates up + * Try to select item at the given index. If it's disabled or a divider, keep trying by incrementing + * index by 'direction' each time (wrapping around if needed). + * @param {number} index If out of bounds, index either wraps around to remain in range (e.g. -1 yields + * last item, length+1 yields 2nd item) or if noWrap set, clips instead (e.g. -1 yields + * first item, length+1 yields last item). + * @param {number} direction Either +1 or -1 + * @param {boolean=} noWrap Clip out of range index values instead of wrapping. Default false (wrap). */ - DropdownEventHandler.prototype._rotateSelection = function (distance) { - var len = this.$items.length, - pos; - - if (this._selectedIndex < 0) { - // set the initial selection - pos = (distance > 0) ? distance - 1 : len - 1; - + DropdownEventHandler.prototype._tryToSelect = function (index, direction, noWrap) { + // Wrap around if index out of bounds (>= len or < 0) + var len = this.$items.length; + if (noWrap) { + // Clip to stay in range (and set direction so we don't wrap in the recursion case either) + if (index < 0) { + index = 0; + direction = +1; + } else if (index >= len) { + index = len - 1; + direction = -1; + } } else { - // adjust current selection - pos = this._selectedIndex; - - // Don't "rotate" until all items have been shown - if (distance > 0) { - if (pos === (len - 1)) { - pos = 0; // wrap - } else { - pos = Math.min(pos + distance, len - 1); - } - } else { - if (pos === 0) { - pos = (len - 1); // wrap - } else { - pos = Math.max(pos + distance, 0); - } + index = index % len; + if (index < 0) { + index = len + index; } } - - // If the item to be selected is a divider, then rotate one more. - if ($(this.$items[pos]).hasClass("divider")) { - this._rotateSelection((distance > 0) ? (distance + 1) : (distance - 1)); + + var $item = this.$items.eq(index); + if ($item.hasClass("divider") || $item.find("a.disabled").length) { + // Desired item is ineligible for selection: try next one + this._tryToSelect(index + direction, direction, noWrap); } else { - this._setSelectedIndex(pos, true); + this._setSelectedIndex(index, true); } }; - + /** * @return {number} The number of items per scroll page. */ @@ -234,6 +240,9 @@ define(function (require, exports, module) { if (!this.selectionCallback || !this.$list || !$link) { return; } + if ($link.is(".disabled")) { + return; + } this.selectionCallback($link); PopUpManager.removePopUp(this.$list); @@ -285,9 +294,10 @@ define(function (require, exports, module) { viewOffset = self.$list.offset(), elementOffset = $item.offset(); - // Only set selected if in view - if (elementOffset.top < viewOffset.top + self.$list.height()) { - if (viewOffset.top <= elementOffset.top) { + // Only set selected if enabled & in view + // (dividers are already screened out since they don't have an "a" tag in them) + if (!$link.is(".disabled")) { + if (elementOffset.top < viewOffset.top + self.$list.height() && viewOffset.top <= elementOffset.top) { self._setSelectedIndex(self.$items.index($item), false); } } diff --git a/src/widgets/DropdownButton.js b/src/widgets/DropdownButton.js index 817bbc02fe9..42817db2117 100644 --- a/src/widgets/DropdownButton.js +++ b/src/widgets/DropdownButton.js @@ -59,8 +59,9 @@ define(function (require, exports, module) { * @param {!Array.<*>} items Items in the dropdown list. It generally doesn't matter what type/value the * items have, except that any item === "---" will be treated as a divider. Such items are not * clickable and itemRenderer() will not be called for them. - * @param {?function(*, number):!string} itemRenderer Optional function to convert a single item to HTML - * (see itemRenderer() docs below). If not provided, items are assumed to be plain text strings. + * @param {?function(*, number):!string|{html:string, enabled:boolean} itemRenderer Optional function to + * convert a single item to HTML (see itemRenderer() docs below). If not provided, items are + * assumed to be plain text strings. */ function DropdownButton(label, items, itemRenderer) { this.items = items; @@ -142,7 +143,9 @@ define(function (require, exports, module) { * Called for each item when rendering the dropdown. * @param {*} item from items array * @param {number} index in items array - * @return {!string} Formatted & escaped HTML + * @return {!string|{html:string, enabled:boolean}} Formatted & escaped HTML, either as a simple string + * or as the 'html' field in an object that also conveys enabled state. Disabled items inherit gray + * text color and cannot be selected. */ DropdownButton.prototype.itemRenderer = function (item, index) { return _.escape(String(item)); @@ -163,8 +166,12 @@ define(function (require, exports, module) { if (item === "---") { html += "
  • "; } else { - html += "
  • "; - html += this.itemRenderer(item, i); + var rendered = this.itemRenderer(item, i), + itemHtml = rendered.html || rendered, + disabledClass = (rendered.html && !rendered.enabled) ? "disabled" : ""; + + html += "
  • "; + html += itemHtml; html += "
    • "; } }.bind(this)); From 23a3e91eb52f52827589d778dc19c20319b9a359 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Tue, 5 Aug 2014 02:45:45 -0700 Subject: [PATCH 4/5] Code review fixes: * Use .hasClass() instead of .is() where possible * More succinct math operators in DropdownEventHandler._tryToSelect() * Document language-change events * Improve some more JS code hints docs --- src/document/Document.js | 3 +++ .../default/JavaScriptCodeHints/ScopeManager.js | 2 +- src/extensions/default/JavaScriptCodeHints/main.js | 8 ++++---- src/language/LanguageManager.js | 6 ++++++ src/utils/DropdownEventHandler.js | 11 ++++++----- 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/document/Document.js b/src/document/Document.js index 9cae05e50c8..13591eb3884 100644 --- a/src/document/Document.js +++ b/src/document/Document.js @@ -65,6 +65,9 @@ define(function (require, exports, module) { * * __deleted__ -- When the file for this document has been deleted. All views onto the document should * be closed. The document will no longer be editable or dispatch "change" events. + * + * __languageChanged__ -- When the value of getLanguage() has changed. 2nd argument is the old value, + * 3rd argument is the new value. * * @constructor * @param {!File} file Need not lie within the project. diff --git a/src/extensions/default/JavaScriptCodeHints/ScopeManager.js b/src/extensions/default/JavaScriptCodeHints/ScopeManager.js index a9497620691..5c2d669dc7f 100644 --- a/src/extensions/default/JavaScriptCodeHints/ScopeManager.js +++ b/src/extensions/default/JavaScriptCodeHints/ScopeManager.js @@ -1137,7 +1137,7 @@ define(function (require, exports, module) { * Called each time a new editor becomes active. * * @param {Session} session - the active hinting session - * @param {Document} document - the document of the editor that has changed + * @param {!Document} document - the document of the editor that has changed * @param {?Document} previousDocument - the document of the editor is changing from */ function handleEditorChange(session, document, previousDocument) { diff --git a/src/extensions/default/JavaScriptCodeHints/main.js b/src/extensions/default/JavaScriptCodeHints/main.js index e3a6bd1fc24..f5b7ccf40f7 100644 --- a/src/extensions/default/JavaScriptCodeHints/main.js +++ b/src/extensions/default/JavaScriptCodeHints/main.js @@ -563,7 +563,7 @@ define(function (require, exports, module) { * When the editor is changed, reset the hinting session and cached * information, and reject any pending deferred requests. * - * @param {Editor} editor - editor context to be initialized. + * @param {!Editor} editor - editor context to be initialized. * @param {?Editor} previousEditor - the previous editor. */ function initializeSession(editor, previousEditor) { @@ -575,10 +575,10 @@ define(function (require, exports, module) { } /* - * Install editor change listeners + * Connects to the given editor, creating a new Session & adding listeners * - * @param {Editor} editor - editor context on which to listen for - * changes + * @param {?Editor} editor - editor context on which to listen for + * changes. If null, 'session' is cleared. * @param {?Editor} previousEditor - the previous editor */ function installEditorListeners(editor, previousEditor) { diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index d9aef8d0e69..ea210dceec9 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -120,6 +120,12 @@ * isBinary: true * }); * + * + * LanguageManager dispatches two events: + * + * - languageAdded -- When any new Language is added. 2nd arg is the new Language. + * - languageModified -- When the attributes of a Language change, or when the Language gains or loses + * file extension / filename mappings. 2nd arg is the modified Language. */ define(function (require, exports, module) { "use strict"; diff --git a/src/utils/DropdownEventHandler.js b/src/utils/DropdownEventHandler.js index eba62b687c4..6b1f24e002a 100644 --- a/src/utils/DropdownEventHandler.js +++ b/src/utils/DropdownEventHandler.js @@ -171,7 +171,7 @@ define(function (require, exports, module) { * @param {boolean=} noWrap Clip out of range index values instead of wrapping. Default false (wrap). */ DropdownEventHandler.prototype._tryToSelect = function (index, direction, noWrap) { - // Wrap around if index out of bounds (>= len or < 0) + // Fix up 'index' if out of bounds (>= len or < 0) var len = this.$items.length; if (noWrap) { // Clip to stay in range (and set direction so we don't wrap in the recursion case either) @@ -183,9 +183,10 @@ define(function (require, exports, module) { direction = -1; } } else { - index = index % len; + // Wrap around to keep index in bounds + index %= len; if (index < 0) { - index = len + index; + index += len; } } @@ -240,7 +241,7 @@ define(function (require, exports, module) { if (!this.selectionCallback || !this.$list || !$link) { return; } - if ($link.is(".disabled")) { + if ($link.hasClass("disabled")) { return; } @@ -296,7 +297,7 @@ define(function (require, exports, module) { // Only set selected if enabled & in view // (dividers are already screened out since they don't have an "a" tag in them) - if (!$link.is(".disabled")) { + if (!$link.hasClass("disabled")) { if (elementOffset.top < viewOffset.top + self.$list.height() && viewOffset.top <= elementOffset.top) { self._setSelectedIndex(self.$items.index($item), false); } From ba1427c00c3614aa11df9add1058b96370160860 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Tue, 5 Aug 2014 23:38:04 -0700 Subject: [PATCH 5/5] Fix language switcher persistence to work when no custom file extension mappings are set yet - don't leave default value of pref undefined. Clarify two other cases of definePreference() calls where we really *do* want the default value to be undefined by passing it explicitly. Also: add TODOs for unused Session arg in JS code hints. --- src/extensibility/Package.js | 2 +- src/extensions/default/JSLint/main.js | 2 +- .../default/JavaScriptCodeHints/ScopeManager.js | 4 ++-- src/language/LanguageManager.js | 10 +++++----- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/extensibility/Package.js b/src/extensibility/Package.js index dd26bfa47cc..dfbbb9c8b31 100644 --- a/src/extensibility/Package.js +++ b/src/extensibility/Package.js @@ -41,7 +41,7 @@ define(function (require, exports, module) { NodeConnection = require("utils/NodeConnection"), PreferencesManager = require("preferences/PreferencesManager"); - PreferencesManager.definePreference("proxy", "string"); + PreferencesManager.definePreference("proxy", "string", undefined); var Errors = { ERROR_LOADING: "ERROR_LOADING", diff --git a/src/extensions/default/JSLint/main.js b/src/extensions/default/JSLint/main.js index c3ec9705dcc..90540d84731 100644 --- a/src/extensions/default/JSLint/main.js +++ b/src/extensions/default/JSLint/main.js @@ -50,7 +50,7 @@ define(function (require, exports, module) { */ var _lastRunOptions; - prefs.definePreference("options", "object") + prefs.definePreference("options", "object", undefined) .on("change", function (e, data) { var options = prefs.get("options"); if (!_.isEqual(options, _lastRunOptions)) { diff --git a/src/extensions/default/JavaScriptCodeHints/ScopeManager.js b/src/extensions/default/JavaScriptCodeHints/ScopeManager.js index 5c2d669dc7f..4ee1b3988be 100644 --- a/src/extensions/default/JavaScriptCodeHints/ScopeManager.js +++ b/src/extensions/default/JavaScriptCodeHints/ScopeManager.js @@ -1036,7 +1036,7 @@ define(function (require, exports, module) { /** * Do the work to initialize a code hinting session. * - * @param {Session} session - the active hinting session + * @param {Session} session - the active hinting session (TODO: currently unused) * @param {!Document} document - the document the editor has changed to * @param {?Document} previousDocument - the document the editor has changed from */ @@ -1136,7 +1136,7 @@ define(function (require, exports, module) { /** * Called each time a new editor becomes active. * - * @param {Session} session - the active hinting session + * @param {Session} session - the active hinting session (TODO: currently unused by doEditorChange()) * @param {!Document} document - the document of the editor that has changed * @param {?Document} previousDocument - the document of the editor is changing from */ diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index ea210dceec9..06ae334b45e 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -1006,7 +1006,7 @@ define(function (require, exports, module) { * } */ function _updateFromPrefs(pref) { - var newMapping = PreferencesManager.get(pref) || {}, + var newMapping = PreferencesManager.get(pref), newNames = Object.keys(newMapping), state = _prefState[pref], last = state.last, @@ -1103,14 +1103,14 @@ define(function (require, exports, module) { // 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 () { + pm.definePreference(_EXTENSION_MAP_PREF, "object", {}).on("change", function () { _updateFromPrefs(_EXTENSION_MAP_PREF); }); - pm.definePreference(_NAME_MAP_PREF, "object").on("change", function () { + pm.definePreference(_NAME_MAP_PREF, "object", {}).on("change", function () { _updateFromPrefs(_NAME_MAP_PREF); }); + _updateFromPrefs(_EXTENSION_MAP_PREF); + _updateFromPrefs(_NAME_MAP_PREF); }); });