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

Conversation

RaymondLim
Copy link
Contributor

Implement registerHintProvider api with extra parameters suggested by Glenn. This change also makes the problematic shouldShowHintsOnKey api no longer needed. It also resolves the conflicts between two or more providers by allowing each provider to register its specificity.

The new api has two extra parameters -- modes for an array of supported modes, and a number to set the priority of the provider when there are more than one provider to show code hints in the same context.

We also add a new api for code hints providers. The new api, wantInitialSelection, let the providers to decide whether to select the first hint by default or not.

… Glenn. This change also makes the problematic shouldShowHintsOnKey api no longer needed. It also resolves the conflicts between two or more providers by allowing each provider to register its specificity.
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. :)

*/
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.

@redmunds
Copy link
Contributor

redmunds commented Dec 6, 2012

Done with initial review.

});

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.

@peterflynn
Copy link
Member

@iwehrman is this obsolete now that #2387 has landed?

@iwehrman
Copy link
Contributor

Yep.

@peterflynn
Copy link
Member

Closing per above

@peterflynn peterflynn closed this Dec 22, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants