Preferences ID cleanups - Part II

[TomMalbran]

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.

[lkcampbell]

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.

[TomMalbran]

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.

[redmunds]

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

[redmunds]

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 ...

[redmunds]

Done with initial review.

[TomMalbran]

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.

[redmunds]

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.

[TomMalbran]

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

[redmunds]

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

[TomMalbran]

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().

[redmunds]

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

[TomMalbran]

@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.

[redmunds]

The paths Array gets built every time this function is called and it's the same every time. Create a function called something like getExtensionPaths() that builds and returns the array the first time it's called, and simply returns the array on all subsequent calls.

[TomMalbran]

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.

[redmunds]

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.

[TomMalbran]

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.

[redmunds]

I'll see if I can find a reproducible recipe.

I think also disabling LanguageManager-test.js may be too much.

After re-enabling PreferencesManager-test.js, I think you'd need to do something like this:

    spyOn(brackets.app, "getApplicationSupportDirectory").andReturn(rootBracketsPath);

@jasonsanjose Can you comment on item 4 in the latest reply from @TomMalbran ?

[jasonsanjose]

Hey @TomMalbran. I want to help get this landed today so we can land #3097. Will you be online and possibly on IRC?

[jasonsanjose]

For item 4, I think the jasmine grunt task leaves those folders behind when the test fails.

[TomMalbran]

@jasonsanjose Yes I can. I just Logged in there. And you are right, those files aren't created after the installation as I thought. So I could delete those files then.

The getApplicationSupportDirectory error comes after loading Editor from the SpecRunnerUtils.

[jasonsanjose]

Filed #3170. @TomMalbran will disable the tests for now until we can properly mock the required shell APIs.

[TomMalbran]

@redmunds @jasonsanjose Done! And it finally works :)

[redmunds]

Thanks @jasonsanjose and @TomMalbran . Merging.