Preferences Code Hints #11130

Merged
merged 10 commits into from Jun 2, 2015

Projects

None yet

9 participants

@sprintr
Contributor
sprintr commented May 16, 2015

This PR implements code hints for the preferences we use in brackets. Additionally it also shows a little description of the active preference to the user.

prefrence

CC:
@redmunds @ryanstewart @MiguelCastillo

@sprintr sprintr changed the title from ] to [Review Only] Preferences Code Hints May 16, 2015
@rroshan1
Contributor

Glad to see it ready!! 😃

@petetnt
Member
petetnt commented May 19, 2015

This is pretty awesome 👍

The data.json and it's description field should be localization/internationalization-enabled and it would be nice if extensions could extend that (most likely by adding an optional description-value for PreferencesManager.definePreference)

PreferencesManager.definePreference("some.preferenceHere", "boolean", true, Strings.some.preferenceHere.description);

edit: Actually, going with the PreferencesManager.definePreference route and baking the values into src/nls/lang/Strings.js would make localization and updating extensions to support PrefsCodeHints a breeze. If the PreferenceManager supplies the descriptions and types, data.json wouldn't be needed at all provided that the descriptions would be added to all the built-in definePreferences.

edit 2: CodeFolding already does this but it's not visibile to UI so the variables are just in the Prefs.js https://github.com/adobe/brackets/blob/9dfbebe2c3961eef0aa29546e364395a5af3efc0/src/extensions/default/CodeFolding/Prefs.js#L12-L41

@abose abose commented on the diff May 19, 2015
test/spec/JSONUtils-test.js
+ parentKeyName: "jslint.options",
+ isArray: false,
+ exclusionList: [],
+ shouldReplace: false
+ });
+ });
+ it("should detect a correct exclusionList for an object", function () {
+ // Between " and tabSize"
+ ctxInfo = JSONUtils.getContextInfo(testEditor, {line: 15, ch: 13});
+ expect(ctxInfo.exclusionList).toEqual([
+ "useTabChar", "closeBrackets"
+ ]);
+ });
+ it("should detect an array", function () {
+ // After [
+ ctxInfo = JSONUtils.getContextInfo(testEditor, {line: 24, ch: 23});
@abose
abose May 19, 2015 Contributor

refactoring candidate with a for loop.

@abose
abose May 19, 2015 Contributor

Cancel that, i see that this is just a test file 👍

@abose
Contributor
abose commented May 19, 2015

+1
@sprintr Why is this marked as review only?

@sprintr
Contributor
sprintr commented May 19, 2015

@abose Wanted to get some input on this so I marked it review only.

@petetnt Going to add i18n support today. Some preferences have got their own UI, so we don't want to have each and every pref in the code hints. I was thinking about providing an API to extension devs, so they can plug in their hints. Something like PreferencesManager.setHints() and PreferencesManager.getHints() should do the job.

As far as the core brackets preferences, we should use data.json, so translators (who are encouraged to commit on the web) can easily translate descriptions in a single file.

@rroshan1 Glad you liked it. :)

@abose abose added this to the Release 1.4 milestone May 21, 2015
@sprintr
Contributor
sprintr commented May 24, 2015

Added internationalization support, moved all the descriptions to nls/root/strings.js

@MarcelGerber
Member

I'm not too set about adding all these strings to the core nls file, as it's quite a lot of new strings.
Maybe we should consider creating another nls file for this?

@sprintr
Contributor
sprintr commented May 25, 2015

@MarcelGerber We can also move it to its own strings.js file. I didn't see any other core extension doing it.

@abose
Contributor
abose commented May 25, 2015

I think it would be ok; if not easier to manage in the core file itself. adding js translation files for separate features would eventually decrease the maintainability of translations[as i saw in several other large projects]. For instance you would have to search over several files to find a string const; but with just one file, they are generally easy to locate; also people would need to go through several files to see if some strings changed to figure out translations.

@sprintr
Contributor
sprintr commented May 26, 2015
  • Added a new API PreferencesManager.getAllPreferences to return all the preferences defined.
  • Added support for preference hinting by third-party extensions using the preferences they have defined. Here is a tiny extension to demonstrate how it can be done.

@abose Is there anything else remaining to cover in this PR?

@petetnt
Member
petetnt commented May 26, 2015

Nice 👍

@TomMalbran TomMalbran commented on an outdated diff May 26, 2015
src/preferences/PreferencesBase.js
@@ -1324,6 +1328,15 @@ define(function (require, exports, module) {
},
/**
+ * Returns a clone of all preferences defined.
+ *
+ * @return {Object}
+ */
+ getAllPreferences: function () {
+ return $.extend(true, {}, this._knownPrefs);
@TomMalbran
TomMalbran May 26, 2015 Contributor

Instead of $.extend for cloning you can use _.clone and _.cloneDeep from lowdash which is already included in the core.

@TomMalbran TomMalbran commented on the diff May 26, 2015
src/extensions/default/PrefsCodeHints/main.js
+ }
+
+ if (ctxInfo.tokenType === JSONUtils.TOKEN_KEY) {
+ // Provide hints for keys
+
+ // Get options for parent key else use general options.
+ if (data[ctxInfo.parentKeyName] && data[ctxInfo.parentKeyName].keys) {
+ keys = data[ctxInfo.parentKeyName].keys;
+ } else if (ctxInfo.parentKeyName === "language") {
+ keys = LanguageManager.getLanguages();
+ option.type = "object";
+ } else {
+ keys = data;
+ }
+
+ hints = $.map(Object.keys(keys), function (key) {
@TomMalbran
TomMalbran May 26, 2015 Contributor

You could just use Object.keys(keys).map(function (key) { ... })

@sprintr
sprintr May 27, 2015 Contributor

Array.prototype.map will return undefined if it doesn't match. A possible workaround would be to use a Array.prototype.filter on the array returned by map to exclude undefined. $.map works fine for this scenario.

@abose abose was assigned by nethip May 27, 2015
@sprintr
Contributor
sprintr commented May 27, 2015

@abose Can you give this PR a review.

@abose
Contributor
abose commented May 27, 2015

@sprintr will do some test with the changes and update by tomorrow 👍
Also if you feel that this PR is good to go, You could remove the [review only] tag .

@sprintr sprintr changed the title from [Review Only] Preferences Code Hints to Preferences Code Hints May 27, 2015
@abose abose commented on the diff May 28, 2015
src/extensions/default/PrefsCodeHints/unittests.js
@@ -0,0 +1,573 @@
+/*
@abose
abose May 28, 2015 Contributor

Could this file be moved to the /test/ folder?

@sprintr
sprintr May 28, 2015 Contributor

This is a standard practice that most of the core extensions do.

@abose abose commented on the diff May 28, 2015
src/extensions/default/PrefsCodeHints/main.js
+ * highlighted.
+ *
+ * @param {Array.<Object>} hints - the list of hints to format
+ * @param {string} query - querystring used for highlighting matched
+ * portions of each hint
+ * @return {Array.jQuery} sorted Array of jQuery DOM elements to insert
+ */
+ function formatHints(hints, query) {
+
+ var hasMetadata = hints.some(function (token) {
+ return token.type || token.description;
+ });
+
+ StringMatch.basicMatchSort(hints);
+ return hints.map(function (token) {
+ var $hintItem = $("<span>").addClass("brackets-pref-hints"),
@abose
abose May 28, 2015 Contributor

codehints
The width of the selection keeps jumping; Would it be possible to fix the width and wrap the hint description. Maybe with and some css with display: block; visibility: visible/hidden; white-space: pre-wrap;

@abose abose and 2 others commented on an outdated diff May 28, 2015
src/extensions/default/PrefsCodeHints/main.js
+ isPrefDocument = false,
+ isPrefHintsEnabled = false;
+
+ var stringMatcherOptions = {
+ preferPrefixMatches: true
+ };
+
+ // List of parent keys for which no key hints will be provided.
+ var parentKeyBlacklist = [
+ "language.fileExtensions",
+ "language.fileNames",
+ "path"
+ ];
+
+ // Define a preference for code hinting.
+ PreferencesManager.definePreference("codehint.PrefHints", "boolean", true);
@abose
abose May 28, 2015 Contributor

How about adding one more argument to the definePreference API and (maybe the last parameter as description/ object with possible values for the prefs) and then raise a preferences- code hints missing warning if the last parameter is missing - so that extension devs can directly incorporate code hints to their preferences?

@sprintr
sprintr May 28, 2015 Contributor

The last (forth) parameter is optional, we can ping extension authors about incorporating their hints if they want to.

@sprintr
sprintr May 28, 2015 Contributor

We also don't want to have all the preferences in code hints, because some of the preferences are managed from UI.

@redmunds
redmunds May 28, 2015 Contributor

We also don't want to have all the preferences in code hints, because some of the preferences are managed from UI.

Some people prefer to edit preferences manually, so I think all preferences should be in code hints. Also, it's tricky to set a UI managed preference when it's language or path specific, so that's another case for manually editing preferences that have GUI controls.

@sprintr
sprintr May 28, 2015 Contributor

Preferences related to themes and fonts don't work if changed via brackets.json which is why they don't appear in code hints.

Edit: They do work when changed from UI.

@nethip
Contributor
nethip commented May 28, 2015

@sprintr Thanks for putting up this PR 👍 Really appreciate it!

I had a small suggestion. I think right now one has to manually go about editing the json file to make sure the preference name, along with its values kind of popup in code hints, while editing the preference file. How about adding another parameter to PreferencesManager.definePreference() that would take the list of preference values, along with the description, as an optional value.

This way extension writers can make sure their extension's preference names and values popup in the code hint.

Also we need to clearly document what needs to be done, going forward, to create a new preference in Brackets core. I was thinking if we can automate the job, but I might sound crazy 😼

@sprintr
Contributor
sprintr commented May 28, 2015

@nethip I wrote a tiny extension to demonstrate how extension authors can plugin their hints. Just modified PreferencesManager.definePreference.

@abose
Contributor
abose commented May 29, 2015

@sprintr
250px works very well now :)
I still think that all the preferences that can be configured using the preferences file can be hinted. Maybe we could specify in the wiki or in the hints itself that some preferences need a relaunch of brackets to take effect.

@sprintr
Contributor
sprintr commented May 29, 2015

@abose I am going to do two things tomorrow.

  1. Allow all the preferences in code hints.
  2. Move the descriptions of preferences to the files where they are defined. So we won't need updating data.json with new prefs in future.
@abose
Contributor
abose commented May 30, 2015

+1 :)
But that would result in a major rewrite of this pr i guess ..

@sprintr
Contributor
sprintr commented May 30, 2015
  1. Each and every preference defined is now available through code hints.
  2. Added src/extensions/default/PrefCodeHints/unittest-files/preferences.json which carries a few preferences to run unit tests against. This approach will allow us not to break unit tests if we add new preferences in the future.
  3. The descriptions of all the defined preferences have now been moved to where they are defined.
  4. The descriptions of preferences that are not defined still lies in data.json.
  5. The code hints won't jump to different width if a code hint doesn't have description.
  6. The value of a value token will be replaced if the very next token is a value token.

@abose Rebased it against master. If it looks good to you then you can proceed to merge it.

@MarcelGerber
Member

I have one extension whose preference name is so long that a horizontal scrollbar is shown.
The horizontal scrollbar now covers the info text:
image

@MarcelGerber MarcelGerber commented on the diff May 31, 2015
src/view/ThemeSettings.js
@@ -156,8 +156,12 @@ define(function (require, exports, module) {
prefs.set("themeScrollbars", defaults.themeScrollbars);
}
- prefs.definePreference("theme", "string", defaults.theme);
- prefs.definePreference("themeScrollbars", "boolean", defaults.themeScrollbars);
+ prefs.definePreference("theme", "string", defaults.theme, {
+ description: Strings.DESCRIPTION_THEME
@MarcelGerber
MarcelGerber May 31, 2015 Member

For some reason, this pref doesn't show up for me.

@MarcelGerber
MarcelGerber Jun 1, 2015 Member

Sorry, my fault. It does show up.

@MarcelGerber MarcelGerber commented on the diff May 31, 2015
src/brackets.js
@@ -437,7 +438,9 @@ define(function (require, exports, module) {
}
if (brackets.platform === "win" && !brackets.inBrowser) {
- PreferencesManager.definePreference("_windowsScrollFix", "boolean", true).on("change", enableOrDisableWinScrollFix);
+ PreferencesManager.definePreference("_windowsScrollFix", "boolean", true, {
@MarcelGerber
MarcelGerber May 31, 2015 Member

Is there some way to exclude a preference?
This is more or less a "hidden" preference that no one should ever need to change himself (and it will also go away at some point), so I'd appreciate it if this pref wasn't shown.
It wouldn't be much of an issue normally, but right now, this pref is the first to show up in the list...

@abose
abose Jun 1, 2015 Contributor

Exclude certain preferences from code hints: will add an issue to track this after this is merged.

@abose abose commented on an outdated diff Jun 1, 2015
...efault/PrefsCodeHints/styles/brackets-prefs-hints.css
@@ -0,0 +1,29 @@
+.brackets-pref-hints .matched-hint {
+ font-weight: 500;
+}
+.dark .brackets-pref-hints .matched-hint {
+ color: #ccc;
+}
+
+.brackets-pref-hints .hint-obj {
+ display: inline-block;
+ width: 250px;
@abose
abose Jun 1, 2015 Contributor

Changing this to min-width 300px and the below width to 300px could be a solution to the horizontal scrollbar problem.

@abose
Contributor
abose commented Jun 1, 2015

@sprintr works very well now 👍 .will merge this today after the horizontal scrollbar merge is in.

@sprintr
Contributor
sprintr commented Jun 1, 2015

@abose We should wait until #11195 get merged since that seems to be a nice way to do it. I'll add marquee effect to hints exceeding limits and functionality to exclude hints. Have to remove few merge conflicts too.

@abose
Contributor
abose commented Jun 2, 2015

@sprintr Right now the scroll-bar fix truncates long code hints. Instead of truncating, if we change the css for .brackets-pref-hints .hint-obj from width: 250px; to min-width 250px ; Then instead of truncating the hint; the popup will take the size of the largest preference variable. The hint description will be wrapped at 250px. This would avoid any truncation and also resolve the layout problems. I think this might be more suited than truncating variable names.

@MarcelGerber
Member

@sprintr LanguageManager actually defines its preferences, so you can modify the code there instead of including them in data.json: https://github.com/adobe/brackets/blob/5d4269ca359fc52095d5e7704a6b8a70c74dcd4c/src/language/LanguageManager.js#L1147-L1152

With #11197, pretty much all preferences (except the meta language and path) will be defined.

@sprintr
Contributor
sprintr commented Jun 2, 2015

@MarcelGerber You can now check marquee and exclusion of code hints. for exclusion add exclude: true in the options.

@abose We now have a marquee animation to show code hints longer than the box.

@MarcelGerber
Member

Very nice! I also love the marquee effect

@MarcelGerber MarcelGerber commented on an outdated diff Jun 2, 2015
src/extensions/default/PrefsCodeHints/main.js
+ return $hintItem;
+ });
+ }
+
+ /**
+ * @constructor
+ */
+ function PrefsCodeHints() {
+ this.ctxInfo = null;
+
+ // Add all the preferences defined except the excluded ones.
+ var preferences = PreferencesManager.getAllPreferences(),
+ preference;
+ Object.keys(preferences).forEach(function (pref) {
+ preference = preferences[pref];
+ if (preference.excluded) {
@MarcelGerber
MarcelGerber Jun 2, 2015 Member

Maybe something more distinct, like excludeFromHints?

@MarcelGerber MarcelGerber and 1 other commented on an outdated diff Jun 2, 2015
src/brackets.js
@@ -437,7 +438,9 @@ define(function (require, exports, module) {
}
if (brackets.platform === "win" && !brackets.inBrowser) {
- PreferencesManager.definePreference("_windowsScrollFix", "boolean", true).on("change", enableOrDisableWinScrollFix);
+ PreferencesManager.definePreference("_windowsScrollFix", "boolean", true, {
+ description: Strings.DESCRIPTION_WINDOW_SCROLL_FIX
@MarcelGerber
MarcelGerber Jun 2, 2015 Member

You could just exclude that pref (and remove the corresponding string) 👍

@sprintr
sprintr Jun 2, 2015 Contributor

@MarcelGerber Cool. I'll do that. There is another pref that is not being used defaultExtension, we can exclude that one too.

@sprintr
Contributor
sprintr commented Jun 2, 2015

@MarcelGerber Removed the width of hint object and it seems we don't need marquee anymore. Can you check it.

@abose
Contributor
abose commented Jun 2, 2015

@sprintr some width jumps are there with just width. If you change it to min-width it will work i think .

@sprintr
Contributor
sprintr commented Jun 2, 2015

@abose Yeah, it works that way. It only happens where no description is available.

@abose
Contributor
abose commented Jun 2, 2015

Verified. All unit tests passing. Merging.

@abose abose merged commit 0b17978 into adobe:master Jun 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abose
Contributor
abose commented Jun 2, 2015

@sprintr Thanks for this awesome contribution 🎉

@sprintr
Contributor
sprintr commented Jun 2, 2015

@abose Awesome!

@sprintr sprintr deleted the sprintr:pref-code-hints branch Jun 2, 2015
@busykai
Member
busykai commented Jun 2, 2015

@sprintr, great! this is really helpful!

@nethip
Contributor
nethip commented Jun 8, 2015

Great collaboration work on this PR! @sprintr @abose Does it make sense to update our existing documentation with the updated usage of definePreference() and also share this as an example?

@abose
Contributor
abose commented Jun 9, 2015

Issue #11200 tracks the doc changes required and hints for the new preferences API
@ryanstewart is reviewing the code hints being shown for preferences.

@nethip
Contributor
nethip commented Jun 10, 2015

🆒

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