updating extension to call Brackets' new Preference APIs #169
Conversation
| @@ -218,7 +218,8 @@ define(function (require, exports, module) { | |||
| if (lastTwentyFonts.length > 20) { | |||
| lastTwentyFonts.splice(20, lastTwentyFonts.length - 20); | |||
| } | |||
| prefs.setValue(PREFERENCES_FONT_HISTORY_KEY, lastTwentyFonts); | |||
| prefs.set(PREFERENCES_FONT_HISTORY_KEY, lastTwentyFonts); | |||
| prefs.save(); | |||
There was a problem hiding this comment.
One less space in indentation on this line.
There was a problem hiding this comment.
@RaymondLim Ouch. Nice catch. Will submit a correction.
| prefs = PreferencesManager.getPreferenceStorage(PREFERENCES_CLIENT_ID); | ||
| lastTwentyFonts = prefs.getValue(PREFERENCES_FONT_HISTORY_KEY); | ||
| prefs = PreferencesManager.getExtensionPrefs(PREFERENCES_CLIENT_ID); | ||
| PreferencesManager.convertPreferences(module, { "ewf-font-history": "user com.adobe.edgewebfonts.ewf-font-history" }); |
There was a problem hiding this comment.
@bchintx Have you tested with font history migration? I asked because I'm not familiar with accessing the old local storage. What I noticed here is you're combining PREFERENCES_CLIENT_ID and PREFERENCES_FONT_HISTORY_KEY using dot. If PREFERENCES_FONT_HISTORY_KEY is stored as a property of PREFERENCES_CLIENT_ID, then it makes sense. Just want to make sure that this code works as expected.
There was a problem hiding this comment.
@RaymondLim Hmm. Good point. It looks like this extension prefixes it's preferences differently than, say, the Edge Inspect extension. As a result, the conversion isn't working. Let me investigate.
|
Done with review. Just have one question to confirm that we're migrating the old pref correctly. |
|
@RaymondLim Thanks for the review and the comment. Actually, I tested it under the debugger and you were right -- the conversion wasn't happening correctly because the client ID wasn't getting correctly resolved to the old module. I've just pushed another change to this branch, which will fix the problem. It does require a Brackets fix that was just merged as adobe/brackets#7415. The two together will fix the conversion of old values. Ready for next review. Thanks! |
|
Looks good! Merging. |
updating extension to call Brackets' new Preference APIs
updating extension to call Brackets' new Preference APIs