Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Simple code hints preferences to enable/disable individual/all code hints providers. #8272

Merged
merged 3 commits into from
Jul 19, 2014
Merged

Simple code hints preferences to enable/disable individual/all code hints providers. #8272

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52e49ab
Provide simple code hints preferences to enable/disable individual/al…
RaymondLim Jun 27, 2014
1e4d8e1
Explicitly control JS Hints provider with preferences for performance…
RaymondLim Jul 16, 2014
07f241b
Making some changes for review feedbacks.
RaymondLim Jul 18, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 26 additions & 9 deletions src/editor/CodeHintManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,18 +242,23 @@ define(function (require, exports, module) {
CodeHintList = require("editor/CodeHintList").CodeHintList,
PreferencesManager = require("preferences/PreferencesManager");

var hintProviders = { "all" : [] },
lastChar = null,
sessionProvider = null,
sessionEditor = null,
hintList = null,
deferredHints = null,
keyDownEditor = null;
var hintProviders = { "all" : [] },
lastChar = null,
sessionProvider = null,
sessionEditor = null,
hintList = null,
deferredHints = null,
keyDownEditor = null,
codeHintsEnabled = true;


PreferencesManager.definePreference("showCodeHints", "boolean", codeHintsEnabled);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 3rd param to definePreference is the default value, so this should be hard-coded to true.

PreferencesManager.definePreference("insertHintOnTab", "boolean", false);


PreferencesManager.on("change", "showCodeHints", function () {
codeHintsEnabled = PreferencesManager.get("showCodeHints");
});

/**
* Comparator to sort providers from high to low priority
*/
Expand Down Expand Up @@ -347,7 +352,15 @@ define(function (require, exports, module) {
* @return {?Array.<{provider: Object, priority: number}>}
*/
function _getProvidersForLanguageId(languageId) {
return hintProviders[languageId] || hintProviders.all;
var providers = hintProviders[languageId] || hintProviders.all;

// Exclude providers that are explicitly disabled in the preferences.
// All code hint providers that do not have their constructor
// names listed in the preferences are enabled by default.
return providers.filter(function (provider) {
var prefKey = "codehint." + provider.provider.constructor.name;
return PreferencesManager.get(prefKey) !== false;
});
}

var _beginSession;
Expand Down Expand Up @@ -451,6 +464,10 @@ define(function (require, exports, module) {
* @param {Editor} editor
*/
_beginSession = function (editor) {
if (!codeHintsEnabled) {
return;
}

// Don't start a session if we have a multiple selection.
if (editor.getSelections().length > 1) {
return;
Expand Down
37 changes: 30 additions & 7 deletions src/extensions/default/JavaScriptCodeHints/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,20 +48,39 @@ define(function (require, exports, module) {
Session = require("Session"),
Acorn = require("thirdparty/acorn/acorn");

var session = null, // object that encapsulates the current session state
cachedCursor = null, // last cursor of the current hinting session
cachedHints = null, // sorted hints for the current hinting session
cachedType = null, // describes the lookup type and the object context
cachedToken = null, // the token used in the current hinting session
matcher = null, // string matcher for hints
ignoreChange; // can ignore next "change" event if true;
var session = null, // object that encapsulates the current session state
cachedCursor = null, // last cursor of the current hinting session
cachedHints = null, // sorted hints for the current hinting session
cachedType = null, // describes the lookup type and the object context
cachedToken = null, // the token used in the current hinting session
matcher = null, // string matcher for hints
jsHintsEnabled = true, // preference setting to enable/disable the hint session
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code never gets the preferences to initialize jsHintsEnabled. This only gets set correctly when pref is changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we're initializing the default value regardless of whether the prefs are available or not. This is why I use it on the 3rd param to definePreference on line 68. But I can see that you want the other way around. I'm fine to hard-code true for the 3rd param to definePreference. Then if we ever want to flip the default, we have to update these two places at the same time.

ignoreChange; // can ignore next "change" event if true;


// Define the defaultExclusions which are files that are known to cause Tern to run out of control.
PreferencesManager.definePreference("jscodehints.defaultExclusions", "array", []);

// This preference controls when Tern will time out when trying to understand files
PreferencesManager.definePreference("jscodehints.inferenceTimeout", "number", 5000);

// This preference controls whether to create a session and process all JS files or not.
PreferencesManager.definePreference("codehint.JSHints", "boolean", jsHintsEnabled);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 3rd param to definePreference is the default value, so this should be hard-coded to true.


PreferencesManager.on("change", "codehint.JSHints", function () {
jsHintsEnabled = PreferencesManager.get("codehint.JSHints");
});

PreferencesManager.on("change", "showCodeHints", function () {
var showHintsEnabled = PreferencesManager.get("showCodeHints");

if (jsHintsEnabled && !showHintsEnabled) {
jsHintsEnabled = showHintsEnabled;
} else if (!jsHintsEnabled && showHintsEnabled) {
jsHintsEnabled = PreferencesManager.get("codehint.JSHints");
}
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is not quite right. jsHintsEnabled needs to be recomputed whenever either of the prefs change, right? Also, if pref is not set (i.e. undefined), then default is true (not false). Something like this:

    function areHintsEnabled() {
        return (PreferencesManager.get("codehint.JSHints") !== false) && (PreferencesManager.get("showCodeHints") !== false); 
    }

    PreferencesManager.on("change", "codehint.JSHints", function () {
        jsHintsEnabled = areHintsEnabled();
    });

    PreferencesManager.on("change", "showCodeHints", function () {
        jsHintsEnabled = areHintsEnabled();
    });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if pref is not set (i.e. undefined), then default is true (not false).

Both of these prefs have default values, so this statement is not correct, but there is no UI so user needs to manually set values, so it's still a good idea to compare that returned value is !== false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redmunds Thanks for the code. You're right that I forgot to consider the cases where either of the prefs is not set.

/**
* Sets the configuration, generally for testing/debugging use.
* Configuration keys are merged into the current configuration.
Expand Down Expand Up @@ -583,6 +602,10 @@ define(function (require, exports, module) {
* @param {Editor} previousEditor - the previous editor
*/
function installEditorListeners(editor, previousEditor) {
if (!jsHintsEnabled) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check this after resetCachedHintContext().

// always clean up cached scope and hint info
resetCachedHintContext();

Expand Down