[RFE] Mozilla Mail should support the setting and reading the mail-followup-to header

RESOLVED FIXED in mozilla1.9.1b2

Status

--
enhancement
RESOLVED FIXED
16 years ago
8 years ago

People

(Reporter: nobody, Assigned: bugs-mozilla-90ximscv)

Tracking

Trunk
mozilla1.9.1b2

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

16 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312

Although not an RFC, the setting and reading of the mail-followup-to header is a
valuable component of actively sending and replying to mailing lists.  It
provides an automated way for posters to indicate whether or not they are
subscribed to lists, and if they desire to be copied on followups.  Supporting
this header minimizes misdirected email and facilitates smooth list flow.

Please see http://cr.yp.to/proto/replyto.html for more discussion.  A
crossplatform Mail client that supports this header would be a boon for users of
mailing lists everywhere.

Reproducible: Always

Steps to Reproduce:

Comment 1

16 years ago
Couldn't find a dupe of this, so confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: MailNews → Core
(Assignee)

Comment 2

14 years ago
Created attachment 167469 [details] [diff] [review]
patch: support for interpreting mail-followup-to and mail-reply-to

The patch adds support for interpreting mail-followup-to and mail-reply-to
header fields in messages I reply to.  It does not add support for setting
these header fields.
(Assignee)

Updated

14 years ago
Attachment #167469 - Flags: review?(ducarroz)
(Assignee)

Updated

14 years ago
Attachment #167469 - Flags: review?(ducarroz) → review?(bienvenu)

Comment 3

14 years ago
Comment on attachment 167469 [details] [diff] [review]
patch: support for interpreting mail-followup-to and mail-reply-to

thx for doing this.

can we get a -uw diff so I can ignore the white space changes?

this can just be else if (!replyTo...

+	   else
+	   { // default behaviour for messages without Mail-Reply-To
+	     if (! replyTo.IsEmpty())
+	     {
+	       compFields->SetTo(replyTo);
+	       needToRemoveDup = PR_TRUE;
+	     }
+	   }
(Assignee)

Comment 4

14 years ago
Created attachment 169079 [details] [diff] [review]
[checked in] patch: support for interpreting mail-followup-to and mail-reply-to - v2

I think ignoring whitespaces is not what we want because there are changes in
indention which would also be inored.  In the new patch I omitted the
unnecessary whitespace changes.
Attachment #167469 - Attachment is obsolete: true
Attachment #169079 - Flags: review?(bienvenu)

Updated

14 years ago
Attachment #169079 - Flags: review?(bienvenu) → review+
(Assignee)

Updated

14 years ago
Attachment #169079 - Flags: superreview?(mscott)
(Assignee)

Updated

14 years ago
Attachment #169079 - Flags: superreview?(mscott) → superreview?(dmose)
(Assignee)

Updated

14 years ago
Attachment #169079 - Flags: superreview?(dmose) → superreview?(Henry.Jia)

Comment 5

14 years ago
I can't see why supporting Mail-Followup-To (or Mail-Reply-To) is useful.

These fields are designed to require (as in MUST) a conforming MUAs to follow
the prescriptive advice.  Imagine if the RFCs required 'return receipts' by
defaults?

So, at best both these headers are suggestions as to where the originator of a
message would like further communication.  There is, already, a defined field
for this purpose: Reply-To.

Should a sender want responses only back to them? Set Reply-To to their own address.

Should a sender want responses back to the list (since they are subscribed)? Set
Reply-To to the list address.

Should a sender want responses both to them and to the list? Set Reply-To to
both the their own and the list addresses.

Now that we know the use cases for both Mail-Followup-To (M-F-T) and
Mail-Reply-To (M-R-T), it should be obvious that generation of those headers is
unnecessary.  It would be good if Mozilla Mail/Thundebird provided some
mechanism to easily set the senders desire (and thus Reply-To appropriately) though.

What should Mozilla Mail/Thunderbird do in the face of M-F-T and M-R-T headers?

Here are the rules that should keep the original senders intent intact as much
as possible:

 - if Reply-To exists, it trumps M-F-T and M-R-T. They are wholley ignored.

 - If Reply-To does not exist, then what you need to do is dependant upon what
kind of response a user is composing and which headers exist.

   - a Reply-To-All and a M-F-T header:
     treat the M-F-T as if it was a Reply-To header

   - a Reply-To-All and a M-R-T header:
     treat the M-R-T as if it was a Reply-To header

   - a Reply and a M-F-T header:
     ignore the contents of the M-F-T

   - a Reply and a M-R-T header:
     treat the M-R-T as if it was a Reply-To header

   - a Reply-To-All and both a M-F-T and M-R-T header
     treat the M-F-T as if it was a Reply-To header

   - a Reply and both a M-F-T and M-R-T header
     it's ambiguous, but I'd suggest:
     treat the M-R-T as if it was a Reply-To header
(In reply to comment #5)
> These fields are designed to require (as in MUST) a conforming MUAs to follow
> the prescriptive advice.

No.  They're intended to override From+To+Cc as a useful default, which of
course the user can still edit.

> There is, already, a defined field for this purpose: Reply-To.

Reply-To has become ambiguous, since the distinction between reply-to-author and
reply-to-all was not recognized early on.  Read the cr.yp.to link.

> Should a sender want responses only back to them? Set Reply-To to their own
> address.
> 
> Should a sender want responses back to the list (since they are subscribed)?
> Set Reply-To to the list address.

No.  This is not what Reply-To means.  Reply-To does not specify whether to
reply to just the author, or to everyone; it says "*if* you reply to the author,
use this address".

MFT does not specify whether to reply to just the author, or to everyone either;
it says "*if* you reply to everyone, use these addresses".

There is no header field that specifies whether replies should go to just the
author, or to everyone.  There is no need for such a header field.  The author
cannot know in advance what the content of any replies will be, and so cannot
predict which audience would be appropriate.

>  - if Reply-To exists, it trumps M-F-T and M-R-T. They are wholley ignored.

Bad idea.  If MFT/MRT are set, they are more likely to match the author's intent
than Reply-To is, because Reply-To is frequently subjected to all kinds of abuse
(being set by mailing list processors, etc.), due to people attributing
different meanings to it.

There is no need for new suggestions on how to interpret MFT/MRT.  The
specification on cr.yp.to spells it out already.  Inventing a new, different
interpretation will only cause interoperability problems with other mail programs.

Comment 7

14 years ago
(In reply to comment #6)

> > There is, already, a defined field for this purpose: Reply-To.
> 
> Reply-To has become ambiguous, since the distinction between reply-to-author and
> reply-to-all was not recognized early on.  Read the cr.yp.to link.

Whether or not Reply-To is ambiguous is arguable. Dan Bernstein, and yourself,
argue it has.  RFC2822 and DRUMS argued it hasn't.  

> > Should a sender want responses only back to them? Set Reply-To to their own
> > address.
> > 
> > Should a sender want responses back to the list (since they are subscribed)?
> > Set Reply-To to the list address.
> 
> No.  This is not what Reply-To means.  Reply-To does not specify whether to
> reply to just the author, or to everyone; it says "*if* you reply to the author,
> use this address".

This is *precisely* what Reply-to means. Please read RFC2822.

> MFT does not specify whether to reply to just the author, or to everyone either;
> it says "*if* you reply to everyone, use these addresses".
> 
> There is no header field that specifies whether replies should go to just the
> author, or to everyone.  There is no need for such a header field.  The author
> cannot know in advance what the content of any replies will be, and so cannot
> predict which audience would be appropriate.

That you for explaining exactly why MFT and MRT are not needed.  "The author
cannot know in advance what the content of any reply will be, and so cannot
predict which audience would be appropriate."

This is why using and advocating M-F-T and M-R-T is a waste of time.  Reply-To
covers all the cases that an author will have, is widely supported and understood.

> There is no need for new suggestions on how to interpret MFT/MRT.  The
> specification on cr.yp.to spells it out already.  Inventing a new, different
> interpretation will only cause interoperability problems with other mail 
> programs.

Fortunately widespread implementation of M-F-T and M-R-T has not occurred. 
Interopability problems already exist with M-F-T as a number of list managers
insert the field if the poster is a subscriber to the list.

Dan's sugesstion in the case of Reply-To and both M-F-T and M-R-T is:

Mail-Followup-To/(To+Cc+(Mail-Reply-To/Reply-To/From)) for follow-up
Mail-Reply-To/Reply-To/From for reply-to-author.

My suggestion changes this to be:

Reply-To/Mail-Followup-To/(To+Cc+(Reply-To/From)) for follow-up
Reply-To/Mail-Reply-To/From for reply-to-author

Dan isn't very interested in backwards compatibility of past email messages but
I feel that it would be a loss if a newer version of Mozilla Mail/Thunderbird
acted unpredictably should I choose to response to an old message.

Inserting Reply-To at the head of the list of fields to choose in the case of a
message containing all three fields would mean later versions would act the same
as previous version in constructing a response.
(In reply to comment #7)
> This is *precisely* what Reply-to means.

You're right; I didn't phrase that well.  It would be better to say: Reply-To
forces the author to make a decision that she is not in a position to make -
i.e., whether replies should be sent privately or publicly.  What she is in a
position to express is where she would like to receive replies herself, whether
she is subscribed to one of the lists in To/Cc, and if the thread topic has
drifted, where a more appropriate list is.  MFT/MRT allows her to express these
things, even simultaneously.  Reply-To does not.

> That you for explaining exactly why MFT and MRT are not needed.  "The author
> cannot know in advance what the content of any reply will be, and so cannot
> predict which audience would be appropriate."

I meant that the author cannot know whether replies should be public or private.
 The author can know which list(s) would likely be appropriate, and which
address should be used for private replies.

> Interopability problems already exist with M-F-T as a number of list managers
> insert the field if the poster is a subscriber to the list.

Since the MFT spec explicitly says that list processors should not do that, we
can write those ones off as clueless.  There will always be clueless people that
violate any spec you put in front of them.  This does not constitute an argument
against the usefulness of the spec.

> Reply-To/Mail-Followup-To/(To+Cc+(Reply-To/From)) for follow-up

If an author sets Reply-To to a different personal address than what is in From,
and I want to follow up to the list, this behavior would get in my way.

> Reply-To/Mail-Reply-To/From for reply-to-author

If a list sets Reply-To to point to the list, and I want to reply to just the
author, this behavior would get in my way.

> Dan isn't very interested in backwards compatibility of past email messages
> but I feel that it would be a loss if a newer version of Mozilla
> Mail/Thunderbird acted unpredictably should I choose to response to an old
> message.

Old messages don't have MFT or MRT.  New versions of Mozilla will behave the
same for those messages regardless of what they do with MFT/MRT.  There is no
compatibility issue here.

Now, if a message has MFT/MRT, then an older Mozilla that didn't recognize
MFT/MRT would behave differently from a newer one that does.  But this is not a
problem, because the new behavior is better: when you choose reply-to-author,
you get MRT, which is the author's preferred address for private replies.  When
you choose reply-to-all, you get MFT, which is the author's suggestion for
public replies.  If Reply-To were used for both cases, then it would necessarily
be wrong for one.

Comment 9

14 years ago
(In reply to comment #8)

Exactly. While I think the previous poster (comment #7) is right that allowing
Reply-To to trump is a more corrrect interpretation of RFC 2822 doing so would,
like you say, get in the way of me dealing with the mail I receive.

In fact, I'd appreciate the following additions to your suggestions:

1) Parse List-Post as a secondary alternative to Mail-Followup-To
2) If Mail-Reply-To is not set but either Mail-Followup-To or List-Post are
found and Reply-To matches them, then assume evil Reply-To munging has occurred
and ignore Reply-To for a reply-to-author operation.

I don't care if these are options that have to be explicitly enabled or are
default behavior.
(In reply to comment #9)
> 1) Parse List-Post as a secondary alternative to Mail-Followup-To

List-Post is set by the list processor on all list messages, so it doesn't tell
you whether the previous author is subscribed, or should get an extra copy.  The
list address will also already be among From+To+Cc, so I don't see what benefit
this gives over From+To+Cc, unless a new reply-to-list-only feature is added. 
Also, for cross-posted threads, this will only give you one of the lists.

> 2) If Mail-Reply-To is not set but either Mail-Followup-To or List-Post are
> found and Reply-To matches them, then assume evil Reply-To munging has
> occurred and ignore Reply-To for a reply-to-author operation.

This could be helpful, yes.

Updated

14 years ago
Attachment #169079 - Flags: superreview?(Henry.Jia) → superreview+

Comment 11

14 years ago
(In reply to comment #7)

> Whether or not Reply-To is ambiguous is arguable. Dan Bernstein, and yourself,
> argue it has.  RFC2822 and DRUMS argued it hasn't.  

False. RFC 2822, sec. 3.6.3 states:

"Some mail applications have automatic reply commands that include the
destination addresses of the original message in the destination addresses of
the reply. How those reply commands behave is implementation dependent and is
beyond the scope of this document. In particular, whether or not to include the
original destination addresses when the original message had a 'Reply-To:' field
is not addressed here."

RFC 2822 deliberately avoids discussing how to handle additional enumerated
recipients whenever a message contains a "Reply-To:" field. The real problem
here is that "Reply-To:" alone cannot handle the real-life situation of a user
who wishes to differentiate between "Reply to the author of this message" and
"Reply to everyone who has received this message". RFC 2822 is content to
consider this an implementation issue. Mail-Followup-To is intended to clarify
this omission of detail by serving as a practical solution for people who don't
like phrases like "an implementation issue".

> > > Should a sender want responses back to the list (since they are subscribed)?
> > > Set Reply-To to the list address.
> > 
> > No.  This is not what Reply-To means.  Reply-To does not specify whether to
> > reply to just the author, or to everyone; it says "*if* you reply to the author,
> > use this address".
> 
> This is *precisely* what Reply-to means. Please read RFC2822.

The "Reply-To:" field as defined in RFC 2822 does not make a distinction between
"Reply to the author" and "Reply to all recipients". Both options are real
scenarios that happen every day, and both can be supported, but not by the
"Reply-To:" field alone. RFC 2822, sec. 3.6.2 states:

"When the 'Reply-To:' field is present, it indicates the mailbox(es) to which
the author of the message suggests that replies be sent. In the absence of the
'Reply-To:' field, replies SHOULD by default be sent to the mailbox(es)
specified in the 'From:' field unless otherwise specified by the person
composing the reply."

The problem here, again, is that the author of a message can have more than one
place to suggest replies be sent. Neither "Reply-To:" nor anything in RFC 2822
make mention of giving authors any control over the different types of replies a
message can create. We need to move beyond RFC 2822 in order to handle the
distinction it ignores between public replies and personal replies.

Comment 12

14 years ago
fix checked in, thx, Daniel.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

14 years ago
The bug is not fixed completely. Look at the second comment:

"The patch adds support for interpreting mail-followup-to and mail-reply-to
header fields in messages I reply to.  It does not add support for setting
these header fields."

My patch fixed only one part, so we can reopen the bug or split it into two bugs.
(In reply to comment #13)
> The bug is not fixed completely. [...] "The patch [...] does not add support
> for setting these header fields."

There is rudimentary support.  Per bug 61520 comment 16, you can specify any 
headers you like on a per-identity basis, but you need to edit the prefs.js file 
manually; there's no UI for it.

Similarly, you can add a line like this:
 user_pref("mail.compose.other.header", "Mail-Followup-To,Mail-Reply-To");

This will place those alternative headers into the dropdown next to the 
addressing fields, so you can add the headers on a per-message basis instead.
(Assignee)

Comment 15

14 years ago
(In reply to comment #14)

> [...] you can specify any headers you like on a per-identity basis [...]

That doesn't work for Mail-Followup-To because one would need this on a per
recipient basis and even that would work only in the simple cases where I have
only one recipient.

> Similarly, you can add a line like this:
> user_pref("mail.compose.other.header", "Mail-Followup-To,Mail-Reply-To");
> 
> This will place those alternative headers into the dropdown next to the 
> addressing fields, so you can add the headers on a per-message basis instead.

That's a good workaround for missing support for setting the MFT header, but you
have to do the whole thing manually.

Maybe I will implement complete MFT support later, but I'll need help.
(Assignee)

Comment 16

14 years ago
Created attachment 184698 [details] [diff] [review]
patch: support for setting mail-followup-to and mail-reply-to

The patch adds support for setting mail-followup-to and mail-reply-to
header fields in outgoing messages.  It does not add support for setting
these header fields.  The configuration (user.js) must contain a list of
mailing list addresses the user is subscribed to and a list of mailing list
addresses where the Reply-To header is set automatically.  Mail-Followup-To
will not be set if the first list is missing and Mail-Reply-To will not be set
if the second is missing.  If the second list's first character is a '*'
Mail-Reply-To will always be set.

Example configuration:
user_pref("mail.identity.id1.mft-addresses", "ml-1@abc.xyz, ml-2@abc.xyz");
user_pref("mail.identity.id1.mrt-addresses", "ml-2@abc.xyz, ml-3@abc.xyz");

Add this line to the config to be able to set MFT and MRT with the UI:
user_pref("mail.compose.other.header", "Mail-Followup-To,Mail-Reply-To");
(Assignee)

Comment 17

14 years ago
Created attachment 184699 [details] [diff] [review]
patch: support for setting mail-followup-to and mail-reply-to

The patch adds support for setting mail-followup-to and mail-reply-to
header fields in outgoing messages.  It does not add support for setting
these header fields.  The configuration (user.js) must contain a list of
mailing list addresses the user is subscribed to and a list of mailing list
addresses where the Reply-To header is set automatically.  Mail-Followup-To
will not be set if the first list is missing and Mail-Reply-To will not be set
if the second is missing.  If the second list's first character is a '*'
Mail-Reply-To will always be set.

Example configuration:
user_pref("mail.identity.id1.mft-addresses", "ml-1@abc.xyz, ml-2@abc.xyz");
user_pref("mail.identity.id1.mrt-addresses", "ml-2@abc.xyz, ml-3@abc.xyz");

Add this line to the config to be able to set MFT and MRT with the UI:
user_pref("mail.compose.other.header", "Mail-Followup-To,Mail-Reply-To");
(Assignee)

Updated

14 years ago
Attachment #184699 - Flags: review?(bienvenu)

Updated

14 years ago
Attachment #167469 - Flags: review?(bienvenu)

Updated

14 years ago
Attachment #184699 - Flags: review?(bienvenu)
(Assignee)

Comment 18

14 years ago
Please reopen the bug because only one part of the work is done.  Look at
comment #2 and comment #13.  My last patch does the second part, but theres a
little bug in it I wasn't able to fix up to now.  The bug occurs if the
generated header doesn't fit in one line.  Any ideas?
(Assignee)

Comment 19

14 years ago
Created attachment 191291 [details] [diff] [review]
patch: support for setting mail-followup-to and mail-reply-to - v2

The new patch fixes the bug mentioned in comment #18.
Attachment #184698 - Attachment is obsolete: true
Attachment #184699 - Attachment is obsolete: true
Attachment #191291 - Flags: review?(ducarroz)

Comment 20

14 years ago
re-opening - Daniel informs me that it's only partly fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

14 years ago
Attachment #191291 - Flags: review?(ducarroz) → review?(Henry.Jia)

Comment 21

13 years ago
Comment on attachment 191291 [details] [diff] [review]
patch: support for setting mail-followup-to and mail-reply-to - v2

I'll try to look at this.
Attachment #191291 - Flags: review?(Henry.Jia) → review?(bienvenu)

Comment 22

12 years ago
What's the status of this? I still get private CCs from Thunderbird users replying to my mailing list posts. Has it been included in any release yet?

Comment 23

11 years ago
Comment on attachment 191291 [details] [diff] [review]
patch: support for setting mail-followup-to and mail-reply-to - v2

My guess is that this patch needs some work to work with frozen linkages...so this patch has unfortunately bit-rotted.
Attachment #191291 - Flags: review?(bienvenu) → review-
Daniel Farber: are you still around? Do you plan on updating the patch? (For the frozen linkage nsXPIDLCString is gone, that's probably the main thing to update)
Assignee: ducarroz → bugs--mozilla.org
Status: REOPENED → NEW
QA Contact: esther → composition

Comment 25

11 years ago
Since I'm still very interested in this feature I'd like to update the patch.  But I'm not familiar with Mozilla development.  These two patches for this bug are my only code contribution so far so I'm not sure what must be changed in the patch.

Is replacing the string class the only thing I have to do?  This should be no problem?!
Yes that would basically be it. (And then get reviews of course.)

See <http://developer.mozilla.org/en/docs/Migrating_from_Internal_Linkage_to_Frozen_Linkage> for pointers about which things not to use.

Comment 27

11 years ago
Created attachment 327347 [details] [diff] [review]
support for setting mail-followup-to and mail-reply-to - v3

A new version of the patch with some code cleanups and without nsXPIDLCString.

To let Thunderbird automatically set the Mail-Followup-To header, the user must provide a list of mailing lists he is subscribed to via
mail.identity.<id#>.subscribed_mailing_lists (comma separated list)

If this setting is missing or empty or if the user set MFT manually in the UI, this patch does nothing.

Similar for Mail-Reply-To:  To let Thunderbird automatically set the Mail-Reply-To header, the user must provide a list of mailing lists he writes to and that mangle the Reply-To header via
mail.identity.<id#>.replyto_mangling_mailing_lists

If this setting is missing or empty or if the user set MRT manually in the UI, this patch does nothing.
Attachment #327347 - Flags: review?

Comment 28

11 years ago
Comment on attachment 327347 [details] [diff] [review]
support for setting mail-followup-to and mail-reply-to - v3

Daniel meant to ask me for a review, but I think I'll push the review to Standard8, and I'll do the sr.
Attachment #327347 - Flags: superreview?(bienvenu)
Attachment #327347 - Flags: review?(bugzilla)
Attachment #327347 - Flags: review?

Comment 29

11 years ago
The previous patch (patch: support for setting mail-followup-to and mail-reply-to - v2, attachment 191291 [details] [diff] [review]) is obsoleted by attachment 327347 [details] [diff] [review], but I am not authorized to edit attachment 191291 [details] [diff] [review].  Can someone with sufficient privileges mark 191291 as obsolete, please?
Attachment #191291 - Attachment is obsolete: true
Attachment #169079 - Attachment description: patch: support for interpreting mail-followup-to and mail-reply-to - v2 → [checked in] patch: support for interpreting mail-followup-to and mail-reply-to - v2
Product: Core → MailNews Core
(In reply to comment #27)
> To let Thunderbird automatically set the Mail-Followup-To header, the user must
> provide a list of mailing lists he is subscribed to via
> mail.identity.<id#>.subscribed_mailing_lists (comma separated list)

Daniel, how are you intending that the user set this after this bug, will it be via some UI, or some other method, or just hidden prefs?
Comment on attachment 327347 [details] [diff] [review]
support for setting mail-followup-to and mail-reply-to - v3

Here are some comments on the patch, note that some of the comments apply throughout the patch.

Would you be interested in trying to write some unit tests for these? See https://wiki.mozilla.org/MailNews:Automated_Testing for more information and please feel free to ask if you need any help. Note, that unit tests aren't a requirement for getting this patch in, but can help us with the review and ensuring that it doesn't regress later.

>+// Add Mail-Followup-To header
>+// See bug #204339 and http://cr.yp.to/proto/replyto.html for details
>+nsresult
>+nsMsgComposeAndSend::AddMailFollowupToHeader() {
>+  nsresult rv;
>+
>+  // Get OtherRandomHeaders...
>+  nsCAutoString customHeaders(mCompFields->GetOtherRandomHeaders());

Using nsDependentCString would be more efficient here. It avoids a string copy.

>+  // ...and look for MFT-Header.  Stop here if MFT is already set.
>+  NS_NAMED_LITERAL_CSTRING(mftHeaderLabel, "Mail-Followup-To: ");
>+  if ((StringHead(customHeaders, mftHeaderLabel.Length()) == mftHeaderLabel) ||
>+      FindInReadable(NS_LITERAL_CSTRING("\r\n") + mftHeaderLabel, customHeaders))

To cope with the frozen API strings, customHeaders.Find(...) would be better.

>+    return NS_OK;
>+
>+  // Get list of subscribed mailing lists
>+  nsCAutoString mailing_lists;
>+  rv = mUserIdentity->GetCharAttribute("subscribed_mailing_lists", mailing_lists);
>+  // Stop here if this list is missing or empty
>+  if (! NS_SUCCEEDED(rv) || mailing_lists.IsEmpty())
>+    return NS_OK;

You should use if (NS_FAILED(rv)...

>+  // Get a list of all recipients excluding bcc
>+  nsCAutoString to, cc, recipients;
>+  to = mCompFields->GetTo();
>+  cc = mCompFields->GetCc();

Again, use nsDependentCString (for to and cc), but please also initialise them with the data on their own lines i.e.
nsDependentCString to(mCompFields->GetTo());

>+  if (to.IsEmpty() && cc.IsEmpty())
>+    // We have bcc recipients only, so we don't add the Mail-Followup-To header
>+    return NS_OK;
>+  if (! to.IsEmpty() && cc.IsEmpty())
>+    recipients = to;
>+  else if (to.IsEmpty() && ! cc.IsEmpty())
>+    recipients = cc;
>+  else
>+    recipients = to + NS_LITERAL_CSTRING(", ") + cc;

Please add a blank line after the return NS_OK. It'll make it easier to read.

Please drop the spaces after ! they aren't necessary, and its not in our general coding style.

For the last recipients assignment, please could you make it:

{
  recipients.Assign(to);
  recipients.AppendLiteral(", ");
  recipients.Append(cc);
}

This will ensure it works with or without the frozen api.

>+  // Create nsIMsgHeaderParser object
>+  nsCOMPtr<nsIMsgHeaderParser> headerParser =
>+          do_GetService(NS_MAILNEWS_MIME_HEADER_PARSER_CONTRACTID);
>+  if (! headerParser)
>+    return NS_ERROR_FAILURE;

Can you indent do_GetService by only two spaces, and if you parse &rv as the second argument, then use NS_ENSURE_SUCCESS(rv, rv); instead of the if ... return.

>+  // Remove duplicate addresses in recipients
>+  nsCAutoString recipients_no_dups;
>+  rv = headerParser->RemoveDuplicateAddresses("UTF-8", recipients.get(),
>+          nsnull, PR_FALSE, getter_Copies(recipients_no_dups));

This API has now changed, thankfully you just have to drop the "UTF-8" argument.

>+  if (! NS_SUCCEEDED(rv))
>+    return NS_ERROR_FAILURE;

Please use NS_ENSURE_SUCCESS(rv, rv); - we'll get a notification in debug builds that something is wrong, and anyone trying to call this function will get the actual error result rather than just a generic result.

>+  // Set Mail-Followup-To
>+  char * mimeHeader = nsMsgI18NEncodeMimePartIIStr(recipients.get(), PR_TRUE,
>+      mCompFields->GetCharacterSet(), mftHeaderLabel.Length(), PR_TRUE);
>+  if (! mimeHeader)
>+    return NS_ERROR_FAILURE;

Please insert a blank line after this.

>+  customHeaders.Append(mftHeaderLabel);
>+  customHeaders.Append(mimeHeader);
>+  customHeaders.Append(NS_LITERAL_CSTRING("\r\n"));

You can just do AppendLiteral("\r\n").
Attachment #327347 - Flags: review?(bugzilla) → review-

Comment 32

11 years ago
(In reply to comment #30)
> Daniel, how are you intending that the user set this after this bug, will it be
> via some UI, or some other method, or just hidden prefs?

Just hidden prefs for now.  Of course a gui would be nice, but I don't want to study Mozilla UI development, at least at this time.

Please keep in mind that MFT-support is a feature for advanced users and these users can easily set up some hidden prefs.  A configuration UI may be implemented later and independent from this patch.

Of course good documentation is necessary, so if there's a wiki or something similar for all these hidden preferences, please give me a pointer, I will provide detailed docs in english and german.

Comment 33

11 years ago
Created attachment 344335 [details] [diff] [review]
support for setting mail-followup-to and mail-reply-to - v4
Attachment #327347 - Attachment is obsolete: true
Attachment #344335 - Flags: superreview?(bienvenu)
Attachment #344335 - Flags: review?(bugzilla)
Attachment #327347 - Flags: superreview?(bienvenu)
Attachment #344335 - Flags: review?(bugzilla) → review+
Comment on attachment 344335 [details] [diff] [review]
support for setting mail-followup-to and mail-reply-to - v4

+    rv = headerParser->RemoveDuplicateAddresses(recipients.get(),
+            nsnull, PR_FALSE, getter_Copies(recipients_no_dups));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // Remove reply-to mangling mailing lists from recipients...
+    nsCAutoString recipients_without_mailing_lists;
+    rv = headerParser->RemoveDuplicateAddresses(recipients_no_dups.get(),
+          mailing_lists.get(), PR_FALSE, getter_Copies(recipients_without_mailing_lists));
+    NS_ENSURE_SUCCESS(rv, rv);

nit: on the indented lines (the continuation of the function calls), please just put them 2-space indented from the previous line.

r=me with that fixed. Sorry again for the big delay in getting the original patch reviewed.

Comment 35

10 years ago
Created attachment 345106 [details] [diff] [review]
[checked in] support for setting mail-followup-to and mail-reply-to - v4.0.1

Should follow Mozilla coding style now.
Attachment #344335 - Attachment is obsolete: true
Attachment #345106 - Flags: superreview?(bienvenu)
Attachment #344335 - Flags: superreview?(bienvenu)

Comment 36

10 years ago
Comment on attachment 345106 [details] [diff] [review]
[checked in] support for setting mail-followup-to and mail-reply-to - v4.0.1

thx for doing this. The settings for the headers should be documented somewhere, on a wiki.mozilla.org page, perhaps. We should have a page or pages that documents all the thunderbird hidden prefs

Updated

10 years ago
Attachment #345106 - Flags: superreview?(bienvenu) → superreview+

Updated

10 years ago
Keywords: checkin-needed
Comment on attachment 345106 [details] [diff] [review]
[checked in] support for setting mail-followup-to and mail-reply-to - v4.0.1

http://hg.mozilla.org/comm-central/rev/a9219080b2f2
Attachment #345106 - Attachment description: support for setting mail-followup-to and mail-reply-to - v4.0.1 → [checked in] support for setting mail-followup-to and mail-reply-to - v4.0.1
Status: NEW → RESOLVED
Last Resolved: 14 years ago10 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2

Comment 38

10 years ago
(In reply to comment #36)
> (From update of attachment 345106 [details] [diff] [review])
> thx for doing this. The settings for the headers should be documented
> somewhere, on a wiki.mozilla.org page, perhaps.

https://wiki.mozilla.org/Thunderbird:Help_Documentation:Mail-Followup-To_and_Mail-Reply-To (not finished yet)

> We should have a page or pages
> that documents all the thunderbird hidden prefs

I didn't find one, so I created a new page:
https://wiki.mozilla.org/Thunderbird:Help_Documentation:Hidden_Preferences

You are welcome to join this page's discussion.
This FIXED bug is flagged with in‑testsuite?   It would be great if assignee or someone else can clear the flag if a test is not appropriate.  And if appropriate, create a test and plus the flag to finish off the bug.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.