654009 - Reply to list: automatically determine From: address [SeaMonkey Part]

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, enhancement)

Description

[Philip Chee]

+++ This bug was initially created as a clone of Bug #461669 +++

> 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

Comment 1

[Philip Chee]

Attachment: Patch v1.0

>  function GetIdentityForHeader(aMsgHdr, aType)
> 
> +    // Reverse the array so that the last delivered-to header will show at front.
> +    deliveredTos.reverse();
> +
> +    // Get the last "delivered-to" that is in the defined identities.
> +    for (let i = 0; i < deliveredTos.length; i++) {

I don't understand why Thunderbird didn't just start at the end of the array and then i--

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #1)
> I don't understand why Thunderbird didn't just start at the end of the array
> and then i--

tl;dr: That should work.

I'd guess that's the basic difference: They're less picky when it comes to things like this. Effectiveness is the same and efficiency doesn't make a huge difference here. And the patch author (listed as "(New to Bugzilla) (First Patch)") already had to go through dozens of comments and half a dozen iterations so may they tried to be nice.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 529373 [details] [diff] [review]
Patch v1.0

>+  if (aType == Components.interfaces.nsIMsgCompType.ReplyToList) {
>+    var key = "delivered-to";
>+
>+    var tempIdentity = "";
>+
>+    // Get the delivered-to headers.
>+    var deliveredTos = [];
>+    var index = 0;
>+    while (tempIdentity = currentHeaderData[key]) {
>+      deliveredTos.push(tempIdentity.headerValue.toLowerCase());
>+      key = "delivered-to" + index++;
>+    }

Nit: "key" and "tempIdentity" should probably go directly above the loop, too, since they are only used there.


Ratty, do you know the minimum requirements to test this? Do I need to install that "ReplyToList add-on", have at least two identities and fake a mail from a mailing list?

Comment 4

[Philip Chee]

> Ratty, do you know the minimum requirements to test this? Do I need to install
> that "ReplyToList add-on", have at least two identities and fake a mail from a
> mailing list?

Bah the mailing list I was testing this on had the Reply-To: header set up properly so I wasn't testing for what I thought.

And now that you mentioned it yes I think we have to install the ReplyToList addon since we didn't implement Bug 45715.

Comment 5

[Philip Chee]

I don't know. Does SeaMonkey run mozmill tests? If so I'll try porting the mozmill test in the TB patch.

Comment 6

[[e-mail]]

(In reply to comment #5)
> Does SeaMonkey run mozmill tests?
Not as far as I know.

Comment 7

[Philip Chee]

Without having to port Bug 45715 - "Reply to List" [button/(context) menu item]
What we need to do is to create a fake .eml based on the headers in the TB mozmill test

var msgId = Cc["@mozilla.org/uuid-generator;1"]
              .getService(Ci.nsIUUIDGenerator)
              .generateUUID() + "@mozillamessaging.invalid";

var source = "From - Sat Nov  1 12:39:54 2008\n" +
              "X-Mozilla-Status: 0001\n" +
              "X-Mozilla-Status2: 00000000\n" +
              "Delivered-To: <[e-mail]>\n" +
              "Delivered-To: <" + identityString1 + ">\n" +
              "Delivered-To: <[e-mail]>\n" +
              "Message-ID: <" + msgId + ">\n" +
              "Date: Wed, 11 Jun 2008 20:32:02 -0400\n" +
              "From: Tester <[e-mail]>\n" +
              "User-Agent: Thunderbird 3.0a2pre (Macintosh/2008052122)\n" +
              "MIME-Version: 1.0\n" +
              "List-ID: <list.mozillamessaging.invalid>\n" +
              "List-Post: <list.mozillamessaging.invalid>, \n" +
              "    <mailto: [e-mail]>\n" +
              "To: [e-mail]\n" +
              "Subject: " + "a subject" + "\n" +
              "Content-Type: text/html; charset=ISO-8859-1\n" +
              "Content-Transfer-Encoding: 7bit\n" +
               "\n" + "text body" + "\n";

Then call:
ComposeMsgByType(Components.interfaces.nsIMsgCompType.ReplyToList) with the message focused.

Comment 8

[[e-mail]]

Comment on attachment 529373 [details] [diff] [review]
Patch v1.0

>+    while (tempIdentity = currentHeaderData[key]) {
I'd prefer this to use (key in currentHeaderData)

>+    // Reverse the array so that the last delivered-to header will show at front.
>+    deliveredTos.reverse();
>+
>+    // Get the last "delivered-to" that is in the defined identities.
Not sure why we can't just set the hint to deliveredTos.reverse().join(",")
(or build up the hint as we go along).

>+    hintForIdentity = aMsgHdr.recipients + aMsgHdr.ccList;
[This could probably use an extra ","]

Comment 9

[Philip Chee]

It looks like this is pretty useless without implementing Thunderbird Bug 45715 to get some ReplyToList UI. And that requires string changes so =>FUTURE/SM2.2.
I'm going to KIV this until after 2.1 is out.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #9)
> I'm going to KIV this until after 2.1 is out.

Philip, if you're not actively working on this, maybe we should add it to the status meeting notes under Feature List, Planning?

Comment 11

[Philip Chee]

> Philip, if you're not actively working on this, maybe we should add it to the > status meeting notes under Feature List, Planning?
You mean for the next meeting? or in the minutes of the meeting on 26th? Anyway this is a good idea but needs to note that Bug 45715 has to be ported as well.

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #11)
> or in the minutes of the meeting on 26th?

Added there so it semi-automatically propagates to the following ones.

Comment 13

[Tony Mechelynck [:tonymec]]

The "Display Mailing List Header" extension helps, but not fully. It works for me on SeaMonkey 2.6a1 with compatibility override (even though it only advertises compatibility for Tb 1.0 - 2.0.0.*, Sm 1.0 - 1.0) and even without Mnenhy or Enigmail enabled (features "Post to List" with normal headers displayed, and in addition "List Help" "List Subscribe" and "List Unsubscribe" with full headers).

Comment 14

[Tony Mechelynck [:tonymec]]

Sorry: the same author has also a "Reply to List" extension, http://www.juergen-ernst.de/addons/replytolist.html : it works on all current SeaMonkey versions AFAICT (adding a "customizable" Reply-to-List button to the palette) after adding an <em:targetApplication> section to install.rdf.

Comment 15

[[e-mail]]

Attachment: Possible patch

The "enabled" test is a bit messy so I went for hiding the menuitems instead.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

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

AFAICS this works as advertised. I wonder why you're using "y" as accesskey, though. Wouldn't "N" for Newsgroups, "L" for List and "S" for Sender be more appropriate?

Comment 17

[[e-mail]]

(In reply to Jens Hatlak from comment #16)
> AFAICS this works as advertised. I wonder why you're using "y" as accesskey,
> though. Wouldn't "N" for Newsgroups, "L" for List and "S" for Sender be more
> appropriate?
Admittedly I didn't look to see whether "L" was free; I'm using "y" because the existing "Reply to Newsgroup" uses it, and so I knew there were no conflicts. FYI "N" is used by "New Message" and "S" by "Save As".

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #17)
> Admittedly I didn't look to see whether "L" was free; I'm using "y" because
> the existing "Reply to Newsgroup" uses it, and so I knew there were no
> conflicts. FYI "N" is used by "New Message" and "S" by "Save As".

I guess you're referring to what's free according to messenger.dtd. In the real world, where's a clash for "N"? There is no "New Message" in the Reply button drop-down and none in the message context menu either. You're right about "Save As", though.

Anyway, not much of a problem to me if you keep "y"; personally I'm not using accesskeys (I'm a big fan of shortcuts, though).

Comment 19

[[e-mail]]

"N", as I said, is used by "New Message". It's in the "Message" menu.

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #19)
> "N", as I said, is used by "New Message". It's in the "Message" menu.

Ah, I forgot to check the menu. Initially I only checked the button drop-down and found your statements confusing (I'm still wondering whether it's clever that button drop-downs share accesskeys with menus).

Comment 21

[Ian Neal]

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

>+++ b/suite/mailnews/mailWindowOverlay.js
Should a comment be added here to explain what the code does? (As other code blocks have)
>+  var replyListMenuItem = document.getElementById("replyListMainMenu");
>+  if(replyListMenuItem)
>+  {
>+      replyListMenuItem.setAttribute("hidden", !isNews && IsListPost() ? "" : "true");
>+  }
>+
Within this function there is a mixture of .hidden = <expr> and setAttribute("hidden", <expr>) which is the best to settle on?
Similarly for space between if and ( and whether to use {} for a single line if. My preference is to use .hidden if it works, have the space but not the {}.

+++ b/suite/mailnews/mailWindowOverlay.xul
Should we have a key for Reply to List? TB uses "l" with modifiers="accel, shift"

>+++ b/suite/mailnews/msgHdrViewOverlay.js
Don't we also need to port the changes to processHeaders and nsDummyMsgHeader.prototype from Bug 45715?

>+function IsListPost()
>+{
>+  if ("list-post" in currentHeaderData)
>+    return /<mailto:.+@.+>/.test(currentHeaderData["list-post"].headerValue);
>+
>+  return false;
>+}
Wouldn't it be great if messages knew what they were... (out of scope of this bug obviously)

Should we change the default reply mode for list posts?

Comment 22

[[e-mail]]

(In reply to Ian Neal from comment #21)
> (From update of attachment 682450 [details] [diff] [review])
> Should a comment be added here to explain what the code does? (As other code
> blocks have)
Will do.

> Within this function there is a mixture of .hidden = <expr> and
> setAttribute("hidden", <expr>) which is the best to settle on?
I just copied and pasted a block; I should have looked more carefully as to which block I copied and pasted. I'll copy the newer style.

> Should we have a key for Reply to List? TB uses "l" with modifiers="accel,
> shift"
I didn't add one because Reply to Sender Only doesn't have one either. We probably need to decide on how to use our scarce access keys effectively, for instance one alternative proposal is to use Accel+Shift+L for forward as opposite of default type.

> Don't we also need to port the changes to processHeaders and
> nsDummyMsgHeader.prototype from Bug 45715?
Indeed, otherwise we can't reply to list posts in .eml files!

> Should we change the default reply mode for list posts?
I don't think Thunderbird does; I didn't look for a bug on it.

Comment 23

[[e-mail]]

Pushed comm-central changeset 3de3b092de26.

Comment 24

[Philip Chee]

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

> +function InitMessageReply(aPopup)
> +{
> +  var isNews = gFolderDisplay.selectedMessageIsNews;
> +  //For mail messages we say reply. For news we say ReplyToSender.
> +  // We show Reply to Newsgroups only for news messages.
> +  aPopup.childNodes[0].hidden = isNews; // Reply
> +  aPopup.childNodes[1].hidden = isNews || !IsListPost(); // Reply to List
> +  aPopup.childNodes[2].hidden = !isNews; // Reply to Newsgroup
> +  aPopup.childNodes[3].hidden = !isNews; // Reply to Sender Only
If an extension inserts its own menu items then this stops working. Yes I know these menu items don't have IDs.

> +function IsListPost()
Certainly a more sensible name than the Thunderbird IsReplyListEnabled() but I wonder if it would be slightly better for extension compatibility?

ex post facto f+

Comment 25

[Karsten Düsterloh]

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

Cool to see this implemented finally! :)

But it doesn't fully work for me: 
- Using the drop down list entry on the Reply toolbar button or from the Message menu do work; a compose window opens and the list address is preset as To header.
- Using the thread pane's context menu list entry opens the compose window with the quoted message, but no To address is prefilled.

I see no suspicious errors at the console.

Comment 26

[Ian Neal]

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

>+    <menuitem id="mailContext-replyList"
>+              label="&contextReplyList.label;"
>+              accesskey="&contextReplyList.accesskey;"
>+              oncommand="MsgReplyGroup(event);"/>
This should probably be MsgReplyList(event)