| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
Layer made state-less, just as PathLayer.
|
Thanks @busykai! I can't guarantee that I'll get to this tomorrow, but I will try. |
| * @param {string} oldLanguageID Old language id | ||
| * @return {Array.<string>} List of changed IDs | ||
| */ | ||
| languageChanged: function (languageID, oldLanguageID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, Layers were these generic things that you could attach to Scopes. If the Layer object needed additional information (a filename or a language), that was set directly on the Layer object. The Scope didn't need to know anything about that.
Languages, like files, have this issue where the default context has knowledge of the language and needs to notify if the default context changes. I just wonder if we should have a generic mechanism for this so that we don't need to spread knowledge of languages all over (in Scope and PreferencesSystem)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed that as I tried first to make Layer actually remember current context. Then I realized that current PathLayer which I was taking as an example, did not have any state associated with the Scope context changes, only with "bigger" Brackets components (e.g. ProjectManager). So I made LanguageLayer similar to what's been done before without changing much of the communication model.
I think what we need to do is to generalize scope-specific and layer-specific changes. Since they all will react to context, we should have single contextChanged notification/method which will be propagated to scopes and further down to the layers. That would help make it clean -- layers will not bring any specific methods related to the context changes that Scope will have to adopt.
I cannot think, however, of any good solution to generalize dependency of ProjectScope on ProjectManager or LanguageLayer on changes to the language of the current document. I actually think these being explicit is OK. Or more broadly, in my mind it is ok for PreferencesSystem to depend on (know about) language and project changes. We can make them events, perhaps, as well, but these events will then have to be aware of structure of preferences context (e.g. that there should be language or filename field).
Is this in line at all with what you're thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that PreferencesManager will glue DocumentManager to the LanguageLayer. PreferencesManager stands as the integration point. PreferencesSystem would not need to know about languages.
Imagine that the LanguageLayer doesn't exist in PreferencesBase.js and is instead more conceptually separate. That's what I had in mind originally, it was just in the same file for convenience, but I wanted things like LanguageLayer to be possible to create independent of the PreferencesSystem.
I like the idea of a contextChanged event or something of that sort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dangoor since release 40 is branched already, i think it will make sense to make these changes in this same PR. no rush landing it.
|
Review complete. The changes look good! There's just my questions of whether we should land this after language switching and whether we should try to make layers more generic. |
|
@peterflynn said that he's finishing up review for "replace across files" and yesterday mentioned that he plans to get to #6409 this week. |
…uage-layer Conflicts: src/document/DocumentManager.js
…uage-layer Conflicts: src/document/DocumentManager.js
Also, some fixes to broken document language overrides.
PreferencesManager is made the only owner of the state. PreferencesBase only stores the default scope order. Exception is made for ProjectScope (which still holds the project root directory). There are two failing tests due to rework.
All tests pass.
…uage-layer Conflicts: src/preferences/PreferencesBase.js test/spec/PreferencesBase-test.js
…uage-layer Conflicts: src/document/DocumentManager.js src/editor/Editor.js src/preferences/PreferencesBase.js test/spec/PreferencesBase-test.js
|
hey @ingorichter, not Sunday night! i've merged the PR with latest master as the new eventing caused conflicts (was merged today). |
|
@busykai Is there a limitation which prefs can be specified per language? I was wondering why |
Also, augment the tests to test for it.
|
@ingorichter, actually this was a bug (both in Also, fixed the outdated eventing for |
|
@busykai Great! I have another question:
|
|
@ingorichter, it seems to be a prefixed setting (an extension, right?). There are two issues:
|
|
@ingorichter, done. DocumentManager.on("currentDocumentChange", updateFromPrefs);
|
|
@busykai If the current document changes and causes a change in the selected language layer, does that cause a preference change event? Currently, changing documents will change which path layer is selected and send change events for any prefs that might have changed. I haven't looked into the details of the problem with the bookmarks extension as you have, but I wanted to be sure that the language layer behaves in a way that is similar/consistent to the path layer. |
|
@dangoor, yes. Actually my previous comment is incorrect, thanks for spotting it. No tracking of the current document is needed, but EDIT: so the answer is, yes it behaves exactly as |
|
@busykai Great, thanks for checking! |
|
@busykai that is great! Now it works fine for me. Thanks! |
|
Getting closer!!! :) @ingorichter, thank you for thorough testing of it!!! Couple of things were really broken by multiple big changes! |
|
Looks good to me. I have tested it for a while and haven't found anything new. |
|
@busykai @ingorichter Hey folks, when are we looking to merge this one? |
|
@busykai @MiguelCastillo I wanted to merge it now? Any objections? |
|
@busykai @ingorichter Sounds good to me |
Fix #6558. Resurrect language layer
|
@ingorichter, @MiguelCastillo thanks! yay! i'm going prepare #7362 right away! |
|
@busykai nice work! |
|
I'm so happy this finally landed. :D |
|
@busykai agreed, thanks for adding this in! |
|
@busykai Can you update Brackets wiki with info about this? Not sure if it belongs on Preferences page or Preferences System page. Thanks. |
|
@redmunds, added this paragraph to Preferences System. Also, added description of prefs for linting configuration and extended example for changes made by #7362. |
|
@busykai Looks good. Thanks. @ryanstewart I think this link to Preferences System Basics may be what you're looking for to put in blog post. |
This fixes #6558.
LanguageLayer allows specifying language-specific prefs in the following manner:
{ "spaceUnits": 4, "language": { "html": { "spaceUnits": 2 } } }Language layers are added to
userandprojectscopes. Path layer will take precedence over language layer.LanguageLayers could be used by #7362. Related to PR #6409 (which creates a special event to inform language changes) and should be taken into account by whichever lands last.
CC: @dangoor