528503 - Multiple comma-separated nicknames or partial addresses entered on single address line do no longer auto-complete after Enter was hit

Status: RESOLVED INVALID

(MailNews Core :: Composition, defect)

Description

[rsx11m]

I've noticed a major difference in behavior between TB 2.0 and 3.0 versions:

 - On current 2.0 releases, you can enter multiple addresses "ab,cd,ef" into
   a single line; they will turn red and don't autocomplete right away, but
   when hitting Enter they *do* autocomplete with each entry on a single line.

 - With 3.0 builds, entering "ab,cd,ef" won't autocomplete them either and
   turns the line to red, when hitting Enter they expand onto one line each
   *but* will stay red and do *not* autocomplete.

This may be related to bug 392932 but appears to be a regression on its own.

Also seen with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091111 SeaMonkey/2.0.1pre.

Comment 1

[rsx11m]

xref bug 497722.

Comment 4

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #0)
> I've noticed a major difference in behavior between TB 2.0 and 3.0 versions:
> 
>  - On current 2.0 releases, you can enter multiple addresses "ab,cd,ef" into
>    a single line; they will turn red and don't autocomplete right away, but
>    when hitting Enter they *do* autocomplete with each entry on a single
> line.
> 

rsx11m, can you elaborate on this with an example (including STR, actual result, expected result)?
This would only work for comma-separated list of fully typed (or at least unique substrings of) email addresses, right? Or is it about resolving comma-separated list of nicknames, as in Bug 325458 Comment 21?

Comment 5

[rsx11m]

No, it's not just about nicknames. If I remember correctly, in Thunderbird 2.0 and earlier you could enter partial addresses in a comma-separated list and they would autocomplete like single addresses do now. That broke with 3.0.

More details with STR are in bug 567672 (duplicate of this one).

Comment 6

[rsx11m]

Potential candidate per bug 392932 comment #16: bug 370306 attachment 325961 [details] [diff] [review], checked in on 2008-06-20 for 3.0b2.

Comment 8

[Sisim Biva]

I think the add-on MRC Compose (https://addons.mozilla.org/en-US/thunderbird/addon/mrc-compose/) solves this bug.

Comment 9

[[e-mail]]

Attachment: Possible patch

Comment 10

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8976970 [details] [diff] [review]
Possible patch

I think gds is no longer active.
Sorry was a bit swamped but will see that I check this one out soon.

Neil, against which tree was this one done. It doesn't look like comm-central?

Comment 11

[gene smith]

I'm still here. Been meaning to try/test the patch but I probably don't have the knowledge of the code in this area to actually do a good review.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8976970 [details] [diff] [review]
Possible patch

Review of attachment 8976970 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +1085,5 @@
>    },
>  
>    autoCompleteNextAddress:function()
>    {
> +    this.numSessionsToSearch = 2;

why is this always 2 now? even if ldap isn't being used.

Comment 13

[Frank-Rainer Grahl (:frg)]

Sorry Gene if I jumped the gun.

The patch doesn't appy clean to me in c-c.

Comment 14

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8976970 [details] [diff] [review]
Possible patch

Some formalities:

+  abSearch: Components.classes["@mozilla.org/autocomplete/search;1?name=addrbook"].getService(Components.interfaces.nsIAutoCompleteSearch),
+  ldapSearch: Components.classes["@mozilla.org/autocomplete/search;1?name=ldap"].getService(Components.interfaces.nsIAutoCompleteSearch),

It is Cc and Ci now only after the latest round of changes in Gecko made them global constants. The c-c source was mass changed for this.

>   QueryInterface : function(iid)
>   {
>-      if (iid.equals(Ci.nsIAutoCompleteListener) ||
>      if (iid.equals(Ci.nsIAutoCompleteObserver) ||
>           iid.equals(Ci.nsISupports))

This should be
>  QueryInterface: ChromeUtils.generateQI(["nsIAutoCompleteObserver"]),

SeaMonkey has not ported this yet. We are broken since 2.56 and fixing up 2.57 first.

> +++ b/suite/mailnews/compose/addressingWidgetOverlay.js
...
> -    }
> +    this.processAllResults();

You missed a closing bracket either before or after here.

And same question as Magnus. Can an ldap search be made without ldap?

Clearing feedback for now until a new patch arrives. I will backport it to 2.53 to test it then.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 8976970 [details] [diff] [review]
Possible patch

Review of attachment 8976970 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing r? awaiting comments

Comment 16

[gene smith]

Attachment: 528503-gds.diff

Applied the patch manually (patch -p1 < 528503.diff) resulting in one conflict/reject (as pointed out above). I changed 

QueryInterface: ChromeUtils.generateQI(["nsIAutoCompleteListener"]),

to

QueryInterface: ChromeUtils.generateQI(["nsIAutoCompleteObserver"]),

and re-built.

It work when I put in To: string like this:

av,gdsm,gene,k9

it expanded to 4 separate To: lines on hitting enter. Without the patch, it didn't expand.

I can't really comment on the code validity other than it seems to work. Also, I don't run suite/seamonkey so can't test both files. However, the suite file patched OK with no conflicts.

Attached is a conflict-free patch based on the fix above.

Comment 17

[Frank-Rainer Grahl (:frg)]

Clearing old NIs

Comment 18

[Thomas D. (:thomas8)]

This has outlived its feasibility, especially for the reported scenario of actually typing names in. Current TB behaviour prevents this in many ways. Also, 16 comments and only two age-old dupes in 12 years don't indicate much user interest.

• As comma can be a valid part of display name, Joe, Erna, Jane <[e-mail]> is currently correctly parsed as a single valid address (making this bug impossible and invalid).
• Autocomplete search algorithm has changed long back and we now do partial matching in more fields on each of your search words, returning more results initially (which works much against this proposal here), but adding another search fragment will narrow down fast and flexible, Google style.
• Currently it also can't work at all as long as we still autocomplete on comma.
• When you type them in one by one, let's say we would not autocomplete on comma, why not just press Enter or Tab after each recipient and actually see what you get, instead of letting TB go into risky guesswork? (Pasting a comma-separated list in looks like an unlikely scenario and risky if the list wasn't from yourself, and if it was, well, you better have a list of all the email addresses or a mailing list or some other way...)

Alas, this proposal can never work in a safe and predictable way, except for edge cases. Highly error-prone. No longer useful where we all have 100s of addresses in our ABs. Violating standards, too (see above).

We can't have a comma-separated list of words or partial addresses and then just autocomplete all of them with a random match.
For search words which are not email addresses, you'd have to be 100% sure that your searchword only matches a single entry, or the right entry first, but first matches and number of matches can change at any time - you can never be 100% sure that it works. Taking that risk of letting TB pick addresses for you for a whole list of people is just insane. Even autocompleting only when there's a single match would not help - how do I notice those addresses where I mistyped a single character and TB did not autocomplete?

For search words which look like email addresses - how to we figure that if you have [e-mail] in your address book (where subdomain ac could mean anything, e.g. access control), and you have a shorter [e-mail] in your comma-separated list, that you really want to autocomplete to the longer one and not keep the valid shorter address, [e-mail] (ac = Ascension Island), even though it's not in your AB?

Whichever way you look at this, it's a can of worms which we can never get safe and right and useful at the same time.

Comment 19

[rsx11m]

I agree with comment #18 to that extent, but for the record, let's revert the bug title to specifically what it was filed for.