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

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Jim Porter (:squib)]

Attachment: 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.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

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?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442820 [details] [diff] [review]
Use FormatDisplayName in selectionsummaries.js

Yeah, I think I'll let protz take this one.  :)

Comment 3

[Jim Porter (:squib)]

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

I tried it and it looks good.

Comment 4

[Jonathan Protzenko [:protz]]

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.

Comment 5

[Jim Porter (:squib)]

Attachment: 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.

Comment 6

[Jonathan Protzenko [:protz]]

(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.

Comment 7

[Jim Porter (:squib)]

Attachment: Better fallback for malformed headers

Ok, this version falls back to the raw header value if it can't parse the header.

Comment 8

[Jonathan Protzenko [:protz]]

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!

Comment 9

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/98799925a9fd