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

Fix issue with looking up objects in atom.config #897

Conversation

savetheclocktower
Copy link
Contributor

@savetheclocktower savetheclocktower commented Jan 28, 2024

…when a project-specific config is present.

Identify the Bug

The symptom: indentation queries that used test.config predicates weren't reacting to changes in config. If a certain indentation behavior is dependent on a setting, and I toggle the setting, I shouldn't have to relaunch the window to get the behavior to change. But that's not what I was experiencing!

Description of the Change

We cache config lookups in ScopeResolver for performance. To know when to invalidate that cache, instead of creating a new listener every time an individual predicate consumes a specific setting, we listen once on the entire config object and clear our cache whenever it changes for any reason.

Ordinarily, a call to atom.config.get() without an argument:

  • looks up the ordinary base config (an object with top-level properties core, editor, and all my package names)
  • deep-merges (a copy of) that object with (a copy of) an object that holds all the default values for all config options, obviously preferring the former in cases of overlap, so that the entire object would contain every configuration property and value

But because I have an empty .atom folder at the root of my project (because at one point I was applying project-specific settings via atomic-management), a call to atom.config.get() with no argument:

  • looked up the ordinary base config (an object with top-level properties core, editor, and all my package names)
  • looked up the project-specific config (an empty object)
  • preferred the empty object because it's truthy
  • blended the empty object with (a copy of) an object that holds all the default values for all config options

The logic that prefers a project-specific value when present… well, it works great with primitive values, but not with objects.

The effect here is that, when I called atom.config.get() with no argument, I got an object filled with all of the default values. Every time, no matter how weird my actual config settings got. This bug propagated to onDidChange because onDidChange handlers that listen for object values will only get called if an object is substantively different in value, not just reference, and the value never seemed to change, even when I was changing options.

The fix is pretty easy:

  • look up the ordinary base config (an object with top-level properties core, editor, and all my package names)
  • look up the project-specific config (an empty object)
  • notice they're both objects and blend the base into (a copy of) the project-specific config, preferring the project-specific config when things clash
  • blend that combined object with (a copy of) an object that holds all the default values for all config options

Alternate Designs

I did not think of any. This is a pattern that is used later in the same function, so it was the obvious solution.

Possible Drawbacks

I can't think of any. Blending these objects will be slightly costlier than before, but there's no way around it if you're offering people this abstraction.

Verification Process

Run the specs.

Release Notes

  • Fixed an issue with reacting to changes in configuration when using project-specific settings overrides.

…when a project-specific config is present.

Most people don't use a project-specific config, which is why this bug has been present for ages. Read the new spec for an explanation.
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not the most familiar with the config API, this PR has changes that look good, and most importantly has specs that prove it functions as intended. Think with that we should be good to merge, nice find on this issue!

@savetheclocktower
Copy link
Contributor Author

Gonna close this because it was already part of #859. I needed it for a fix in there, and truth be told I never would've imagined that someone would review all of #859 first :)

@confused-Techie
Copy link
Member

Well glad someone did then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants