582494 - Lookup implementors of nsIAutoCompleteResult

Status: RESOLVED FIXED

(Core Graveyard :: Tracking, defect)

Description

[David Zbarsky (:dzbarsky)]

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

Comment 1

[[e-mail]]

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.

Comment 2

[David Zbarsky (:dzbarsky)]

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.

Comment 3

[David Zbarsky (:dzbarsky)]

Sorry, you were right.  I will update these.  I think some tests may also need updates.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Fix up the implementers and tests

Comment 5

[[e-mail]]

(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 6

[[e-mail]]

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.

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: Fix up the implementers and tests

Sorry, I should have been more careful when I was writing this

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: Fix up the implementers and tests

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: [checked in] Fix up the implementers and tests

Comment 10

[[e-mail]]

(In reply to comment #9)
> Created attachment 461811 [details] [diff] [review]
> Fix up the implementers and tests r=enndeakin
                                    ^^^^^^^^^^^
Did you mean that? ;-)

Comment 12

[Mark Banner (:standard8)]

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.

Comment 13

[Mark Banner (:standard8)]

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.