-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Themes in Brackets core #7616
Themes in Brackets core #7616
Changes from 11 commits
d12d87b
657df85
5cb4085
abf3e49
5473380
a1dea7e
6f1df7a
361d715
7a8997f
2856006
b0df2fc
1c64c1a
a572d22
e68c3ec
43b5abb
92ac359
fb54d2f
bc7054f
767c9e5
5c6f83b
d83bf29
0854441
f9d29db
d55254f
f686926
02515b1
e037fc0
8ee527b
fe05d09
f8eac83
1ba4ac0
5bd2d89
e46a93d
b2d0702
768d2f0
2c74247
a1f6c6a
bce17ad
9832add
6cf0eae
803c26f
fc498a8
796322a
2f94472
ff3c065
ab93c01
1d81228
8d94e86
7834318
280c83e
72f6354
43ed137
1c05805
ea1aab0
1416cd9
8bfd51e
ef8b5cd
2c06fd2
c38e68d
79f7350
aebe379
8cef24a
710d4e0
9894c0e
72f5f34
57df196
7f5a33c
d71bbcb
cdfa4be
1491ad3
9188e5c
22fc569
67abe19
e14f151
d302434
31cb2a6
b6a7ef3
86f78eb
a102a51
0ce29ce
c2d26f2
8f2619f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,9 +44,11 @@ define(function (require, exports, module) { | |
Package = require("extensibility/Package"), | ||
Async = require("utils/Async"), | ||
ExtensionLoader = require("utils/ExtensionLoader"), | ||
ExtensionUtils = require("utils/ExtensionUtils"), | ||
FileSystem = require("filesystem/FileSystem"), | ||
Strings = require("strings"), | ||
StringUtils = require("utils/StringUtils"); | ||
StringUtils = require("utils/StringUtils"), | ||
ThemeManager = require("themes/ThemeManager"); | ||
|
||
// semver.browser is an AMD-compatible module | ||
var semver = require("extensibility/node/node_modules/semver/semver.browser"); | ||
|
@@ -132,6 +134,21 @@ define(function (require, exports, module) { | |
|
||
$(exports).triggerHandler("registryUpdate", [id]); | ||
} | ||
|
||
|
||
/** | ||
* @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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: 2 spaces |
||
* ThemeManager | ||
*/ | ||
function loadTheme(id) { | ||
var extension = extensions[id]; | ||
if ( extension.installInfo && extension.installInfo.metadata && extension.installInfo.metadata.theme ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK, trailing and leading whitespace in parentheses is not common, or not even accepted, in Brackets' coding conventions. |
||
ThemeManager.loadFile(extension.installInfo.path + "/" + extension.installInfo.metadata.theme); | ||
} | ||
} | ||
|
||
|
||
/** | ||
* @private | ||
|
@@ -185,30 +202,6 @@ define(function (require, exports, module) { | |
return result.promise(); | ||
} | ||
|
||
/** | ||
* @private | ||
* Loads the package.json file in the given extension folder. | ||
* @param {string} folder The extension folder. | ||
* @return {$.Promise} A promise object that is resolved with the parsed contents of the package.json file, | ||
* or rejected if there is no package.json or the contents are not valid JSON. | ||
*/ | ||
function _loadPackageJson(folder) { | ||
var file = FileSystem.getFileForPath(folder + "/package.json"), | ||
result = new $.Deferred(); | ||
FileUtils.readAsText(file) | ||
.done(function (text) { | ||
try { | ||
var json = JSON.parse(text); | ||
result.resolve(json); | ||
} catch (e) { | ||
result.reject(); | ||
} | ||
}) | ||
.fail(function () { | ||
result.reject(); | ||
}); | ||
return result.promise(); | ||
} | ||
|
||
/** | ||
* @private | ||
|
@@ -245,10 +238,11 @@ define(function (require, exports, module) { | |
status: (e.type === "loadFailed" ? START_FAILED : ENABLED) | ||
}; | ||
synchronizeEntry(id); | ||
loadTheme(id); | ||
$(exports).triggerHandler("statusChange", [id]); | ||
} | ||
|
||
_loadPackageJson(path) | ||
ExtensionUtils.loadPackageJson(path) | ||
.done(function (metadata) { | ||
setData(metadata.name, metadata); | ||
}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these CodeMirror addons added automatically with themes? Is there some connection between themes and these addons? If there is some reason why we must enable these addons, we should do so in the main place where we enable CM addons (which I don't know offhand, but I'm sure it's there somewhere 😁) |
||
* Brackets Themes Copyright (c) 2014 Miguel Castillo. | ||
* | ||
* Licensed under MIT | ||
*/ | ||
|
||
|
||
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
/*global $, define, require */ | ||
|
||
define(function () { | ||
"use strict"; | ||
|
||
var CodeMirror = require("thirdparty/CodeMirror2/lib/codemirror"); | ||
|
||
function initAddons() { | ||
// Set some default value for codemirror... | ||
CodeMirror.defaults.highlightSelectionMatches = true; | ||
CodeMirror.defaults.styleSelectedText = true; | ||
} | ||
|
||
function init() { | ||
var deferred = $.Deferred(); | ||
|
||
/** | ||
* This is where is all starts to load up... | ||
*/ | ||
require([ | ||
"thirdparty/CodeMirror2/addon/selection/mark-selection", | ||
"thirdparty/CodeMirror2/addon/search/match-highlighter" | ||
], deferred.resolve); | ||
|
||
return deferred.done(initAddons).promise(); | ||
} | ||
|
||
return { | ||
ready: init() | ||
}; | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can probably just go in Settings.js |
||
* Brackets Themes Copyright (c) 2014 Miguel Castillo. | ||
* | ||
* Licensed under MIT | ||
*/ | ||
|
||
|
||
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
/*global $, define, require */ | ||
|
||
define(function(require) { | ||
"use strict"; | ||
|
||
var FileUtils = require("file/FileUtils"); | ||
var cm_path = FileUtils.getNativeBracketsDirectoryPath() + "/thirdparty/CodeMirror2"; | ||
|
||
return { | ||
"fontSize": 12, | ||
"lineHeight": '1.3em', | ||
"fontType": "'SourceCodePro-Medium', MS ゴシック, 'MS Gothic', monospace", | ||
"customScrollbars": true, | ||
"theme": ["default"], | ||
"paths": [ | ||
{path:require.toUrl("./") + "../../themes"}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing space after colon (Coding Conventions) |
||
{path:require.toUrl("./theme/")}, | ||
{path:cm_path + "/theme"} | ||
] | ||
}; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be merged into ViewCommandHandlers. In fact, it may make sense to put the code in |
||
* Brackets Themes Copyright (c) 2014 Miguel Castillo. | ||
* @author Brad Gearon | ||
* | ||
* Licensed under MIT | ||
*/ | ||
|
||
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
/*global $, define, require */ | ||
|
||
define(function (require) { | ||
"use strict"; | ||
|
||
var Settings = require("themes/Settings"), | ||
DefaultSettings = require("themes/DefaultSettings"), | ||
ViewCommandHandlers = require("view/ViewCommandHandlers"), | ||
PreferencesManager = require("preferences/PreferencesManager"); | ||
|
||
function ViewCommandsManager () { | ||
$(ViewCommandHandlers).on("fontSizeChange", updateThemeFontSize); | ||
$(Settings).on("change:fontSize", updateBracketsFontSize); | ||
updateBracketsFontSize(); | ||
} | ||
|
||
function updateThemeFontSize (evt, adjustment, fontSize /*, lineHeight*/) { | ||
Settings.setValue("fontSize", fontSize); | ||
} | ||
|
||
function updateBracketsFontSize() { | ||
var fontSize = Settings.getValue("fontSize"), | ||
fontSizeNumeric = Number(fontSize.replace(/px|em/, "")), | ||
fontSizeOffset = fontSizeNumeric - DefaultSettings.fontSize; | ||
|
||
if(!isNaN(fontSizeOffset)) { | ||
PreferencesManager.setViewState("fontSizeAdjustment", fontSizeOffset); | ||
PreferencesManager.setViewState("fontSizeStyle", fontSize); | ||
} | ||
} | ||
|
||
// Let's make sure we use Themes fonts by default | ||
return { | ||
init: ViewCommandsManager | ||
}; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The font settings should be merged with the Increase/Decrease Font Size commands. I am thinking that we should make the Since we don't have an UI for the preferences I am not sure if we should keep the UI used with the themes or just don't use any UI at the moment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe all these can be part of a different PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a UI to change themes at the very least, and I'd rather we have a dialog box with a few appearance-related settings than add a top-level menu. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind keeping the dialog. But when saving, it should only change the preferences values and the code in View Commands Manger can deal with the preference change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TomMalbran Yeah, optimally font values will be read/set from a core api. I remember some conversation we had around this here #7185. The idea was to make possibly make _adjustFontSize public. Adding the settings as you explain as view states will definitely help in the long run. If some makes those changes, I will be happy to make use of them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've opened #7800 to log that we should change the font size handling before landing themes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great. Once #7800 is merged, we should remove all the preferences here, and just call the methods from the View Commands. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeap, that would be super awesome and will clean up the code real nicely. |
||
* Brackets Themes Copyright (c) 2014 Miguel Castillo. | ||
* | ||
* Licensed under MIT | ||
*/ | ||
|
||
|
||
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
/*global $, define, require */ | ||
|
||
define(function () { | ||
"use strict"; | ||
|
||
var $lineHeight = $("<style type='text/css' id='lineHeight'>").appendTo("head"), | ||
$fontSize = $("<style type='text/css' id='fontSize'>").appendTo("head"), | ||
$fontType = $("<style type='text/css' id='fontType'>").appendTo("head"); | ||
|
||
var Settings = null; | ||
|
||
|
||
function FontSettings(_settings) { | ||
Settings = _settings; | ||
$(Settings).on("change:lineHeight", FontSettings.updateLineHeight); | ||
$(Settings).on("change:fontSize", FontSettings.updateFontSize); | ||
$(Settings).on("change:fontType", FontSettings.updateFontType); | ||
FontSettings.update(); | ||
} | ||
|
||
|
||
FontSettings.updateLineHeight = function () { | ||
clearFonts(); | ||
var value = Settings.getValue("lineHeight"); | ||
$lineHeight.text(".CodeMirror{" + "line-height: " + value + "; }"); | ||
}; | ||
|
||
|
||
FontSettings.updateFontSize = function () { | ||
clearFonts(); | ||
var value = Settings.getValue("fontSize"); | ||
$fontSize.text(".CodeMirror{" + "font-size: " + value + " !important; }"); | ||
}; | ||
|
||
|
||
FontSettings.updateFontType = function () { | ||
clearFonts(); | ||
var value = Settings.getValue("fontType"); | ||
$fontType.text(".CodeMirror{" + "font-family: " + value + " !important; }"); | ||
}; | ||
|
||
|
||
FontSettings.update = function () { | ||
clearFonts(); | ||
FontSettings.updateLineHeight(); | ||
FontSettings.updateFontSize(); | ||
FontSettings.updateFontType(); | ||
}; | ||
|
||
|
||
function clearFonts() { | ||
// Remove this tag that is intefering with font settings set in this module | ||
$("#codemirror-dynamic-fonts").remove(); | ||
} | ||
|
||
return FontSettings; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
Copyright (c) 2014 Miguel Castillo. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file just goes away as all of our code is MIT and newly contributed code comes in under the CLA. |
||
|
||
Licensed under MIT | ||
|
||
Permission is hereby granted, free of charge, to any person obtaining a | ||
copy of this software and associated documentation files (the "Software"), | ||
to deal in the Software without restriction, including without limitation | ||
the rights to use, copy, modify, merge, publish, distribute, sublicense, | ||
and/or sell copies of the Software, and to permit persons to whom the | ||
Software is furnished to do so, subject to the following conditions: | ||
|
||
The above copyright notice and this permission notice shall be included in | ||
all copies or substantial portions of the Software. | ||
|
||
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING | ||
FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER | ||
DEALINGS IN THE SOFTWARE. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/** | ||
* Brackets Themes Copyright (c) 2014 Miguel Castillo. | ||
* | ||
* Licensed under MIT | ||
*/ | ||
|
||
|
||
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
/*global $, define, require */ | ||
|
||
define(function(require) { | ||
"use strict"; | ||
var Commands = require("command/Commands"), | ||
CommandManager = require("command/CommandManager"), | ||
Strings = require("strings"), | ||
Settings = require("themes/Settings"); | ||
|
||
CommandManager.register(Strings.CMD_THEMES, Commands.THEMES, Settings.open); | ||
}); |
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 under
// VIEW
since is the first element in the menu, and should we callCMD_THEMES
?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.
We should.
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.
It is under views... Nothing starts with CMD in that file.
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.
It should be
exports.CMD_THEMES
. The team decided to do this so that there are no issues when moving/renaming menus. Commands should no longer be assigned to a menu, soCMD_
is the solution.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.
@SAplayer Just to clear, themes will be the ONLY definition that starts with CMD... Is that what you are looking to do with the rest of the definition?
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.
@TomMalbran I see what you mean by being under //VIEW... I read it as put it under the //VIEW group rather than make it the first definition. Yeah, sure I can move that.
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.
All the Find commands start with
CMD_
.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.
@SAplayer @TomMalbran Really guys, I am not trying to be a hard ass or anything... I prefer to follow convention. Majority of the does NOT start with CMD_, therefore I didn't start mine with CMD_. I will change it to CMD_ so that we can move on, but I am really looking to understand what the plan is long term. And it seems like everything will be eventually changed to CMD_.
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.
It is something new. Here is more info: #7556