Closed Bug 438861 Opened 13 years ago Closed 12 years ago

Autocomplete does not pass back previous results to a second search

Categories

(Toolkit :: Autocomplete, defect)

1.9.0 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached file Testcase
For MailNews autocomplete migration to toolkit, we want to have multiple autocompletesearch items, i.e. multiple searches on one autocomplete.

The first two of these (at least) return their results to the listener/controller straight away when startSearch is called.

When we do a subsequent search (e.g. going from "S" to "St"), only the first search defined in the "autocompletesearch" attribute gets its previous result back. The second search just gets null.

I'm attaching a xpcshell test that fails due to this bug.

This is a real pain because we can't do efficient searching on subsequent changes.
Attached patch XPFE fix (obsolete) — Splinter Review
Attachment #324817 - Flags: review?(ajschult)
Comment on attachment 324817 [details] [diff] [review]
XPFE fix

>@@ -649,7 +649,7 @@
>           {
>             this.mLastResults[aSessionName] = null;
>             if (firstReturn)
>-              this.clearResults(false);
>+              this.clearResultElements(false);

Hmm.  This seems to be the only place data is cleared out now.  Should this be doing something to null out the param like clearResultData?

>             this.searchFailed();
>             return;
>           } else if (aStatus == 

Feel free to remove else-after-return here.

>-            this.clearResults(false); // clear results, but don't repaint yet
>+            this.clearResultElements(false); // clear results, but don't repaint yet

Comment is better now than it was before.  :)

>           // if all searches are done and they all failed...

>-              this.clearResults(true); // clear data and repaint empty
>+              this.clearResultElements(true); // clear data and repaint empty

Comment isn't right anymore.  does this only not need to clear the data because there's no data to clear?  Is that true even if called from startLookup?  I'd think startLookup would want to clear any results for the session that failed.

It looks like clearResults is now only called by the desctructor.  Perhaps just inline clearResults to there.
(In reply to comment #2)
>(From update of attachment 324817 [details] [diff] [review])
>>             this.mLastResults[aSessionName] = null;
>Should this be doing something to null out the param like clearResultData?
param is going away with the toolkit switch, no need to worry about it.

>>-              this.clearResults(true); // clear data and repaint empty
>>+              this.clearResultElements(true); // clear data and repaint empty
>Comment isn't right anymore.  does this only not need to clear the data because
>there's no data to clear?  Is that true even if called from startLookup?  I'd
>think startLookup would want to clear any results for the session that failed.
You're right, this change is probably incorrect.
Attachment #324817 - Attachment is obsolete: true
Attachment #324817 - Flags: review?(ajschult)
Attachment #327390 - Flags: review?(ajschult) → review+
Fix checked in.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 327390 [details] [diff] [review]
[checked in] Addressed review comments

>           {
>             this.mLastResults[aSessionName] = null;
>             if (firstReturn)
>-              this.clearResults(false);
>+              this.clearResultElements(false);
>             this.searchFailed();
>             return;
>-          } else if (aStatus == 
>-                     Components.interfaces.nsIAutoCompleteStatus.failureItems){
>+          if (aStatus == Components.interfaces.nsIAutoCompleteStatus.failureItems)
>             ++this.mFailureItems;
>-          }

You missed adding the } back in after the return statement. I've checked in a fixed as autocomplete is pretty much broken without that.
Blocks: 443837
Patch checked into mozilla-central in Bug 443837
Reopening - this is NOT fixed for toolkit (which this bug was originally filed for. The testcase attached should still be valid for toolkit builds (though I haven't checked it yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #327390 - Attachment description: Addressed review comments → [checked in] Addressed review comments
Attached patch Toolkit fix (obsolete) — Splinter Review
This is the fix for toolkit. It also fixes several other bugs:

- mMatchCounts was initially set to the length of the searchCount, not the capacity. I'm not sure why this hasn't really showed up at all, possibly because ClearResults() was clearing the list.
- When looking up the previous results index, a pointer to the previous results was used, this may not be the case, if the caller created a new set of results and copied the relevant ones from the previous, additionally, we know the index anyway.

Alongside those, the main part of this fix is to not clear out the stored results when we're just doing a follow-on search (and just zero the matches), and therefore this keeps the results available for the next search.

Includes the testcase that I did when I originally filed this bug.
Assignee: nobody → bugzilla
Status: REOPENED → ASSIGNED
Attachment #334328 - Flags: review?(enndeakin)
Comment on attachment 334328 [details] [diff] [review]
Toolkit fix

>+  PRUint32 length = 0;
>+  mResults->Count(&length);
>+  if (aSearchIndex >= (PRInt32)length) {
I don't think you can guarantee the order of the results.

>+      return;
>+    }
>+    else {
Nit: else after return
(In reply to comment #10)
> (From update of attachment 334328 [details] [diff] [review])
> >+  PRUint32 length = 0;
> >+  mResults->Count(&length);
> >+  if (aSearchIndex >= (PRInt32)length) {
> I don't think you can guarantee the order of the results.

Actually you can, see nsAutoCompleteController::OnSearchResult:

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#668
What I mean is, what happens if search 2 returns results before search 1 does?
Comment on attachment 334328 [details] [diff] [review]
Toolkit fix

nsAutoCompleteController::ProcessResult(
>   if (aResult)
>     aResult->GetMatchCount(&matchCount);
> 
>-  PRInt32 oldIndex = mResults->IndexOf(aResult);
>-  if (oldIndex == -1) {
>+  PRUint32 length = 0;
>+  mResults->Count(&length);
>+  if (aSearchIndex >= (PRInt32)length) {
>     // cache the result
>     mResults->AppendElement(aResult);
>     mMatchCounts.AppendElement(matchCount);
>   }
>   else {
>     // replace the cached result
>-    mResults->ReplaceElementAt(aResult, oldIndex);
>+    mResults->ReplaceElementAt(aResult, aSearchIndex);
>     oldMatchCount = mMatchCounts[aSearchIndex];
>-    mMatchCounts[oldIndex] = matchCount;
>+    mMatchCounts[aSearchIndex] = matchCount;
>   }

I don't understand this code much at all. Can you explain this part. What does mMatchCounts hold?

Is a result just one item of output (that is, there is one result per line in the autocomplete box), or multiple items?
Attached patch Toolkit fix v2 (obsolete) — Splinter Review
Here's a much simpler fix that Neil Rashbrook suggested to me over irc.

mResults is an array of the results that were returned from the previous searches; each element in the array is an nsIAutoCompleteResult item returned from the search containing the individual results, so we'll get one item per search type used (e.g. history / addrbook / mydomain).

This patch caches the mResults array before calling StartSearch. This means when the first search comes back and forces all the results arrays to be cleared (which could be synchronous and occur before starting the search on the second search session), we'll still have a local cache of the results to pass the previous results correctly back to the searches that haven't been started yet.
Attachment #334328 - Attachment is obsolete: true
Attachment #334686 - Flags: review?(enndeakin)
Attachment #334328 - Flags: review?(enndeakin)
Comment on attachment 334686 [details] [diff] [review]
Toolkit fix v2

Yes, this looks much better. Do we want to cleart the cached items once all searches have completed?
Attachment #334686 - Flags: review?(enndeakin) → review+
(In reply to comment #15)
> (From update of attachment 334686 [details] [diff] [review])
> Yes, this looks much better. Do we want to cleart the cached items once all
> searches have completed?
> 
Its a cache local to the function, therefore shouldn't the array clear these for us?
Attached patch Toolkit fix v3Splinter Review
My apologies, I missed including the testcase last time, not sure if you wanted to look at it again or not.
Attachment #334686 - Attachment is obsolete: true
Attachment #334893 - Flags: review?(enndeakin)
Attachment #334893 - Flags: review?(enndeakin) → review+
I checked this in as changeset id 379931a16cd7, I then backed out under id 20663933d4b0 due to what looked like a Lk/MH regression (all platforms, worst on Linux). However, Lk/MH didn't go back down again. So I think that the tinderbox regression was actually due to something else (although no other patches landed), so I'll try relanding in a couple of days.
Checked in again as changeset 18409:166ebc402d37

No significant changes in Lk/MH so it looks like it was originally a tinderbox issue.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/04bd0b3e026d
fix xpfe autocomplete bustage from bug 438861 to be really in sync with cvs
You need to log in before you can comment on or make changes to this bug.