Need to support new toolkit async autocomplete

RESOLVED FIXED

Status

SeaMonkey
Autocomplete
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 327936 [details] [diff] [review]
Proposed patch

* 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)
(Assignee)

Comment 2

10 years ago
(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 4

9 years ago
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+
(Assignee)

Comment 5

9 years ago
Not true, new constants don't need a UUID rev, see nsIPrompt.idl for example.
(Assignee)

Comment 6

9 years ago
Pushed changeset 321797a42ad4 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 7

9 years ago
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

Comment 8

9 years ago
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.
(Assignee)

Updated

9 years ago
Blocks: 453999
(Assignee)

Comment 9

9 years ago
Created attachment 339684 [details] [diff] [review]
Follow-up fix, v1

The autocomplete doesn't close if it received a noMatchYet status.
Attachment #339684 - Flags: review?(ajschult)
(Assignee)

Comment 10

9 years ago
Created attachment 339686 [details] [diff] [review]
Follow-up fix, v2

Another approach to fixing the problem.
Attachment #339686 - Flags: review?(ajschult)

Comment 11

9 years ago
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+
(Assignee)

Comment 12

9 years ago
Created attachment 340310 [details] [diff] [review]
Second follow-up fix

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)
(Assignee)

Comment 13

9 years ago
Pushed changeset 80bf84dd5d23 to mozilla-central.

Comment 14

9 years ago
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?
(Assignee)

Comment 15

9 years ago
(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 16

9 years ago
Comment on attachment 340310 [details] [diff] [review]
Second follow-up fix

OK, r=ajschult
Attachment #340310 - Flags: review?(ajschult) → review+
(Assignee)

Comment 17

9 years ago
Pushed changeset 9929cf926c62 to mozilla-central.
You need to log in before you can comment on or make changes to this bug.