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.
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)
(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-
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.
(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.
Ok, this version falls back to the raw header value if it can't parse the header.
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+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
You need to log in before you can comment on or make changes to this bug.