[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.