Code Hint : Add warning to docs that TagHints.prototype.getQueryInfo gets called multiple times when starting query

[joelrbrandt]

UPDATE: Based on the discussion below, we've decided that fixing this bug requires updating the API with a warning about the potential of multiple calls in a short amount of time (described below), and does not require any code changes.

---

TagHints.prototype.getQueryInfo gets called three times in rapid succession when I type a "<" in an HTML file. (I.e. when I start a new tag and have an empty query.)

I suspect that all the providers are getting called multiple times.

When letters are added to the query (or removed) it seems to only get called once as intended.

There's a chance this is related to #1411, but I doubt it. :-)

[pthiess]

Reviewed - @RaymondLim

[joelrbrandt]

@RaymondLim -- PeterF pointed out that if you set a breakpoint, you may not see the multiple calls. This may be because the breakpoint is hit before the keyup event gets generated, and then you switch windows. So, the keyup never happens.

Consider trying to debug with console.log statements rather than breakpoints. :-(

[DennisKehrig]

Seems like one for keydown, one for keyup (<) and another one for keyup (Shift key)

[DennisKehrig]

To me this seems legitimate. The keydown opens the list, the other two keyup events are handled like any later keyup events in that they potentially change the query (i.e. by moving the cursor). So in both cases the current provider is asked for what the resulting query is.

If there's anything to optimize then it should be changing updateList to only do something if the query actually changed from before. Even if you just press and release Shift, without any other key, updateList is called. I think it's appropriate to check whether the query changed, just to be on the save side, but if not, why do more?

[pthiess]

Raymond please verify.

[joelrbrandt]

After review, we've decided that the behavior is correct.

However, we should document in the API that this method may get called multiple times in quick succession. So, if the user of the API needs to do an expensive computation, he should cache the result and return the same thing if he receives the same query.

Fixing this bug requires updating the API with this warning, and does not require any code changes.

[peterflynn]

One other thing feels weird about this to me, though: when the code hint list is triggered (via keystroke), we call shouldShowHintsOnKey() on all providers to see which one is "interested." But then a moment later when the list UI is actually created, we call getQueryInfo() on all providers again -- we completely ignore which provider was "chosen" via shouldShowHintsOnKey().

I would expect that if a provider returns false from shouldShowHintsOnKey(), its getQueryInfo() would not be called and it could not ultimately be chosen as the provider that takes over the list.

The way it works now seems like a bit of a burden on providers, since all the logic that checks if the editor mode is one you support, etc. has to be run in both of those functions rather than just the first one...

[DennisKehrig]

Hey @peterflynn, @joelrbrandt, @RaymondLim,
since this is a medium priority bug, before I do anything about it: does anyone currently feel responsible for solving this?

[joelrbrandt]

@DennisKehrig This will get fixed during this sprint as part of our code hint manager improvement story.

@iwehrman -- I'm pretty sure this is already on your radar, but we should be aware of this as we're rethinking the API.

@peterflynn @RaymondLim Assigning to myself for tracking, and adding the sprint 18 milestone.

[joelrbrandt]

Confirming closed after pull #2387

getHints (which is the new name for getQueryInfo) is only called at most once per provider on each keystroke (including opening the dialog).