Move descriptions to their preferences #11201

Merged
merged 1 commit into from Jun 12, 2015

Projects

None yet

5 participants

@sprintr
Contributor
sprintr commented Jun 2, 2015

All the descriptions of the preferences have been moved to their preferences and we no longer have data.json 😄

@MarcelGerber MarcelGerber commented on an outdated diff Jun 2, 2015
src/extensions/default/PrefsCodeHints/main.js
@@ -39,7 +39,16 @@ define(function (require, exports, module) {
Strings = brackets.getModule("strings"),
ThemeManager = brackets.getModule("view/ThemeManager"),
_ = brackets.getModule("thirdparty/lodash"),
- data = JSON.parse(require("text!data.json")),
+ data = {
@MarcelGerber
MarcelGerber Jun 2, 2015 Member

Maybe also add a comment stating what this data is used for.

@abose abose added this to the Release 1.4 milestone Jun 2, 2015
@abose
Contributor
abose commented Jun 2, 2015

Fixes part of #11200

@busykai busykai and 3 others commented on an outdated diff Jun 2, 2015
src/language/CodeInspection.js
- prefs.definePreference(PREF_PREFER_PROVIDERS, "array", []);
+ prefs.definePreference(PREF_PREFER_PROVIDERS, "array", [], {
+ description: Strings.DESCRIPTION_LINTING_PREFER,
+ values: ["JSLint", "JSHint"],
@busykai
busykai Jun 2, 2015 Member

This should not specify value.

@TomMalbran
TomMalbran Jun 2, 2015 Contributor

Maybe it could autocomplete with the registered linters? That would be cool.

@busykai
busykai Jun 2, 2015 Member

Yes, it could be done, I guess. My point, though, it should not have default in the property definition (there's no default for this pref).

@sprintr
sprintr Jun 2, 2015 Contributor

Ok, I will add that.

@busykai
busykai Jun 2, 2015 Member

Just to be clear: what I am asking you to do is to remove line 617. Hinting on registered plugins is not necessarily needed for this PR.

@MarcelGerber
MarcelGerber Jun 3, 2015 Member

If you could just go ahead and remove this line (and also squash commits if you like), I could merge this PR in.

@MarcelGerber MarcelGerber self-assigned this Jun 3, 2015
@sprintr
Contributor
sprintr commented Jun 5, 2015

@abose @MarcelGerber I think it is ready for merging in.

@TomMalbran TomMalbran commented on the diff Jun 6, 2015
src/language/CodeInspection.js
@@ -199,6 +199,21 @@ define(function (require, exports, module) {
}
/**
+ * Returns an array of the IDs of providers registered for a specific language
+ *
+ * @param {!string} languageId
+ * @return {Array.<string>} Names of registered providers.
@TomMalbran
TomMalbran Jun 6, 2015 Contributor

This should be {?Array.<string>}.
Or maybe it should return an empty array on the null case.

@abose abose commented on the diff Jun 6, 2015
src/extensions/default/HTMLCodeHints/main.js
- PreferencesManager.definePreference("codehint.TagHints", "boolean", true);
@abose
abose Jun 6, 2015 Contributor

why is codehint.TagHints is removed?

@sprintr
sprintr Jun 6, 2015 Contributor

It is still there, I added description to it.

@MarcelGerber MarcelGerber commented on the diff Jun 7, 2015
src/extensions/default/PrefsCodeHints/main.js
_ = brackets.getModule("thirdparty/lodash"),
- data = JSON.parse(require("text!data.json")),
+ languages = LanguageManager.getLanguages(),
@MarcelGerber
MarcelGerber Jun 7, 2015 Member

We should also listen to the LanguageManager events languageAdded and languageModified (?).

@MarcelGerber
MarcelGerber Jun 7, 2015 Member

You could do the same for the list of themes by listening to ExtensionManagers' statusChange event.

@sprintr
sprintr Jun 7, 2015 Contributor

statusChange gets fired every time an extension gets loaded. We would need an event that is fired only when an extension gets installed/uninstalled, so we can update our preferences data.

@TomMalbran
TomMalbran Jun 7, 2015 Contributor

Does it makes a big difference to have this once instead of calling it when getting the list of hints?

You are already doing it for the code hint providers. So I think we can do something similar for both languages and themes.

@sprintr
sprintr Jun 7, 2015 Contributor

We can check if the AppInit.appReady has been called, after that we can update our preferences, theme data on each statusChange event.

@MarcelGerber
MarcelGerber Jun 7, 2015 Member

AFAIK, all extensions are loaded after appReady has been called.

@sprintr
sprintr Jun 7, 2015 Contributor

Not the ones you install after that, we need another event for that.

@sprintr
sprintr Jun 7, 2015 Contributor

@MarcelGerber I gave this a shot but since statusChange event is fired before the theme gets registered, ThemeManager.getAllThemes() still provides the old list of themes. But it works for getting code hints from newly installed extensions.

@sprintr
sprintr Jun 7, 2015 Contributor

@MarcelGerber @TomMalbran Theme hints are not cached anymore, that should fix the issue of themes. Languages will still use languageAdded event.

@TomMalbran
TomMalbran Jun 7, 2015 Contributor

@sprintr I still don't see the point of having a cache of something that could be grabbed at the point of getting the hints like it is done with the code hint providers. getLanguages copies the object, but since you will have to iterate through the list eventually to create the ui, should be fine to get them on getHints when you actually need them. And the same goes for the themes, and maybe even the preferences. Seems easier than keeping a copy that needs to be synched.

@sprintr sprintr commented on the diff Jun 10, 2015
src/language/CodeInspection.js
@@ -199,6 +199,21 @@ define(function (require, exports, module) {
}
/**
+ * Returns an array of the IDs of providers registered for a specific language
+ *
+ * @param {!string} languageId
+ * @return {Array.<string>} Names of registered providers.
+ */
+ function getProviderIDsForLanguage(languageId) {
+ if (!_providers[languageId]) {
+ return [];
+ }
@sprintr
sprintr Jun 10, 2015 Contributor

@MarcelGerber It now returns empty array.

@MarcelGerber
Member

Merging. Thanks @sprintr.

@MarcelGerber MarcelGerber merged commit cc0bb85 into adobe:master Jun 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sprintr sprintr deleted the sprintr:move-prefs-description branch Jun 12, 2015
@abose
Contributor
abose commented Jun 15, 2015

@sprintr the jasmine unit tests suite is failing after this PR merge commit. with the following error
Uncaught Error: Module name "language/CodeInspection" has not been loaded yet for context: _
Usually its a good idea to run the unit tests suite to catch errors like this :)

@sprintr
Contributor
sprintr commented Jun 15, 2015

@abose Debugging this right now. Seems a tricky one.

@sprintr sprintr referenced this pull request Jun 15, 2015
Merged

Fix unittests #11275

@abose
Contributor
abose commented Jun 16, 2015

Thanks @sprintr for the fix.
@MarcelGerber Usually we go through a pass of unit tests, integration and extension tests as baseline(+additional test cases) before merging any pull requests. 🚕

@MarcelGerber
Member

@abose I'm sorry. I've looked through all the changes and, well, I didn't expect unit test failures as all this PR changed is the added descriptions and one newly added function.
I'll be more careful the next time.

@abose
Contributor
abose commented Jun 16, 2015

@MarcelGerber No probs :) We also used to do the same thing until we noticed that in some cases even minor changes could sometimes cause a ripple. So just stuck to doing the basic smokes just before merge.

@MarcelGerber MarcelGerber referenced this pull request Jun 23, 2015
Closed

Docs for Prefs code hints #11200

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment