Lookup implementors of nsIAutoCompleteResult

RESOLVED FIXED

Status

defect
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

Trunk
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The fix for Bug 556007 introduces nsIAutoCompleteResult::GetActualValueAt which returns the value that should be placed into the element. (this may be different from the displayed value, as in the case of Bug 556007).

To preserve existing behavior, we should add 
GetActualValueAt(PRInt32 aIndex, nsAString& _retval)
{
  return GetValueAt(aIndex, _retval);
}

to the implementers of nsIAutoCompleteResult
See Also: → 556007
Out of interest, why wasn't the new method called GetLabelAt? (This is the terminology we use for XUL menulists, but it doesn't necessarily make sense for autocomplete, so I thought I'd ask.) Anyway, implementers appear to include:

mailnews/db/gloda/components/glautocomp.js
mailnews/addrbook/src/nsAbAutoCompleteMyDomain.js
mailnews/addrbook/src/nsAbAutoCompleteSearch.js

Also mailnews/addrbook/public/nsIAbAutoCompleteResult.idl needs a new uuid.
Hmmm it's not really a label, its the actual value that will be filled in. (and submitted to the server if its a form, etc.)

Why do those need to be changed?  The places you listed seem to be comm-central, while my patch modifies nsIAutoCompleteResult in mozilla-central.
Sorry, you were right.  I will update these.  I think some tests may also need updates.
Assignee: nobody → dzbarsky
Attachment #461474 - Flags: review?(neil)
Attachment #461474 - Flags: review?(neil) → review?(neil)
Attachment #461474 - Flags: review?(neil)
Attachment #461474 - Flags: review?(neil)
(In reply to comment #2)
> Hmmm it's not really a label, its the actual value that will be filled in. (and
> submitted to the server if its a form, etc.)
Actually I was thinking that the label would be the displayed value and the value would be the actual value filled in.
Comment on attachment 461474 [details] [diff] [review]
Fix up the implementers and tests

>   getValueAt: function getValueAt(aIndex) {
>     return this._searchResults[aIndex].value;
>   },
> 
>+  getActualValueAt: function getActualValueAt(aIndex) {
>+    return getValueAt(aIndex);
>+  },
This doesn't work in JavaScript; unlike C++, object members aren't in scope.

>   getValueAt: function getValueAt(aIndex) {
>     return this._searchResults[aIndex].value;
>   },
> 
>+  getActualValueAt: function getActualValueAt(aIndex) {
>+    return this._searchResults[aIndex].value;
>+  },
Half the cases you duplicated the method, and the other half you tried to reuse getValue at... the inconsistency looks odd.
Attachment #461474 - Flags: review?(neil) → review-
Sorry, I should have been more careful when I was writing this
Attachment #461474 - Attachment is obsolete: true
Attachment #461669 - Flags: review?(neil)
Attachment #461669 - Attachment is obsolete: true
Attachment #461669 - Flags: review?(neil)
(In reply to comment #9)
> Created attachment 461811 [details] [diff] [review]
> Fix up the implementers and tests r=enndeakin
                                    ^^^^^^^^^^^
Did you mean that? ;-)
Duplicate of this bug: 601965
Comment on attachment 461811 [details] [diff] [review]
[checked in] Fix up the implementers and tests

This looks good, thanks for doing the patch. r=Standard8. I'll land this on your behalf as well so we can get the errors fixed.
Attachment #461811 - Attachment description: Fix up the implementers and tests r=enndeakin → Fix up the implementers and tests
Attachment #461811 - Flags: review+
Comment on attachment 461811 [details] [diff] [review]
[checked in] Fix up the implementers and tests

Checked in: http://hg.mozilla.org/comm-central/rev/36c82cd8464d

I don't know if you have anything else to cover in this bug, so leaving open for now.
Attachment #461811 - Attachment description: Fix up the implementers and tests → [checked in] Fix up the implementers and tests
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.