Closed
Bug 438861
Opened 16 years ago
Closed 16 years ago
Autocomplete does not pass back previous results to a second search
Categories
(Toolkit :: Autocomplete, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(3 files, 3 obsolete files)
8.27 KB,
text/plain
|
Details | |
1.41 KB,
patch
|
ajschult784
:
review+
|
Details | Diff | Splinter Review |
10.21 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•16 years ago
|
||
Attachment #324817 -
Flags: review?(ajschult)
Comment 2•16 years ago
|
||
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.
Comment 3•16 years ago
|
||
(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.
Comment 4•16 years ago
|
||
Attachment #327390 -
Flags: review?(ajschult)
Updated•16 years ago
|
Attachment #324817 -
Attachment is obsolete: true
Attachment #324817 -
Flags: review?(ajschult)
Updated•16 years ago
|
Attachment #327390 -
Flags: review?(ajschult) → review+
Comment 5•16 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 6•16 years ago
|
||
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.
Comment 7•16 years ago
|
||
Patch checked into mozilla-central in Bug 443837
Assignee | ||
Comment 8•16 years ago
|
||
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 → ---
Assignee | ||
Updated•16 years ago
|
Attachment #327390 -
Attachment description: Addressed review comments → [checked in] Addressed review comments
Assignee | ||
Comment 9•16 years ago
|
||
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 10•16 years ago
|
||
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
Assignee | ||
Comment 11•16 years ago
|
||
(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
Comment 12•16 years ago
|
||
What I mean is, what happens if search 2 returns results before search 1 does?
Comment 13•16 years ago
|
||
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?
Assignee | ||
Comment 14•16 years ago
|
||
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 15•16 years ago
|
||
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+
Assignee | ||
Comment 16•16 years ago
|
||
(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?
Assignee | ||
Comment 17•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #334893 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 18•16 years ago
|
||
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.
Assignee | ||
Comment 19•16 years ago
|
||
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: 16 years ago → 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1
Comment 20•7 years ago
|
||
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.
Description
•