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 11 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
71 changes: 46 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,14 @@ define(function (require, exports, module) {
CLOSE_TAGS = "closeTags",
cmOptions = {};

/** @type {number} Constants */
var MIN_SPACE_UNITS = 0,
MIN_TAB_SIZE = 1,
DEFAULT_SPACE_UNITS = 4,
DEFAULT_TAB_SIZE = 4,
MAX_SPACE_UNITS = 10,
MAX_TAB_SIZE = 10;

// Mappings from Brackets preferences to CodeMirror options
cmOptions[SMART_INDENT] = "smartIndent";
cmOptions[USE_TAB_CHAR] = "indentWithTabs";
Expand All @@ -101,8 +110,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, MIN_TAB_SIZE, MAX_TAB_SIZE);
}
});
PreferencesManager.definePreference(SPACE_UNITS, "number", DEFAULT_SPACE_UNITS, {
validator: function (value) {
return ValidationUtils.isIntegerInRange(value, MIN_SPACE_UNITS, MAX_SPACE_UNITS);
}
});
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 +895,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 @@ -1877,97 +1894,97 @@ define(function (require, exports, module) {
* 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 Expand Up @@ -1998,4 +2015,8 @@ define(function (require, exports, module) {
exports.Editor = Editor;
exports.BOUNDARY_CHECK_NORMAL = BOUNDARY_CHECK_NORMAL;
exports.BOUNDARY_IGNORE_TOP = BOUNDARY_IGNORE_TOP;
exports.MIN_SPACE_UNITS = MIN_SPACE_UNITS;
exports.MAX_SPACE_UNITS = MAX_SPACE_UNITS;
exports.MIN_TAB_SIZE = MIN_TAB_SIZE;
exports.MAX_TAB_SIZE = MAX_TAB_SIZE;
});
47 changes: 25 additions & 22 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 @@ -130,15 +131,16 @@ define(function (require, exports, module) {
return;
}

value = Math.max(Math.min(Math.floor(value), 10), 1);
if (Editor.getUseTabChar()) {
Editor.setTabSize(value);
if (editor.getUseTabChar()) {
value = Math.max(Math.min(Math.floor(value), Editor.MAX_TAB_SIZE), Editor.MIN_TAB_SIZE);
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.

This is fine, but seeing it here and below makes me think that this also belongs in the "model" (the preferences system). The most succinct form would be:

definePreference("tabSize", "integer", DEFAULT_TAB_SIZE, {
    min: MIN_TAB_SIZE,
    max: MAX_TAB_SIZE
});

But, even without going to that extreme, we could still do this:

PreferencesManager.definePreference(TAB_SIZE, "number", DEFAULT_TAB_SIZE, {
    validate: function (value) {
        return ValidationUtils.isIntegerInRange(value, MIN_TAB_SIZE, MAX_TAB_SIZE);
    },
    coerce: function (value) {
        return Math.max(Math.min(Math.floor(value), MAX_TAB_SIZE), MIN_TAB_SIZE);
    }
});

and have coerce run on PreferencesSystem.set. That way, this code would not be duplicated between the status bar and some potential preferences UI (cc @zaggino).

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'm not sure that will get us much. In this particular case, that still requires that you validate that value is an integer, which is the hard part.

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.

What about if we also run validator on set(). If there is a validator and it fails, do not set value and return false, otherwise set value and return true.

The behavior in the status bar would change. Currently, for values out-of-range it changes them to upper or lower boundary (which is kinda weird anyway). Instead, value would not change at all, which is what happens now if value is wrong type (e.g. enter "x" for "spaceUnits").

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.

Running the validator on set seems like a good idea. I like that more than coerce.

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 implemented validator in set(). Added a unit test, so there's 1 for get() and 1 for set().

editor.setTabSize(value);
} else {
Editor.setSpaceUnits(value);
value = Math.max(Math.min(Math.floor(value), Editor.MAX_SPACE_UNITS), Editor.MIN_SPACE_UNITS);
editor.setSpaceUnits(value);
}

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

// column position may change when tab size changes
_updateCursorInfo();
Expand Down Expand Up @@ -184,8 +186,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 +202,8 @@ define(function (require, exports, module) {
_updateLanguageInfo(current);
_updateFileInfo(current);
_initOverwriteMode(current);
_updateIndentType();
_updateIndentSize();
_updateIndentType(current);
_updateIndentSize(current);
}
}

Expand All @@ -219,21 +221,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
2 changes: 1 addition & 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 @@ -88,7 +88,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
⚡