684865 - When receiving a new email, the notification doesn't display the email address of the sender when he has no name

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Frédéric Buclin]

Some users didn't set their real name when sending an email, so only their email address is displayed in the From: field, i.e.

  [e-mail]

instead of

  Mr Foo <[e-mail]>

When such emails arrive in the inbox, the notification which popup doesn't mention who sent the email. All we see is "... from " at the end of the message. If no name is set, then it should fallback to "... from [e-mail]" so that we can at a glance see who sent the email and decide if it should be read immediately or can wait a bit while doing some other work.

Comment 1

[Frédéric Buclin]

Note to self: newMailNotification_messagetitle is located in chrome/en-US/locale/en-US/messenger/messenger.properties and is called by nsMessengerUnixIntegration::BuildNotificationBody() in mailnews/base/src/nsMessengerUnixIntegration.cpp line 347 only.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix

msg_parse_Header_addresses returns 0-length string, so we should also check the string.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 603152 [details] [diff] [review]
Fix

I'm not quite sure I see why this is happening. test_nsIMsgHeaderParser2.js already has at least one test for this and we use the function in lots of places.

Can you try to extend the test_nsIMsgHeaderParser2.js test to show this case please?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix with a new test

I do not prefer that a test checks several things (in this case this bug and others) because it's difficult to find cause if regression happens.

Comment 5

[Magnus Melin [:mkmelin]]

If the thing to test is very related, keeping it in a separate file isn't nice imho. E.g. it makes it very difficult to find when creating related tests. (And at the very least, the file name of the test should be descriptive.)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Magnus Melin from comment #5)
> If the thing to test is very related, keeping it in a separate file isn't
> nice imho. E.g. it makes it very difficult to find when creating related
> tests. (And at the very least, the file name of the test should be
> descriptive.)

I do not think it is difficult to create related tests. But I agree your comment in parentheses. Bug number file name is too bad. That was my neglect. Can you suggest better name for this test please?

Comment 7

[Magnus Melin [:mkmelin]]

test_nsIMsgHeaderParser-parseHeadersWithArray.js? BTW, the test file doesn't have a licence block (MPL2 or such).

Comment 8

[Mark Banner (:standard8)]

Just to follow up on the separate test file - I'm happy with that, in theory it could be said the parseHeadersWithArray check shouldn't actually be in test_nsIMsgHeaderParser2.js, as it isn't quite related to the tests in the rest of the file.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 609667 [details] [diff] [review]
Fix with a new test

>diff --git a/mailnews/mime/test/unit/test_bug684865.js b/mailnews/mime/test/unit/test_bug684865.js

Please name this file test_parseHeadersWithArray.js and update the xpcshell.ini to match. r=me with that fixed.

Comment 10

[Ludovic Hirlimann [:Usul]]

Hiro ?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Change the test file name,

carrying over review+.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 623582 [details] [diff] [review]
Change the test file name,

Oops! Wrong patch

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: This is correct!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/83a7ea3d40c2

Comment 15

[David :Bienvenu]

So an alternative would be to change

      author.Assign(names[0] ? names[0] : emails[0]);

to 

author.Assign(names[0] && *(names[0]) ? names[0] : emails[0]);

in http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessengerUnixIntegration.cpp#337