Code hints are sometimes displayed when they shouldn't be

[joelrbrandt]

Big thanks to @iwehrman for helping understand and suggest solutions for this bug.

Code hints are sometimes displayed when they shouldn't be. For example, with the EWF extension installed, FontHints are sometimes displayed in CSS files when the extension doesn't intend for this to happen.

Steps to reproduce:

1. Install EWF extension and open a CSS file with a font-family rule.
2. Put cursor at the end of a font name, but before a comma.
3. Press space.

Expected:
List doesn't show up

Actual:
List shows up.

Cause:
The problem seems to be related to the decoupling of all steps in the code hint process (shouldShowHintsOnKey, getQueryInfo, and search functions that providers implement)

In this case, the AttrHints provider (which is supposed to provide attribute hints for HTML) says it will provide hints on a "space" key. Then, CodeHintManager asks every single provider for query info. AttrHints returns nothing (which is correct) but FontHint can offer hints (and doesn't know space was typed) so it does. This causes the hint list to appear.

Possible solutions:

1. CodeHintManager could pass context (e.g. editor, mode) to providers when calling "shouldShowHintsOnKey" so they can make a better decision.
2. CodeHintManager could only call getQueryInfo on providers that said yes to "shouldShowHintsOnKey"
3. Providers could be registered on a per-mode basis and then shouldShowHintsOnKey would only get called for providers that are registered for the current mode (related to Jason sanjose/js test driver #1 / may lessen the need for Jason sanjose/js test driver #1)
4. possible that "shouldShowHintsOnKey" should be renamed to something like "wantToProvideHintsOnKey" to make its intent more clear to implementers of providers.

Other suggestions:

1. Right now, multiple hint providers can say they want to show hints, but only one (first one registered to return query info) ends up providing hints. This means that it's impossible to build extension that override our built in providers. Could fix by making it the case that there's only one (or zero) provider per mode. But then we'd need to merge all of the HTML providers into one.
2. Division of labor between "getQueryInfo" and "search" seems strange because it seems to be the case that getQueryInfo and search are always called as a pair (once a provider is chosen). Instead, it might make sense to have one function that gets called the first time (i.e. when selecting a provider and displaying the list for the first time) and one function that gets called each time the user types (i.e. updates the query/filters the list).

[njx]

Yes, I've definitely run into this before--the way our API works right now is confusing.

[RaymondLim]

@joelrbrandt I would say we should have solution 4 to rename "shouldShowHintsOnKey" which is misunderstood by most extension developers. You're not alone to bring this up. See Andre's questions in CSS hinting. Note that your solution 4 does not fix this issue. It just tries to solve the confusion of this api.

"shouldShowHintsOnKey" is intended to let code hints providers to auto trigger hint list on a specific key rather than having users to explicitly press Ctrl-Space to show code hints. So it's not the real culprit of this specific issue except its name is a misnomer and misunderstood by most. It is not intended to claim as exclusive hint provider after a specific key is hit by the user.

Assuming we don't have this api and users always have to press Ctrl-Space to show hints, the issue you described here still shows up because your getQueryInfo is providing a queryStr (instead of a null queryStr) to CodeHintManger to show the relevant hint list. You may think it is the responsibility of CodeHintManager to provide the context of the current selection or cursor position. Actually, it is not. For tag hints and attribute hints we have HTMLUtils.js to provide the html context with getTagInfo api, but we don't have a similar one for css context. So the correct solution for this issue is to create an general api in CSSUtils.js that provides the CSS context info to EWF and CSS Hints provider. Once we have that API, then it is the responsibility of individual hint provider to return the correct queryStr based on the context. And here you should not make the decision of showing your code hints based on the last character user just typed. In your case, you would like to show hints only if you have font-family property name followed by a : and any other font names followed by a comma. You don't even have to care whether a space is immediately before the cursor since it is optional.

Current implementation of EWF and Andre's CSS hinting both provide a non-null query string when a specific css property name is found. This is not the right solution and if you put both extensions in Brackets, the second one won't work anymore depending on which one gets registered first. So both extensions need to be updated to use the new general api that I'm proposing so that only one provider can claim to show code hints at a time, but not both at the same time.

[joelrbrandt]

@RaymondLim Thanks for the thoughtful reply.

@iwehrman and I submitted pull #2261 because we found a related and slightly more nuanced bug. It's a bit difficult to explain, so if the next few paragraphs below doesn't make sense, let me know. :-)

We're working on building code autocomplete for JavaScript. The bug we're experiencing is that the code hint dialog does not close if the user decides not to autocomplete, and instead types the whole (possibly new) identifier and then hits space.

Consider a JavaScript file that already contains the identifier foo in scope, and suppose the programmer is adding a new identifier f. When he types "f", the autocomplete will come up with foo in the list, as intended. But, he only wants to type "f" so he just hits space.

Right now, before closing, the CodeHintManager calls shouldShowHintsOnKey (because the user hit space) and our code completion provider says "no" (because the user just hit a space, which is not something we want to trigger on). But, the CodeHintManager also calls getQueryInfo on all the providers before closing the dialog. With JavaScript, its always possible to produce a list of suggestions when the cursor is surrounded by whitespace. Our code hint provider has no way of knowing that the user didn't explicitly ask to see a code hint dialog. (I.e. the same call would get made if the user hit ctrl-space). So, we return a list. This causes the dialog to stay up, which is not the desired behavior.

In short, it seems to me that code hint providers need a way of knowing what recent action a user took in order to decide if they should return query results. Right now, the only place that the user action is passed to the provider is in showHintsOnKey. Thus, it seems like we need to do one of two things:

1. In CodeHintManager, respect the returned value from showHintsOnKey and not call a provider's getQueryInfo method if it said it didn't want to provide hints. This is what pull Only query hint providers that have offered to show hints #2261 implements. This also has the added benefit that it makes the getQueryInfo calling slightly more efficient.
2. Pass user action information to getQueryInfo just as we do with showHintsOnKey. To me, this seems less desirable for the reasons you outlined.

Thoughts?

On a related note, I'm aware that there's a bug in the EWF extension. If this request is merged, we'll need a more intelligent shouldShowHintsOnKey function in that extension. Right now it is (erroneously) relying on the fact that AttrHints always says that hints should be shown, and EWF gets called as a result.

[RaymondLim]

@joelrbrandt Thanks for your further explanations!

Yes, there is an unintented side effect of shouldShowHintsOnKey api. And my initial review of @iwehrman's pull request confirms that he really fixes the side effect and still maintains the original intention of shouldShowHintsOnKey api to give every provider a chance to show hints on the same key.

And I'll be logging separate issues in EWF for those I mentioned in the previous comments.

[njx]

Marking sprint 18 since we're working on this now anyway--maybe this is how we can track the work item of revamping the code hint API?

[pthiess]

Reviewed, in sprint already..

[joelrbrandt]

Confirming fixed after pull #2387. Closing.

EWF still needs to be updated to fix the particular issue, but we now have the API we need in order to be able to fix it. I've filed a bug (assigned to me) for this: adobe/brackets-edge-web-fonts#56