Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Validate editor preferences #7186

Merged
dangoor merged 29 commits into from
Mar 31, 2014
Merged

Validate editor preferences #7186

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
1274227
Editor Preferences Validation - interim checkin
redmunds Mar 12, 2014
1b1a604
start ValidationUtils module
redmunds Mar 12, 2014
7f721fc
Merge remote-tracking branch 'origin/master' into randy/issue-7020
redmunds Mar 12, 2014
8d5482e
convert remaining editor prefs
redmunds Mar 13, 2014
bc7b0bf
cleanup
redmunds Mar 13, 2014
d0ef02f
fix range limit of zero
redmunds Mar 13, 2014
3719aa2
more tests; cleanup comments
redmunds Mar 13, 2014
cfb0cce
implement preference validator
redmunds Mar 13, 2014
3428312
update for preference validator
redmunds Mar 13, 2014
f476ac5
changes for code review
redmunds Mar 13, 2014
8268997
more code review changes
redmunds Mar 13, 2014
fd72a4f
fix unit test; use ValidationUtils for validating\!
redmunds Mar 14, 2014
0b441d9
fix validation
redmunds Mar 14, 2014
5d22e35
add unit test for validator
redmunds Mar 14, 2014
0824ff6
implement validator on set
redmunds Mar 14, 2014
a32a39b
code cleanup
redmunds Mar 14, 2014
b6fb768
add jsdoc info
redmunds Mar 14, 2014
3a158fa
merge latest master
redmunds Mar 18, 2014
35b18b8
add test for negative space units
redmunds Mar 18, 2014
2418344
restore original API as deprecated
redmunds Mar 18, 2014
bc71a60
return both and from set()
redmunds Mar 20, 2014
a845bab
merge latest master
redmunds Mar 20, 2014
d2d4c72
fix tests
redmunds Mar 20, 2014
9883d3c
merge with latest master
redmunds Mar 23, 2014
835bc45
use lodash to generate validator function
redmunds Mar 23, 2014
2062a78
revert to old API with a few small changes
redmunds Mar 27, 2014
3d82669
jsdoc updates
redmunds Mar 27, 2014
aca8a81
Merge remote-tracking branch 'origin/master' into randy/issue-7020
redmunds Mar 27, 2014
76a5b91
fix whitespace
redmunds Mar 28, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/editor/CSSInlineEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ define(function (require, exports, module) {
*/
function _addRule(selectorName, inlineEditor, path) {
DocumentManager.getDocumentForPath(path).done(function (styleDoc) {
var newRuleInfo = CSSUtils.addRuleToDocument(styleDoc, selectorName, Editor.getUseTabChar(), Editor.getSpaceUnits());
var newRuleInfo = CSSUtils.addRuleToDocument(styleDoc, selectorName, inlineEditor.getUseTabChar(), inlineEditor.getSpaceUnits());
inlineEditor.addAndSelectRange(selectorName, styleDoc, newRuleInfo.range.from.line, newRuleInfo.range.to.line);
inlineEditor.editor.setCursorPos(newRuleInfo.pos.line, newRuleInfo.pos.ch);
});
Expand Down
78 changes: 53 additions & 25 deletions src/editor/Editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ define(function (require, exports, module) {
ViewUtils = require("utils/ViewUtils"),
Async = require("utils/Async"),
AnimationUtils = require("utils/AnimationUtils"),
ValidationUtils = require("utils/ValidationUtils"),
_ = require("thirdparty/lodash");

/** Editor preferences */
Expand All @@ -88,6 +89,11 @@ define(function (require, exports, module) {
CLOSE_TAGS = "closeTags",
cmOptions = {};

/** @type {number} Constants: default preference values */
var DEFAULT_SPACE_UNITS = 4,
DEFAULT_TAB_SIZE = 4;


// Mappings from Brackets preferences to CodeMirror options
cmOptions[SMART_INDENT] = "smartIndent";
cmOptions[USE_TAB_CHAR] = "indentWithTabs";
Expand All @@ -101,8 +107,16 @@ define(function (require, exports, module) {

PreferencesManager.definePreference(SMART_INDENT, "boolean", true);
PreferencesManager.definePreference(USE_TAB_CHAR, "boolean", false);
PreferencesManager.definePreference(TAB_SIZE, "number", 4);
PreferencesManager.definePreference(SPACE_UNITS, "number", 4);
PreferencesManager.definePreference(TAB_SIZE, "number", DEFAULT_TAB_SIZE, {
validator: function (value) {
return ValidationUtils.isIntegerInRange(value, 1, null);
}
});
PreferencesManager.definePreference(SPACE_UNITS, "number", DEFAULT_SPACE_UNITS, {
validator: function (value) {
return ValidationUtils.isIntegerInRange(value, 0, null);
Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 13, 2014

Choose a reason for hiding this comment

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

In https://github.com/adobe/brackets/blob/randy/issue-7020/src/editor/EditorStatusBar.js#L134 we make the value be between 1 and 10 for both preferences. Should we make 10 the max here too? And should we fix the min there so that it can be 0 for space units? Once we do that, the min and max values should be exported constants so that we can use them in EditorStatusBar.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 13, 2014

Choose a reason for hiding this comment

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

Ah, I forgot about that code! I agree that these should be consistent and I'll define a constant.

I thought @RaymondLim said that 0 is valid for space units, so that's why it's allowed.

Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 13, 2014

Choose a reason for hiding this comment

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

I don't mean changing the 0 value here, but allow the user to set it to 0 when changing it in the Status Bar.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 13, 2014

Choose a reason for hiding this comment

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

OK, that's what I did.

}
});
PreferencesManager.definePreference(CLOSE_BRACKETS, "boolean", false);
PreferencesManager.definePreference(SHOW_LINE_NUMBERS, "boolean", true);
PreferencesManager.definePreference(STYLE_ACTIVE_LINE, "boolean", false);
Expand Down Expand Up @@ -878,7 +892,7 @@ define(function (require, exports, module) {
*/
Editor.prototype.getColOffset = function (pos) {
var line = this._codeMirror.getRange({line: pos.line, ch: 0}, pos),
tabSize = Editor.getTabSize(),
tabSize = this.getTabSize(),
column = 0,
i;

Expand Down Expand Up @@ -1872,102 +1886,116 @@ define(function (require, exports, module) {

// Global settings that affect Editor instances that share the same preference locations

/**
* Sets whether to use smart indenting.
* Affects any editors that share the same preference location.
* @param {boolean} value
*/
Editor.prototype.setSmartIndent = function (value) {
Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 13, 2014

Choose a reason for hiding this comment

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

Do we need this function? It is not used anywhere in Brackets. Is it just for extensions? If it is we don't have a function for CLOSE_TAGS.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 13, 2014

Choose a reason for hiding this comment

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

I removed these methods. I originally was going to use them for validation, but used validator functions instead.

PreferencesManager.set(SMART_INDENT, value);
};

/** @type {boolean} Gets whether the current editor uses smart indenting */
Editor.prototype.getSmartIndent = function () {
return PreferencesManager.get(SMART_INDENT, this.document.file.fullPath);
};

/**
* Sets whether to use tab characters (vs. spaces) when inserting new text.
* Affects any editors that share the same preference location.
* @param {boolean} value
*/
Editor.setUseTabChar = function (value) {
Editor.prototype.setUseTabChar = function (value) {
PreferencesManager.set(USE_TAB_CHAR, value);
};

/** @type {boolean} Gets whether the current editor uses tab characters (vs. spaces) when inserting new text */
Editor.getUseTabChar = function () {
return PreferencesManager.get(USE_TAB_CHAR);
Editor.prototype.getUseTabChar = function () {
Copy link
Copy Markdown
Contributor

@dangoor dangoor Mar 14, 2014

Choose a reason for hiding this comment

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

Why are these moving from Editor to Editor.prototype? We'd have to look at the backwards compatibility implications of this.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 14, 2014

Choose a reason for hiding this comment

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

I did this so we have this.document.file.fullPath.

Yes, I had to rework the code in EditorStatusBar. One implication (of the new preferences changes in general, and not this change in particular) is that the status bar now updates for the focused editor. So, if you happen to have different prefs for .html and .css files, then when you switch focus between main and inline css editor, the prefs in status bar with change for the current file being edited. Or should status bar always show prefs for main editor?

Copy link
Copy Markdown
Contributor

@dangoor dangoor Mar 17, 2014

Choose a reason for hiding this comment

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

That seems like a positive change to me.

Still, this is a change to public API, which means we should probably deprecate the old API and not just remove it. You could use the Brackets Extension Grabber to see if there are any extensions that call Editor.set* or Editor.get*, or you could just add deprecation warnings.

Copy link
Copy Markdown
Member

@peterflynn peterflynn Mar 17, 2014

Choose a reason for hiding this comment

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

@redmunds The status bar already behaves this way (e.g. the language indicator updates). Are you saying there was a bug where the indent indicator didn't update on focus change before?

Copy link
Copy Markdown
Member

@peterflynn peterflynn Mar 17, 2014

Choose a reason for hiding this comment

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

I just tested in master and the status bar already updates the indent indicator too when focus switches between the inline editor and the full-size editor -- so the behavior isn't changing here.

Copy link
Copy Markdown
Contributor

@dangoor dangoor Mar 18, 2014

Choose a reason for hiding this comment

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

@peterflynn Agreed. Thanks for doing the search!

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 18, 2014

Choose a reason for hiding this comment

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

I just tested in master and the status bar already updates the indent indicator too when focus switches between the inline editor and the full-size editor -- so the behavior isn't changing here.

Yes, the PreferencesManager uses the current file when a file is not passed as a param, but I can see that being a problem with split view or multiple window support.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 18, 2014

Choose a reason for hiding this comment

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

Yes, I will restore the original API. Any tips or sample code I can look at for displaying "deprecated" messages to console?

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 18, 2014

Choose a reason for hiding this comment

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

Original API restored and deprecated.

Copy link
Copy Markdown
Member

@peterflynn peterflynn Mar 18, 2014

Choose a reason for hiding this comment

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

@redmunds Good points. Pretty sure the old static APIs would still work fine with split views as well, for the same reason inlines do today (activeEditor changes with focus, and there's only one statusbar shared between the views)... but it's definitely much cleaner to be explicitly calling out an Editor instance and thus explicitly requesting a file-specific pref.

return PreferencesManager.get(USE_TAB_CHAR, this.document.file.fullPath);
};

/**
* Sets tab character width.
* Affects any editors that share the same preference location.
* @param {number} value
*/
Editor.setTabSize = function (value) {
Editor.prototype.setTabSize = function (value) {
PreferencesManager.set(TAB_SIZE, value);
};

/** @type {number} Get indent unit */
Editor.getTabSize = function () {
return PreferencesManager.get(TAB_SIZE);
/** @type {number} Get tab character width */
Editor.prototype.getTabSize = function () {
return PreferencesManager.get(TAB_SIZE, this.document.file.fullPath);
};

/**
* Sets indentation width.
* Affects any editors that share the same preference location.
* @param {number} value
*/
Editor.setSpaceUnits = function (value) {
Editor.prototype.setSpaceUnits = function (value) {
PreferencesManager.set(SPACE_UNITS, value);
};

/** @type {number} Get indentation width */
Editor.getSpaceUnits = function () {
return PreferencesManager.get(SPACE_UNITS);
Editor.prototype.getSpaceUnits = function () {
return PreferencesManager.get(SPACE_UNITS, this.document.file.fullPath);
};

/**
* Sets the auto close brackets.
* Affects any editors that share the same preference location.
* @param {boolean} value
*/
Editor.setCloseBrackets = function (value) {
Editor.prototype.setCloseBrackets = function (value) {
PreferencesManager.set(CLOSE_BRACKETS, value);
};

/** @type {boolean} Gets whether the current editor uses auto close brackets */
Editor.getCloseBrackets = function () {
return PreferencesManager.get(CLOSE_BRACKETS);
Editor.prototype.getCloseBrackets = function () {
return PreferencesManager.get(CLOSE_BRACKETS, this.document.file.fullPath);
};

/**
* Sets show line numbers option.
* Affects any editors that share the same preference location.
* @param {boolean} value
*/
Editor.setShowLineNumbers = function (value) {
Editor.prototype.setShowLineNumbers = function (value) {
PreferencesManager.set(SHOW_LINE_NUMBERS, value);
};

/** @type {boolean} Returns true if show line numbers is enabled for the current editor */
Editor.getShowLineNumbers = function () {
return PreferencesManager.get(SHOW_LINE_NUMBERS);
Editor.prototype.getShowLineNumbers = function () {
return PreferencesManager.get(SHOW_LINE_NUMBERS, this.document.file.fullPath);
};

/**
* Sets show active line option.
* Affects any editors that share the same preference location.
* @param {boolean} value
*/
Editor.setShowActiveLine = function (value) {
Editor.prototype.setShowActiveLine = function (value) {
PreferencesManager.set(STYLE_ACTIVE_LINE, value);
};

/** @type {boolean} Returns true if show active line is enabled for the current editor */
Editor.getShowActiveLine = function () {
return PreferencesManager.get(STYLE_ACTIVE_LINE);
Editor.prototype.getShowActiveLine = function () {
return PreferencesManager.get(STYLE_ACTIVE_LINE, this.document.file.fullPath);
};

/**
* Sets word wrap option.
* Affects any editors that share the same preference location.
* @param {boolean} value
*/
Editor.setWordWrap = function (value) {
Editor.prototype.setWordWrap = function (value) {
PreferencesManager.set(WORD_WRAP, value);
};

/** @type {boolean} Returns true if word wrap is enabled for the current editor */
Editor.getWordWrap = function () {
return PreferencesManager.get(WORD_WRAP);
Editor.prototype.getWordWrap = function () {
return PreferencesManager.get(WORD_WRAP, this.document.file.fullPath);
};

// Set up listeners for preference changes
Expand Down
44 changes: 23 additions & 21 deletions src/editor/EditorStatusBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,27 +64,28 @@ define(function (require, exports, module) {
$fileInfo.text(_formatCountable(lines, Strings.STATUSBAR_LINE_COUNT_SINGULAR, Strings.STATUSBAR_LINE_COUNT_PLURAL));
}

function _updateIndentType() {
var indentWithTabs = Editor.getUseTabChar();
function _updateIndentType(editor) {
Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 14, 2014

Choose a reason for hiding this comment

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

I know that this code didn't had any JSDocs before, but should we add them now?

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 14, 2014

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 14, 2014

Choose a reason for hiding this comment

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

Thanks. Functions without docs look ugly, hehe.

var indentWithTabs = editor.getUseTabChar();
$indentType.text(indentWithTabs ? Strings.STATUSBAR_TAB_SIZE : Strings.STATUSBAR_SPACES);
$indentType.attr("title", indentWithTabs ? Strings.STATUSBAR_INDENT_TOOLTIP_SPACES : Strings.STATUSBAR_INDENT_TOOLTIP_TABS);
$indentWidthLabel.attr("title", indentWithTabs ? Strings.STATUSBAR_INDENT_SIZE_TOOLTIP_TABS : Strings.STATUSBAR_INDENT_SIZE_TOOLTIP_SPACES);
}

function _getIndentSize() {
return Editor.getUseTabChar() ? Editor.getTabSize() : Editor.getSpaceUnits();
function _getIndentSize(editor) {
return editor.getUseTabChar() ? editor.getTabSize() : editor.getSpaceUnits();
}

function _updateIndentSize() {
var size = _getIndentSize();
function _updateIndentSize(editor) {
var size = _getIndentSize(editor);
$indentWidthLabel.text(size);
$indentWidthInput.val(size);
}

function _toggleIndentType() {
Editor.setUseTabChar(!Editor.getUseTabChar());
_updateIndentType();
_updateIndentSize();
var current = EditorManager.getActiveEditor();
current.setUseTabChar(!current.getUseTabChar());
_updateIndentType(current);
_updateIndentSize(current);
}

function _updateCursorInfo(event, editor) {
Expand Down Expand Up @@ -116,7 +117,7 @@ define(function (require, exports, module) {
$cursorInfo.text(cursorStr + selStr);
}

function _changeIndentWidth(value) {
function _changeIndentWidth(editor, value) {
$indentWidthLabel.removeClass("hidden");
$indentWidthInput.addClass("hidden");

Expand All @@ -131,14 +132,14 @@ define(function (require, exports, module) {
}

value = Math.max(Math.min(Math.floor(value), 10), 1);
if (Editor.getUseTabChar()) {
Editor.setTabSize(value);
if (editor.getUseTabChar()) {
editor.setTabSize(value);
} else {
Editor.setSpaceUnits(value);
editor.setSpaceUnits(value);
}

// update indicator
_updateIndentSize();
_updateIndentSize(editor);

// column position may change when tab size changes
_updateCursorInfo();
Expand Down Expand Up @@ -184,8 +185,8 @@ define(function (require, exports, module) {

$(current).on("cursorActivity.statusbar", _updateCursorInfo);
$(current).on("optionChange.statusbar", function () {
_updateIndentType();
_updateIndentSize();
_updateIndentType(current);
_updateIndentSize(current);
});
$(current).on("change.statusbar", function () {
// async update to keep typing speed smooth
Expand All @@ -200,8 +201,8 @@ define(function (require, exports, module) {
_updateLanguageInfo(current);
_updateFileInfo(current);
_initOverwriteMode(current);
_updateIndentType();
_updateIndentSize();
_updateIndentType(current);
_updateIndentSize(current);
}
}

Expand All @@ -219,21 +220,22 @@ define(function (require, exports, module) {
$indentWidthLabel
.on("click", function () {
// update the input value before displaying
$indentWidthInput.val(_getIndentSize());
var current = EditorManager.getActiveEditor();
$indentWidthInput.val(_getIndentSize(current));

$indentWidthLabel.addClass("hidden");
$indentWidthInput.removeClass("hidden");
$indentWidthInput.focus();

$indentWidthInput
.on("blur", function () {
_changeIndentWidth($indentWidthInput.val());
_changeIndentWidth(current, $indentWidthInput.val());
})
.on("keyup", function (event) {
if (event.keyCode === KeyEvent.DOM_VK_RETURN) {
$indentWidthInput.blur();
} else if (event.keyCode === KeyEvent.DOM_VK_ESCAPE) {
_changeIndentWidth(false);
_changeIndentWidth(current, false);
}
});
});
Expand Down
3 changes: 2 additions & 1 deletion src/extensions/default/JSLint/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ define(function (require, exports, module) {
var CodeInspection = brackets.getModule("language/CodeInspection"),
PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
Strings = brackets.getModule("strings"),
ValidationUtils = brackets.getModule("utils/ValidationUtils"),
Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 13, 2014

Choose a reason for hiding this comment

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

Not required anymore

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 13, 2014

Choose a reason for hiding this comment

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

Good catch! Fixed.

_ = brackets.getModule("thirdparty/lodash");

var prefs = PreferencesManager.getExtensionPrefs("jslint");
Expand Down Expand Up @@ -88,7 +89,7 @@ define(function (require, exports, module) {

if (!options.indent) {
// default to using the same indentation value that the editor is using
options.indent = PreferencesManager.get("spaceUnits");
options.indent = PreferencesManager.get("spaceUnits", fullPath);
Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 13, 2014

Choose a reason for hiding this comment

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

Any reason why we don't use editor.getSpaceUnits here?

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 13, 2014

Choose a reason for hiding this comment

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

We don't have an Editor instance.

Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 13, 2014

Choose a reason for hiding this comment

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

I see. I guess is fine then.

Copy link
Copy Markdown
Contributor

@dangoor dangoor Mar 14, 2014

Choose a reason for hiding this comment

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

Nice change! At first, I figured this wasn't required, but since we have the path in hand, we're better off using it. That makes this function potentially usable for mass linting independent of the current editor.

}

// If the user has not defined the environment, we use browser by default.
Expand Down
9 changes: 7 additions & 2 deletions src/preferences/PreferencesBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1102,7 +1102,8 @@ define(function (require, exports, module) {
type: type,
initial: initial,
name: options.name,
description: options.description
description: options.description,
validator: options.validator
});
this.set(id, initial, {
location: {
Expand Down Expand Up @@ -1431,7 +1432,11 @@ define(function (require, exports, module) {
if (scope) {
var result = scope.get(id, context);
if (result !== undefined) {
return _.cloneDeep(result);
var pref = this.getPreference(id);
var validator = pref && pref.validator;
if (!validator || validator(result)) {
return _.cloneDeep(result);
}
Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 13, 2014

Choose a reason for hiding this comment

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

There should be an else that returns the default value, or the initial one.

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 13, 2014

Choose a reason for hiding this comment

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

I don't think we want an else case here. The default value should be set via definePreference call and its value should be returned from default scope if other scopes fail to return a value.

Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 13, 2014

Choose a reason for hiding this comment

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

Sure. Didn't saw the for loop. It should be done after the loop.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Mar 13, 2014

Choose a reason for hiding this comment

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

The default values are stored in the lowest priority scope, so if none of the other scopes return anything, then the default value will be returned. So, we don't need to handle default value.

Copy link
Copy Markdown
Contributor

@dangoor dangoor Mar 14, 2014

Choose a reason for hiding this comment

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

Nice! I think I saw this comment elsewhere, but a test in PreferencesBase-test for this would be a good addition.

}
}
}
Expand Down
Loading
⚡