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 all commits
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
152 changes: 122 additions & 30 deletions src/editor/CodeHintManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,56 @@ define(function (require, exports, module) {
KeyEvent = require("utils/KeyEvent");


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

/** Comparator to sort providers based on their specificity */
function _providerSort(a, b) {
return b.specificity - a.specificity;
}

/**
* If there is any provider for all modes, then add it to each individual
* mode providers list and re-sort them based on their specificity.
*/
function _mergeAllModeToIndividualMode() {
var allModeProviders = [];
if (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.

All other references to hintProviders is done as hintProviders[mode], but "all" is referenced as hintProviders.all . Since the "all" list is created the same as the other modes, I think it would be easier to understand the code is you used hintProviders["all"] instead of hintProviders.all .

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 with you for clarity, but there will be a JSLint error saying ["all"] is better written in dot notation. And I can't find any JSLint option to suppress this type of errors.

allModeProviders = hintProviders.all;

// Remove "all" mode list since we don't need it any more after
// merging them to each individual mode provider lists.
delete hintProviders.all;

$.each(hintProviders, function (key, value) {
if (hintProviders[key]) {
hintProviders[key] = hintProviders[key].concat(allModeProviders);
hintProviders[key].sort(_providerSort);
}
});
}
}

/**
* Return the array of hint providers for the given mode.
* If this is called for the first time, then we check if any provider wants to show
* hints on all modes. If there is any, then we merge it into each individual
* mode provider list.
*
* @param {(string|Object<name: string>)} mode
* @return {Array.<{provider: Object, modes: Array.<string>, specificity: number}>}
*/
function _getEnabledHintProviders(mode) {
if (hintProviders.all) {
_mergeAllModeToIndividualMode();
}

var modeName = (typeof mode === "string") ? mode : mode.name;
return hintProviders[modeName] || [];
}
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 @@ -140,7 +184,7 @@ define(function (require, exports, module) {

if (count === 0) {
this.close();
} else {
} else if (this.currentProvider.wantInitialSelection()) {
// Select the first item in the list
this.setSelectedIndex(0);
}
Expand All @@ -149,7 +193,7 @@ define(function (require, exports, module) {

/**
* Selects the item in the hint list specified by index
* @param {Number} index
* @param {number} index
*/
CodeHintList.prototype.setSelectedIndex = function (index) {
var items = this.$hintMenu.find("li");
Expand Down Expand Up @@ -199,7 +243,13 @@ define(function (require, exports, module) {

// Up arrow, down arrow and enter key are always handled here
if (event.type !== "keypress") {

// If we don't have a selection in the list, then just update the list and
// show it at the new location for Return and Tab keys.
if (this.selectedIndex === -1 && (keyCode === KeyEvent.DOM_VK_RETURN || keyCode === KeyEvent.DOM_VK_TAB)) {
this.updateQueryAndList();
return;
}

if (keyCode === KeyEvent.DOM_VK_RETURN || keyCode === KeyEvent.DOM_VK_TAB ||
keyCode === KeyEvent.DOM_VK_UP || keyCode === KeyEvent.DOM_VK_DOWN ||
keyCode === KeyEvent.DOM_VK_PAGE_UP || keyCode === KeyEvent.DOM_VK_PAGE_DOWN) {
Expand Down Expand Up @@ -239,7 +289,7 @@ define(function (require, exports, module) {

/**
* Return true if the CodeHintList is open.
* @return {Boolean}
* @return {boolean}
*/
CodeHintList.prototype.isOpen = function () {
// We don't get a notification when the dropdown closes. The best
Expand All @@ -257,20 +307,33 @@ 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 = [];

if (triggeredHintProviders.length > 0) {
enabledProviders = triggeredHintProviders;
} else {
enabledProviders = _getEnabledHintProviders(mode);
}
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.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.editor = editor;
Menus.closeAll();

this.currentProvider = null;
$.each(hintProviders, function (index, item) {
var query = item.getQueryInfo(self.editor, self.editor.getCursorPos());
$.each(enabledProviders, function (index, item) {
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;
}
});

if (!this.currentProvider) {
return;
}
Expand All @@ -284,7 +347,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,14 +374,13 @@ define(function (require, exports, module) {
this.$hintMenu.remove();
if (hintList === this) {
hintList = null;
shouldShowHintsOnChange = false;
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()


/**
* Computes top left location for hint list so that the list is not clipped by the window
* @return {Object.<left: Number, top: Number> }
* @return {Object.<left: number, top: number> }
*/
CodeHintList.prototype.calcHintListLocation = function () {
var cursor = this.editor._codeMirror.cursorCoords(),
Expand Down Expand Up @@ -386,24 +448,32 @@ define(function (require, exports, module) {
* @param {KeyboardEvent} event
*/
function handleKeyEvent(editor, event) {
var provider = null;
var mode = editor.getModeForSelection(),
enabledProviders = [],
key;

// Check for Control+Space
if (event.type === "keydown" && event.keyCode === 32 && event.ctrlKey) {
triggeredHintProviders = [];
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();
enabledProviders = _getEnabledHintProviders(mode);

triggeredHintProviders = [];
if (enabledProviders.length > 0) {
key = String.fromCharCode(event.charCode);
// Check if any provider wants to start showing hints on this key.
$.each(enabledProviders, function (index, item) {
if (item.provider.shouldShowHintsOnKey(key)) {
triggeredHintProviders.push(item);
}
});

if (triggeredHintProviders.length > 0) {
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 +487,7 @@ define(function (require, exports, module) {
*
*/
function handleChange(editor) {
if (shouldShowHintsOnChange && keyDownEditor === editor) {
shouldShowHintsOnChange = false;
if (triggeredHintProviders.length > 0 && keyDownEditor === editor) {
keyDownEditor = null;
showHint(editor);
}
Expand All @@ -435,7 +504,8 @@ define(function (require, exports, module) {
* @param {Object.< getQueryInfo: function(editor, cursor),
* search: function(string),
* handleSelect: function(string, Editor, cursor),
* shouldShowHintsOnKey: function(string)>}
* shouldShowHintsOnKey: function(string),
* wantInitialSelection: function()>}
*
* Parameter Details:
* - getQueryInfo - examines cursor location of editor and returns an object representing
Expand All @@ -447,9 +517,31 @@ define(function (require, exports, module) {
* 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.
* - wantInitialSelection - return true if the provider wants to select the first hint item by default.
*
* @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 {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, specificity) {
var providerObj = { provider: providerInfo,
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
20 changes: 18 additions & 2 deletions src/extensions/default/HTMLCodeHints/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,14 @@ define(function (require, exports, module) {
return key === "<";
};

/**
* Check whether to select the first item in the list by default
* @return {boolean} return true to highlight the first item.
*/
TagHints.prototype.wantInitialSelection = function () {
return true;
};

/**
* @constructor
*/
Expand Down Expand Up @@ -478,10 +486,18 @@ define(function (require, exports, module) {
return (key === " " || key === "'" || key === "\"" || key === "=");
};

/**
* Check whether to select the first item in the list by default
* @return {boolean} return true to highlight the first item.
*/
AttrHints.prototype.wantInitialSelection = function () {
return true;
};

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