[prefs] Prefs object format not written as expected

core-ai-bot commented 3 years ago

Issue by redmunds Tuesday Mar 04, 2014 at 02:19 GMT Originally opened as https://github.com/adobe/brackets/issues/7066

In brackets.json, I'm seeing these prefs:

{
    "useTabChar": false,
    "tabSize": 4,
    "spaceUnits": 4,
    "closeBrackets": false,
    "showLineNumbers": true,
    "styleActiveLine": false,
    "wordWrap": false,
    "linting.enabled": true,
    "linting.collapsed": false,
    "quickview.enabled": true
}

This should be written as:

{
    "useTabChar": false,
    "tabSize": 4,
    "spaceUnits": 4,
    "closeBrackets": false,
    "showLineNumbers": true,
    "styleActiveLine": false,
    "wordWrap": false,
    "linting": {
        "enabled": true,
        "collapsed": false
    },
    "quickview": {
        "enabled": true
    }
}

core-ai-bot commented 3 years ago

Comment by dangoor Tuesday Mar 04, 2014 at 14:54 GMT

Not exactly. My original thought for prefs was that they would typically be simple values, because it makes the overriding behavior easier to reason about. I'll use jslint.options as an example because it highlights the problem. Imagine that we have this in the project-level .brackets.json:

    "jslint.options": {
        "maxerr": 50
    }

and this in the user-level brackets.json:

    "jslint.options": {
        "vars": true,
        "plusplus": true,
        "devel": true,
        "nomen": true,
        "maxerr": 50
    }

The project-level jslint.options overrides the user-level one completely (so none of vars, plusplus, devel and nomen would be set).

Back to your example: linting.collapsed can be set at the user-level (which is probably going to be the case most often) and linting.enabled can be set at the project- or even path-levels productively and the two can be set independently.

So, an object in a more specific context defeats an object in a less specific context. The other place where objects are used in the prefs are in layers, but those have custom defined behavior for how values are looked up in the layer.

One last note, an extension can easily get a PrefixedPreferenceSystem which automatically adds the linting. style prefix to help ensure that preferences have unique names. So, most extensions just think about setting simple "enabled" and "collapsed" preferences and the dotted prefixes are added automatically.

We could make prefs work as you suggest, but we'd have to define the rules for merging of the objects.

core-ai-bot commented 3 years ago

Comment by redmunds Monday Mar 10, 2014 at 16:55 GMT

It seems that the linting examples I gave are leftovers from older code. If I delete them then change the setting, they seem to be set somewhere else that I can't find.@RaymondLim where are lint settings now stored?

The JSLint options are an interesting example. I'd like to be able to turn off JSLint in the default case, and then have it be enabled for projects that require it. Maybe the default needs to be changed to false?

I think the case with quickview is that we want extensions to qualify settings to avoid conflict with settings from core and other extensions. Using a . seems ot conflict with JSON, so maybe we should use some other qualifier (e.g. -)?

core-ai-bot commented 3 years ago

Comment by dangoor Monday Mar 10, 2014 at 18:01 GMT

The linting settings should be stored wherever they are set. For example, if you open codemirror.js in the Brackets repo, linting.enabled is set to false. If you then change the setting, it will change it in the codemirror.js part of .brackets.json in the Brackets project. For other files, it would change in your user-level prefs file (the one you can get to via the Debug menu).

. is just as valid (and annoying :grin:) as - in identifiers. When the key is "quickview.enabled" you can't say obj.quickview.enabled, you have to obj["quickview.enabled"]. It's exactly the same with -. Of course, it doesn't make much of a difference the way prefs are used right now (PreferencesManager.get("quickview.enabled")).

Stylistically, I like the idea you're presenting...

"linting": {
    "enabled": true
}

is definitely nicer. But, it requires working through how those objects are manipulated and as seen in #7133 worrying about who is mutating which values where.

core-ai-bot commented 3 years ago

Comment by RaymondLim Monday Mar 10, 2014 at 18:09 GMT

@dangoor I'm also confused on this linting setting. For example, I turn off linting on Editor.js file and quit. When I relaunch Brackets, the setting is back on. I tried to locate .brackets.json file both in "editor" subfolder and "brackets", but none of them exists.

core-ai-bot commented 3 years ago

Comment by dangoor Monday Mar 10, 2014 at 18:24 GMT

@RaymondLim You're right. I just tried that. That looks like a regression. I'll investigate.

core-ai-bot commented 3 years ago

Comment by njx Monday Mar 10, 2014 at 18:30 GMT

Marking medium priority, sprint 37 for the last issue - but perhaps it should be split off as a separate bug?

core-ai-bot commented 3 years ago

Comment by dangoor Monday Mar 10, 2014 at 18:31 GMT

@njx Yeah, that's a separate bug (high priority). The main issue discussed in this bug is definitely not sprint 37 and may not quite be medium because it's more code design than an actual problem.

core-ai-bot commented 3 years ago

Comment by dangoor Monday Mar 10, 2014 at 18:31 GMT

I'll file a new issue

core-ai-bot commented 3 years ago

Comment by peterflynn Monday Mar 10, 2014 at 18:54 GMT

So what's the actual remaining issue here? Is it just debate about whether related prefs should be grouped using "-" or nesting rather than using "." as they are today?

core-ai-bot commented 3 years ago

Comment by dangoor Monday Mar 10, 2014 at 19:17 GMT

@peterflynn yes.

brackets-archive / bracketsIssues

[prefs] Prefs object format not written as expected #12548