Autocomplete in reviewer field can miss quickly-typed chars

[GoogleCodeExporter]

Affected Version: 2.1.3

What steps will reproduce the problem?
1. Open a change.
2. Very rapidly type some part of the desired username.

What is the expected output? What do you see instead?

Sometimes, the last character doesn't get sent to the autocompletion, causing 
the drop-down to show inaccurate suggestions if there are other users in the 
system whose names stem from the first N-1 characters of what's been typed.

For example, if I rapidly type "jimmys" intending to get "Jimmy Stewart 
<jimmys@bedfordfalls.us>, the list of completion suggestions might display all 
the "jimmy" users that Gerrit knows about, as if I hadn't typed the final "s".

This can make it dangerous to quickly type a complete username (thinking it 
will autocomplete to exactly the person you want) and press tab and space; 
you'll add the wrong person to the review.

Please provide any additional information below.

Original issue reported on code.google.com by dsand...@google.com on 23 Jun 2010 at 8:40

[GoogleCodeExporter]

Original comment by sop@google.com on 24 Jun 2010 at 9:50

• Changed state: Accepted
• Added labels: Milestone-2.1.4

[GoogleCodeExporter]

Original comment by sop@google.com on 4 Aug 2010 at 11:03

• Removed labels: Milestone-2.1.4

[GoogleCodeExporter]

This appears to be a non serialization issue in the way that SuggestBox is 
designed.  It dispatches every keystroke and forgets them.  The SuggestOracle 
then calls back the SuggestBox when it is ready with an answer.  But since the 
AccountSuggestOracle further dispatches the lookups via RPC, I believe that the 
RPC responses can return out of order (depends on the server response times).  
If an earlier lookup returns after a later one (which is somewhat encouraged by 
the fact that earlier less constrained lookups likely have larger data sets and 
take longer to complete)
then, the earlier lookup results will replace the later results potentially 
displaying invalid results.  

It would be nice if SuggestBox kept track of requests and dropped any replies 
belonging to requests older than the last applied reply's request, but I am not 
sure if the GWT project would endorse such a "fix".

Original comment by mf...@codeaurora.org on 10 Sep 2010 at 11:05

[GoogleCodeExporter]

Apparently it is a known issue.

http://code.google.com/p/google-web-toolkit/issues/detail?id=3341

My RPCSuggestOracle works by dropping old requests instead of limiting the 
amount of requests on the wire.  It seems that most browsers limit the wire 
requests to 2 anyway.  The problem with limiting that I didn't like is that it 
gives priority to the oldest request which is the one that is likely invalid 
now anyway since new characters have been typed.  Additionally, if typing from 
a blank state, each character is likely to limit the search criteria, the 
oldest response might simply take longer than (and even timeout) a simpler 
response created by a longer later query string.  Of course, ideally, older 
requests would be canceled upon receiving a newer response, but I cannot figure 
out how to do that with the current RPC mechanisms used by Gerrit.  The current 
GWT docs suggest that RPC calls can return a (http) Request object, this has a 
cancel method.  But Gerrit's mechanisms do not seem to support this. :(

Original comment by mf...@codeaurora.org on 13 Sep 2010 at 9:13

[GoogleCodeExporter]

Issue 720 has been merged into this issue.

Original comment by sop@google.com on 15 Sep 2010 at 5:49

[GoogleCodeExporter]

To support cancelling an active request, we'd need
to change gwtjsonrpc to permit the Request object
as a return value from an RPC method, rather than
requiring void.

That will look funny on the server side, because
we cannot create a GWT Request object there, but
the signature of the method still requires it as
the return value.  So we'd have to return null.

A short-term workaround might be to do what we do
in PatchScreen.  Have the Gerrit UI object keep a
counter, and don't call onSuggestionsReady if the
current counter doesn't match the current RPC.

See PatchScreen refresh() on line 362.

Original comment by sop@google.com on 15 Sep 2010 at 6:09

[GoogleCodeExporter]

Yeah, I was wondering if we might need to add the Request to the server side.  
If so, I had the same idea, simply return null, not pretty, but it seems better 
than not canceling the unneeded request.

As for the short term workaround, it sounds like it will simply drop anything 
that is not the latest RPC.  That is what my proposed solution (not yet 
uploaded) does, but by simply comparing the request to the latest request (is 
there an advantage to keeping the count?).  So, if that is good solution, I 
will submit it.  If you eventually figure out how to allow us to cancel, I have 
a proposed update to my current solution that will enable that too.

Original comment by mf...@codeaurora.org on 15 Sep 2010 at 6:20

[GoogleCodeExporter]

Fixed by I33b2419cec5a8e9878c2eedb227b402bb9897455

Original comment by sop@google.com on 15 Sep 2010 at 11:40

• Changed state: Submitted
• Added labels: FixedIn-2.1.6

[GoogleCodeExporter]

Original comment by sop@google.com on 15 Dec 2010 at 8:09

• Changed state: Released