499180 - hide reply-to when it is the same as the from address

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

In the detailed header view we display the reply-to address even if it's the same as the from address.  It seems like we should be doing a simple check to see if they are the same and hiding the reply-to address if so.

ex:

from:     Bryan <[e-mail]>
subject:  Extra line in detailed header view
reply-to: [e-mail]

This would save an extra line of header space for what I don't think is useful information.  I couldn't find an existing bug for this.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I bet there are more cases than just that where it makes sense to hide the reply-to.  For example, in cases where we're only showing the display name (and not the email address) in the from line.

Comment 2

[David Ascher (:davida)]

Attachment: patch v1

This patch hides the Reply-To: line if its value matches _either_ the From header (in which case it's completely useless), or the To header, which is the case for many mailing lists.  (A later version of this patch could detect matches in substrings of the To field, but that seems like more complexity than needed at this stage).

In my test data, the (few) messages that end up with a reply-to header showing are those which seem to have _some_ real value, e.g. "noreply@facebook", which is somewhat useful and hard to automate.

Asking for ui-review first.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 398530 [details] [diff] [review]
patch v1

ui-r+ as the idea here is great.  However this patch failed on me with errors of msgHeaderParser until I added the declaration like below.

>diff --git a/mail/base/content/msgHdrViewOverlay.js b/mail/base/content/msgHdrViewOverlay.js
>--- a/mail/base/content/msgHdrViewOverlay.js
>+++ b/mail/base/content/msgHdrViewOverlay.js
>@@ -494,6 +494,24 @@
>           delete currentHeaderData.sender;
>       }
> 
>+      if (("from" in currentHeaderData) &&
>+          ("to" in currentHeaderData) &&
>+          ("reply-to" in currentHeaderData)) {

        var msgHeaderParser = Components.classes["@mozilla.org/messenger/headerparser;1"]
                                        .getService(Components.interfaces.nsIMsgHeaderParser);

>+        var replyToMailbox = kMailboxSeparator +
>+          msgHeaderParser.extractHeaderAddressMailboxes(
>+            currentHeaderData["reply-to"].headerValue) + kMailboxSeparator;
>+        var fromMailboxes = kMailboxSeparator +
>+          msgHeaderParser.extractHeaderAddressMailboxes(
>+            currentHeaderData.from.headerValue) + kMailboxSeparator;
>+        var toMailboxes = kMailboxSeparator +
>+          msgHeaderParser.extractHeaderAddressMailboxes(
>+            currentHeaderData.to.headerValue) + kMailboxSeparator;
>+
>+        if (replyToMailbox == fromMailboxes || replyToMailbox == toMailboxes) {
>+          delete currentHeaderData["reply-to"];
>+        }
>+      }
>+
>       this.onEndHeaders();
>     },
>

Comment 4

[David Ascher (:davida)]

Attachment: updated patch

here's an updated patch.  feels safe to me for b4, and best to have it in b4 if possible.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 399749 [details] [diff] [review]
updated patch

>+      if (("from" in currentHeaderData) &&
>+          ("to" in currentHeaderData) &&
>+          ("reply-to" in currentHeaderData)) {
>+        var replyToMailbox = kMailboxSeparator +
>+          msgHeaderParser.extractHeaderAddressMailboxes(
>+            currentHeaderData["reply-to"].headerValue) + kMailboxSeparator;
>+        var fromMailboxes = kMailboxSeparator +
>+          msgHeaderParser.extractHeaderAddressMailboxes(
>+            currentHeaderData.from.headerValue) + kMailboxSeparator;
>+        var toMailboxes = kMailboxSeparator +
>+          msgHeaderParser.extractHeaderAddressMailboxes(
>+            currentHeaderData.to.headerValue) + kMailboxSeparator;
>+
>+        if (replyToMailbox == fromMailboxes || replyToMailbox == toMailboxes) {
>+          delete currentHeaderData["reply-to"];
>+        }

No brace for the one line if please.

Hm, why are we adding kMailboxSeparator+foo+kMailboxSeparator to all of these? Since they are only compared to each other, doesn't it work to just compare the values?

Also, please add a comment explaining what the code block does.

A minor concern is that reply-to addresses tend to often lack the pretty name, so when you reply you get the correct address but users may wonder why the name isn't included too. But i guess we can live with that.

r=mkmelin with the above fixed/explained.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #5)
> A minor concern is that reply-to addresses tend to often lack the pretty name,
> so when you reply you get the correct address but users may wonder why the name
> isn't included too. But i guess we can live with that.

Good point.  Do you think that's an easy fix?  I can file a follow up bug for it so this doesn't get slowed down.

Comment 7

[David Ascher (:davida)]

Attachment: patch with review comments addressed

patch w/ review comments addressed (modulo followup bug).  carrying forward r+ and ui-review+

Comment 8

[Magnus Melin [:mkmelin]]

Standards-wise i think it's working as designed, was just pointing out that it may be unexpected. If we want to "fix" it we could compare and hide reply-to only if the name is also set there. Not sure we want to though.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Or we could try to pull a display name from the address book for reply-to cases that offer no display name.

Comment 10

[Magnus Melin [:mkmelin]]

Doable but there's some wontfixed bug for doing that with replies... (for normal reply when addresses don't have names/have other names than we have in the ab).

Comment 11

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/1e3c9a227b04

Comment 12

[Justin]

This is still not fixed.  Reply-to field shows even though from is the same address.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #12)
> This is still not fixed.  Reply-to field shows even though from is the same
> address.

Justin, can you file a new bug and attach the email that this didn't work for?  Just add a comment here for the new bug when you have it filed.  Otherwise these issues can get lost, thanks.