Closed Bug 460287 Opened 16 years ago Closed 15 years ago

Growl notifications should give more info

Categories

(Thunderbird :: General, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: davida, Assigned: humph)

References

Details

Attachments

(4 files, 9 obsolete files)

30.84 KB, text/plain
Details
30.84 KB, text/plain
Details
8.34 KB, text/plain
Details
18.71 KB, patch
standard8
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
Telling me that I have a new message isn't, in general, that helpful. 

If Tb could say "3 New messages from John Lilly, David Humphrey, and Bugzilla", then I could decide whether to interrupt what i'm doing, based on how I felt about those three senders.
From bug 308552 comment 63, bienvenu had suggested the following in an earlier irc discussion:

* Add summary info to Growl alerts (e.g., Subject, Sender, body text [~80
chars])
* Do one alert per message vs. all new messages with count
* Make alerts clickable such that you are taken to the new message, as per
newmailalert.xul/js on win32
Having experienced one-per with content, with GrowlMail, the Mail.app addon that ships with Growl, please let's don't. Or, sigh, pref it. The thing that's cool about Growl is that it's a little out-of-the-way thing you can ignore if you're busy with something. If I get home from work, and have a couple hundred new bugspams, I'm going to have a hard time ignoring (the ones that fit on the screen out of) a couple hundred notifications, or even a batch of five or six if they're so big that they don't all fit on one column. Then you have a maximum of ten seconds to read all of them, since we're not firing them as messages arrive, but all in a batch after they've all come in.
Blocks: 459484
When I first thought about fixing this, I was thinking "one notification per new mail."  Then I read Phil's thoughts, and really agreed with him.  So, David, as per your original (perhaps flippant) request, here is exactly what you asked for :)

Mark, I've made it work, but I'm not sure if this is how you want it done.  I'd be glad to know how to improve this.

I've tried to get info on good practices for the max length of a message in a growl notification (e.g., info without extraneous interruption), but there is none.  Likely there needs to be a max length of the text, duplicate names stripped out, etc.  But this is enough for us to discuss and make improvements.
Attachment #346935 - Flags: ui-review?(clarkbw)
Attachment #346935 - Flags: review?(bugzilla)
humph: awesome!  this is a really good direction to take.  once we've had the change to play with it a little I think we'll be able to define exactly what we want a lot better.

davida: if you can grab the patch (attachment 346935 [details] [diff] [review]) for this weekend perhaps we can play with this on the flight down to SFO.
It wasn't flippant! As long as it doesn't always claim that the mails I get are from john, you and bugzilla, that is ;-).  I'll play with it.
Along with the duplicates (one notification repeating "bugzilla-daemon" 115 times beats 115 separate ones, but not by enough) and the l10n for "from" (and whether you have to localize the order of the parts of the phrase), you might need to check on whether you need to localize the comma - I'm not sure whether or not it's an acceptable list separator for all locales.
Not sure how to suggest it should be implemented in the short term, but i think it'd be useful to be able to specify that growl notifications only happen for some messages, e.g. only for contacts tagged "important" or some such.
Comment on attachment 346935 [details] [diff] [review]
[WIP] First pass at adding notification info

For some reason, I just can't get this to give me info other than "x has n new messages" (imap account, with or without offline storage being set).

For the l10n string, you probably want to format it similar to how biffNotification_message is done.

If you want to limit max chars, I would do that before you merge the authors into the string or something like that.
Attachment #346935 - Flags: review?(bugzilla)
[update - to build now, you have to build in mailnews/base && mailnews/]

I'll do another rev on this, but I'm going to need some help with the l10n.  I spoke to Pike about it, and now I'm only more confused:

10:47 < humph> does ", " need l10n when used as a list separator in a string?
10:50 <@humph> 10:48 <@Pike> humph: yes
10:50 <@humph> 10:48 <@Pike> though it tends to be harder than that
10:50 <@humph> 10:48 < humph> any good example you can point me at in the tree?
10:50 <@humph> 10:48 <@Pike> no, we just have bad examples
10:50 <@humph> 10:50 <@Pike> humph: I have no idea how to make that right with 
               RTL. The outer sentence might be RTL, and the direction of each 
               email might be different

Also, Clark/David, any response from you re: UI and what I'm doing here?  If I need major changes, I'd love to do that all at once with the other things above.

Thanks
Bringing conversation from #l10n into the bug at Pike's suggestion:

Trying to figure out the right solution for this:

[Mail Account Name] has 3 new messages from John Lilly, david.humphrey@senecac.on.ca, Bugzilla

My starting point is:

http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#344

I'm wondering about the ", " separator and how to localize it properly, but further discussions have led me to the conclusion that this is harder than I first thought.

Pike also mentioned concern about the way we pluralize here, which might want a spin-off bug.  I'll let him elaborate.
This is mostly a review comment for the biff string:

For plurals, please consult https://developer.mozilla.org/En/Localization_and_Plurals. We might need to add an XPCOM interface to that jsm, this isn't the first time that C++ is an excuse for bad plural handling.

The account name should be part of the full localized string.

There probably is a need to add LRM and RLM to the string, depending on whether the account name or the email address have a different writing direction than the overarching text.

I assume it will look something like this in the end:

#1 has one new message from #3.;#1 has #2 new messages from #3.

The different variants for the different plural forms would be concatenated using a ;.
#1 will be replaced with the account name, bullet proofed against bidi
#2 will be the number of messages
#3 will be the concatenated list of addresses.

As this is for Growl, I'm not sure how good that works, as the notification isn't going through our own layout, right?

Simon, am I making up strange stories here? Could we document on how to do concat bidi strings right on MDC?
Even if email addresses are in right-to-left scripts, they should be rendered as name@domain, not domain@name, so they should be wrapped in LRE-PDF (U+202A-U+202C)
(In reply to comment #7)
> Not sure how to suggest it should be implemented in the short term, but i think
> it'd be useful to be able to specify that growl notifications only happen for
> some messages, e.g. only for contacts tagged "important" or some such.

I have a plan for exposing a hook so we can do this in chrome or extensions:


[loop across sender name/address list]

nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1");
nsCOMPtr<nsISupportsPRBool> notify = do_CreateInstance(NS_SUPPORTS_PRBOOL_CONTRACTID);

notify->SetData(PR_TRUE);
os->NotifyObservers(notify, "newmail-notification-requested", sender);

PRBool includeSender;
notify->GetData(&includeSender);

if (includeSender)
{
...add this name/address to list of people in the pending notification
} 

Growl();


This would give more flexibility for extensions to deal with which things get into the notification (some might want to filter on domain, have a whitelist or blacklist, etc.).  I could even do the same thing above the loop to determine if the entire folder should cause this (e.g., if you want to have a filter send things to a folder, but not have that folder cause biff/newmail alerts at all, like bugmail).

I know there are some bugs where people request this sort of thing.  I'm thinking to do this for Mac now here, and then roll it across all other platforms when I get to bug 458507.

DavidA, will that work for you?
I'd appreciate a careful look at my string code here.  I found the msg db to be a maze of back and forth between wide and narrow strings.  However, after much work I think I have it.

I tried to follow the l10n advice (thanks for that), but wasn't able to do all of it in cpp.  Rather than break all of our other nsMessenger*Integration code, I'm doing a compromise here.  When I get to bug 458507, I'll try to correct this.  Note that I was forced to escape the trailing space in ', ' since the stringbundle wants to eat it otherwise.

I've added a hook for extension developers to be able to make a decision per email author.  This will allow white/blacklist type extensions for new mail notifications.  

I have not included any maximum length for the notification string.  If you have thoughts, let me know. 

I remove duplicates, which can seem strange when your message count doesn't equal the number of names, but I think it's tidier than repeating the names.
Attachment #346935 - Attachment is obsolete: true
Attachment #350420 - Flags: superreview?
Attachment #350420 - Flags: review?(bugzilla)
Attachment #346935 - Flags: ui-review?(clarkbw)
I've tried this patch in my own (german) build  (I've also edited the german messenger.properties with the new strings). I don't know why, but everytime I now get a new email Thunderbird crashes after I get the growl notification. If I restart Thunderbird, than Thunderbird retrieving the same email one more time (and than Thunderbird crashes).
And if the author of the email contains an german umlaut in his name, the text in the growl notification looks a little strange.
> now get a new email Thunderbird crashes after I get the growl notification. If

Awesome :(  Do you have a stack trace?

> And if the author of the email contains an german umlaut in his name, the text
> in the growl notification looks a little strange.

I wonder how l10n-friendly growl is going to turn out to be?  Do you have experience with growl and the German umlaut in other apps?
Attachment #350420 - Flags: review-
Comment on attachment 350420 [details] [diff] [review]
Growl notifications with author name(s)

There are several users of the biff strings, just changing one caller is gonna be evil. See http://mxr.mozilla.org/comm-central/search?string=biffNotification&find=%2Fmail&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central for more.

If you change localizable strings, please change their name, so that the localization tools can pick those changes up.

I didn't look at the conversion between wide and narrow strings, but those might be the root for the funny chars in German senders. Though it'd help to see a screen shot of that to actually know which funny chars we're seeing.
Attached file Crash Log
I've made a fresh new build with this patch and had a look that the patch applied correctly. But this doesn't seem to be the problem.


(In reply to comment #16)

> Awesome :(  Do you have a stack trace?
I can attach the crash log if this would help you.


> I wonder how l10n-friendly growl is going to turn out to be?  Do you have
> experience with growl and the German umlaut in other apps?

I've tried to display a german sentence with growlnotify from the command line: 
$ growlnotify -m "Gewürzte heiße Möhrchen kosten 5 €"
This text is displayed corretly in the growl notification. So it seems growl doesn't have a problem with special characters.
But if I receive a mail in Thunderbird and the name of the author is e.g. "Peter März", than the name is displayed as "=?iso-8859-15?Q?Peter_M=E4ller?="
Sorry, in Comment #18 it should be "=?iso-8859-15?Q?Peter_M=E4rz?="
Axel, thanks for the review, and for catching my error on the properties.  I'll fix those when I do the next patch.

Nomis, thanks for testing this, and for the info you provided.  Your crash is interesting, because I'm not on that stack (NotifyIntPropertyChanged would eventually get to me, but it's down a few frames from me even being called).  In this crash case, was it the very first email that you got with the running build, or a subsequent one?

If you're willing to test again, I'd be interested to see if it happens at a different spot where I am on the stack.

I'm really interested to know that growl can handle the umlaut case, so I'll have to research this more.  I'm not clear at the moment how I can avoid this widening/narrowing operations.
The strings Nomis is seeing are the standard mail way to encode non-ascii strings in mail fields. I guess there's a utility function somewhere in mailnews to get those converted to unicode.
(In reply to comment #20)
> In this crash case, was it the very first email that you got with the running
> build, or a subsequent one?
Yes, it was the very first email that I get with the running build. Because you mentioned this, I've now used the same patched build (as before) once more. And now it doesn't crash on new mails anymore (???). I used a build without this patch in the meantime.

> If you're willing to test again, I'd be interested to see if it happens at a
> different spot where I am on the stack.
Yes sure, I'm willing to test again. At the moment it looks stable (I hope). If it will crash one more time I let you know.
> Yes, it was the very first email that I get with the running build. Because you
> mentioned this, I've now used the same patched build (as before) once more. And
> now it doesn't crash on new mails anymore (???). I used a build without this
> patch in the meantime.

I think I've got...the problem is that I am releasing the header when I shouldn't be, and then later, when something tries to use it, it crashes.  The crash will be in somewhat random places, as a result.

I'll put up a patch, but in the mean time you can try commenting out nsMessengerOSXIntegration.cpp:674

//         NS_RELEASE(hdr); 

That's our crash, I think!
I've added a new set of properties, which is ugly, but will become less ugly with bug 458507 I think.  I've removed my NS_RELEASE call on the headers, which was causing the crash.

I assume this will still have the same issue with l10n strings showing in the Growl alert.  I'd appreciate a look from string-api eyes more experienced than mine.
Attachment #350420 - Attachment is obsolete: true
Attachment #350512 - Flags: review?(bugzilla)
Attachment #350420 - Flags: superreview?
Attachment #350420 - Flags: review?(bugzilla)
Attachment #350512 - Flags: review?(l10n)
If tried the new patch and this seems to fix the crash, but only for a single mail.
But If you have multiple accounts and receive mails for more than one account at the same time, than I get a crash. For that I've sended myself a mail to two accounts. I've tested this two times to be sure it is reproducible. But maybe this has nothing to do with this bug and is antoher bug (e.g. bug for growl on multiple accounts).
I've attached the log file.
But the good thing is, now it works perfect for single mails.
Nomis,

Thanks for your help here.  I'm unable to reproduce this crash, and if you can do it without too much trouble, would you mind hooking up gdb and getting a backtrace so I can get the frame one higher than Apple's crash report gives (my experience is that gdb can get you closer to the crash)?  I wonder if you're going through code I'm not due to l10n work on the names of the authors (mine are all ASCII), or if you're hitting something else.  This most recent stack still doesn't have me on it, and also doesn't show signs of issues with the headers.  It's possible you're seeing something unrelated to this patch, but I'm not going to rule it out yet.

Doing some more testing of this, I think I'm going to need to limit the items returned from db->GetNewList() so that it only takes the recent new mail.  Right now it will biff for 2 messages, but sometimes show 4 names, since it has old "new mail" still in that list.  Not sure if this is a bug, or just something I need to work around, by sending the new count to my function and getting those N items from the end of the list.
(In reply to comment #26)
> if you can do it without too much trouble, would you mind hooking up gdb and getting a
> backtrace so I can get the frame one higher than Apple's crash report gives (my
> experience is that gdb can get you closer to the crash)?
Hm, if it will help you I can try it. But I've never done this before. At this time I'm only familiar with generating a mailnews logging. I found this in the internet: https://developer.mozilla.org/en/Debugging_on_Mac_OS_X#Debugging_from_the_Terminal_with_gdb
And will follow the steps. But it seems to me I must create a build with "enable-debug" for this first.


>  I wonder if you're
> going through code I'm not due to l10n work on the names of the authors (mine
> are all ASCII), or if you're hitting something else.  This most recent stack
> still doesn't have me on it, and also doesn't show signs of issues with the
> headers.
You must know that I also include the patches from Bug 463176 in my own build. Maybe a regression from that? Or does it make any differences if the account is POP3 or IMAP? Mine is POP3.
> Hm, if it will help you I can try it. But I've never done this before. At this
> time I'm only familiar with generating a mailnews logging. I found this in the
> internet:
> https://developer.mozilla.org/en/Debugging_on_Mac_OS_X#Debugging_from_the_Terminal_with_gdb
> And will follow the steps. But it seems to me I must create a build with
> "enable-debug" for this first.

It's OK, don't worry about it.  I left a build running over night and was able to get it to crash with gdb attached.  It's crashing here:

#0  0x09fcc396 in nsMessengerOSXIntegration::GetNewMailAuthors (this=0x755b50, aFolder=0xafb3384, aAuthors=@0xbfffdb14) at /Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.cpp:628

For some reason folder is nsnull after I do this:

  nsresult rv = aFolder->GetChildWithURI(folderUri, PR_TRUE, PR_TRUE,
                                         getter_AddRefs(folder));

and it finally crashes here when I try to use it:

  rv = folder->GetMsgDatabase(nsnull, getter_AddRefs(db));

This can happen (GetChildWithURI returns nsnull if it doesn't find the child), but I'm not clear why it is happening.  I'll work on it some more and post another patch when I figure it out.

> You must know that I also include the patches from Bug 463176 in my own build.
> Maybe a regression from that? Or does it make any differences if the account is
> POP3 or IMAP? Mine is POP3.

I don't think that matters here, no.  Thanks for letting me know, though.
(In reply to comment #28)
> It's OK, don't worry about it.
Oh, I don't worry about it, I'm learning. :)


So, I've tried to debug my debug build using gdb in Xcode. And this is the output (I hope). I doesn't understand all of that but I think I can't find you in the list. But I only get this (reproducible) crash if I apply our patch.
For this build I only include your patch and the de files to this patch. And because I get the OS X beachball after Xcode started my debug build I forced to quit it.
(In reply to comment #29)
> Created an attachment (id=350572) [details]
> gdb output for the crash if I receive mails on two different accounts at the
> same time

Thanks!  This is just the output of the debug build spamming stderr/stdout, not the stack trace.

> Oh, I don't worry about it, I'm learning. :)

:)  So for future, here's what you can do:

* make a debug build, let's say with objdir-debug
* run your build
* get the pid for your build: ps | grep Shredder
* attach gdb: gdb /path/to/objdir-debug/mozilla/dist/ShredderDebug.app/Contents/MacOS/thunderbird-bin -p [pid you got here]
* now you're at the gdb command prompt
* tell gdb to let Shredder continue running: c
* when it crashes, you'll get dropped back at the command prompt, get a backtrace like this: bt

Ping me on irc (humph) and I can walk you through it if you want some time.

> So, I've tried to debug my debug build using gdb in Xcode. And this is the
> output (I hope). I doesn't understand all of that but I think I can't find you
> in the list. But I only get this (reproducible) crash if I apply our patch.
> For this build I only include your patch and the de files to this patch. And
> because I get the OS X beachball after Xcode started my debug build I forced to
> quit it.

I've found the issue, I just need to finish my patch.  You are 100% right about it being related to multiple accounts.  Currently nsMessengerOSXIntegration::GetFirstFolderWithNewMail assumes you want a child in the first cached folder with new mail, which in the case of a second account is going to be wrong 50% of the time, and return nsnull instead of the folder that just got new mail.  Crash.

Fixing this means reworking a couple other things, but I'll get you another patch to test.
Nomis, I've fixed this crash, but I'd love for you to test.  Currently I have added a sanity check on !folder, but commented it out so that it will crash if my fix logic is wrong--it shouldn't crash.  If this fix does it for you, I'll uncomment the check and file a new patch for review so we can look at the l10n string issues.

While I was fixing this I noticed that we do the wrong thing with folderURIs and the onclick handler for the growl alerts.  Filed bug 467159 to deal with it later, but for now I'm leaving the broken "use first folder" logic in place so we can test this bug accurately.

Thanks for your continued help, really appreciated.
Attachment #350512 - Attachment is obsolete: true
Attachment #350512 - Flags: review?(l10n)
Attachment #350512 - Flags: review?(bugzilla)
(In reply to comment #30)
> Thanks!  This is just the output of the debug build spamming stderr/stdout, not
> the stack trace.
Ooooh. Than you can ignore this ;)

> :)  So for future, here's what you can do:
Thank you very much. I've tried this and it worked perfect. :)
I now get an similar output like you in comment 28 (but I take the pid from the Activity Monitor)
 
> Ping me on irc (humph) and I can walk you through it if you want some time.
Maybe, I might take you up on that. 

(In reply to comment #31)
> Created an attachment (id=350574) [details]
> Patch with fix for folder==nsnull crash
> 
> Nomis, I've fixed this crash, but I'd love for you to test.  Currently I have
> added a sanity check on !folder, but commented it out so that it will crash if
> my fix logic is wrong--it shouldn't crash.  If this fix does it for you, I'll
> uncomment the check and file a new patch for review so we can look at the l10n
> string issues.
Good news. I've tried the new patch and my build doesn't crash on multiple accounts anymore. Good work!
Thanks to Nomis' help, this patch solves all the known issues that were causing crashes.  I now need to address the l10n display issues in the Growl alert, and I'm hoping that you can look at what I'm doing with strings.  I can't see how to avoid the narrow/wide back and forth, but perhaps you can.
Attachment #350574 - Attachment is obsolete: true
Attachment #350580 - Flags: review?(l10n)
Attachment #350580 - Flags: review?(bugzilla)
I have a question. Normally the sender address is in the kind of "author name <email@address.com>". In this case the displayed name in the growl message is the "author name". But sometimes the sender address is only "email@address.com", and than the name in the growl message is the email address. But if I have this person in my address book, is it than possible to use the senders name from the address book instead of the email address for the growl message?
But this is only a "nice to have" (for me) and not sooo important.
I think you want mime2DecodedAuthor instead of author, http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgHdr.idl#113
> I think you want mime2DecodedAuthor instead of author,

I can do that, but what I really want is the name portion of the author, and to get it, I have to pass the author to nsIMsgHeaderParser::ExtractHeaderAddressName, which takes and returns nsACString.

This also answers Nomis' question: given |David Humphrey <david.humphrey@senecac.on.ca>|, I want to use just the name "David Humphrey" so I can keep the alert text short.  However, if I only get |<david.humphrey@senecac.on.ca>|, I'll use that.

So I could just use the entire author vs. name to insure I get a proper wide string.  Or I could write my own name extractor in here, which seems silly to me.

Thoughts?
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#380 says the code. Among more insightful comments, there's a "sigh".

GetMime2DecodedAuthor, utf-16 -> utf-8, ExtractHeaderAddressName, utf-8 -> utf-16

Oh, and then jump a bridge.
Attached patch Patch with GetMime2DecodedAuthor (obsolete) — Splinter Review
I've rewritten to favour nsAString over nsACString, and use GetMime2DecodedAuthor vs. GetAuthor.  It works as before for me, so I'd appreciate if you could test, Nomis, with your string cases.
Attachment #350580 - Attachment is obsolete: true
Attachment #350763 - Flags: review?(l10n)
Attachment #350763 - Flags: review?(bugzilla)
Attachment #350580 - Flags: review?(l10n)
Attachment #350580 - Flags: review?(bugzilla)
(In reply to comment #38)
> Created an attachment (id=350763) [details]
> Patch with GetMime2DecodedAuthor
> 
> I've rewritten to favour nsAString over nsACString, and use
> GetMime2DecodedAuthor vs. GetAuthor.  It works as before for me, so I'd
> appreciate if you could test, Nomis, with your string cases.

I've now tested the new patch and this solves the german umlaut problem. Now german umlauts are displayed prober in the growl message. :-) 
Should it solve anything else?
Comment on attachment 350763 [details] [diff] [review]
Patch with GetMime2DecodedAuthor

Based on Nomis' test, I think this is finally ready for a proper review.  Sorry for all the failed attempts.
Attachment #350763 - Flags: superreview?(bienvenu)
Attachment #350763 - Flags: review?(l10n) → review+
Comment on attachment 350763 [details] [diff] [review]
Patch with GetMime2DecodedAuthor

A nit on the localization notes, the trailing space comment should say that it needs to be encoded as \u0020 to not get stripped.

You might want to ask :bs on irc on PromiseFlatString, I think that isn't required anymore anywhere.
Attachment #350763 - Flags: review?(bugzilla) → review-
Comment on attachment 350763 [details] [diff] [review]
Patch with GetMime2DecodedAuthor

>+    nsString authors;
>+    nsresult rv = GetNewMailAuthors(aFolder, authors);
>+
>+    // If all senders are vetoed, the authors string will be empty.
>+    if (NS_FAILED(rv) || authors.Length() == 0)

nit: authors.IsEmpty()

>+      const PRUnichar *formatStrings[2] = { numNewMsgsText.get(), authors.get() };
>+
>+      nsXPIDLString finalText;

Please don't use nsXPIDLString, we're not going to have that when we move to frozen api, and we've eliminated all of mailnews/ uses of it.

>+  NS_ENSURE_TRUE(mFoldersWithNewMail, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsIMsgFolder> folder;
>+  nsCOMPtr<nsIWeakReference> weakReference;

These two don't need declaring until they are used (after the if !count)

>+nsMessengerOSXIntegration::GetNewMailAuthors(nsIMsgFolder* aFolder, 
>+                                             nsAString& aAuthors)
...
>+  // Gets a list of names or email addresses for the authors of all new
>+  // mail in/below the given folder.  Duplicates are removed.
>+  // Extension developers can listen for "newmail-notification-requested"
>+  // and then make a decision about including a given author or not.
>+  // As a result, it is possible that the resulting length of aAuthors
>+  // will be 0.
>+
...
>+  nsresult rv = aFolder->GetChildWithURI(folderUri, PR_TRUE, PR_TRUE, 
>+                                         getter_AddRefs(folder));
>+  if (NS_FAILED(rv) || !folder) // kick out if no child folder
>+    return NS_ERROR_FAILURE;

This is confusing. You say you'll kick out if no child folder, but in the comment above, you say you're getting a list for mail *in*/below the given folder.

>+      for (PRUint32 i = 0; i < numNewKeys; i++) 

So I go away for two weeks, come back with a 1000 new messages in my inbox, from, say 100 different people, how big is my alert going to be? Do we need to apply a limit here?

>+      // Flatten name list into string, delineating with proper l10n separator
>+      nsCOMPtr<nsIStringBundle> bundle; 
>+      GetStringBundle(getter_AddRefs(bundle));
>+      if (!bundle)
>+        return NS_ERROR_FAILURE;
>+
>+      nsString listSeparator;
>+      bundle->GetStringFromName(NS_LITERAL_STRING("macBiffNotification_separator").get(), getter_Copies(listSeparator));
>+
>+      for (PRUint32 i = 0; i < names.Length(); i++)
>+      {
>+        if (i > 0)
>+          aAuthors.Append(listSeparator);
>+        aAuthors.Append(names[i]);
>+      }
>+    }

Why not just do this as part of the previous step and save the intermediate array usage?
> So I go away for two weeks, come back with a 1000 new messages in my inbox,
> from, say 100 different people, how big is my alert going to be? Do we need to
> apply a limit here?

Hence comment 3 and comment 14 :)  I agree, but no one will give me any input on this.  What should it be?

The rest of your comments I'll address in a new patch, once I know how to deal with the long lists.
(In reply to comment #43)
> > So I go away for two weeks, come back with a 1000 new messages in my inbox,
> > from, say 100 different people, how big is my alert going to be? Do we need to
> > apply a limit here?
> 
> Hence comment 3 and comment 14 :)  I agree, but no one will give me any input
> on this.  What should it be?


Maybe something like(?):

---------------------------------------------
example@xy.com

30 new messages from
Donald Duck, Beispiel@xy.de,
Heinz Kunz, Max Mustermann,
email@mozilla.com
and 25 others.
---------------------------------------------

To bring up an idea for discussion.
This patch (350763) doesn't work anymore (for me). The last version I build successfully with this patch was 20090119, today I get the following error:

/temp/src/mailnews/base/src/nsMessengerOSXIntegration.cpp: In member function ‘nsresult nsMessengerOSXIntegration::GetNewMailAuthors(nsIMsgFolder*, nsAString_internal&)’:
/temp/src/mailnews/base/src/nsMessengerOSXIntegration.cpp:643: error: no matching function for call to ‘nsIMsgFolder::GetMsgDatabase(int, nsGetterAddRefs<nsIMsgDatabase>)’
../../../mozilla/dist/include/msgbase/nsIMsgFolder.h:572: note: candidates are: virtual nsresult nsIMsgFolder::GetMsgDatabase(nsIMsgDatabase**)
make[5]: *** [nsMessengerOSXIntegration.o] Error 1
make[4]: *** [src_libs] Error 2
make[3]: *** [libs_tier_app] Error 2
make[2]: *** [tier_app] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

I get exactly the same error if I build it with mozilla-1.9.1 and with mozilla-central.
(In reply to comment #45)

nice :(  I'll add that to the list of things to fix.

Speaking of which, I'd really like to tie this up.  Bryan/David/??, can I get some thoughts on how to deal with long lists of mail and how many to show?  I don't love the idea in comment 44, only because it will involve yet another special case l10n string (i.e., I assume I can't use that fixed order).  Even something as simple as ", ..." probably needs localizing, I'm not sure.

Before this rots further, can we agree on a plan?
I kinda like the idea in comment 44 myself.  It's very close to what bryan had wanted to do in the original message pane stuff.  It'd be good to know how to localize that kind of thing.
I think this error (Comment #45) is due to the changes from bug 473458. So I looked at the line nsMessengerOSXIntegration.cpp:643 (after applying the patch) and found similar lines in the changes from bug 473458. Therefore I changed the line
rv = folder->GetMsgDatabase(nsnull, getter_AddRefs(db));
into
rv = folder->GetMsgDatabase(getter_AddRefs(db));
(similar to the patch from bug Bug 473458). And now it compiles without this error. But I don't know if this is a perfect fix for this error. But maybe it will help you to fix it.
Hardware: PowerPC → All
Attached patch Patch v7 (obsolete) — Splinter Review
Fixed review comments, thanks Standard8.  This patch also wires in the NewMail atom added in bug 465381, so users will get alerts *every* time there is new mail vs. just on biff.  This also fixes an issue so that I now only report the most recent "new" mail vs. "new" mail you might have received an hour ago (i.e., if you get new mail coming in before you clear biff state).  I've also written the code for comment 44.

Nomis, you're good at breaking my stuff :), can you try this patch for me before I ask for another review?  Thanks.
Attachment #350763 - Attachment is obsolete: true
Attachment #350763 - Flags: superreview?(bienvenu)
(In reply to comment #49)
> Nomis, you're good at breaking my stuff :), can you try this patch for me
> before I ask for another review?  Thanks.


Sure :)

It compiles just fine. It works if I receive new mails in one account. It doesn't crash if I receive mails in multiple accounts at the same time (but displays only the first one, but this is a different bug). And it workes if the sender contains german umlauts in its name. :)

I also tested what will happen if I receive a bunch of new mails (331 in my case). First it downloaded all new messages (takes about one minute) and after that I get the growl notification. But this notification doesn't display me the amount of mails. But maybe this was a bug in my localization. I will test this with an en-US build again...
(In reply to comment #50)
> I also tested what will happen if I receive a bunch of new mails (331 in my
> case). First it downloaded all new messages (takes about one minute) and after
> that I get the growl notification. But this notification doesn't display me the
> amount of mails. But maybe this was a bug in my localization. I will test this
> with an en-US build again...

Hm, yes sorry, this was my fault, there was a missing string in my l10n de files.
It also workes if I receive a bunch of new mails. :) But the amount is not correct. I received 331 new mails and the message says 331316 mails. And at the end it says "and  more", without any number.
Attached patch Patch v8 (obsolete) — Splinter Review
Fixed two small things Nomis pointed out.  See comment 49 for more details.
Attachment #359807 - Attachment is obsolete: true
Attachment #360005 - Flags: superreview?(bienvenu)
Attachment #360005 - Flags: review?(bugzilla)
(In reply to comment #52)
> Fixed two small things Nomis pointed out.  See comment 49 for more details.
And I can confirm that this is fixed now. I use this patch now for a few days in my own TB without any problems.
So hopefully we will see this patch in now, after all the work you have done for this. :-)
Attachment #360005 - Flags: review?(bugzilla) → review-
Comment on attachment 360005 [details] [diff] [review]
Patch v8

nit: There's a lot of whitespace errors (e.g. on end of lines), please check against http://beaufour.dk/jst-review/ and fix.

>+#include "nsReadableUtils.h"

If you really need this, please include nsStringGlue.h instead (which includes this file, but allows migration to frozen linkage easier).

>+    // If this isn't the root folder, get it so we can report for it.
>+    // GetRootFolder always returns the server's root, so calling on the root itself is fine.
>+    nsCOMPtr<nsIMsgFolder> rootFolder;
>+    rv = aFolder->GetRootFolder(getter_AddRefs(rootFolder));
>+    if (NS_FAILED(rv))
>+      return;

I thing you should either drop the rv assignment and assess the result in the if statement, or use NS_ENSURE_SUCCESS(rv, ) so that we know if this starts failing for some reason.

>+      nsresult rv = aFolder->GetChildWithURI(folderUri, PR_TRUE, PR_TRUE, 
>+                                         getter_AddRefs(childFolder));

nit: incorrect indentation.

>+  else if (mNewMailReceivedAtom == aProperty)
>+  {
>+    nsCOMPtr<nsIMsgFolder> rootFolder;
>+    nsresult rv = aFolder->GetRootFolder(getter_AddRefs(rootFolder));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIWeakReference> weakFolder = do_GetWeakReference(rootFolder);

nit: blank line after the NS_ENSURE_SUCCESS please.

>+    if (mFoldersWithNewMail->IndexOf(weakFolder) == kNotFound)
>+      mFoldersWithNewMail->AppendElement(weakFolder);
>+    FillToolTipInfo(aFolder, aNewValue);

nit: blank line before FillToolTip Info please.

>-  nsresult rv;
>-  NS_ENSURE_TRUE(mFoldersWithNewMail, NS_ERROR_FAILURE);
>+  // Get a list of names or email addresses for the folder's authors 
>+  // with new mail. Note that we only process the most recent "new" 
>+  // mail (aNewCount), working from most recently added. Duplicates 
>+  // are removed, and names are displayed to a set limit
>+  // (kMaxDisplayCount) with the remaining count being returned in 
>+  // aNotDisplayed. Extension developers can listen for 
>+  // "newmail-notification-requested" and then make a decision about
>+  // including a given author or not. As a result, it is possible that
>+  // the resulting length of aAuthors will be 0.
>+  nsCOMPtr <nsIMsgDatabase> db;
>+  nsresult rv = aFolder->GetMsgDatabase(getter_AddRefs(db));
>+  PRUint32 numNewKeys = 0;
>+  if (NS_SUCCEEDED(rv) && db)
>+  {
>+    PRUint32 *newMessageKeys;
>+    rv = db->GetNewList(&numNewKeys, &newMessageKeys);
>+    if (NS_SUCCEEDED(rv))
>+    {
>+      nsCOMPtr<nsIMsgHeaderParser> parser = 
>+        do_GetService(NS_MAILNEWS_MIME_HEADER_PARSER_CONTRACTID, &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
...
>+      nsCOMPtr<nsIObserverService> os =
>+        do_GetService("@mozilla.org/observer-service;1", &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
...
>+      // Get proper l10n list separator -- ", " in English
>+      nsCOMPtr<nsIStringBundle> bundle; 
>+      GetStringBundle(getter_AddRefs(bundle));
>+      if (!bundle)
>+        return NS_ERROR_FAILURE;


Although it is currently slightly more optimal in the failure case, could we put the calls after the if before the GetNewList call so that if they fail we don't leak memory?

>+          nsString author;
>+          rv = hdr->GetMime2DecodedAuthor(author);
>+          NS_ENSURE_SUCCESS(rv, rv);

I think if this fails, you either want to break or continue rather than just bailing out the function - you may just have a really bad header? In any case, we want to free that memory if you're going to bail out.

>+          nsCString name;
>+          rv = parser->ExtractHeaderAddressName(NS_ConvertUTF16toUTF8(author),
>+                                                name);
>+          NS_ENSURE_SUCCESS(rv, rv);

Same comment applies.

>+          NS_ConvertUTF8toUTF16 utf16name(name);
>+          // Don't add unwanted or duplicate names
>+          if (includeSender && 
>+              !FindInReadable(utf16name, aAuthors, comparator))

Using aAuthors.Find would be much better here (and future-proof).

+  nsresult GetFirstFolderWithNewMail(nsIMsgFolder* aFolder, nsACString& aFolderURI);
...
+  nsresult GetNewMailAuthors(nsIMsgFolder* aFolder, nsAString& aAuthors, PRInt32 aNewCount, PRInt32* aNotDisplayed);

As these are both internal functions, you can pass ns(C)String directly, i.e. without making them 'A'.
Attached patch Patch v9 (obsolete) — Splinter Review
Patch with fixes for Standard8's review (thanks for catching all that).
Attachment #360005 - Attachment is obsolete: true
Attachment #360926 - Flags: superreview?(bienvenu)
Attachment #360926 - Flags: review?(bugzilla)
Attachment #360005 - Flags: superreview?(bienvenu)
One question, will we see this in b2 or is this for b3? Because this has l10n impact and string freeze for b2 is tomorrow. OK, its not sooo important to have this in, but it would be nice.
(In reply to comment #56)

I, for one, would really like to get this in.  Aside from the value of the patch itself, it blocks other work I need to do and also needs to get in by the next release.
Comment on attachment 360926 [details] [diff] [review]
Patch v9

a few nits:

+  nsCOMPtr <nsIMsgDatabase> db;
+  nsresult rv = aFolder->GetMsgDatabase(getter_AddRefs(db));

no space after nsCOMPtr.

+  nsCOMPtr<nsIWeakReference> weakReference;
+  weakReference = do_QueryElementAt(mFoldersWithNewMail, 0);
+  nsCOMPtr<nsIMsgFolder> folder;
+  folder = do_QueryReferent(weakReference);

these could both be one statement each, e.g., nsCOMPtr<nsIMsgFolder> folder = do_QueryReference(weakReference);

sadly, kNotFound should be -1, because we get it from the string stuff that's going away when we change linkage.
+    if (mFoldersWithNewMail->IndexOf(weakFolder) == kNotFound)
+      mFoldersWithNewMail->AppendElement(weakFolder);
Attached patch Patch v10Splinter Review
Fixed bienvenu's comments (thanks).  Also note, I fixed 3 uses of kNotFound, even though you only mentioned the one--I assume the same applies to all 3.  If I'm wrong, I can revert the other 2.
Attachment #360926 - Attachment is obsolete: true
Attachment #361356 - Flags: superreview?(bienvenu)
Attachment #361356 - Flags: review?(bugzilla)
Attachment #360926 - Flags: superreview?(bienvenu)
Attachment #360926 - Flags: review?(bugzilla)
Comment on attachment 361356 [details] [diff] [review]
Patch v10

>+# LOCALIZATION NOTE(macBiffNotification is Mac only):
>+#  %1$S is the number of new messages
>+#  %2$S is a list of names and/or email addresses separated by biffNotification_separator
>+#  %3$S is the number of new messages not displayed in the biff alert
>+macBiffNotification_message=%1$S new message from %2$S.
>+macBiffNotification_messages=%1$S new messages from %2$S.
>+macBiffNotification_messages_extra=%1$S new messages from %2$S and %3$S more.
>+# Used to separate names/email addresses in a list.  Note the trailing space ', '
>+macBiffNotification_separator=,\u0020

FTR this isn't the "new" way of doing plurals in localizations. However, afaik there is no c++ method to manage the "new" way of doing plurals, and given the rest of this file is in this format, then I'm happy we take this and if its not good enough then we should fix the whole lot somewhere else (including writing a c++ method for plurals).

r=me.
Attachment #361356 - Flags: review?(bugzilla) → review+
Attachment #361356 - Flags: superreview?(bienvenu) → superreview+
Blocks: 473145
Checked in: http://hg.mozilla.org/comm-central/rev/5a40d301caec
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
I don't like filing new bugs for fresh patches, so I'll ask here: please use plurals support that is already in the toolkit.

Stuff like this:
  macBiffNotification_message=%1$S new message from %2$S.
  macBiffNotification_messages=%1$S new messages from %2$S.
is not really acceptable.

Thanks in advance.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #62)
> I don't like filing new bugs for fresh patches, so I'll ask here: please use
> plurals support that is already in the toolkit.
> 
> Stuff like this:
>   macBiffNotification_message=%1$S new message from %2$S.
>   macBiffNotification_messages=%1$S new messages from %2$S.
> is not really acceptable.

No. As I said in comment 60 the plural support isn't available for c++ code. We want to get this feature to allow other patches to progress.

Toolkit doesn't support plurals for c++ code, so the fix should be made first, then we can fix the mailnews c++ instances for plurals.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
(In reply to comment #63)
> (In reply to comment #62)
> > I don't like filing new bugs for fresh patches, so I'll ask here: please use
> > plurals support that is already in the toolkit.

> No. As I said in comment 60 the plural support isn't available for c++ code. We
> want to get this feature to allow other patches to progress.

I should have said. If plural support was available for c++ code, then I wouldn't have let this go in without supporting it.
Ahhh, OK. Sorry for not reading the thread before making noise.
Filed bug 477831 for C++ pluralization issue.
No longer blocks: 473145
Target Milestone: Thunderbird 3.0b2 → ---
Target Milestone: --- → Thunderbird 3.0b2
Blocks: 473145
You need to log in before you can comment on or make changes to this bug.