Themes in Brackets core #7616

MiguelCastillo · 2014-04-24T01:08:51Z

Initial commit. A few items remain to be done, but themes are now fully functional as a core component so you can start testing this out.

https://github.com/adobe/brackets/wiki/Themes

ThemeManager goes into core.
ExtensionLoader has special logic for themes extensions that don’t have JS
Settings pared down to just your “General” screen
All of the other loading/scanning code can go away
Use jQuery promises, sorry :*(
Settings screen should not use Knockout
Use new prefs API
Unit tests for theme loading and the customizations (scrollbars, etc.)

dangoor · 2014-04-24T01:36:15Z

src/themes/DefaultSettings.js

@@ -0,0 +1,24 @@
+/**


This can probably just go in Settings.js

marcelgerber · 2014-04-24T01:39:24Z

All used strings should be localized.

And btw, Travis failed (because of missing JSLint comments).

marcelgerber · 2014-04-24T01:41:54Z

src/themes/Settings.js

+        return PreferencesManager.ready.then(function() {
+            if ( Settings.getValue("paths") === (void 0) ) {
+                Settings.setValue("paths", DefaultSettings.paths);
+            }


I guess prefs.definePreference() should be used here to define default settings

MiguelCastillo · 2014-04-24T01:48:47Z

Yeah, there are a few items in my list for code cleanup and integrate with what brackets already has. I think that by tomorrow or friday, code will be more inline with what we all expect :)

MiguelCastillo · 2014-04-24T01:49:39Z

Love how quickly you guys are providing feedback! Its awesome

dangoor · 2014-04-24T01:59:15Z

For some added context for those tuning in to this pull request right now: I asked Miguel if he could convert his extension into a pull request against core as something that we could begin discussing and also so that it was possible for it to load themes as extensions, which is not possible in an extension today. I added (REVIEW ONLY) because there's definitely more coming before this feature lands.

marcelgerber · 2014-04-24T02:02:52Z

src/command/Commands.js

@@ -109,6 +109,7 @@ define(function (require, exports, module) {
    exports.SORT_WORKINGSET_BY_NAME     = "view.sortWorkingSetByName";  // WorkingSetSort.js            _handleSortWorkingSetByName()
    exports.SORT_WORKINGSET_BY_TYPE     = "view.sortWorkingSetByType";  // WorkingSetSort.js            _handleSortWorkingSetByType()
    exports.SORT_WORKINGSET_AUTO        = "view.sortWorkingSetAuto";    // WorkingSetSort.js            _handleAutomaticSort()
+    exports.THEMES                      = "view.themes";                // themes


Stay in sync with the detailed comments of the other entries.

We should probably also use CMD_THEMES after the change done with the Find commands

This works by checking if the “theme” field exists in the package.json to determine if the extension ought to be treated as a theme or not.

So that we can preemptively figure out if the extension is a theme or a regular extension.

MiguelCastillo · 2014-04-26T13:07:20Z

Alright, I think we are at a point where feedback would be great. Themes code is fully integrated into Brackets.

I created a theme that is already in the Extension Registry. The theme has to have a main.js or the registry wouldn't allow me to load it. So, we might need to adjust something there. This is the git link for the theme also
https://github.com/MiguelCastillo/Brackets-VisualStudioTheme

I am going to evaluate inline editor and mixin support. #4850

marcelgerber · 2014-04-27T18:44:01Z

src/extensibility/ExtensionManager.js

+    /**
+     * @private
+     * Verifies if an extension is a theme based on the presence of the field "theme"
+     * in the package.json.  If it is a theme, then the theme file is just loaded by the


nit: 2 spaces

marcelgerber · 2014-04-27T18:52:41Z

https://github.com/adobe/brackets/wiki/Brackets-Coding-Conventions

larz0 · 2014-06-12T20:31:43Z

@MiguelCastillo give me 5mins, I got disconnected from IRC for pasting too much console.log.

larz0 · 2014-06-13T20:26:42Z

@MiguelCastillo it seems like "Restore font size" won't work. Just a heads up.

FezVrasta · 2014-06-17T12:50:43Z

Umh sorry for my intrusion ^_^"

I've cloned this PR but when I apply a theme using "View > Themes" nothing happens... Is it normal?

MiguelCastillo · 2014-06-17T13:44:19Z

@FezVrasta Ah! No intrusion at all! We want your feedback.

You don't get a dialog at all?

FezVrasta · 2014-06-17T13:45:51Z

I can open the window where I can choose the theme but when I click save the dialog is closed without any change of the theme.

MiguelCastillo · 2014-06-17T13:48:06Z

Would you like to jump in IRC so that I can help you, help me fix the issue?

FezVrasta · 2014-06-17T13:50:25Z

Sure, I'm there.

This is the log I get:

GET file:///C:/Program%20Files%20(x86)/Brackets/dev/src/extensions/default/Thor…me/C:/Program%20Files%20(x86)/Brackets/dev/src/styles/brackets_colors.less  less-1.7.0.min.js:13
r less-1.7.0.min.js:13
s less-1.7.0.min.js:13
I.imports.push less-1.7.0.min.js:13
a.importVisitor.visitImport less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.visitor.visitArray less-1.7.0.min.js:15
a.Ruleset.accept less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.visitor.visitArray less-1.7.0.min.js:15
a.Ruleset.accept less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.importVisitor.run less-1.7.0.min.js:15
E.parse less-1.7.0.min.js:13
lessifyTheme ThemeManager.js:120
(anonymous function) ThemeManager.js:185
(anonymous function) jquery-2.1.0.min.js:2
j jquery-2.1.0.min.js:2
k.fireWith jquery-2.1.0.min.js:2
(anonymous function) jquery-2.1.0.min.js:2
j jquery-2.1.0.min.js:2
k.fireWith jquery-2.1.0.min.js:2
e.(anonymous function) jquery-2.1.0.min.js:2
(anonymous function) FileUtils.js:64
(anonymous function) File.js:126
(anonymous function) AppshellFileSystem.js:392
GET file:///C:/Program%20Files%20(x86)/Brackets/dev/src/extensions/default/Thor…rogram%20Files%20(x86)/Brackets/dev/src/styles/brackets_theme_default.less  less-1.7.0.min.js:13
r less-1.7.0.min.js:13
s less-1.7.0.min.js:13
I.imports.push less-1.7.0.min.js:13
a.importVisitor.visitImport less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.visitor.visitArray less-1.7.0.min.js:15
a.Ruleset.accept less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.visitor.visitArray less-1.7.0.min.js:15
a.Ruleset.accept less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.importVisitor.run less-1.7.0.min.js:15
E.parse less-1.7.0.min.js:13
lessifyTheme ThemeManager.js:120
(anonymous function) ThemeManager.js:185
(anonymous function) jquery-2.1.0.min.js:2
j jquery-2.1.0.min.js:2
k.fireWith jquery-2.1.0.min.js:2
(anonymous function) jquery-2.1.0.min.js:2
j jquery-2.1.0.min.js:2
k.fireWith jquery-2.1.0.min.js:2
e.(anonymous function) jquery-2.1.0.min.js:2
(anonymous function) FileUtils.js:64
(anonymous function) File.js:126
(anonymous function) AppshellFileSystem.js:392
GET file:///C:/Program%20Files%20(x86)/Brackets/dev/src/extensions/default/Thor…%20Files%20(x86)/Brackets/dev/src/styles/brackets_codemirror_override.less  less-1.7.0.min.js:13
r less-1.7.0.min.js:13
s less-1.7.0.min.js:13
I.imports.push less-1.7.0.min.js:13
a.importVisitor.visitImport less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.visitor.visitArray less-1.7.0.min.js:15
a.Ruleset.accept less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.visitor.visitArray less-1.7.0.min.js:15
a.Ruleset.accept less-1.7.0.min.js:15
a.visitor.visit less-1.7.0.min.js:15
a.importVisitor.run less-1.7.0.min.js:15
E.parse less-1.7.0.min.js:13
lessifyTheme ThemeManager.js:120
(anonymous function) ThemeManager.js:185
(anonymous function) jquery-2.1.0.min.js:2
j jquery-2.1.0.min.js:2
k.fireWith jquery-2.1.0.min.js:2
(anonymous function) jquery-2.1.0.min.js:2
j jquery-2.1.0.min.js:2
k.fireWith jquery-2.1.0.min.js:2
e.(anonymous function) jquery-2.1.0.min.js:2
(anonymous function) FileUtils.js:64
(anonymous function) File.js:126
(anonymous function) AppshellFileSystem.js:392
Uncaught [object Object] less-1.7.0.min.js:13
(anonymous function) less-1.7.0.min.js:13
(anonymous function) ThemeManager.js:125
p less-1.7.0.min.js:13
a.importVisitor.run less-1.7.0.min.js:15
E.parse less-1.7.0.min.js:13
lessifyTheme ThemeManager.js:120
(anonymous function) ThemeManager.js:185
(anonymous function) jquery-2.1.0.min.js:2
j jquery-2.1.0.min.js:2
k.fireWith jquery-2.1.0.min.js:2
(anonymous function) jquery-2.1.0.min.js:2
j jquery-2.1.0.min.js:2
k.fireWith jquery-2.1.0.min.js:2
e.(anonymous function) jquery-2.1.0.min.js:2
(anonymous function) FileUtils.js:64
(anonymous function) File.js:126
(anonymous function) AppshellFileSystem.js:392

dangoor · 2014-06-17T15:21:44Z

src/view/ThemeManager.js

+    function loadCurrentThemes() {
+        return $.when(undefined, _.map(getCurrentThemes(), function (theme) {
+            if (!theme) {
+                return $.Deferred().reject("Theme not found");


this should actually be:

return new $.Deferred().reject("Theme not found").promise();

Unfortunately, that error doesn't give a lot of information (what theme wasn't found?). It appears that information is not available in this context. It's not clear to me why getCurrentThemes() would even return an empty value.

dangoor · 2014-06-17T15:30:30Z

My suggestions for tests would be:

write tests for the logic in Theme and ThemeSettings. Stuff like getDisplayName is really easy to write fast unit tests for. With a little tweaking, ThemeSettings may be easy to test just by rendering the form to a div and causing changes to happen and see what happens.
A lot of what's here is just integration code and visuals (CSS). Because the built-in themes work in the same way as extension themes, these will naturally get a lot of real user testing. An automated test that tries changing the theme and verifies that new stylesheets were applied and such could be valuable just for ensuring the basic connection of all of the parts...

…This cleaned up the rejected promises being returned. Fixed issue in loadCurrentTheme incorrect $.when call. Cleaned up a few other small issues

Conflicts: src/nls/root/strings.js src/styles/brackets_codemirror_override.less

…ht-theme

…er for the settings UI

dangoor · 2014-07-08T13:09:29Z

Closing this PR in favor of #8302.

MiguelCastillo · 2014-07-11T22:24:55Z

@FezVrasta Hey! I fixed the issue you are running into in Windows... If you want to give latest another try, that would be really nice :) https://github.com/MiguelCastillo/brackets branch themes-v1.

FezVrasta · 2014-07-16T07:32:00Z

Yes seems fixed now :)

Initial commit to integrate themes into brackets core

d12d87b

dangoor changed the title ~~Themes in Brackets core~~ (REVIEW ONLY) Themes in Brackets core Apr 24, 2014

dangoor reviewed Apr 24, 2014
View reviewed changes

src/themes/DefaultSettings.js

@@ -0,0 +1,24 @@

/**

Copy link

Contributor

dangoor Apr 24, 2014

Choose a reason for hiding this comment

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

This can probably just go in Settings.js

marcelgerber reviewed Apr 24, 2014
View reviewed changes

MiguelCastillo added 4 commits April 23, 2014 21:50

Delete stale Init.js file

657df85

Cleaned up jslint errors/warnings

5cb4085

Removed string.js. Not needed anymore

abf3e49

Removed custom themes from the themes extension

5473380

Cleaned up extra white spaces

a1dea7e

marcelgerber reviewed Apr 24, 2014
View reviewed changes

MiguelCastillo added 5 commits April 23, 2014 22:10

Adjusted comments to follow convention

6f1df7a

Adding CMD_THEMES to nls strings.js

361d715

Checking support to load themes as extensions

7a8997f

This works by checking if the “theme” field exists in the package.json to determine if the extension ought to be treated as a theme or not.

Refactored ExtensionLoader so that it can preload package.json

2856006

So that we can preemptively figure out if the extension is a theme or a regular extension.

Removed logic to load CodeMirror themes automatically

b0df2fc

marcelgerber reviewed Apr 27, 2014
View reviewed changes

dangoor reviewed Jun 17, 2014
View reviewed changes

Removed extra call the get promise object. Redundant.

22fc569

MiguelCastillo added 7 commits June 19, 2014 22:58

Removed stale dependency

67abe19

Added a default to be loaded when the current theme isn't available. …

e14f151

…This cleaned up the rejected promises being returned. Fixed issue in loadCurrentTheme incorrect $.when call. Cleaned up a few other small issues

Merge remote-tracking branch 'upstream/master'

d302434

Conflicts: src/nls/root/strings.js src/styles/brackets_codemirror_override.less

Export reset settings interface and set the default theme to thor-lig…

31cb2a6

…ht-theme

Changed loop for iterating through new settings. Feedback from Kevin

b6a7ef3

Cleaned up a bit of the logic for selecting themes in the event handl…

86f78eb

…er for the settings UI

Cleaned up some of the comments and stale variables

a102a51

dangoor added Community labels Jun 26, 2014

MiguelCastillo added 3 commits June 26, 2014 21:04

Merge remote-tracking branch 'upstream/master'

0ce29ce

Removing extra promise call

c2d26f2

Conflict resolution for CodeMirror

8f2619f

MiguelCastillo mentioned this pull request Jul 3, 2014

Themes in Core v1 #8302

Merged

dangoor closed this Jul 8, 2014

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Themes in Brackets core brackets-archive/bracketsIssues#6857

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Themes in Brackets core #7616

Themes in Brackets core #7616

MiguelCastillo commented Apr 24, 2014

dangoor Apr 24, 2014

marcelgerber commented Apr 24, 2014

marcelgerber Apr 24, 2014

MiguelCastillo commented Apr 24, 2014

MiguelCastillo commented Apr 24, 2014

dangoor commented Apr 24, 2014

marcelgerber Apr 24, 2014

TomMalbran Apr 24, 2014

MiguelCastillo commented Apr 26, 2014

marcelgerber Apr 27, 2014

marcelgerber commented Apr 27, 2014

larz0 commented Jun 12, 2014

larz0 commented Jun 13, 2014

FezVrasta commented Jun 17, 2014

MiguelCastillo commented Jun 17, 2014

FezVrasta commented Jun 17, 2014

MiguelCastillo commented Jun 17, 2014

FezVrasta commented Jun 17, 2014

dangoor Jun 17, 2014

dangoor commented Jun 17, 2014

dangoor commented Jul 8, 2014

MiguelCastillo commented Jul 11, 2014

FezVrasta commented Jul 16, 2014

Themes in Brackets core #7616

Themes in Brackets core #7616

Conversation

MiguelCastillo commented Apr 24, 2014

dangoor Apr 24, 2014

Choose a reason for hiding this comment

marcelgerber commented Apr 24, 2014

marcelgerber Apr 24, 2014

Choose a reason for hiding this comment

MiguelCastillo commented Apr 24, 2014

MiguelCastillo commented Apr 24, 2014

dangoor commented Apr 24, 2014

marcelgerber Apr 24, 2014

Choose a reason for hiding this comment

TomMalbran Apr 24, 2014

Choose a reason for hiding this comment

MiguelCastillo commented Apr 26, 2014

marcelgerber Apr 27, 2014

Choose a reason for hiding this comment

marcelgerber commented Apr 27, 2014

larz0 commented Jun 12, 2014

larz0 commented Jun 13, 2014

FezVrasta commented Jun 17, 2014

MiguelCastillo commented Jun 17, 2014

FezVrasta commented Jun 17, 2014

MiguelCastillo commented Jun 17, 2014

FezVrasta commented Jun 17, 2014

dangoor Jun 17, 2014

Choose a reason for hiding this comment

dangoor commented Jun 17, 2014

dangoor commented Jul 8, 2014

MiguelCastillo commented Jul 11, 2014

FezVrasta commented Jul 16, 2014