pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.33k stars 140 forks source link

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

Closed savetheclocktower closed 9 months ago

savetheclocktower commented 9 months ago

…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:

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:

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:

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

savetheclocktower commented 9 months ago

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 commented 9 months ago

Well glad someone did then!