[CLOSED] Preferences id cleanup

[core-ai-bot]

Issue by WebsiteDeveloper
Saturday Mar 02, 2013 at 08:57 GMT
Originally opened as adobe/brackets#3018

---

Cleaned up the preferences ids
See #3016

---

WebsiteDeveloper included the following code: https://github.com/adobe/brackets/pull/3018/commits

[core-ai-bot]

Comment by TomMalbran
Saturday Mar 02, 2013 at 19:54 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Saturday Mar 02, 2013 at 19:58 GMT

---

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

[core-ai-bot]

Comment by WebsiteDeveloper
Sunday Mar 03, 2013 at 10:29 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Sunday Mar 03, 2013 at 11:15 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Sunday Mar 03, 2013 at 11:23 GMT

---

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

[core-ai-bot]

Comment by WebsiteDeveloper
Sunday Mar 03, 2013 at 11:27 GMT

---

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.

[core-ai-bot]

Comment by TomMalbran
Sunday Mar 03, 2013 at 21:41 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Sunday Mar 03, 2013 at 21:54 GMT

---

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.

[core-ai-bot]

Comment by TomMalbran
Sunday Mar 03, 2013 at 22:50 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Monday Mar 04, 2013 at 18:41 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Monday Mar 04, 2013 at 20:56 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Monday Mar 04, 2013 at 21:03 GMT

---

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.

[core-ai-bot]

Comment by TomMalbran
Monday Mar 04, 2013 at 21:09 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Monday Mar 04, 2013 at 21:13 GMT

---

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

[core-ai-bot]

Comment by redmunds
Sunday Mar 10, 2013 at 00:20 GMT

---

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