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

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

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).

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.

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

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

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.

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

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.

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

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

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.

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.

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.

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

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.

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.

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.

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.

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

Comment on attachment 411787 [details] [diff] [review] not working WIP (Marking this obsolete, cause Kefu seems to have a better patch.)

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.

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

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.

Comment on attachment 440022 [details] [diff] [review] Revised Patch with all the "dump" removed Looks good, thx! r=mkmelin

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

Comment on attachment 440022 [details] [diff] [review] Revised Patch with all the "dump" removed sr=me for the mime changel

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

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

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

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.

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

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

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

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.

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

Typo, I meant "please cc me" :)