Scoped themes to apply only to the editor (#editor-holder) #8447
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ define(function (require, exports, module) { | |
| prefs = PreferencesManager.getExtensionPrefs("brackets-themes"); | ||
|
|
||
| var loadedThemes = {}, | ||
| currentTheme = null, | ||
| defaultTheme = "thor-light-theme", | ||
| commentRegex = /\/\*([\s\S]*?)\*\//mg, | ||
| scrollbarsRegex = /::-webkit-scrollbar(?:[\s\S]*?)\{(?:[\s\S]*?)\}/mg, | ||
|
|
@@ -73,7 +74,7 @@ define(function (require, exports, module) { | |
| * dialog, and to properly add a theme into CodeMirror along with the rest of brackets. | ||
| * | ||
| * @param {File} file for the theme | ||
| * @param {{name: string, title: string}} options to configure different | ||
| * properties in the theme | ||
| */ | ||
| function Theme(file, options) { | ||
|
|
@@ -86,9 +87,9 @@ define(function (require, exports, module) { | |
| // is a theme1.css and a theme1.less that are entirely different themes... | ||
|
|
||
| this.file = file; | ||
| this.name = options.name || (options.title || fileName).toLocaleLowerCase().replace(/[\W]/g, '-'); | ||
| this.displayName = options.title || toDisplayName(fileName); | ||
| this.dark = options.dark === true; | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -156,7 +157,7 @@ define(function (require, exports, module) { | |
| filename: fixPath(theme.file._path) | ||
| }); | ||
|
|
||
| parser.parse("#editor-holder {" + content + "}", function (err, tree) { | ||
| if (err) { | ||
| deferred.reject(err); | ||
| } else { | ||
|
|
@@ -197,79 +198,83 @@ define(function (require, exports, module) { | |
|
|
||
|
|
||
| /** | ||
| * Get all current theme objects that are loaded in the editor. | ||
| * | ||
| * @return {Array.<Theme>} collection of the current theme instances | ||
| */ | ||
| function getCurrentTheme() { | ||
| if (!currentTheme) { | ||
| currentTheme = loadedThemes[prefs.get("theme")] || loadedThemes[defaultTheme]; | ||
| } | ||
|
|
||
| return currentTheme; | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Gets all available themes that can be loaded in the editor. | ||
| * @return {Array.<Theme>} collection of all available themes | ||
| */ | ||
| function getAllThemes() { | ||
| return loadedThemes.map(function (theme) { | ||
|
There was a problem hiding this comment. ahh, this doesn't work because |
||
| return theme; | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * @private | ||
| * Process and load the current theme into the editor | ||
| * | ||
| * @return {$.Promise} promise object resolved with the theme object and all | ||
| * corresponding new css/less and scrollbar information | ||
| */ | ||
| function loadCurrentTheme() { | ||
| var theme = getCurrentTheme(); | ||
|
|
||
| var pending = theme && FileUtils.readAsText(theme.file) | ||
| .then(function (content) { | ||
| var result = extractScrollbars(content); | ||
| theme.scrollbar = result.scrollbar; | ||
| return result.content; | ||
| }) | ||
| .then(function (content) { | ||
| return lessifyTheme(content, theme); | ||
| }) | ||
| .then(function (style) { | ||
| return ExtensionUtils.addEmbeddedStyleSheet(style); | ||
| }) | ||
| .then(function (styleNode) { | ||
| // Remove after the style has been applied to avoid weird flashes | ||
| if (theme.css) { | ||
| $(theme.css).remove(); | ||
| } | ||
|
|
||
| theme.css = styleNode; | ||
| return theme; | ||
| }); | ||
|
|
||
| return $.when(pending); | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. Ah yes. I commited those changes last night but didn't push. Will do that in a minute There was a problem hiding this comment. Oh no what's there is good. There is a the check if the theme is not falsy before it reads the text. The $.when is just a shortcut to always return a promise There was a problem hiding this comment. Oh, I see... it's not necessarily a promise if the current theme is null or undefined. |
||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Refresh current theme in the editor | ||
| * | ||
| * @param {boolean} force is to force reload the current theme | ||
| */ | ||
| function refresh(force) { | ||
| if (force) { | ||
| currentTheme = null; | ||
| } | ||
|
|
||
| $.when(force && loadCurrentTheme()).done(function () { | ||
| var editor = EditorManager.getActiveEditor(); | ||
| if (!editor || !editor._codeMirror) { | ||
| return; | ||
| } | ||
|
|
||
| var cm = editor._codeMirror; | ||
| ThemeView.setDocumentMode(cm); | ||
| ThemeView.updateThemes(cm); | ||
| }); | ||
|
|
@@ -285,9 +290,9 @@ define(function (require, exports, module) { | |
| * @return {$.Promise} promise object resolved with the theme to be loaded from fileName | ||
| */ | ||
| function loadFile(fileName, options) { | ||
| var deferred = new $.Deferred(), | ||
| file = FileSystem.getFileForPath(fileName), | ||
| currentThemeName = prefs.get("theme"); | ||
|
|
||
| file.exists(function (err, exists) { | ||
| var theme; | ||
|
|
@@ -298,9 +303,9 @@ define(function (require, exports, module) { | |
| ThemeSettings._setThemes(loadedThemes); | ||
|
|
||
| // For themes that are loaded after ThemeManager has been loaded, | ||
| // we should check if it's the current theme. If it is, then we just | ||
| // load it. | ||
| if (currentThemeName === theme.name) { | ||
| refresh(true); | ||
| } | ||
|
|
||
|
|
@@ -317,11 +322,11 @@ define(function (require, exports, module) { | |
| /** | ||
| * Loads a theme from an extension package. | ||
| * | ||
| * @param {Object} themePackage is a package from the extension manager for the theme to be loaded. | ||
| * @return {$.Promise} promise object resolved with the theme to be loaded from the pacakge | ||
| */ | ||
| function loadPackage(themePackage) { | ||
| var fileName = themePackage.path + "/" + themePackage.metadata.theme.file; | ||
|
There was a problem hiding this comment. Does this need to guard against Also, the two built-in themes need their metadata updated. (I've done this in my local copy... if the changes required are minor enough, I will likely just make them and then merge so that I can work from this when updating the themes...) There was a problem hiding this comment. Yeah they do. It's a simple change. It's just basically what we had talked about. Converting the theme string into an object with a file field... I am storing off the dark flag but it's not being added to the dom quite yet. I can adjust the default themes if you'd prefer... Or if you already there go ahead and dot it :) |
||
| return loadFile(fileName, themePackage.metadata); | ||
| } | ||
|
|
||
|
|
@@ -365,31 +370,23 @@ define(function (require, exports, module) { | |
| } | ||
| } | ||
|
|
||
| function loadThemeFiles(theme) { | ||
| // Iterate through each name in the themes and make them theme objects | ||
| var deferred = theme.files.forEach(function (themeFile) { | ||
| return loadFile(theme.path + "/" + themeFile); | ||
| }); | ||
|
|
||
| return $.when.apply(undefined, deferred); | ||
| } | ||
|
|
||
| FileSystem.getDirectoryForPath(path).getContents(readContent); | ||
| return result.then(loadThemeFiles); | ||
| } | ||
|
|
||
|
|
||
| prefs.on("change", "customScrollbars", function () { | ||
| refresh(); | ||
| ThemeView.updateScrollbars(getCurrentTheme()); | ||
| }); | ||
|
|
||
| prefs.on("change", "fontSize", function () { | ||
|
|
@@ -421,16 +418,44 @@ define(function (require, exports, module) { | |
| refresh(); | ||
| }); | ||
|
|
||
|
|
||
| // We need to postpone setting the prefs on change event handler for themes | ||
| // to prevent flickering | ||
| AppInit.appReady(function () { | ||
| prefs.on("change", "theme", function () { | ||
|
There was a problem hiding this comment. One thing that's worth noting about the prefs change events is that they only signal a possible change in the preference value, not a certain one. It may be the case that the flickering you saw was because it was refreshing the editor multiple times when it didn't really need to. There was a problem hiding this comment. Regardless, it does seem like we'd want to confirm that the theme is truly changing so that we don't emit There was a problem hiding this comment. The flickering was happening because I try to pre-emptively load a theme into the editor while themes are being loaded. This is so that when a theme is removed, and added later, it can be reapplied when it is installed... https://github.com/adobe/brackets/blob/master/src/view/ThemeManager.js#L309 So, that called refresh(true)... Then because I subscribe my theme change listener so early, that would eventually get called, which would in turn call refresh(true)... That double refresh true causes the flickering. I had to be a little more strategic about setting up the listener so that I don't load the same theme twice when brackets is starting up. I had done the alternative in the past of keeping track of an initted flag and what not, but that got pretty messy. This was the cleanest and least intrusive change I came up with. |
||
| // Save the theme that's about to be replaced to allow for some transition to prevent flicker | ||
| var oldTheme = getCurrentTheme(); | ||
|
|
||
| // Refresh editor with the new theme | ||
| refresh(true); | ||
|
|
||
| // Process the scrollbars for the editor | ||
| ThemeView.updateScrollbars(getCurrentTheme()); | ||
|
|
||
| // Expose event for theme changes | ||
| $(exports).trigger("themeChange", getCurrentTheme()); | ||
|
|
||
| // Delay removing the old style element to avoid flashes when transitioning between themes | ||
| if (oldTheme) { | ||
| setTimeout(function (theme) { | ||
| if (theme.css) { | ||
| $(theme.css).remove(); | ||
| } | ||
| }, 0, oldTheme); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
|
|
||
| ThemeView.updateFonts(); | ||
| ThemeView.updateScrollbars(); | ||
|
|
||
| exports.refresh = refresh; | ||
| exports.loadFile = loadFile; | ||
| exports.loadPackage = loadPackage; | ||
| exports.loadDirectory = loadDirectory; | ||
| exports.getCurrentTheme = getCurrentTheme; | ||
| exports.getAllThemes = getAllThemes; | ||
|
|
||
| // Exposed for testing purposes | ||
| exports._toDisplayName = toDisplayName; | ||
|
|
||
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.
This should be
options.theme.dark === true, I believe.