[CLOSED] Expand options for preferences lookups

[core-ai-bot]

Issue by dangoor
Friday Jan 31, 2014 at 16:00 GMT
Originally opened as adobe/brackets#6715

---

This is a fix for #6578, along with an API improvement as discussed briefly in 6578. The API improvement is backwards compatible, with the exception of a change to the Editor's optionChange event which has been noted in the Release Notes for sprint 37.

[redmunds] This also fixes #6682 and #6756.

cc@busykai@peterflynn

---

dangoor included the following code: https://github.com/adobe/brackets/pull/6715/commits

[core-ai-bot]

Comment by dangoor
Friday Jan 31, 2014 at 16:01 GMT

---

@busykai I've mostly posted this not-ready-yet code for you so that you can see where I'm going with this, give me comments if you have any and possibly synchronize your work.

[core-ai-bot]

Comment by busykai
Friday Jan 31, 2014 at 16:17 GMT

---

@dangoor thanks! perhaps i should do the same with my fix for #6660

[core-ai-bot]

Comment by busykai
Friday Jan 31, 2014 at 16:22 GMT

---

@dangoor the only consequence i see after a brief review is that scopes and scopes orders should be managed for each context. will be easy as the only thing i will need is to pick up the named context instead of default (as least it seems that way).

[core-ai-bot]

Comment by dangoor
Monday Feb 03, 2014 at 23:50 GMT

---

The current implementation of PreferencesSystem (the object that implements the new preferences management code) is simpler than the previous prototype implementation. In that previous prototype (and the current implementation), I was able to opportunistically support an EditorConfig-style model where settings files could appear in any directory above the file being edited (I call these path-based Scopes).

Note the "file being edited" part of that. Peter Flynn has brought up cases like whole-project linting or even split views where you need the preferences for a specific file. This gets further complicated by the fact that you may have a file open which is not even in the current project.

I've been thinking about how to implement proper support for all of that and my thinking is that this will add a good deal of additional complexity to the implementation of path-based Scopes. I suggest that we actually rip out the code for path-based Scopes, because I don't think the complexity is worthwhile.

• we will still have project-level prefs in .brackets.json at the top of the project
• that file can still contain sections for, for example, thirdparty/codemirror/**.js to specify how things should work file files within the project
• it should still be possible to implement support for .editorconfig files in an extension

Essentially, what I'm proposing is that the complexity be moved from Brackets core code into any EditorConfig extension that people might write. If we decide to support EditorConfig in code that we support, we could still implement it in an extension.

What do you think,@busykai

[core-ai-bot]

Comment by busykai
Tuesday Feb 04, 2014 at 00:04 GMT

---

let me see if i get it right: instead of having PathScopes, there will be a fixed (non-globbed or static) "project" scope and PathLayers (and other type of layers later) will still be supported. default scope order will be "session", "project", "user", "default"

if it is so, it makes a lot of sense to me. firstly, it will be much easier (predictable) to work with scopes from an extension (project will be named and known instead of dynamic as with path scopes now). secondly, it's hard to predict what pattern may become popular upfront and adopting an established pattern of working with/structuring config files later makes sense.

[core-ai-bot]

Comment by dangoor
Tuesday Feb 04, 2014 at 01:28 GMT

---

Yes, you've got my plan exactly right. With this change as it is now,
"project" exists as an alias for the project-level prefs file, so that bit
of predictability is there, but the complexity that I could see brewing was
the real problem.

If someone wanted to add EditorConfig support in the future, I'm thinking
that they could probably implement a custom Scope to do that (and it would
likely contain all of the complexity that I'm imagining is required to make
it work properly and efficiently)

[core-ai-bot]

Comment by dangoor
Tuesday Feb 04, 2014 at 03:26 GMT

---

Oh, I should mention that there is another way in which path-based Scopes are superior: when you’re editing files that are outside of your current project.

If you edit a file within your project, the project Scope will be loaded and settings from it will be applied to the files within the project. However, if you drag a random file into Brackets, even if there’s a .brackets.json somewhere in the file tree above it, only the user and default settings will be applied for editing that file. With path-based Scopes, the .brackets.json file above the outside-of-project-file would be found and its settings applied.

That’s more compelling to me than the EditorConfig case, but I don’t know if it’s worth adding complexity for. It is also behavior we can choose to adopt later.

[core-ai-bot]

Comment by dangoor
Wednesday Feb 05, 2014 at 04:08 GMT

---

FYI, I've pushed commits that move things forward in the path I wrote about above. At this point, there's some tying up of loose ends I need to do as well as testing.

@busykai I added the new project scope in a way that undoes the protection from failing user scopes that you had added previously. I figured that's okay given that you've got a more robust solution in the works.

[core-ai-bot]

Comment by busykai
Wednesday Feb 05, 2014 at 20:25 GMT

---

@dangoor, that's ok to add it that way. will resolve it as part of merge.

i'm seeing you've yanked some tests related to the path scopes. i wanted to refactor them to catch async failures. some that should fail now (due to my changes) actually pass. i believe, though it would be better if we make it a separate step -- i'll merge after you finish this one and refactor the tests.

[core-ai-bot]

Comment by dangoor
Wednesday Feb 05, 2014 at 20:56 GMT

---

This is ready for review. I haven't had a full run of the integration tests yet and I intend to do some manual testing, but I've checked off everything on my list so far and done some initial testing.

[core-ai-bot]

Comment by busykai
Thursday Feb 06, 2014 at 03:49 GMT

---

@dangoor one general comment/question: ProjectManager/DocumentManager notify PreferenceManager and not the other way around. Is it done so that PreferencesManager could be loaded first thing and get priority notifications? Perhaps it would make a lot of sense to make ProjectScope a separate module and more a "thing in itself", including listening to ProjectManager and DocumentManager from it, instead of these notifying PreferencesManager. What do you think?

[core-ai-bot]

Comment by dangoor
Thursday Feb 06, 2014 at 17:49 GMT

---

@busykai It's structured as it is now because many modules require PreferencesManager and I believe ProjectManager and EditorManager already had PreferencesManager loaded, so it made some amount of sense to set up the event handling as I have which avoids circular dependencies.

Adding a separate ProjectScope module would have a circular dependency again: PreferencesManager depends on ProjectScope which depends on ProjectManager which depends on PreferencesManager.

Basically, my dislike for circular dependencies is greater than my dislike for things like ProjectManager needing to know that there's a preferences manager that's interested in what project is being edited...

[core-ai-bot]

Comment by dangoor
Thursday Feb 06, 2014 at 21:21 GMT

---

Just found a funny bug. This looks normal:

pm._manager._defaultContext.scopeOrder
["session", "project", "user", "default"]

If I drag a file from outside the project into Brackets, this happens:

pm._manager._defaultContext.scopeOrder
Array[129]

I'll have to track this down later. I also want to do some profiling after I've finished the PreferencesManager change to reduce the construction of new contexts.

[core-ai-bot]

Comment by busykai
Friday Feb 07, 2014 at 03:08 GMT

---

@dangoor, right, right! circular dependency is the the word i was struggling to find. thanks, got it.

i guess my idea was simpler: separate projectScope from the rest of the preference base/manager into a separate module and tie it to the ProjectManager (including loading it) since it's semantically tie to it anyways (including project changes). project scope, but not PathScope it is based on (instance of). i got your design point of keeping all the pref code together though.

[core-ai-bot]

Comment by dangoor
Monday Feb 10, 2014 at 17:05 GMT

---

It looks like the DocumentCommandHandler tests are passing again.

[core-ai-bot]

Comment by busykai
Wednesday Feb 12, 2014 at 03:23 GMT

---

Hope this will land soon. Will merge HEAD of it and with utility routine #6719 will be pending for review. Good that DocumentCommandHandler tests are fixed.

[core-ai-bot]

Comment by redmunds
Wednesday Feb 12, 2014 at 19:51 GMT

---

I am seeing 1 test fail: PreferencesManager: "should find preferences in the project"

[core-ai-bot]

Comment by redmunds
Wednesday Feb 12, 2014 at 19:51 GMT

---

Done with review.

[core-ai-bot]

Comment by dangoor
Thursday Feb 13, 2014 at 21:05 GMT

---

OK, I've addressed the review comments.

The PreferencesManager integration test is passing for me. Can you update and try it again?

[core-ai-bot]

Comment by redmunds
Thursday Feb 13, 2014 at 21:35 GMT

---

I am seeing JSLint errors in: Editor.js, PreferencesBase.j, and PreferencesManager.js.

[core-ai-bot]

Comment by dangoor
Thursday Feb 13, 2014 at 21:53 GMT

---

Ugh. I had accidentally collapsed my error panel and so I missed the one in Editor.js. The other two files (PreferencesBase and PreferencesManager) don't have JSLint errors for me. Those two files, possibly not coincidentally, rely on the jslint.options from .brackets.json (they don't have a /*jslint comment).

What kind of JSLint errors are you seeing? Is there anything in your console? (And what Brackets are you running?)

[core-ai-bot]

Comment by redmunds
Thursday Feb 13, 2014 at 22:10 GMT

---

My Brackets version is your branch :)

Here is what I have in my user options:

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

Here are the first 50 errors:

40  Unexpected dangling '_' in '_'.  _ = require("thirdparty/lodash");
47  Combine this with the previous 'var' statement.  var PREFERENCES_CLIENT_ID = "com.adobe.brackets.preferences";
54  Combine this with the previous 'var' statement.  var CLIENT_ID_PREFIX = "com.adobe.brackets.";
58  Combine this with the previous 'var' statement.  var preferencesKey,
71  Unexpected dangling '_' in '_getExtensionPaths'.     function _getExtensionPaths() {
90  Unexpected dangling '_' in '_getExtensionPaths'.     var paths = exports._getExtensionPaths();
91  Combine this with the previous 'var' statement.  var pathExp, pathUrl, clientID;
115 Unexpected dangling '_' in '_doNotCreate'.   function getPreferenceStorage(clientID, defaults, _doNotCreate) {
127 Unexpected dangling '_' in '_doNotCreate'.   if (_doNotCreate) {
135 Unexpected dangling '_' in '_'.  _.forEach(defaults, function (value, key) {
157 Unexpected dangling '_' in '_reset'.     function _reset() {
169 Unexpected dangling '_' in '_initStorage'.   function _initStorage(storage) {
178 Unexpected dangling '_' in '_reset'.     _reset();
196 Unexpected dangling '_' in '_initStorage'.   _initStorage(localStorage);
206 Unexpected dangling '_' in '_reset'.     exports._reset = _reset;
206 Unexpected dangling '_' in '_reset'.     exports._reset = _reset;
207 Unexpected dangling '_' in '_getExtensionPaths'.     exports._getExtensionPaths = _getExtensionPaths;
207 Unexpected dangling '_' in '_getExtensionPaths'.     exports._getExtensionPaths = _getExtensionPaths;
213 Combine this with the previous 'var' statement.  var SETTINGS_FILENAME = "brackets.json",
217 Combine this with the previous 'var' statement.  var userPrefFile = brackets.app.getApplicationSupportDirectory() + "/" + SETTINGS_FILENAME;
228 Combine this with the previous 'var' statement.  var preferencesManager = new PreferencesBase.PreferencesSystem();
230 Combine this with the previous 'var' statement.  var userScopeLoading = preferencesManager.addScope("user", new PreferencesBase.FileStorage(userPrefFile, true));
239 Combine this with the previous 'var' statement.  var projectStorage = new PreferencesBase.FileStorage(undefined, true),
261 Unexpected dangling '_' in '_includeProjectScope'.   function _includeProjectScope(filename) {
275 Unexpected dangling '_' in '_toggleProjectScope'.    function _toggleProjectScope() {
276 Unexpected dangling '_' in '_includeProjectScope'.   if (_includeProjectScope() === projectScopeIsIncluded) {
295 Unexpected dangling '_' in '_setProjectSettingsFile'.    function _setProjectSettingsFile(settingsFile) {
297 Unexpected dangling '_' in '_toggleProjectScope'.    _toggleProjectScope();
310 Unexpected dangling '_' in '_setCurrentEditingFile'.     function _setCurrentEditingFile(currentFile) {
312 Unexpected dangling '_' in '_toggleProjectScope'.    _toggleProjectScope();
351 Combine this with the previous 'var' statement.  var prefsID = getClientID(clientID);
355 Combine this with the previous 'var' statement.  var convertedKeysMap = prefStorage.convertedKeysMap;
369 Combine this with the previous 'var' statement.  var stateManager = new PreferencesBase.PreferencesSystem();
370 Combine this with the previous 'var' statement.  var userStateFile = brackets.app.getApplicationSupportDirectory() + "/" + STATE_FILENAME;
380 Combine this with the previous 'var' statement.  var CURRENT_PROJECT = {};
388 Combine this with the previous 'var' statement.  var CURRENT_FILE;
393 Combine this with the previous 'var' statement.  var scopeOrderWithProject = null;
398 Combine this with the previous 'var' statement.  var scopeOrderWithoutProject = null;
411 Unexpected dangling '_' in '_adjustScopeOrderForProject'.    function _adjustScopeOrderForProject(scopeOrder, includeProject) {
418 Combine this with the previous 'var' statement.  var newScopeOrder;
421 Combine this with the previous 'var' statement.  var before = scopeOrder.indexOf("user");
425 Unexpected dangling '_' in '_'.  newScopeOrder = _.first(scopeOrder, before);
427 Unexpected dangling '_' in '_'.  newScopeOrder.push.apply(newScopeOrder, _.rest(scopeOrder, before));
429 Unexpected dangling '_' in '_'.  newScopeOrder = _.without(scopeOrder, "project");
443 Unexpected dangling '_' in '_normalizeContext'.  function _normalizeContext(context) {
448 Unexpected dangling '_' in '_includeProjectScope'.   context.scopeOrder = _includeProjectScope(context.filename) ?
460 Unexpected dangling '_' in '_updateCurrentProjectContext'.   function _updateCurrentProjectContext() {
463 Unexpected dangling '_' in '_adjustScopeOrderForProject'.    scopeOrderWithProject = _adjustScopeOrderForProject(context.scopeOrder, true);
464 Unexpected dangling '_' in '_adjustScopeOrderForProject'.    scopeOrderWithoutProject = _adjustScopeOrderForProject(context.scopeOrder, false);
468 Unexpected dangling '_' in '_updateCurrentProjectContext'.   _updateCurrentProjectContext();
468 Too many errors. (84% scanned).  _updateCurrentProjectContext();

[core-ai-bot]

Comment by dangoor
Friday Feb 14, 2014 at 01:56 GMT

---

Did you open brackets at the top level folder, or at the src folder?

[core-ai-bot]

Comment by redmunds
Friday Feb 14, 2014 at 16:38 GMT

---

Did you open brackets at the top level folder, or at the src folder?

Ah. My project was set to the src folder.

[core-ai-bot]

Comment by dangoor
Friday Feb 14, 2014 at 16:46 GMT

---

@redmunds Yeah, that's an unfortunate side effect of changing pref file reading to be "project-based" rather than "path-based". Project-based is easier to reason about, but a bit less flexible.

I don't think there's anything else to be done for this pull request. In a subsequent one with@busykai, we'll work through the issues with "pending scopes".

[core-ai-bot]

Comment by redmunds
Friday Feb 14, 2014 at 17:07 GMT

---

@dangoor There are quite a few commits. Do you think it should be rebased?

[core-ai-bot]

Comment by redmunds
Friday Feb 14, 2014 at 21:28 GMT

---

Merging.