294140 - email autocomplete always adds "@localdomain" to addresses

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Ian Neal]

This does the mailnews equivalents of the TB parts of bug 93453 and bug 294107

Comment 1

[Ian Neal]

Attachment: Patch v0.1

This patch:
* Adds the JS to make use of the new prefs introduced in bug 93453 and altered
in bug 294107
* Adds pref/account settings for the new prefs above
* Updates help to reflect the new pref/account settings

Comment 2

[Frank Wein [:mcsmurf]]

BTW: Neil was thinking of another method to solve this problem, maybe you should
talk to him.

Comment 3

[Karsten Düsterloh]

Comment on attachment 183576 [details] [diff] [review]
Patch v0.1

>Index: mailnews/base/prefs/resources/locale/en-US/am-addressing.dtd
>===================================================================
>+<!ENTITY autocompleteToMyDomain.label     "Automatically complete addresses with my domain">

Hm, this sounds somewhat ambiguous. Is my domain used as the completion or are
addresses with my domain automatically completed and, if so, to what? (The
former, I know.)
How about "Automatically append my domain to addresses" or something (better)
like that?

>Index: mailnews/compose/resources/content/MsgComposeCommands.js
>===================================================================
> function setDomainName()
> {
>-  var emailAddr = gCurrentIdentity.email;
>-  var start = emailAddr.lastIndexOf("@");
>-  gAutocompleteSession.defaultDomain = emailAddr.slice(start + 1, emailAddr.length);
>+  if (gCurrentIdentity.autocompleteToMyDomain)
>+  {
>+    var emailAddr = gCurrentIdentity.email;
>+    var start = emailAddr.lastIndexOf("@");
>+    gAutocompleteSession.defaultDomain = emailAddr.slice(start + 1, emailAddr.length);
>+  }
> }

This has issues with recycled compose windows (thus minussing).
As soon as you stumble upon an account with autocompletion turned on, that
settings sticks with the compose window. Any subsequent compose window with
autocompletion turned _off_ will still use the last autocompletion domain!
(And the 2nd argument to String.slice is not needed here, btw).

>+          if (sPrefs.getBoolPref("mail.autoComplete.highlightNonMatches"))
>+            document.getElementById('addressCol2#1').highlightNonMatches = true;
>+

This is pretty useless without any supporting styles like those used by TB, eg.
<http://lxr.mozilla.org/mozilla/source/mail/themes/pinstripe/mail/compose/messe
ngercompose.css#407>.

Comment 4

[Ian Neal]

Attachment: Updated Patch v0.1a

Changes since v0.1:
* Updated account pref label as per review
* Empties default domain when autocompleteToMyDomain is false
* Add css for highlight pref

Comment 5

[Karsten Düsterloh]

Comment on attachment 183777 [details] [diff] [review]
Updated Patch v0.1a

>Index: mailnews/compose/resources/content/MsgComposeCommands.js
>===================================================================
>+function CheckValidEmailAddress(to, cc, bcc)

Since most of this file is violating the Mozilla naming guidelines, these
function arguments are in fact following the prevalent style (but I'm not quite
happy with that)...

Anyway, I found another issue:
If you have @local turned off and enter an address part that is matched by only
2 addressbook entries, the address popup isn't shown, because it has the
minResultsForPopup property of the respective textbox set to 3...
This should be 2 in this case (only), probably in setupAutocomplete. (eg.
http://lxr.mozilla.org/mozilla/source/mailnews/compose/resources/content/MsgCom
poseCommands.js#1017 for LDAP)

Comment 6

[Ian Neal]

Attachment: Revised Patch v0.1b (Checked in)

Changes since v0.1a: (as per reviewer's request)
* Added a reduction to minResultsForPopup for when autocompleteToMyDomain is
off
* Changed arguments to aTo, aCC and aBCC for CheckValidEmailAddress function

Comment 7

[Karsten Düsterloh]

Comment on attachment 183953 [details] [diff] [review]
Revised Patch v0.1b (Checked in)

>Index: mailnews/compose/resources/content/MsgComposeCommands.js
>===================================================================
>+      // When autocompleteToMyDomain is off there is no default entry with the domain
>+      // appended so reduce the minimum results for a popup to 2 in this case.

I feel a certain lack of commas here, but that may just be me. ;-)

Comment 8

[Ian Neal]

(In reply to comment #7)
> (From update of attachment 183953 [details] [diff] [review] [edit])
> >Index: mailnews/compose/resources/content/MsgComposeCommands.js
> >===================================================================
> >+      // When autocompleteToMyDomain is off there is no default entry with
the domain
> >+      // appended so reduce the minimum results for a popup to 2 in this case.
> 
> I feel a certain lack of commas here, but that may just be me. ;-)
> 
I will add commas before checkin :-)

Comment 9

[Ian Neal]

Comment on attachment 183953 [details] [diff] [review]
Revised Patch v0.1b (Checked in)

Rqeuesting sr from David as he wrote the patch for TB and should know the code.

Comment 10

[Ian Neal]

Comment on attachment 183953 [details] [diff] [review]
Revised Patch v0.1b (Checked in)

Requesting approval for fairly low risk, suite-only patch which has been in TB
for a while now.

Comment 11

[Ian Neal]

Comment on attachment 183953 [details] [diff] [review]
Revised Patch v0.1b (Checked in)

Checking in
extensions/help/resources/locale/en-US/mail_help.xhtml;
new revision: 1.61; previous revision: 1.60
mailnews/addrbook/prefs/resources/content/pref-addressing.xul;
new revision: 1.55; previous revision: 1.54
mailnews/addrbook/prefs/resources/locale/en-US/pref-addressing.dtd;
new revision: 1.19; previous revision: 1.18
mailnews/base/prefs/resources/content/am-addressingOverlay.xul;
new revision: 1.5; previous revision: 1.4
mailnews/base/prefs/resources/locale/en-US/am-addressing.dtd;
new revision: 1.8; previous revision: 1.7
mailnews/compose/resources/content/MsgComposeCommands.js;
new revision: 1.367; previous revision: 1.366
mailnews/compose/resources/locale/en-US/composeMsgs.properties;
new revision: 1.80; previous revision: 1.79
themes/classic/messenger/addressingWidget.css;
new revision: 1.5; previous revision: 1.4
themes/modern/messenger/addressingWidget.css;
new revision: 1.6; previous revision: 1.5
done

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> (From update of attachment 183953 [details] [diff] [review] [edit])
> extensions/help/resources/locale/en-US/mail_help.xhtml;
> new revision: 1.61; previous revision: 1.60

nit:
{{
Select this if you want to Mail & Newsgroups to automatically [...]
[...]       If you want Mail & Newsgroups to highlight [...]
}}
Isn't there a 'to' to remove/add !?

Comment 13

[Ian Neal]

Attachment: Fix typo patch (Checked in)

Corrects typo found in comment above

Comment 14

[Ian Neal]

Comment on attachment 189282 [details] [diff] [review]
Fix typo patch (Checked in)

Requesting a= for very simple typo fix

Comment 15

[Ian Neal]

Comment on attachment 189282 [details] [diff] [review]
Fix typo patch (Checked in)

Checking in mail_help.xhtml;
new revision: 1.62; previous revision: 1.61
done

Comment 16

[Scott MacGregor]

I think this broke the Compositions &  Addressing panel in Thunderbird's account
Wizard. I get a entity error in today's build.  

Comment 17

[Scott MacGregor]

Attachment: fix thunderbird bustage (Checked in)

Fixes the thunderbird bustage by not including the new piece of UI in
thunderbird.   We're not interested in exposing this setting via the account
settings UI for thunderbird anyway so this works better than fixing the string
entity.

Comment 18

[Scott MacGregor]

Comment on attachment 189448 [details] [diff] [review]
fix thunderbird bustage (Checked in)

I checked in the fix for the thunderbird bustage.

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk using both SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060104 Mozilla/1.0 and Thunderbird version 1.6a1 (20060103).

I verified that on Thunderbird, we don't add the user's domain by default, and that there is no such UI exposed to enable it.