Closed Bug 499180 Opened 11 years ago Closed 11 years ago

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

Categories

(Thunderbird :: Message Reader UI, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: clarkbw, Assigned: davida)

Details

Attachments

(1 file, 2 obsolete files)

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 <clarkbw@example.com>
subject:  Extra line in detailed header view
reply-to: clarkbw@example.com

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.
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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → david.ascher
Attachment #398530 - Flags: ui-review?(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();
>     },
>
Attachment #398530 - Flags: ui-review?(clarkbw) → ui-review+
Attached patch updated patch (obsolete) — Splinter Review
here's an updated patch.  feels safe to me for b4, and best to have it in b4 if possible.
Attachment #398530 - Attachment is obsolete: true
Attachment #399749 - Flags: review?(mkmelin+mozilla)
Attachment #399749 - Flags: approval-thunderbird3?
Attachment #399749 - Flags: review?(mkmelin+mozilla) → review+
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.
(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.
patch w/ review comments addressed (modulo followup bug).  carrying forward r+ and ui-review+
Attachment #399749 - Attachment is obsolete: true
Attachment #399783 - Flags: ui-review+
Attachment #399783 - Flags: review+
Attachment #399749 - Flags: approval-thunderbird3?
Attachment #399783 - Flags: approval-thunderbird3?
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.
Or we could try to pull a display name from the address book for reply-to cases that offer no display name.
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).
Attachment #399783 - Flags: approval-thunderbird3? → approval-thunderbird3+
Checked in: http://hg.mozilla.org/comm-central/rev/1e3c9a227b04
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b4
This is still not fixed.  Reply-to field shows even though from is the same address.
(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.
You need to log in before you can comment on or make changes to this bug.