If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Multi-message and thread summaries should use FormatDisplayName from bug 474721

RESOLVED FIXED in Thunderbird 3.3a1

Status

Thunderbird
Message Reader UI
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

unspecified
Thunderbird 3.3a1
x86
All
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 442820 [details] [diff] [review]
Use FormatDisplayName in selectionsummaries.js

In bug 474721, I added a common FormatDisplayName function for the message header. For consistency, this should be used wherever possible. This bug is about doing it for the multi-message and thread summary views. Attached is a patch that does this, but doesn't have any tests (yet).

Old way:
--------
Uses extractHeaderAddressName, which works like the message list (header display name, falling back to address)

New way:
--------
Uses FormatDisplayName, which returns 1) "You" (or "You <your@address>"), 2) the address book display name, 3) the header display name when PreferDisplayName=0, or 4) null (see bug 474721 comment 38 for full details). If FormatDisplayName returns null, fall back to the old way.

Updated

7 years ago
Attachment #442820 - Flags: review?(bugmail)
Blake would be the nominal reviewer for this, especially since he reviewed the other bit of this patch.  He may want to defer review to protz for portions, given that protz is quite familiar with the selection summary code.

Would this patch have bitrotted with the other patch?
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attachment #442820 - Flags: review?(bugmail) → review?(bwinton)
Comment on attachment 442820 [details] [diff] [review]
Use FormatDisplayName in selectionsummaries.js

Yeah, I think I'll let protz take this one.  :)
Attachment #442820 - Flags: review?(bwinton) → review?(jonathan.protzenko)
(Assignee)

Comment 3

7 years ago
(In reply to comment #1)
> Would this patch have bitrotted with the other patch?

I tried it and it looks good.
Comment on attachment 442820 [details] [diff] [review]
Use FormatDisplayName in selectionsummaries.js

Patch looks fine, but:
- you might want to make sure numAddresses > 0, otherwise you'll get nasty exceptions thrown (malformed headers do exist), which would prevent the thread summary from being displayed correctly,
- you need tests. There's test-summarization.js in tests/folder-display already, you just need to add a quick test to make sure that the behavior is the one you expect.

You can re-request r? from me from the next patch.
Attachment #442820 - Flags: review?(jonathan.protzenko) → review-
(Assignee)

Comment 5

7 years ago
Created attachment 471793 [details] [diff] [review]
Add tests, handle "no sender" case

This addresses the above review comments and adds tests. The "no sender" case just returns the empty string. Maybe this could be improved, but I'm not sure it would ever happen in practice, so I'm not too worried.
Attachment #442820 - Attachment is obsolete: true
Attachment #471793 - Flags: review?(jonathan.protzenko)
(In reply to comment #5)
> Created attachment 471793 [details] [diff] [review]
> Add tests, handle "no sender" case
Thanks for your work on this patch!
> 
> This addresses the above review comments and adds tests. The "no sender" case
> just returns the empty string. Maybe this could be improved, but I'm not sure
> it would ever happen in practice, so I'm not too worried.

Yes, that's fine. I don't think we should anything subtle in that case, although you might want to return the raw header value instead of the empty string.

The patch looks great. I don't have a development machine at hand, so I can't test the patch right now, but I'll probably give you r+ sometime next week, i.e. as soon as I can run the test myself.
(Assignee)

Comment 7

7 years ago
Created attachment 472995 [details] [diff] [review]
Better fallback for malformed headers

Ok, this version falls back to the raw header value if it can't parse the header.
Attachment #471793 - Attachment is obsolete: true
Attachment #472995 - Flags: review?(jonathan.protzenko)
Attachment #471793 - Flags: review?(jonathan.protzenko)
Comment on attachment 472995 [details] [diff] [review]
Better fallback for malformed headers

The tests pass, and a little bit of testing seems to demonstrate the expected behavior, so r+ing :). Thanks!
Attachment #472995 - Flags: review?(jonathan.protzenko) → review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/98799925a9fd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
You need to log in before you can comment on or make changes to this bug.