Only query hint providers that have offered to show hints #2261
Conversation
|
@iwehrman I just posted my comments in #2258. The issue is not really in shouldShowHintsOnKey api, and getQueryInfo api should be called for every provider so that each provider has a chance to decide whether it should show hints or not. The real issue is the lacking of an api to provide the CSS context info and Joel has implemented his version as getFontTokenAtCursor in EWF/cssFontParser.js. I would say we need a general api in CSSUtils.js and once we have that, you need to switch to use the new api that provides better context info. |
|
@RaymondLim Thanks for the reply! Please see comment in #2258 (comment) |
ghost
assigned
| keyDownEditor = null; | ||
| showHint(editor); | ||
| } else { | ||
| if (hintList) { | ||
| hintList.close(); |
There was a problem hiding this comment.
Why you would like to close the hintlist here? If you do that, then as soon as you type the very first letter the hint list will get closed. Try with <h in a html page and you will see what I mean.
There was a problem hiding this comment.
I can only guess why, but in short t'is to emphasize shouldShowHintsOnKey. I just tried this code with both html and css hinting. With this implementation the hints disappear whenever you press a key, that is not handled in shouldShowHintsOnKey (in your example the '<' will trigger+show html hints, but the 'h' will return false in SSHOK) - for css hinting this kind-of works, because I (naive thinking) added the whole alphabet a-z in SSHOK to return true, thus the hints stay visible.
To get this working one must change shouldShowHintsOnKey to include all keys (e.g. a-z) when to show hints in every provider - though I'm afraid this wouldn't solve the problem since every provider would return true and again every provider calls getQueryInfo
(as far as I understand this change.)
Regards, André
There was a problem hiding this comment.
Yes, the original intent was to use shouldShowHintsOnKey to determine whether to open or close the hint list. But indeed this breaks the HTML hinting and isn't necessary for the JS hinting that I'm working on separately. I will amend the pull request.
|
@RaymondLim Thanks for the review! I've updated the pull request. |
…y handleKeyEvent, as is the case with some unit tests
|
Thanks for making all these changes, but sorry to say that we're making another change in registerHintProvider api (pull request #2279) and we no longer need this pull request. |
|
Closing this pull request as we're working on an alternate solution. |
@RaymondLim @gruehle @joelrbrandt
Partially addresses #2258 by way of solution #2. The shouldShowHintsOnChange flag, which indicated that some provider was willing to provide hints, is removed in favor of a list, enabledHintProviders, that is populated with hint providers for which shouldShowHintsOnKey returns true. Consequently, only those providers that have offered to provide hints are queried. If the hint list is requested explicitly via Ctrl-Space, the individual providers are not initially queried with shouldShowHintsOnKey, and so an additional flag queryAllProviders indicates in this case that hints should be elicited from all providers instead of just those that are enabled.