Fix #6558. Resurrect language layer

[busykai]

This fixes #6558.

LanguageLayer allows specifying language-specific prefs in the following manner:

{
    "spaceUnits": 4,
    "language": {
        "html": {
            "spaceUnits": 2
        }
    }
}

Language layers are added to user and project scopes. 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

[dangoor]

Thanks @busykai!

I can't guarantee that I'll get to this tomorrow, but I will try.

[dangoor]

@busykai Do you have a preference for which should land first between this and #6409?

[dangoor]

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)

[busykai]

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?

[dangoor]

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.

[busykai]

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

[dangoor]

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.

[busykai]

@dangoor, just passing by... :) I guess #6409 should land first. My thinking is it will be easier to rework this based on a use case rather than the other way around and the switch language feature seems to be a wanted one.

[busykai]

@dangoor, it does not seem that #6409 is going anywhere, so I'll finish this up as per the discussion above and let's land it. it'll help configurable linters which I, personally, want really much. :) what do you think?

[dangoor]

@peterflynn said that he's finishing up review for "replace across files" and yesterday mentioned that he plans to get to #6409 this week.

[busykai]

hey @ingorichter, not Sunday night! i've merged the PR with latest master as the new eventing caused conflicts (was merged today).

[ingorichter]

@busykai Is there a limitation which prefs can be specified per language? I was wondering why "showLineNumbers": false doesn't have any effect. Even after restarting Brackets.
If this is the intended behavior, then I happy to merge this.

[busykai]

@ingorichter, actually this was a bug (both in ProjectLayer and LanguageLayer) -- boolean properties were not handled correctly. Fixed now and added tests. Thank you so much for catching it!

Also, fixed the outdated eventing for currentDocumentLanguageChanged (1 test was failing).

[ingorichter]

@busykai Great! I have another question:
This is the content of my .brackets.json

"bracketsEditorBookmarks.panelVisible": true is not working in the LanguageLayer. "showLineNumbers": false is working fine for coffeescript files.
Is this a bug too?

{
    "bracketsEditorBookmarks.panelVisible": true,
    "spaceUnits": 4,
    "useTabChar": false,
    "language": {
        "coffeescript": {
            "spaceUnits": 4,
            "bracketsEditorBookmarks.panelVisible": false,
            "hirse.outline.enabled": true,
            "showLineNumbers": false
        },
        "javascript": {
          "spaceUnits": 4
        }
    }
}

[busykai]

@ingorichter, it seems to be a prefixed setting (an extension, right?). There are two issues:

1. If the extension wanted to be update its preferences (visibility of the panel in this case), it should track currentDocumentChange event. It only does it when a preference is changed or on user action.
2. PrefixedPreferencesSystem.get() required explicit specification of the context. This latter I will fix in a second, but it will not fix the bookmarks extension behavior.

[busykai]

@ingorichter, done. If you add the following to main.js of the bookmarks extension (at the module level):

    DocumentManager.on("currentDocumentChange", updateFromPrefs);

then you'll get the behaviour you expected. All tests green.

[dangoor]

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

[busykai]

@dangoor, yes. Actually my previous comment is incorrect, thanks for spotting it. No tracking of the current document is needed, but PrefixedPreferencesSystem was not aware of either path or language layers until b71b8c9.

EDIT: so the answer is, yes it behaves exactly as PathLayer and triggers properties' change events when there's a change in context.

[dangoor]

@busykai Great, thanks for checking!

[ingorichter]

@busykai that is great! Now it works fine for me. Thanks!

[busykai]

Getting closer!!! :) @ingorichter, thank you for thorough testing of it!!! Couple of things were really broken by multiple big changes!

[ingorichter]

Looks good to me. I have tested it for a while and haven't found anything new.

[MiguelCastillo]

@busykai @ingorichter Hey folks, when are we looking to merge this one?

[ingorichter]

@busykai @MiguelCastillo I wanted to merge it now? Any objections?

[MiguelCastillo]

@busykai @ingorichter Sounds good to me

[busykai]

@ingorichter, @MiguelCastillo thanks! yay! i'm going prepare #7362 right away!

[MiguelCastillo]

@busykai nice work!

[le717]

I'm so happy this finally landed. :D

[lkcampbell]

@busykai agreed, thanks for adding this in!

[redmunds]

@busykai Can you update Brackets wiki with info about this? Not sure if it belongs on Preferences page or Preferences System page. Thanks.

[busykai]

@redmunds, added this paragraph to Preferences System. Also, added description of prefs for linting configuration and extended example for changes made by #7362.

[redmunds]

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