Jens Müller (:tessarakt) Reporter
	Description • 16 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.0.3) Gecko/2008092813 Gentoo Firefox/3.0.3
Build Identifier: version 2.0.0.17 (20081022)

I installed the ReplyToList add-on, which adds the functionality asked for in Bug 45715.

However, I am missing one thing: That Thunderbird automatically figures out which From: address to use (so that the mail gets through to the list). I think this feature works for normal mail (by looking at the To: header, or whatever), right? For mailing list posts there are probably various headers that might be helpful ...

Reproducible: Always

	Joshua Cranmer [:jcranmer]
	Comment 1 • 16 years ago

This is a bug pertaining to an extension. Such bugs are not tracked here; please contact the extension author instead.

	Jens Müller (:tessarakt) Reporter
	Comment 2 • 16 years ago

No, it's not - I mentioned the extension just for illustration (sorry that I didn't make that clear). 

This is a followup to bug 45715 (on which it thus should depend).

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 3 • 15 years ago

I'd love to see this done.  I have a couple identities and find that TB doesn't handle the reply list correctly.

Jens, if you have some time I don't think this would be too difficult to do.

With some modification to the getIdentityForHeader() function you could improve the default choice.
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#120

That function is called from ComposeMessage() and passed the type + mail header
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#164

Add a check for (type == Components.interfaces.nsIMsgCompType.ReplyToList) and then maybe pass in a better hint to the identity searching functions used.

e.g.
getIdentityForServer(server, hintForIdentity)
getBestIdentity(allIdentities, hintForIdentity)

As far as I can tell from my list messages you'd be looking to pass in the 'Delivered-To' header for your hint; though I believe there can be multiples so you might have to loop the call. 

AFAIK this shouldn't be too difficult, also this might make a good student project.

	Abdurrahman
	Comment 4 • 15 years ago

Can this bug be reproduced without the ReplyToList add-on?

	Jens Müller (:tessarakt) Reporter
	Comment 5 • 15 years ago

(In reply to comment #4)
> Can this bug be reproduced without the ReplyToList add-on?

Now that "Reply list" is in TB3 without any add-on: Yes, sure.

Btw: It's not a bug, but an enhancement request.

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 6 • 15 years ago

This was harder than expected because you don't have the current headers of a message easily available and the getBestIdentity function looks for a hint string, instead of some kind of array of identities; a bit unexpected.  Also the getIdentityForHeaders function seems to be called when a message is displayed, not only during compose time so likely it has to be faster than calling out to the MimeMessage parser every time.  I was hoping that meant I'd have access to the currentHeaders data but it doesn't appear that way.

Might give this another try some other time but the fundamental issue seems to remain that multiple identities can be found in the Delivered-To.

Lets say I have an email address:
clarkbw@example.com

And a list identity (where all my mailing lists are sent to) of:
lists@example.com

My mailing lists messages could have the following headers:

Delivered-To: clarkbw@example.com
Delivered-To: lists@example.com
Delivered-To: list@lists
From: joe@lists
To: list@lists

The only path I have right now is to look at the extra identities first and hope to find them in the hint string.  Then check the default identity as the fallback.  This would likely fail if your mail bounced through more than one identity but would at least succeed if you have a default identity and a few others you use independently.

	ramy.wafa+1
	Comment 7 • 15 years ago

Are you going to work on a patch for the enhancement?

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 8 • 15 years ago

I think we're going to need the big guns here as I believe this change is more complicated than I had previously anticipated.  My patch is a start but this smells a lot like the Reply All Toggle in compose mode bug so I'm adding blake in.

	Kefu Zhao Assignee
	Comment 9 • 15 years ago

Trying to reproduce this bug. However, does this bug still fit in the "student-project" category?

	Blake Winton (:bwinton) (:☕️)
	Comment 10 • 15 years ago

Well, let's see how far you get on it.  :)

Thanks,
Blake.

	Kefu Zhao Assignee
	Comment 11 • 15 years ago

Hi. I am a little bit confused by this bug. As Jens mentioned, Thunderbird should automatically figures out which From: address to use in the “reply to list” feature. Actually, what I am not sure is which From address should be the correct one to use? There are multiple scenarios regarding to this bug:

1.	As far as I know, TB can know which account an email is associated with. Can we directly use this account as the “From” address no matter how many “Delivered-To” headers are found?
For example, I have an email in account 1 inbox. If I click reply, the “From” address will be account 1. However, if I drag this email into my account 2 inbox and click reply, the “From” address should be account 2, right? (this is what TB is currently doing)


2.	If I save an email as *.eml file and load it again, TB will has no idea which account it is associated with. In this way, “Delivered-To” and some other headers are used to decide which “From” address to use after clicking “reply”. As a result, I guess this “reply-yo-list” bug is talking about a saved email?

Please correct me if I am wrong.

	Jens Müller (:tessarakt) Reporter
	Comment 12 • 15 years ago

I am surely not the only person who gets mail addressed to different e-mail addresses into the same e-mail account. I have several of them configured as identities of my main account. This bug is e.g. about choosing the correct identity.

	Ben Bucksch (:BenB)
	Comment 13 • 15 years ago

There's a bug for a per-folder setting of the From address. (I can't find it, though.)
This would help with mailing lists (without guessing, but manual user work), under the assumption that people typically filter mailing lists into a separate folder and can set their posting address as From on that folder.

	Kefu Zhao Assignee
	Comment 14 • 15 years ago

First patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

	Blake Winton (:bwinton) (:☕️)
	Comment 15 • 15 years ago

Comment on attachment 432956 [details] [diff] [review]
First patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

>+++ b/mail/base/content/mailCommands.js
>@@ -32,25 +32,32 @@
>+Components.utils.import("resource://app/modules/gloda/mimemsg.js");

So, you import this, but I can't see where you use it.

>+  //dump("in getBestIdentity\n");
>+  //dump("optionalHint: " + optionalHint + "\n");

Instead of commenting these statements out, we should just remove them.

>-
>+  
>+  //dump("\nIdentity count: " + identitiesCount + "\n"); 
>+   

Ditto, and watch out for trailing spaces.

>@@ -94,16 +101,17 @@ function getBestIdentity(identities, opt
>+  dump("Identity returned by getBestIdentity. Identity: " + identity.email + "\n");

We probably want to delete this line, and others like it.  ;)

>@@ -112,48 +120,106 @@ function getIdentityForServer(server, op
>         // dump("identities = " + identities + "\n");
>         // Try and find the best one.
>         identity = getBestIdentity(identities, optionalHint);
>     }
> 
>     return identity;
> }
> 
>-function getIdentityForHeader(hdr, type)
>-{
>-  // If we treat reply from sent specially, do we check for that folder flag here ?
>-  var isTemplate = (type == Components.interfaces.nsIMsgCompType.Template);
>-  var hintForIdentity = isTemplate ? hdr.author : hdr.recipients + hdr.ccList;
>-  var identity = null;
>-  var server;
>+function getIdentityForHeader(hdr, type)

I'm seeing a lot of ^Ms at the end of this function.  Could you make sure
you save it with Unix line-endings?

>+{
>+  // If we treat reply from sent specially, do we check for that folder flag here ?
>+  var isTemplate = (type == Components.interfaces.nsIMsgCompType.Template);
>+  var isReplyToList = (type == Components.interfaces.nsIMsgCompType.ReplyToList);

Since you only use these once each, in an if test, I think you can inline
these variables.

>+  var server;
>+  var hintForIdentity;

I think we should change these to "let"s.

>+  //dump("entering getIdentityForHeader\n");
>+  //dump("hdr.folder:\n");
>+  
>+  if(isReplyToList)
>+  {

The coding standard we use puts the {s on the same line as the if (or
while, or for), and a space between the if and the (.

(But, the closing } is on a different line than the else.)

>+    var index = 0;
>+    var key = "delivered-to";
>+    var allIdentities = accountManager.allIdentities;
>+    
>+    var tempIdentity = "";
>+    
>+    // get the last "delivered-to" that is in the defined identities
>+    while((tempIdentity = currentHeaderData[key]) != undefined)
>+    {
>+        for each (var tempID in fixIterator(allIdentities,
>+                                          Components.interfaces.nsIMsgIdentity)) 
>+        {
>+            dump("\ncurrent identity in tb: " + tempID.email + "\n");
>+            if(tempIdentity.headerValue.toLowerCase().indexOf(tempID.email.toLowerCase()) >=0 )

indexOf is defined to return -1 if it's not found, so we should probably
check that the return value is != -1 instead of >= 0.

>+            {
>+                hintForIdentity = tempID.email;
>+                dump("looping for identity: " + hintForIdentity);
>+                break;
>+            }                                                                           
So, you know that this only breaks out of the for loop, right?

Maybe it would be a better idea to loop through all the headers in
currentHeaderData, extract the ones that start with "delivered-to", sort
and reverse that list, then go through it, and pick the first one with an
identity.

>+        }
>+        
>+        key = String("delivered-to" + index++);

I don't think you need to wrap this in a call to "String"…

>+  if (server && !identity)
>+  {
>+    dump("getIdentityForServer\n");
>+    identity = getIdentityForServer(server, hintForIdentity);
>+  }

If you take out the dump statement, you can also take out the { and }s.

>+++ b/mail/test/mozmill/message-header/test-reply-to-list-from-address-selection.js
>@@ -0,0 +1,196 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging, Inc.

I've been told that this should be "The Mozilla Foundation".

>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Blake Winton <bwinton@latte.ca>
>+ *   Dan Mosedale <dmose@mozillamessaging.com>

I don't remember writing anything in this file…  ;)
(You should probably remove Dan's and my names, and add in yours instead.

>+/*
>+ * Test that TB can automatically select the "From" address in the "reply-to-list" mode.
>+ */

You could wrap that comment so that it wasn't longer than 80 characters.

>+var MODULE_NAME = 'test-message-header';

I think you want to pick a different name for this module.

>+var RELATIVE_ROOT = '../shared-modules';
>+var MODULE_REQUIRES = ['folder-display-helpers', 'window-helpers', 'compose-helpers'];

This line can also wrap.

>+var addMessageToFolder = function(aFolder){
>+  var msgId = Components.classes["@mozilla.org/uuid-generator;1"].getService(Components.interfaces.nsIUUIDGenerator).generateUUID() +"@mozillamessaging.invalid";
>+
>+   let source = "From - Sat Nov  1 12:39:54 2008\n" +

Three-space indent, eh?  That's a curious choice.  ;)

>+function addIdentities() {
>+  // this doesn't work

I think you should remove it if you got something else working.
Also, I wonder if you needed to call this before you called any
AccountManager stuff.  I know Bienvenu had some test code that did
something extremely similar, and it worked.

>+function test_addFolderAndMailAndIdentity(){
>+
>+  //mc.sleep(20000);
>+  
>+  dump("\nbefore add identites\n");
>+  //mc.sleep(3000);
>+  addIdentities();
>+  dump("\nafter add identites\n");
>+  
>+  addMessageToFolder(testFolder);  
>+}
>+
>+function test_Reply_To_List(){
>+  be_in_folder(testFolder);
>+    
>+  dump("\n\nbefore open the message\n\n");
>+
>+  select_click_row(0);
>+  plan_for_new_window("mail:messageWindow");
>+  open_selected_message();
>+  
>+  dump("\n\nbefore clicking reply to list\n\n");
>+  replyToListWindow = composeHelper.open_compose_with_reply_to_list();
>+}

I don't really like it when one test depends on another running before it.
Perhaps you could just have this be one test that adds the identities, and
then the message, and then checks the reply-to-list.  Or put the identities
and message adding code in setupModule, maybe, and open and close the
replyToListWindow in each test.

Also, neither of the above two things are really tests, I don't think.

>+function test_From_Address()
>+{ 
>+   var identityList = replyToListWindow.e("msgIdentity");
>+   dump("\nselected identity: " + identityList.selectedItem.label + "\n");
>+   
>+   // see if it's the correct identity selected
>+   
>+   if(identityList.selectedItem.label.indexOf(identityString1) < 0)
>+        throw new Error("The From address is not correctly selected! Expected: " + identityString1 + "; Actual: " + identityList.selectedItem.label);

I'm sure you can break this up into more than one line of less than 80
characters each.  ;)

>+   else
>+       dump("Correct identity selected!");
>+}

So, I'm going to give it an r-, mainly because I want to see the changes
before you ask someone else for a review, but I like the direction you're
going, and think that most of my complaints are stylistic things.

Later,
Blake.

	Blake Winton (:bwinton) (:☕️)
	Comment 17 • 14 years ago

Comment on attachment 434799 [details] [diff] [review]
Revised Patch

>+++ b/mail/base/content/mailCommands.js
>@@ -114,52 +114,90 @@ function 
>+  // If we treat reply from sent specially, do we check for that fol
>+  // der flag here ?

You shouldn't really split words across lines like that.  ;)  (Just move all of "folder" to the next line.  And get rid of the space before the ?, while you're changing stuff.)

>+    while((tempIdentity = currentHeaderData[key])!=undefined) {

We need spaces around the "!=".

>+    // reverse the array so that the last delivered-to header will show at front

We use full sentences for our comments, with capital letters at the start, and periods at the end.  :)

>+    let identityFound = false;
>+    for (let i=0; i<deliveredTos.length; i++) {
>+        for each (var tempID in fixIterator(allIdentities,

Two space indents, please.

>+++ b/mail/base/content/msgHdrViewOverlay.js
>@@ -471,17 +471,18 @@ var messageHeaderSink = {
>+          else if (lowerCaseHeaderName == "delivered-to")
>+            this.mDummyMsgHeader.deliveredTo = header.headerValue;          

You've got a lot of extra space at the end of this line.

>+++ b/mail/test/mozmill/message-header/test-reply-to-list-from-address-selection.js
>@@ -0,0 +1,146 @@
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation, Inc.

Hmm…  I don't know if the Mozilla Foundation is incorporated.  Maybe just leave it as "The Mozilla Foundation." instead.

There are also some lines with trailing spaces in this file which should be removed.

>+  var server = acctMgr.createIncomingServer("nobody", 
>+                             "Test Local Folders", "pop3");

The "Test Local Folders" should line up with the "nobody" on the previous line.

So, in summary, there's some cleanup for you to do, but no major changes, so you get an r+ from me.  Now you need to set the "addl. review" flag down below to ? to get your second review.

Good luck,
Blake.

	Mark Banner (:standard8)
	Comment 19 • 14 years ago

Comment on attachment 437632 [details] [diff] [review]
Revised first patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

I'm a bit busy at the moment, I'm going to pass this to Magnus to see if he has time to review. It'll need superreview once it has review, so feel free to come back to me for that.

	Magnus Melin [:mkmelin]
	Comment 20 • 14 years ago

Comment on attachment 437632 [details] [diff] [review]
Revised first patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)


>+  let server="";
>+  let hintForIdentity="";
>+
>+  if(type == Components.interfaces.nsIMsgCompType.ReplyToList) {

Style nit; always put space around = and for if/else and other clauses. There are a lot of those, but i'll only comment this one. So, like

let server = ""
let hintForIdentity = ""

if (type == ....)

Please add a code comment about what this block is trying to achieve.

>+    var index = 0;

Please be consistent with var/let. Only use var if you need it.

>+    var key = "delivered-to";
>+    var allIdentities = accountManager.allIdentities;
>+
>+    var tempIdentity = "";
>+
>+    // Get the delivered-to headers.
>+    var deliveredTos = new Array();
>+    while((tempIdentity = currentHeaderData[key]) != undefined) {

You should be able to write just
while (tempIdentity = currentHeaderData[key])

>+    for (let i=0; i<deliveredTos.length; i++) {
>+      for each (var tempID in fixIterator(allIdentities,
>+                Components.interfaces.nsIMsgIdentity)) {
>+        if(deliveredTos[i].indexOf(tempID.email.toLowerCase())!=-1) {
>+          hintForIdentity = tempID.email;
>+          identityFound = true;
>+          break;
>+        }
>+      }
>+      if(identityFound)
>+        break;

You don't need the identityFound variable. Just check |if (hintForIdentity)|



>+ * The Original Code is Thunderbird Mail Client.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation.

You?

>+ * Portions created by the Initial Developer are Copyright (C) 2009

2010?


>+/*
>+ * Test that TB can automatically select the "From" address 
>+ * in the "reply-to-list" mode.
>+ */

"the most suitable identity From address for reply-to-list"

>+var MODULE_NAME = 'test-reply-to-list-from-address-selection';

Minor nit: for code consitency it's preferable to use double quotes except when when you need single quotes.


>+var addMessageToFolder = function (aFolder) {
>+  var msgId = Components.classes["@mozilla.org/uuid-generator;1"]
>+              .getService(Components.interfaces.nsIUUIDGenerator).generateUUID()+


We typlically line these up as
 Components.classes
           .getService

..., but then again. Cc is defined here.

>+  dump("\nSelected identity: " + identityList.selectedItem.label + "\n");

Remove this and the other dump.

>\ No newline at end of file

Whops.

Other than those, seems to work fine.

	Kefu Zhao Assignee
	Comment 21 • 14 years ago

Hi Magnus,

Thank you so much for reviewing my patch. 
I have created a new patch which takes your suggestions. Please give it a look when you have time. 

Thanks again.

Kefu

	Blake Winton (:bwinton) (:☕️)
	Comment 22 • 14 years ago

Comment on attachment 411787 [details] [diff] [review]
not working WIP

(Marking this obsolete, cause Kefu seems to have a better patch.)

	Mark Banner (:standard8)
	Comment 23 • 14 years ago

Comment on attachment 439744 [details] [diff] [review]
Revised first patch that takes "delivered-to" headers into consideration when deciding the From address. (test included)

David knows a bit more about how libmime is working there, so I'm going to let him sr it.

	Magnus Melin [:mkmelin]
	Comment 24 • 14 years ago

(In reply to comment #21)
> I have created a new patch which takes your suggestions. Please give it a look

You only addressed part of them
For instance "if(" should always be "if (", and you still mix "var" and "let" oddly, there's still a dump statement. 
Also there's a few lines with spaces on empty lines and spaces at the end of some lines - please remove those.

	Kefu Zhao Assignee
	Comment 25 • 14 years ago

I am sorry that I didn't clean up the code thoroughly last time, but here is my revised patch with all the "dump" statement removed. Also, some extra space at the end of some lines are removed.

	Magnus Melin [:mkmelin]
	Comment 26 • 14 years ago

Comment on attachment 440022 [details] [diff] [review]
Revised Patch with all the "dump" removed

Looks good, thx! r=mkmelin

	Magnus Melin [:mkmelin]
	Comment 27 • 14 years ago

Minor nit: i<deliveredTos.length should be i < deliveredTos.length

	David :Bienvenu
	Comment 28 • 14 years ago

Comment on attachment 440022 [details] [diff] [review]
Revised Patch with all the "dump" removed

sr=me for the mime changel

	Ludovic Hirlimann [:Usul]
	Comment 29 • 14 years ago

(In reply to comment #27)
> Minor nit: i<deliveredTos.length should be i < deliveredTos.length

Kefu can you provide an updated patch with that change ?

	Kefu Zhao Assignee
	Comment 30 • 14 years ago

Hey guys, I am sorry for the late reply, but here is the final patch with the nits fixed.

	Mark Banner (:standard8)
	Comment 31 • 14 years ago

Checked in: http://hg.mozilla.org/comm-central/rev/8d62b8a94643

Thanks Kefu.

	Tilman Vogel
	Comment 32 • 13 years ago

Could this be made configurable? In my case the last delivered-to entry that matches any of my identities is due to mail forwarding from my "official" address to my mail storage provider.

I want to reply to the mailing list from the "official" address which is registered and has posting privileges. This one is also my default identity and I don't want to pick that one each time manually (and half the times I forget and need to cancel moderated posts...)

I think, the implemented approach is too heuristical to work in all circumstances, so it should be optional.

	Tilman Vogel
	Comment 33 • 13 years ago

This "enhancement" seriously affects my user experience. Could you please reopen and reconsider it?

	Tony Mechelynck [:tonymec]
	Comment 34 • 13 years ago

(In reply to Tilman Vogel from comment #33)
> This "enhancement" seriously affects my user experience. Could you please
> reopen and reconsider it?

I think the best you could do is file another "enhancement" bug mentioning this one, to make the behaviour configurable. But you should prepare very convincing and detailed arguments if you don't want it to be resolved WONTFIX.

	Tilman Vogel
	Comment 35 • 13 years ago

(In reply to Tony Mechelynck [:tonymec] from comment #34)
> I think the best you could do is file another "enhancement" bug mentioning
> this one, to make the behaviour configurable. But you should prepare very
> convincing and detailed arguments if you don't want it to be resolved
> WONTFIX.

Have you read my comment #32? Is it plausible that this enhancement does not work as intended in my case and that it affects the usability of Thunderbird with mailing lists and redirected accounts in general?

	Magnus Melin [:mkmelin]
	Comment 36 • 13 years ago

Tilman: That's possible, but it's better to create a new bug for discussing and finding a resolution to your problem. Please cc on the bug.

	Tilman Vogel
	Comment 37 • 13 years ago

I just opened bug 679295. How can I cc this bug?

	Magnus Melin [:mkmelin]
	Comment 38 • 13 years ago

Typo, I meant "please cc me" :)