[CLOSED] Preferences ID cleanups - Part II

[core-ai-bot]

Issue by TomMalbran
Tuesday Mar 12, 2013 at 00:54 GMT
Originally opened as adobe/brackets#3101

---

I checked the code after #3018 and I noticed that ViewCommandHandlers.js wasn't using the latest getClientId function, probably because the latest changes on both where merged close in time, and that UpdateNotification.js was still using the old id.

---

TomMalbran included the following code: https://github.com/adobe/brackets/pull/3101/commits

[core-ai-bot]

Comment by lkcampbell
Tuesday Mar 12, 2013 at 06:52 GMT

---

The irony is that ViewCommandHandlers.js is the exact file that got all of this work started in the first place :).

Have you thought about creating a similar function for Extension developers? It turns out that the 'main' namespace I found came from an extension using mode.id for its client id.

I don't know exactly what the format would be. Maybe a function in ExtensionUtils.js called getExtensionClientId() that takes an extension name and/or the extension creator's name and returns a suitable client id for extension preferences.

[core-ai-bot]

Comment by TomMalbran
Tuesday Mar 12, 2013 at 07:11 GMT

---

Hehe. Well it was a problem of no merge conflicts, last minutes updates and close merge times.

That is a nice idea. I would still add it to PreferenceManager since the extension would still need to load this to get the storage. It would require a github user account, an extension name (the author should use different names here) and the module id so that each file in the extension can have its own storage. It could then return "com.github." + githubName + "." + extensionName + "." + module.id. But since extensions could leave outside of github too, maybe it can be better to make it extension author, extension name (or both together) and module id. Not sure what would be the best suitable prefix for this case, if one is needed.

[core-ai-bot]

Comment by redmunds
Thursday Mar 14, 2013 at 16:00 GMT

---

I'm seeing 1 other case outside the src folder in test/spec/SpecRunnerUtils.js.

[core-ai-bot]

Comment by redmunds
Thursday Mar 14, 2013 at 16:26 GMT

---

The module.id problem with extensions needs to be fixed because the RecentProjects extension is now broken.

Instead of passing the module.id to getClientId(), what about passing the module object so getClientId() could detect extensions using module.uri and behave accordingly? If (module.id === "main") then ...

[core-ai-bot]

Comment by redmunds
Thursday Mar 14, 2013 at 16:26 GMT

---

Done with initial review.

[core-ai-bot]

Comment by TomMalbran
Friday Mar 15, 2013 at 06:28 GMT

---

1. The TEST_PREFERENCES_KEY set in test/spec/SpecRunnerUtils.js is the key for the local storage and not for PrefenceManager.getPreferenceStorage(). I don't think that that one should be changed, as we didn't change the one in PrefenceManager that has the same purpose.
2. I like having getClientId receive a module and use the module.uri for extensions, but it wouldn't cover the cases for files that aren't main.js inside the extension that use the preferences and could cause id collisions. Could we just check for src/extensions/ or the user extension folder and use the url in those case and the id in the others? This would be great if we need to remove the preferences of deleted extensions by just checking the url in the key.
3. I was checking that we are now always defining a client key with getClientId() and then creating the storage. Wouldn't it be better if we can just call getPreferenceStorage() passing the module and let it create the id. It could even work for extensions using the url, and for backwards compatibility getPreferenceStorage() would be able to handle both a string or a module object as the clientID.

[core-ai-bot]

Comment by redmunds
Friday Mar 15, 2013 at 15:23 GMT

---

1. Yes, you're right.
2. Yes, using the path is a better idea. Note that the path can also be in ~/Library/Application Support/Brackets/extensions (Mac) or AppData/Roaming/Brackets/extensions (Windows). And we need a way to factor that out for Edge Code which is different.
3. That sounds even better.

[core-ai-bot]

Comment by TomMalbran
Friday Mar 15, 2013 at 17:48 GMT

---

Yes. I was thinking of checking against FileUtils.getNativeBracketsDirectoryPath() for the default extensions and ExtensionLoader.getUserExtensionPath() for the user extensions.

[core-ai-bot]

Comment by redmunds
Friday Mar 15, 2013 at 18:02 GMT

---

Perfect. Be sure not to use "default", "user", or "dev" in the generated id so an extension will use the same preferences if moved.

[core-ai-bot]

Comment by TomMalbran
Saturday Mar 16, 2013 at 08:06 GMT

---

I just made both changes. The client id looks great now for the RecentProjects and other extensions. I notice that there could be one problem, when a user extension folder has the same name as a default/dev extension. But this could be avoided having a notice for Extensions developers to use different names, or some sort of renaming in the new extension manager to avoid collisions. But this seems to be the best solution if an extension is moved from the default folder to the user folder.

Travis failed, but I run the tests locally and the all pass. The problem seems to be that it doesn't identify brackets.app.getApplicationSupportDirectory().

[core-ai-bot]

Comment by redmunds
Saturday Mar 16, 2013 at 21:26 GMT

---

This looks good. Just a few comments about code cleanup. Hopefully, the brackets.app.getApplicationSupportDirectory() problem will be fixed by then. :)

[core-ai-bot]

Comment by TomMalbran
Saturday Mar 16, 2013 at 21:45 GMT

---

@redmunds Fixes done.

Is still failing for the same reason. I think is just because Travis runs in the browser and doesn't have access the shell APIs.

I was thinking that handleClientIdChange could be an useful API for future folder/file renaming, for both core and extensions. The only problem is the "newClientID" preference that would need to be removed before the next change. It could use the module as the id too, but might create lots of unwanted prefs values since old modifications wont be deleted.

[core-ai-bot]

Comment by TomMalbran
Monday Mar 18, 2013 at 07:51 GMT

---

Nice idea, I added that function. I also change handleClientIdChange, since it wasn't working when starting a preference storage with defaults for some cases. I did a test and it did restored my old preferences.

[core-ai-bot]

Comment by redmunds
Monday Mar 18, 2013 at 19:11 GMT

---

Everything looks good so far.

One minor quirk is that the recent projects list is not getting populated. That does not seem critical, but maybe there's a problem lingering.

The Travis problem is due to the fact that Travis can only run headless tests. The preference tests in master are headless, but new dependencies added to PreferenceManager.js require brackets.app APIs, so it needs to be fixed in this pull request. This can be fixed by mocking the brackets.app using something like jasmine spies, but we have not yet done any of that for Travis. If you want to take a look at that, then you'll need to install node and grunt as described here: https://github.com/adobe/brackets/wiki/Grunt-Setup Otherwise, you'll need to remove line 54 from brackets/src/Gruntfile.js that specifies to run PreferencesManager-test.js.

[core-ai-bot]

Comment by TomMalbran
Tuesday Mar 19, 2013 at 04:17 GMT

---

1. I don't see the recent projects problem. I deleted the newClientID key from the recent project preferences, opened Brackets Sprint 21, added lots of new projects, closed it and went back to using Brackets Sprint 22 and all the recent projects I added before where added replacing the ones I had before.
2. Changing the Gruntfile didn't fix it. I think is because PreferenceManager is still being used from SpecRunnerUtils when loading the modules. Removing both LanguageManger-test.js and PreferencesManager-test.js fixes it.
3. What do you mean about mocking the brackets.app using something like jasmine spies?
4. And .gitignore is missing the added .grunt folder and _SpecRunner.html created after installing Grunt.