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

registerHintProvider api change in Code Hint Manager #2279

Closed
wants to merge 6 commits into from
Closed

registerHintProvider api change in Code Hint Manager #2279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c2347c4
Implement registerHintProvider api with extra parameters suggested by…
RaymondLim Dec 4, 2012
676ca72
Putting back shouldShowHintsOnKey api since JS code hinting Ian is wo…
RaymondLim Dec 5, 2012
53c1d94
Correct some mistakes in jsDoc.
RaymondLim Dec 6, 2012
ea97d4e
Add one more code hints extension api to let the provider decide on t…
RaymondLim Dec 6, 2012
c099f5e
Use a local variable to avoid modifying the parameter.
RaymondLim Dec 6, 2012
ba32cb5
modeName sounds better.
RaymondLim Dec 6, 2012
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
118 changes: 91 additions & 27 deletions src/editor/CodeHintManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,23 @@ define(function (require, exports, module) {
KeyEvent = require("utils/KeyEvent");


var hintProviders = [],
var hintProviders = {},
hintList,
shouldShowHintsOnChange = false,
triggeredKey = null,
keyDownEditor;

/** Comparator to sort providers based on their specificity */
function _providerSort(a, b) {
if (a.specificity === b.specificity) {
return 0;
}
if (a.specificity > b.specificity) {
return -1;
}
if (a.specificity < b.specificity) {
return 1;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be much simpler to use:

  return (b.specificity - a.specificity);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion!

Yes, simpler in code, but not so simple in logic. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You should avoid changing the value (and possibly the type) of parameters, so I think a new var should be used here:

    var modeName = (typeof mode === "string") ? mode : mode.name;
    return hintProviders[modeName] || [];


/**
* @constructor
Expand Down Expand Up @@ -257,20 +269,47 @@ define(function (require, exports, module) {
* @param {Editor} editor
*/
CodeHintList.prototype.open = function (editor) {
var self = this;
this.editor = editor;
var self = this,
mode = editor.getModeForSelection(),
enabledProviders = [];

mode = (typeof mode === "string") ? mode : mode.name;
enabledProviders = hintProviders[mode];
this.editor = editor;
Menus.closeAll();

// If we have any providers for "all" mode, then append it to enabled
// porviders list and sort them based on their specificity again.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: porviders

if (enabledProviders && hintProviders.all) {
enabledProviders = enabledProviders.concat(hintProviders.all);
enabledProviders.sort(_providerSort);
} else if (hintProviders.all) {
enabledProviders = hintProviders.all;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Both conditions check the hintProviders.all flag, so this should be checked only once. This also makes code more clear:

    if (hintProviders.all) {
        if (enabledProviders.length > 0) {
            enabledProviders = enabledProviders.concat(hintProviders.all);
            enabledProviders.sort(_providerSort);
        } else if (hintProviders.all) {
            enabledProviders = hintProviders.all;
        }
    }


if (!enabledProviders) {
Copy link
Contributor

Choose a reason for hiding this comment

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

enabledProviders is a variable declared as an empty array, so it will always be truthy, I think this should check:
if (enabledProviders.length === 0) {

return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Other places in this function return a boolean value, so I think this needs to return false; Nevermind, that's a $.foreach().

}

this.currentProvider = null;
$.each(hintProviders, function (index, item) {
var query = item.getQueryInfo(self.editor, self.editor.getCursorPos());
$.each(enabledProviders, function (index, item) {
// If we have a triggered key, then skip all the providers that
// do not want to be invoked by the triggered key.
if (triggeredKey && item.provider.triggeredKeys &&
item.provider.triggeredKeys.indexOf(triggeredKey) === -1) {
return true;
}

var query = item.provider.getQueryInfo(self.editor, self.editor.getCursorPos());
if (query.queryStr !== null) {
self.query = query;
self.currentProvider = item;
self.currentProvider = item.provider;
return false;
}
});

triggeredKey = null;

if (!this.currentProvider) {
return;
}
Expand All @@ -284,7 +323,7 @@ define(function (require, exports, module) {
var hintPos = this.calcHintListLocation();

this.$hintMenu.addClass("open")
.css({"left": hintPos.left, "top": hintPos.top});
.css({"left": hintPos.left, "top": hintPos.top});
this.opened = true;

PopUpManager.addPopUp(this.$hintMenu,
Expand All @@ -311,7 +350,7 @@ define(function (require, exports, module) {
this.$hintMenu.remove();
if (hintList === this) {
hintList = null;
shouldShowHintsOnChange = false;
triggeredKey = null;
keyDownEditor = null;
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Other places in this function return a boolean value, so I think the end of function needs to return false; Nevermind, that's a $.foreach()

Expand Down Expand Up @@ -386,24 +425,29 @@ define(function (require, exports, module) {
* @param {KeyboardEvent} event
*/
function handleKeyEvent(editor, event) {
var provider = null;
var provider = null,
mode;

// Check for Control+Space
if (event.type === "keydown" && event.keyCode === 32 && event.ctrlKey) {
showHint(editor);
event.preventDefault();
} else if (event.type === "keypress") {
// Check if any provider wants to show hints on this key.
$.each(hintProviders, function (index, item) {
if (item.shouldShowHintsOnKey(String.fromCharCode(event.charCode))) {
provider = item;
return false;
mode = editor.getModeForSelection();
mode = (typeof mode === "string") ? mode : mode.name;

if (hintProviders[mode]) {
// Check if any provider wants to show hints on this key.
$.each(hintProviders[mode], function (index, item) {
if (item.triggerKeys && item.triggerKeys.indexOf(String.fromCharCode(event.charCode)) !== -1) {
triggeredKey = String.fromCharCode(event.charCode);
return false;
}
});

if (triggeredKey) {
keyDownEditor = editor;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the hint list ought to be closed when there are no triggered hint providers. I see the following scenario with the in-progress JavaScript provider.

  1. The hint list is open after typing the last character of a keyword (e.g., "var");
  2. A space is typed, and the the provider's shouldShowHintsOnKey method is queried;
  3. The provider's shouldShowHintsOnKey method returns false, and so triggeredHintProviders is empty;
  4. The hint list remains open and is updated by querying the "current" (but now outdated) provider's getQueryInfo method;
  5. Many new hints are returned because, from this position, in which the cursor is surrounded by whitespace, many tokens constitute valid hints.

Instead, it seems like the list should be closed when shouldShowHintsOnKey returns false and should only be opened again when that method returns true and provides a non-empty list of hints. Although the provider could attempt to detect this from within getQueryInfo by observing that the cursor is surrounded by whitespace, this would make it tough to handle explicit requests (via ctrl-space) for the hint list. Also note that handleSelect is not called in this scenario, so that method's return value doesn't have a chance to affect the list's status.

Or is there another way I haven't considered for the JavaScript provider to handle this scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to follow-up after an offline discussion, I think the simplest change that would solve the problem from the perspective of the JS Hint provider would be to add the last-pressed key as an extra parameter to getQueryInfo. And perhaps that parameter could be null if the hint list was explicitly requested. That would allow the provider to determine whether or not to present a full list of suggestions (on, e.g., ctrl-space) or an empty list (on, e.g., space).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that additional parameter to getQueryInfo is a practical solution to your JS hinting. I can think of the following three different cases that CHM calls getQueryInfo of the current provider.

  1. When an item is selected by a mouse click and handleSelect method return false to keep the hint list open (used in url hinting). We won't have a last-pressed key in this case.
  2. Ctrl-Space is pressed. We don't provide the last-pressed key since it is not a single key pressed.
  3. Any key that generates a character in the editor. We provide the last-pressed key as the extra param of the getQueryInfo.

});

shouldShowHintsOnChange = !!provider;
if (shouldShowHintsOnChange) {
keyDownEditor = editor;
}
}

Expand All @@ -417,8 +461,7 @@ define(function (require, exports, module) {
*
*/
function handleChange(editor) {
if (shouldShowHintsOnChange && keyDownEditor === editor) {
shouldShowHintsOnChange = false;
if (triggeredKey && keyDownEditor === editor) {
keyDownEditor = null;
showHint(editor);
}
Expand All @@ -434,8 +477,7 @@ define(function (require, exports, module) {
*
* @param {Object.< getQueryInfo: function(editor, cursor),
* search: function(string),
* handleSelect: function(string, Editor, cursor),
* shouldShowHintsOnKey: function(string)>}
* handleSelect: function(string, Editor, cursor)>}
*
* Parameter Details:
* - getQueryInfo - examines cursor location of editor and returns an object representing
Expand All @@ -446,10 +488,32 @@ define(function (require, exports, module) {
* - handleSelect - takes a completion string and inserts it into the editor near the cursor
* position. It should return true by default to close the hint list, but if the code hint provider
* can return false if it wants to keep the hint list open and continue with a updated list.
* - shouldShowHintsOnKey - inspects the char code and returns true if it wants to show code hints on that key.
*
* @param {Array.<string>} modes An array of mode strings in which the provider can show code hints or "all"
* if it can show code hints in any mode.
* @param {!Array.<string>} triggerKeys An array of all the keys that the provider can be triggered to show hints.
* @param {number} specificity A positive number to indicate the priority of the provider. The larger the number,
* the higher priority the provider has. Zero if it has the lowest priority in displaying its code hints.
Copy link
Contributor

Choose a reason for hiding this comment

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

This API change might break extensions that use it. Jochen's Spell Checker (https://github.com/couzteau/SpellCheck) is the only one I am aware of that uses it, but this change should be announced on the brackets-dev forum in case there are others.

*/
function registerHintProvider(providerInfo) {
hintProviders.push(providerInfo);
function registerHintProvider(providerInfo, modes, triggerKeys, specificity) {
var providerObj = { provider: providerInfo,
triggerKeys: triggerKeys || [],
specificity: specificity || 0 };

if (modes) {
modes.forEach(function (mode) {
if (mode) {
if (!hintProviders[mode]) {
hintProviders[mode] = [];
}
hintProviders[mode].push(providerObj);

if (hintProviders[mode].length > 1) {
hintProviders[mode].sort(_providerSort);
}
}
});
}
}

/**
Expand Down
22 changes: 2 additions & 20 deletions src/extensions/default/HTMLCodeHints/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,15 +120,6 @@ define(function (require, exports, module) {
return true;
};

/**
* Check whether to show hints on a specific key.
* @param {string} key -- the character for the key user just presses.
* @return {boolean} return true/false to indicate whether hinting should be triggered by this key.
*/
TagHints.prototype.shouldShowHintsOnKey = function (key) {
return key === "<";
};

/**
* @constructor
*/
Expand Down Expand Up @@ -469,19 +460,10 @@ define(function (require, exports, module) {
return result;
};

/**
* Check whether to show hints on a specific key.
* @param {string} key -- the character for the key user just presses.
* @return {boolean} return true/false to indicate whether hinting should be triggered by this key.
*/
AttrHints.prototype.shouldShowHintsOnKey = function (key) {
return (key === " " || key === "'" || key === "\"" || key === "=");
};

var tagHints = new TagHints();
var attrHints = new AttrHints();
CodeHintManager.registerHintProvider(tagHints);
CodeHintManager.registerHintProvider(attrHints);
CodeHintManager.registerHintProvider(tagHints, ["html"], ["<"], 0);
CodeHintManager.registerHintProvider(attrHints, ["html"], [" ", "'", "\"", "="], 0);

// For unit testing
exports.tagHintProvider = tagHints;
Expand Down