Closed Bug 443370 Opened 16 years ago Closed 16 years ago

Need to support new toolkit async autocomplete


(SeaMonkey :: Autocomplete, defect)

Not set


(Not tracked)



(Reporter: neil, Assigned: neil)




(3 files, 1 obsolete file)

Toolkit autocomplete has two new status codes, RESULT_NOMATCH_ONGOING (which is useless, but then I didn't review the code) and RESULT_SUCCESS_ONGOING.
Attached patch Proposed patchSplinter Review
* Added new async status constants to nsIAutoCompleteListener
* Don't decrement the pending session count for async returns
* Added a new field to autocomplete to track the first return,
  since we can no longer use the pending session count
* Pass the previous async results to addResultElements
* Replace the results in-place in the popup
  (note that this code assumes that none of the existing results change)
Assignee: nobody → neil
Attachment #327936 - Flags: review?(ajschult)
(In reply to comment #1)
> * Replace the results in-place in the popup
The code jumps through hoops to replace the results correctly because at least one autocomplete data source reuses result objects.
Blocks: 370306
Andrew, any chance of looking at the review for this soon - I could really do with it for finishing off the autocomplete migration.
Comment on attachment 327936 [details] [diff] [review]
Proposed patch

>Index: xpfe/components/autocomplete/public/nsIAutoCompleteListener.idl
>+    // the following status values indicate that the async search hasn't ended
>+    const long noMatchYet       = 4;
>+    const long matchSoFar       = 5;

this needs a uuid rev
otherwise looks good

Attachment #327936 - Flags: review?(ajschult) → review+
Not true, new constants don't need a UUID rev, see nsIPrompt.idl for example.
Pushed changeset 321797a42ad4 to mozilla-central.
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 327936 [details] [diff] [review]
Proposed patch

>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml
>+              while (++oldIndex < this.mCounts.length)
>+                row -= this.mCounts[i];

This is what causes the following line in error console for me:

Error: i is not defined
Source File: chrome://global/content/autocomplete.xml
Line: 1328
Checked in the following fix as after IRC conversation with Neil:

-                row -= this.mCounts[i];
+                row -= this.mCounts[oldIndex];

This is either r=Neil or p=Neil/r=me, as you like it. Neil proposed this fix, I fixed and tested it locally, Neil told to check this in if it works :)

Note that the error happened when actually doing async updates to autocomplete which we usually don't have in our code yet, but the places code I'm building locally does that, so I saw the error.
Blocks: 453999
The autocomplete doesn't close if it received a noMatchYet status.
Attachment #339684 - Flags: review?(ajschult)
Attached patch Follow-up fix, v2 (obsolete) — Splinter Review
Another approach to fixing the problem.
Attachment #339686 - Flags: review?(ajschult)
Comment on attachment 339684 [details] [diff] [review]
Follow-up fix, v1

If we can get drop mFailureCount, that seems like the better option

>       <!-- get the total number of results in a specific session or overall if session is null-->

comment needs updating

Attachment #339684 - Flags: review?(ajschult) → review+
It turns out that we stop the search even when we're supposed to be ignoring input events, which seems wrong anyway, and breaks async LDAP quite badly, but would also break places if you turned on auto fill.
Attachment #339686 - Attachment is obsolete: true
Attachment #340310 - Flags: review?(ajschult)
Attachment #339686 - Flags: review?(ajschult)
Pushed changeset 80bf84dd5d23 to mozilla-central.
Comment on attachment 340310 [details] [diff] [review]
Second follow-up fix

>diff -r 4c3dccca8505 xpfe/components/autocomplete/resources/content/autocomplete.xml
>       <method name="processInput">
>         <body><![CDATA[
>           // stop current lookup in case it's async.
>           this.stopLookup();
>           // stop the queued up lookup on a timer
>           this.clearTimer();
>-          if (this.ignoreInputEvent)
>-            return;

hmm, processInput also gets called from keyNavigation.  perhaps just make this an earlier return?
(In reply to comment #14)
> (From update of attachment 340310 [details] [diff] [review])
> hmm, processInput also gets called from keyNavigation.
Yes, but it never sets ignoreInputEvent anyway.
Comment on attachment 340310 [details] [diff] [review]
Second follow-up fix

OK, r=ajschult
Attachment #340310 - Flags: review?(ajschult) → review+
Pushed changeset 9929cf926c62 to mozilla-central.
Pushed by
Toolkit async autocomplete support for xpfe's autocomplete r=ajschult
followup kinda-typo fix to bug 443370, use correct index to fix error when doing async autocomplete updates, r=Neil over IRC, NPOTB for FF
Followup to bug 443370 to fix handling of RESULT_NOMATCH_ONGOING r=ajschult
Followup to bug 443370 autocomplete doesn't ignore input events well enough r=ajschult
You need to log in before you can comment on or make changes to this bug.