Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not delete global scope setting when resetting user settings #19054

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
2 participants
@rafeca
Copy link
Contributor

commented Mar 26, 2019

Identify the Bug

This PR fixes #18372

Description of the Change

Edit: This PR changes the atom.config.set() method to never use the scoped store when setting global config params (even when using the scopeSelector). This is to prevent incorrect values to be stored in the user config file, which happens when the scoped store receives a call to set a global property (since it does not hold any global properties).

This PR changes the logic of the atom.config.resetUserSettings() method to not delete the globally scoped settings (the ones under the scope *) before storing them on the scoped-settings-store.

Before, since the loaded globally scoped settings were not stored in the scoped-settings-store, the store did erase them if a user executed atom.store.set(something, anything, {scopeSelector: '*'}) because it did not have the previous fields.

This seems to have been introduced by #4606 (4 years ago, lol), and my hypothesis is that it may had been just a small mistake when modifying the old logic, but I may be missing something and maybe this was intentional (cc/ @nathansobo if you remember it πŸ˜…).

Alternate Designs

πŸ€·β€β™‚οΈ

Possible Drawbacks

I'm not aware of any potential drawback apart from the risk of breaking some other logic that was depending on the previous behaviour

Verification Process

  • Manual verification
    1. Open Atom
    2. Execute atom.config.set("foo.bar", "baz", {scopeSelector: '*'});
    3. Open ~/.atom/config.cson and check that my previous config is not gone πŸ˜„
  • Added an automated test that fails without this PR change.

Release Notes

atom.config.set does not erase settings when scopeSelector is "*" anymore.

@rafeca rafeca self-assigned this Mar 26, 2019

@rafeca rafeca requested review from maxbrunsfeld and jasonrudolph Mar 26, 2019

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I don't remember how this works super well... but I feel like there was some reason that we didn't put the global settings in the scoped settings store, even though I always wished they were unified whenever I worked on this code. I think the reason may have had to do with mutation. The ScopedPropertyStore doesn't have the best support for mutating properties in place, and a lot of the settings that are frequently mutated (like editor.fontSize) are global.

Does the bug only happen when * is explicitly passed for the scopeSelector option? If so, would another solution be to special-case * in Config.set to behave the same as if no scopeSelector was provided?

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I also might be misunderstanding the new code. With this change, would global settings always be stored both in the config.settings object and in the ScopedPropertyStore? Or would the scoped property store just contain the global settings at the time the config was loaded, but not reflect any updates to the global settings (done via atom.config.set('foo', 'bar'))?

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

I don't remember how this works super well... but I feel like there was some reason that we didn't put the global settings in the scoped settings store, even though I always wished they were unified whenever I worked on this code. I think the reason may have had to do with mutation. The ScopedPropertyStore doesn't have the best support for mutating properties in place, and a lot of the settings that are frequently mutated (like editor.fontSize) are global.

Oh thanks for the context! I think the mutability issues may be related to atom/scoped-property-store#8 πŸ€¦β€β™‚οΈ

Does the bug only happen when * is explicitly passed for the scopeSelector option? If so, would another solution be to special-case * in Config.set to behave the same as if no scopeSelector was provided?

Yeah, I guess I'll have to do this... I tried to avoid adding yet another a special case here but that's going to be the easier way to get this fixed...

I also might be misunderstanding the new code. With this change, would global settings always be stored both in the config.settings object and in the ScopedPropertyStore? Or would the scoped property store just contain the global settings at the time the config was loaded, but not reflect any updates to the global settings (done via atom.config.set('foo', 'bar'))?

Ups, I did not even realize that global settings are stored in a separate data structure 😐. In this case the global settings may get desynchronised between the two data structures with my change which is bad...

Thanks for the insights! I'm gonna change the solution πŸ˜„

@rafeca rafeca force-pushed the rafeca:fix-scoped-settings branch from 32afbc1 to 4fd8003 Mar 26, 2019

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I think the mutability issues may be related to atom/scoped-property-store#8 πŸ€¦β€β™‚οΈ

Oh wow, I knew I had taken a look at this problem before; I forgot that it was right when I joined the team, and I ended up giving up on it 😬 .

@rafeca rafeca merged commit 3828387 into atom:master Mar 27, 2019

2 checks passed

Atom Pull Requests #20190326.13 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rafeca rafeca deleted the rafeca:fix-scoped-settings branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.