Last Comment Bug 684865 - When receiving a new email, the notification doesn't display the email address of the sender when he has no name
: When receiving a new email, the notification doesn't display the email addres...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: 6 Branch
: x86 Linux
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 756971 757846 761654
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-06 08:29 PDT by Frédéric Buclin
Modified: 2012-06-16 11:30 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix (1.37 KB, patch)
2012-03-05 20:37 PST, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Review
Fix with a new test (2.62 KB, patch)
2012-03-27 03:20 PDT, Hiroyuki Ikezoe (:hiro)
standard8: review+
Details | Diff | Review
Change the test file name, (1.85 KB, patch)
2012-05-13 22:10 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Review
This is correct! (2.70 KB, patch)
2012-05-13 22:40 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Review

Description Frédéric Buclin 2011-09-06 08:29:43 PDT
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.

  foo@bar.com

instead of

  Mr Foo <foo@bar.com>

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 foo@bar.com" 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 2011-11-27 13:48:14 PST
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) 2012-03-05 20:37:09 PST
Created attachment 603152 [details] [diff] [review]
Fix

msg_parse_Header_addresses returns 0-length string, so we should also check the string.
Comment 3 Mark Banner (:standard8) 2012-03-27 01:29:42 PDT
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) 2012-03-27 03:20:48 PDT
Created attachment 609667 [details] [diff] [review]
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 2012-03-27 03:46:27 PDT
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) 2012-03-28 21:05:05 PDT
(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 2012-03-28 22:33:34 PDT
test_nsIMsgHeaderParser-parseHeadersWithArray.js? BTW, the test file doesn't have a licence block (MPL2 or such).
Comment 8 Mark Banner (:standard8) 2012-03-29 03:09:23 PDT
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) 2012-04-19 04:41:38 PDT
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] 2012-05-10 06:29:53 PDT
Hiro ?
Comment 11 Hiroyuki Ikezoe (:hiro) 2012-05-13 22:10:25 PDT
Created attachment 623582 [details] [diff] [review]
Change the test file name,

carrying over review+.
Comment 12 Hiroyuki Ikezoe (:hiro) 2012-05-13 22:38:25 PDT
Comment on attachment 623582 [details] [diff] [review]
Change the test file name,

Oops! Wrong patch
Comment 13 Hiroyuki Ikezoe (:hiro) 2012-05-13 22:40:03 PDT
Created attachment 623589 [details] [diff] [review]
This is correct!
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-05-14 15:36:30 PDT
https://hg.mozilla.org/comm-central/rev/83a7ea3d40c2
Comment 15 David :Bienvenu 2012-05-21 11:09:21 PDT
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

Note You need to log in before you can comment on or make changes to this bug.