Closed Bug 443370 Opened 16 years ago Closed 16 years ago

Need to support new toolkit async autocomplete

Categories

(SeaMonkey :: Autocomplete, defect)

Product:
SeaMonkey
Component:
Autocomplete
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(3 files, 1 obsolete file)

Proposed patch
7.38 KB, patch
ajschult784
: review+
Details | Diff | Splinter Review
Follow-up fix, v1
3.01 KB, patch
ajschult784
: review+
Details | Diff | Splinter Review
Second follow-up fix
1.46 KB, patch
ajschult784
: review+
Details | Diff | Splinter Review 
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 patch — — Splinter 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
Status: NEW → ASSIGNED
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

r=ajschult
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.
Status: ASSIGNED → RESOLVED
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 http://hg.mozilla.org/mozilla-central/index.cgi/rev/3d00b7d5ce32 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

r=ajschult
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 frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/fb5ac135ba36
Toolkit async autocomplete support for xpfe's autocomplete r=ajschult
https://hg.mozilla.org/comm-central/rev/8c10c22a3f96
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
https://hg.mozilla.org/comm-central/rev/8f845805d35a
Followup to bug 443370 to fix handling of RESULT_NOMATCH_ONGOING r=ajschult
https://hg.mozilla.org/comm-central/rev/68c03fef05ad
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.

Attachment

General

Creator:
Created:
Updated:
Size: