Preferences id cleanup

[WebsiteDeveloper]

Cleaned up the preferences ids
See #3016

[TomMalbran]

Doing this would also require a 1 time only function (that should be removed in a Sprint or 2) to move the preferences from the old ids to the new ones and removing the preferences on the old ids, so that all the preferences are conserved.

[WebsiteDeveloper]

yeah you're right i didn't think of that.
work in progress...

[WebsiteDeveloper]

@TomMalbran i added a migration function
when this gets merged i will also open an issue to remove those in a sprint or two

[TomMalbran]

Nice work. I am just seeing lots of repeated code. Maybe to simplify it you could just call handleClientIdChange with the new prefs and just the id of the old ones and inside handleClientIdChange check if it wasn't done before as you did and if the old preferences are saved (if you just call getPreferenceStorage with a non-existent preference you will be creating a new storage), do the same copy and at the end you could do a delete prefStorage[oldClientID] just to cleanup the old now unused preferences.

[WebsiteDeveloper]

currently it doesn't really work because sometimes old properties do not exist (with empty local storage) and i get an error because some values are undefined.
i'll try to fix that as soon as possible

[WebsiteDeveloper]

what is interesting, is that the problem fixes itself when restarting brackets,
then all works fine, but the prefs get trashed one time which isn't preferable.

[TomMalbran]

Would it be solved with something like this?

function handleClientIdChange(newPrefs, oldPrefsID) {
    if (!newPrefs.getValue("newClientID") && prefStorage[oldPrefsID]) {
        var oldPrefs = getPreferenceStorage(oldPrefsID);
        var data = oldPrefs.getAllValues();
        newPrefs.setAllValues(data, false);
        delete prefStorage[oldPrefsID];
        newPrefs.setValue("newClientID", true);
    }
}

And then just call PreferencesManager.handleClientIdChange(_prefs, oldPrefsID) from each module.

[WebsiteDeveloper]

I tried something like this but the problem is when somebody would newly install brackets or open brackets with a clean local storage, it would break because the setAllValues method reports the value undefined as invalid.
I tried another workaround which fixed the startup problem but didn't preserve the old prefs.
I will be further investigating.

[TomMalbran]

I just checked that function I wrote with:
handleClientIdChange(getPreferenceStorage("com.adobe.brackets.DocumentManager"), "preferences");
And I got no errors, knowing that preferences wasn't saved as a key before.

[WebsiteDeveloper]

@TomMalbran would you want to test this?
it works for me.

[TomMalbran]

I could give it a try later.

Right now I don't think that the Preference ID from PreferencesManager should be changed, since that is actually the localStorage key and everything else is stored in an object that is stringifyed and stored as the value associated with that key. I would leave that one as how it is.

[WebsiteDeveloper]

in my opinion it is better to change that in the PreferencesManager too because so it all becomes more consistent, and as it doesn't add an awfull lot of work i think it's ok.

[TomMalbran]

But is not actually a Preference Storage key associated with the PreferenceManager file, is just the only Local Storage key used, so it can be different to every other key and there wont be any real collision problem with it.

[WebsiteDeveloper]

yeah i'll just revert that change so there are fewer changes

[redmunds]

Instead of repeating this string everywhere, there should be a getClientId(moduleId) function in PreferencesManager.

[redmunds]

This looks good, but there's a merge conflict, so I can't test it.

[WebsiteDeveloper]

@redmunds i resolved the merge conflict and added a getClientId function
i'll open an issue for the cleanup once this is merged

[redmunds]

Looks good. Merging.