Expand options for preferences lookups #6715
Conversation
The integration tests aren't passing yet, but this adds the infrastructure to support CURRENT_FILE and CURRENT_PROJECT lookups. Additional work remains for ensuring that prefs for a specific file work properly and to improve event dispatch to better support these new contexts
|
@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. |
|
@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). |
|
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.
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 |
|
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. |
|
Yes, you've got my plan exactly right. With this change as it is now, If someone wanted to add EditorConfig support in the future, I'm thinking |
This was the starting point of a refactoring to better handle a larger, disjoint set of path-based Scopes. I can see complexity brewing and I don't think it's worth it for path-based Scopes. After this commit, I'm going to yank out all of the path-based Scopes code and switch to simpler, project-based approach.
|
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. |
|
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. |
This fixes linting.enabled, for example.
|
@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. |
* Changed `Editor.optionChange` event to send Brackets preference names * Updated the doc comments for the editor options functions in Editor.js * Fixed setValueAndSave to use the new behavior.
|
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. |
| * @return {Array.<string>} list of preference IDs that could have changed | ||
| */ | ||
| defaultFilenameChanged: function (data, filename, oldFilename) { | ||
| var relativeFilename = FileUtils.getRelativeFilename(this.prefFilePath, filename), |
There was a problem hiding this comment.
This variable may not be necessary at all. It's not checked and only used to retrieved glob which confuses at first when reading these 4 lines.
There was a problem hiding this comment.
I put that variable there just to make the lines a bit shorter, but I can see how it would get confusing. Will change.
| * | ||
| * @param {Object|string} context CURRENT_FILE, CURRENT_PROJECT or a filename | ||
| */ | ||
| function _normalizeContext(context) { |
There was a problem hiding this comment.
Perhaps it is better to keep these as pre-constructed contexts instead of normalizing them on each get/set. Potentially less object creation, shorter code path.
There was a problem hiding this comment.
Indeed, but I saw that as an optimization. If I remove the public API on PreferencesManager for adding a Scope (which no one should be using yet), then we can use pre-constructed contexts trivially. But, with that addScope capability there, it becomes a little bit more complicated because those pre-constructed contexts would need to be adjusted.
That may not be a big deal, though. I'll think about that.
There was a problem hiding this comment.
I think you're right. I think it would be cleaner to just have pre-constructed contexts.
|
@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? |
|
@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... |
|
Just found a funny bug. This looks normal: If I drag a file from outside the project into Brackets, this happens: 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. |
| } else if (prefName === TAB_SIZE || prefName === SPACE_UNITS) { | ||
| this._codeMirror.setOption("indentUnit", newValue); | ||
| } | ||
|
|
There was a problem hiding this comment.
Since certain prefName values are already being handled as special cases, can't you just call this._codeMirror.setOption(cmOptions[prefName], newValue); in the else case? Something like:
if (prefName === USE_TAB_CHAR) {
this._codeMirror.setOption("indentUnit", newValue === true ?
this._currentOptions[TAB_SIZE] :
this._currentOptions[SPACE_UNITS]
);
} else if (prefName === TAB_SIZE || prefName === SPACE_UNITS) {
this._codeMirror.setOption("indentUnit", newValue);
} else {
this._codeMirror.setOption(cmOptions[prefName], newValue);
}
There was a problem hiding this comment.
The idea was to call _codeMirror.setOption in any case, but in the case of a switch of USE_TAB_CHAR, you also need to change indentUnit to either TAB_SIZE or SPACE_UNITS. The else if clause actually isn't even necessary because TAB_SIZE and SPACE_UNITS both map to CodeMirror's indentUnit option.
One thing that is amiss, though, is that if USE_TAB_CHAR is true and then SPACE_UNITS is changed, indentUnit would get changed to the new SPACE_UNITS value which is incorrect (since it should always have the value of TAB_SIZE in that case...)
There was a problem hiding this comment.
The new block looks like this:
if (oldValue !== newValue) {
this._currentOptions[prefName] = newValue;
if (prefName === USE_TAB_CHAR) {
this._codeMirror.setOption("indentUnit", newValue === true ?
this._currentOptions[TAB_SIZE] :
this._currentOptions[SPACE_UNITS]
);
} else if (prefName === STYLE_ACTIVE_LINE) {
this._updateStyleActiveLine();
} else {
// Set the CodeMirror option as long as it's not a change
// that is in conflict with the useTabChar setting.
var useTabChar = this._currentOptions[USE_TAB_CHAR];
if ((useTabChar && prefName === SPACE_UNITS) ||
(!useTabChar && prefName === TAB_SIZE)) {
return;
}
this._codeMirror.setOption(cmOptions[prefName], newValue);
}
$(this).triggerHandler("optionChange", [prefName, newValue]);
}
|
I am seeing 1 test fail: PreferencesManager: "should find preferences in the project" |
|
Done with review. |
|
OK, I've addressed the review comments. The PreferencesManager integration test is passing for me. Can you update and try it again? |
|
I am seeing JSLint errors in: Editor.js, PreferencesBase.j, and PreferencesManager.js. |
|
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 What kind of JSLint errors are you seeing? Is there anything in your console? (And what Brackets are you running?) |
|
My Brackets version is your branch :) Here is what I have in my user options: Here are the first 50 errors: |
|
Did you open brackets at the top level folder, or at the src folder? |
Ah. My project was set to the src folder. |
|
@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". |
|
@dangoor There are quite a few commits. Do you think it should be rebased? |
|
Merging. |
Expand options for preferences lookups
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
optionChangeevent which has been noted in the Release Notes for sprint 37.[redmunds] This also fixes #6682 and #6756.
cc @busykai @peterflynn