Detection of scoped settings is incorrect for longer chains

[Ingramz]

This is a continuation to #774, which was 'fixed' with #841, however the fix only helps with case where the scope chain is two or less.

The issue revolves around getDefault function in settings-panel, from which line 119 is dead code (unrelated).

The current behavior checks if the settings panel is for a certain grammar scope (@options.scopeName) and based on that returns that the unscoped value (* scope), if unset, it is the editor default value.

This works if a package does not build on top of another package, meaning that we treat scope names as identifiers and no package/language grammar scope name may be a string containing an already existing scope name. I do not know of this limitation, so I do not think this is intended.

However we at language-blade try to build on top of language-php by naming our grammar scope .text.html.php.blade (PHP uses .text.html.php). The intention was to inherit as much from PHP as possible by default, yet leave the possibility for users to override the settings if they wanted to.

Settings panel for Blade will get confused if the setting was overridden from default in PHP (mainly editor.tabLength). Namely if we configure different tabLengths:

1. Set global editor.tabLength to 2 (*)
2. Set PHP editor.tabLength to 4 (.text.html.php)
3. Set Blade editor.tabLength to 2 (.text.html.php.blade)

This was reported at https://github.com/jawee/language-blade/issues/63

What happens at 3 is that since it is a scoped settings panel, isDefault will find via getDefault that the unscoped value (*) is the same as the value we are trying to set, which will clear the configuration field for the specific scope - .text.html.php.blade. Now calling atom.config.get with scope .text.html.php.blade will return the value for .text.html.php, which will in turn override emptied field at the settings panel.

To solve this, I believe changes in config API are required - similarly to excludeSources option, there should be an option to exclude a scope, so that it would be possible to get a default value for a scope, excluding the scope name itself. And when used, for instance in atom.config.get('editor.tabLength', {scope: '.text.html.php.blade', excludeScope: '.text.html.php.blade'}), it would return the value as if there was no value specifically set for that scope (it might return a value for .text.html.php or the unscoped user setting * or the editor default). Using that to detect the default should make it work no matter how long the chain is.

cc @maxbrunsfeld

[Ingramz]

It might be possible to do this without API changes as well by temporarily unsetting and storing the value in a variable, then querying the same scope name again. If the two values differ, then we'll need to set the first value back. I will try if this approach works.

Update: (un)setting those values will cause recursion on the observers, so those have to be temporarily disabled first, something telling me this approach is not worth the effort, however after temporarily disabling observers with a boolean it works as intended.

diff --git a/lib/settings-panel.coffee b/lib/settings-panel.coffee
index 0ea9326..9cb1e54 100644
--- a/lib/settings-panel.coffee
+++ b/lib/settings-panel.coffee
@@ -12,6 +12,7 @@ class SettingsPanel extends CollapsibleSectionPanel

   initialize: (namespace, @options={}) ->
     @disposables = new CompositeDisposable()
+    @shouldObserve = true
     if @options.scopeName
       namespace = 'editor'
       scopedSettings = [
@@ -76,7 +77,8 @@ class SettingsPanel extends CollapsibleSectionPanel
       name = input.attr('id')
       type = input.attr('type')

-      @observe name, (value) ->
+      @observe name, (value) =>
+        return unless @shouldObserve
         if type is 'checkbox'
           input.prop('checked', value)
         else
@@ -114,10 +116,15 @@ class SettingsPanel extends CollapsibleSectionPanel
   getDefault: (name) ->
     if @options.scopeName?
       atom.config.get(name)
+      @shouldObserve = false
+      userSetting = atom.config.get(name, scope: [@options.scopeName])
+      atom.config.unset(name, scopeSelector: @options.scopeName)
+      defaultSetting = atom.config.get(name, scope: [@options.scopeName])
+      atom.config.set(name, userSetting, scopeSelector: @options.scopeName) if userSetting isnt defaultSetting
+      @shouldObserve = true
+      defaultSetting
     else
-      params = {excludeSources: [atom.config.getUserConfigPath()]}
-      params.scope = [@options.scopeName] if @options.scopeName?
-      atom.config.get(name, params)
+      atom.config.get(name, excludeSources: [atom.config.getUserConfigPath()])

   set: (name, value) ->
     if @options.scopeName
@@ -133,7 +140,8 @@ class SettingsPanel extends CollapsibleSectionPanel
       select = $(select)
       name = select.attr('id')

-      @observe name, (value) ->
+      @observe name, (value) =>
+        return unless @shouldObserve
         select.val(value)

       select.change =>
@@ -161,6 +169,7 @@ class SettingsPanel extends CollapsibleSectionPanel
           editorView.setText('')

       @observe name, (value) =>
+        return unless @shouldObserve
         if @isDefault(name)
           stringValue = ''
         else

[maxbrunsfeld]

I'm open to adding APIs to atom.config, but I'm not quite understanding your excludeScope proposal.

Currently, the scope option does not take a selector (e.g. .text.html.php.blade); it takes a ScopeDescriptor - basically an array of scope names that represents a specific location in the DOM (e.g. ['text.html.php.blade', 'support.function.construct.begin.blade', 'keyword.blade']). So it's not exactly clear what the excludeScope option would mean.

It seems like if we are viewing a package's settings, we should use the excludeSources option, and exclude the current package's settings file. Thoughts?

[Ingramz]

@maxbrunsfeld to me it doesn't matter what the final solution will be and I am not trying to say in any way that I know better how an API should look like. Also I am not familiar how excludeSources is used other than to exclude user's config file, which is not desired.

We just need a mechanism to get the value for a property that excludes the exact scope name itself, but would allow the less specific (more generic?) scoped property value to be queried, which should be the default for that package. It can be a simple boolean in the options list, however what I know for sure is that it should be non destructive operation (not involve unsetting the current value to find out the parent value).

[maxbrunsfeld]

We just need a mechanism to get the value for a property that excludes the exact scope name itself, but would allow the less specific (more generic?) scoped property value to be queried, which should be the default for that package.

My question is, do we need to exclude the scope name, or can we exclude the current package's settings? Excluding the package's settings seems like the most straightforward way to determine "What would the value of this setting be if this package were disabled?".

[Ingramz]

@maxbrunsfeld my answer to that question would be either a "no" or "I don't know". If it's possible to to do that, then it is indeed quite a straightforward way of achieving the same thing.

[winstliu]

@Ingramz I recently changed some logic with regards to how scoped setting defaults are calculated. Can you take a look at https://github.com/atom/settings-view/pull/967 and see if that fixes this issue?

[Ingramz]

@50Wliu unfortunately that does not fix the issue.

The part where defaultValue = atom.config.get(name) is done, is roughly equivalent to atom.config.get(name, {scope: '*'}), which is assumed to be parent of majority of languages today. However if there has been a scope configured that is more specific than * but less specific than that language, it will still use the value of *, which is invalid.

To visualize, a chain might look like this (in the order from least specific to most specific): * -> .text -> .text.html -> .text.html.php -> .text.html.php.blade

Even though .text and .text.html are generally not used on their own and cannot be configured using settings-view as there is no language associated with them, they still can be used to provide an intermediary default to scopes such as .text.plain .text.html.basic and .text.html.php (but also .text.html.php.blade). Currently getDefault will not consider the values of .text or .text.html as defaults to .text.html.php, but will straight jump to *.

Does this make any sense?

[kikoseijo]

Helloooooooo!

Anybody at home?

Try make PHP at 4spaces and try to put this package at 2. its imposible and we need more room on html files. we can't work at 4, its too much scroll... 😜

Happy coding!