Validate editor preferences #7186

redmunds · 2014-03-13T02:53:22Z

This is for #7020.

I did a slight refactoring so editor prefs can be validated when ever they change. Currently only validate "tabSize" and "spaceUnits" which are numbers. All other prefs are boolean, so the only thing validation would do is to enforce default if user mucks up the prefs file.

I started a ValidationUtils module that might be useful in other places.

dangoor · 2014-03-13T13:02:35Z

src/utils/ValidationUtils.js

+
+
+    /**
+     * @private (but might be useful in a Validation module)


These aren't private any more.

dangoor · 2014-03-13T13:08:52Z

I think the validation should be done in the PreferencesManager. Something like this:

PreferencesManager.definePreference(SPACE_UNITS, "number", DEFAULT_SPACE_UNITS, {
    validator: function (value) {
        return ValidationUtils.isIntegerInRange(value, 0, null)
    }
});

Then PreferencesSystem.get can validate on the way out and return the default value if the current value is invalid. This eliminates a lot of the change to Editor.js and also makes it so that JSLint.main doesn't need to duplicate the validation logic.

What do you think of that approach?

redmunds · 2014-03-13T16:14:43Z

I like the validator approach -- nice suggestion! I'll implement that.

redmunds · 2014-03-13T19:18:13Z

Changes pushed. Ready for another review.

TomMalbran · 2014-03-13T20:03:19Z

src/extensions/default/JSLint/main.js

@@ -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"),


Not required anymore

Good catch! Fixed.

redmunds · 2014-03-13T21:10:37Z

@TomMalbran Thanks for the review. Changes pushed.

TomMalbran · 2014-03-13T21:15:40Z

src/editor/Editor.js

+    });
+    PreferencesManager.definePreference(SPACE_UNITS, "number", DEFAULT_SPACE_UNITS, {
+        validator: function (value) {
+            return ValidationUtils.isIntegerInRange(value, 0, null);


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.

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.

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.

OK, that's what I did.

redmunds · 2014-03-13T22:51:03Z

@TomMalbran Pushed more changes.

TomMalbran · 2014-03-13T22:57:36Z

src/extensions/default/JSLint/main.js

@@ -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);


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

We don't have an Editor instance.

I see. I guess is fine then.

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.

TomMalbran · 2014-03-13T22:59:50Z

The code looks good to me now.

redmunds · 2014-03-13T23:23:31Z

I'm seeing a PreferencesManager unit test fail, and should add some unit tests for the new validator functionality.

should find preferences in the project

Error: Expected 4 to be 92.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:102:32)
    at null.toBe (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1194:29)
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/spec/PreferencesManager-test.js:137:62)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1024:15)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)
    at onComplete (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1838:18)
    at jasmine.WaitsForBlock.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2322:5)
    at file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2336:12

RaymondLim · 2014-03-13T23:42:11Z

@redmunds You may need to adjust the value in test file .brackets.json which is using 92 as spaceUnits and your validator only allows up to 10. As you adjust the value is test file, then you also need to adjust all the test cases using that specific value.

redmunds · 2014-03-18T17:08:22Z

Changes pushed. Ready for another review.

zaggino · 2014-03-18T19:29:14Z

@redmunds I started a bit on UI and will put together a PR once this is merged so I can include validator strings in that

dangoor · 2014-03-18T19:39:53Z

src/editor/EditorStatusBar.js

+        var valInt = parseInt(value, 10);
+        if (editor.getUseTabChar()) {
+            if (!editor.setTabSize(valInt)) {
+                return;     // validation failed


false does not necessarily mean that validation failed. It just means that the value wasn't set. Should we differentiate? I think we can still change the PreferencesSystem.set API at this point because nothing in Brackets relies on that return value, and I'd be surprised if extensions used that yet. In fact, your use here is the first use I've seen of the set return value.

With the API as it stands now, a false return value means that the caller will have to call a separate function to find out if the given value is valid.

We could easily differentiate. How about returning an object like:

{ valid: {boolean}, stored: {boolean} }

Error message would be added at a later time.

This would be a good time to change it, since that API is unused and the change is simple. Your suggestion seems good to me.

dangoor · 2014-03-18T19:42:05Z

I'm fine with punting on the extra validation features for now. We'll need to address them as soon as we have a preferences UI, but not necessarily before then.

peterflynn · 2014-03-18T19:46:32Z

@redmunds The difference is that factories lead to more succinct and scannable code, with less boilerplate to clutter things. Compare:

    PreferencesManager.definePreference(TAB_SIZE, "number", DEFAULT_TAB_SIZE, {
        validator: function (value) {
            return ValidationUtils.isIntegerInRange(value, MIN_TAB_SIZE, MAX_TAB_SIZE);
        }
    });

vs.

PreferencesManager.definePreference(TAB_SIZE, "number", DEFAULT_TAB_SIZE,
    new Validators.IntValidator(MIN_TAB_SIZE, MAX_TAB_SIZE));

(And the difference could potentially get larger once error strings are introduced).

zaggino · 2014-03-18T19:50:13Z

I think you shouldn't remove options object completely, so it should be

{ validator: new Validators.IntValidator(MIN_TAB_SIZE, MAX_TAB_SIZE) }

for a factory.

redmunds · 2014-03-18T20:34:41Z

@zaggino

I started a bit on UI and will put together a PR once this is merged so I can include validator strings in that

Cool, Thanks.

redmunds · 2014-03-18T21:03:44Z

@peterflynn Seems like the only difference is where the function gets created. I kind of like the idea of a ValidationUtils module that would just provide some basic functions that could be used for any kind of validation, so I'm leaning towards keeping it like it is.

peterflynn · 2014-03-18T21:39:08Z

@redmunds The difference is how many times the function gets created -- the more verbose way adds 3 extra lines of code (or more) per preference. The factory idea only writes out the method body once, within the central utils module.

I think we should still keep the basic functions, but factory wrappers would simplify the code in 90% of cases...

redmunds · 2014-03-20T23:03:08Z

@dangoor Changes pushed.

dangoor · 2014-03-22T02:26:28Z

The validation is okay as it is, and we can optimize more as we see how this usage grows.

The functional approach for shortening this would be: validator: _.partialRight(ValidationUtils.isIntegerInRange, MIN_TAB_SIZE, MAX_TAB_SIZE)

@redmunds One last question: should the Editor.prototype.set* functions return .valid or .stored? The comment currently reads like it should return .stored

redmunds · 2014-03-23T17:06:15Z

@dangoor

The functional approach for shortening this would be: validator: _.partialRight(ValidationUtils.isIntegerInRange, MIN_TAB_SIZE, MAX_TAB_SIZE)

Nice. Done.

One last question: should the Editor.prototype.set* functions return .valid or .stored? The comment currently reads like it should return .stored

I'm having seconds thoughts about this API. What if we keep it Editor.get* and Editor.set* but add an optional fullPath parameter, so code that does not have an editor instance (such as JSLint) can use the API instead of calling PreferencesManager directly?

redmunds · 2014-03-28T02:07:51Z

@dangoor I made the API changes that we discussed. Ready for another review.

dangoor · 2014-03-31T15:29:24Z

src/editor/EditorStatusBar.js

+        var current = EditorManager.getActiveEditor(),
+            fullPath = current && current.document.file.fullPath;
+
+        Editor.setUseTabChar(!Editor.getUseTabChar(fullPath), fullPath);


This is fine and explicit, but to some extent it is redundant because PreferencesManager.get("useTabChar") not qualified by a file will default to the currently edited file.

dangoor · 2014-03-31T15:43:29Z

Looks good. Merging.

Validate editor preferences

redmunds added 6 commits March 12, 2014 10:38

Editor Preferences Validation - interim checkin

1274227

start ValidationUtils module

1b1a604

Merge remote-tracking branch 'origin/master' into randy/issue-7020

7f721fc

convert remaining editor prefs

8d5482e

cleanup

bc7b0bf

fix range limit of zero

d0ef02f

dangoor self-assigned this Mar 13, 2014

dangoor reviewed Mar 13, 2014
View reviewed changes

more tests; cleanup comments

3719aa2

redmunds added 2 commits March 13, 2014 12:05

implement preference validator

cfb0cce

update for preference validator

3428312

TomMalbran reviewed Mar 13, 2014
View reviewed changes

changes for code review

f476ac5

TomMalbran reviewed Mar 13, 2014
View reviewed changes

more code review changes

8268997

TomMalbran reviewed Mar 13, 2014
View reviewed changes

dangoor reviewed Mar 18, 2014
View reviewed changes

redmunds mentioned this pull request Mar 18, 2014

[JSLint] Respects tabSize setting when useTabChar is true #7243

Merged

redmunds added 3 commits March 20, 2014 13:30

return both and from set()

bc71a60

merge latest master

a845bab

fix tests

d2d4c72

redmunds added 2 commits March 23, 2014 09:49

merge with latest master

9883d3c

use lodash to generate validator function

835bc45

redmunds added 4 commits March 27, 2014 13:24

revert to old API with a few small changes

2062a78

jsdoc updates

3d82669

Merge remote-tracking branch 'origin/master' into randy/issue-7020

aca8a81

fix whitespace

76a5b91

dangoor reviewed Mar 31, 2014
View reviewed changes

dangoor added a commit that referenced this pull request Mar 31, 2014

Merge pull request #7186 from adobe/randy/issue-7020

bb96ad2

Validate editor preferences

dangoor merged commit bb96ad2 into master Mar 31, 2014

dangoor deleted the randy/issue-7020 branch March 31, 2014 15:43

This was referenced Aug 30, 2021

[CLOSED] Validate editor preferences brackets-archive/bracketsIssues#6497

Open

[CLOSED] Typing performance improvement brackets-archive/bracketsIssues#6503

Open

Validate editor preferences #7186

Validate editor preferences #7186

Conversation

redmunds commented Mar 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangoor commented Mar 13, 2014

redmunds commented Mar 13, 2014

redmunds commented Mar 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Mar 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Mar 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomMalbran commented Mar 13, 2014

redmunds commented Mar 13, 2014

RaymondLim commented Mar 13, 2014

redmunds commented Mar 18, 2014

zaggino commented Mar 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangoor commented Mar 18, 2014

peterflynn commented Mar 18, 2014

zaggino commented Mar 18, 2014

redmunds commented Mar 18, 2014

redmunds commented Mar 18, 2014

peterflynn commented Mar 18, 2014

redmunds commented Mar 20, 2014

dangoor commented Mar 22, 2014

redmunds commented Mar 23, 2014

redmunds commented Mar 28, 2014

Choose a reason for hiding this comment

dangoor commented Mar 31, 2014