fix merging of old prefs with new prefs #3185
Conversation
| @@ -189,7 +189,7 @@ define(function (require, exports, module) { | |||
| var data = oldPrefs.getAllValues(); | |||
|
|
|||
| if (!$.isEmptyObject(data)) { | |||
| newPrefs.setAllValues(data, false); | |||
| newPrefs.setAllValues(data, true); | |||
There was a problem hiding this comment.
When porting from the old client ID to the new client ID, here in handleClientIdChange, we were taking in a brand new defaults object, then seeing if the old ID was non-empty. If it's not empty, we then set the old pref keys into the new prefs object (which again matches the default properties passed initially to PreferencesManager.getPreferenceStorage(module, defaultPrefs
However, we were passing append==false to setAllValues(). What this did was it cleared out all the new properties defined in the defaults object and only wrote the properties that existed for the old clientID.
With this change to append==true, we take the new defaults object and merge in the old existing pref keys if they exist. This is the behavior that we want and I'm pretty sure the original changes were unintended. Let me know if I've missed something.
This fixes the preferences migration issues I was seeing in @RaymondLim's pull #3097 for word wrap and line numbers.
|
it seems like this slipped in accidentialy in my pull request for the preferences id cleanup it should work with this change, this didn't show up before, because there weren't any new preferences, so it's good you fixed it. |
|
It makes sense since the new preferences and the old preferences should have mostly the same ids, except for the possible new ones in the new preferences and the old preferences can still overwrite the defaults on the new preferences. |
|
Merging now since both @WebsiteDeveloper and @TomMalbran agree. |
fix merging of old prefs with new prefs
@TomMalbran @RaymondLim
See inline comments