Last Comment Bug 308552 - Growl Integration for Mail Alerts on Mac OS X
: Growl Integration for Mail Alerts on Mac OS X
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All Mac OS X
: P3 normal with 13 votes (vote)
: Thunderbird 3.0b1
Assigned To: David Humphrey (:humph)
:
Mentors:
http://growl.info/
: 303476 332637 363762 (view as bug list)
Depends on: 362685 382694 394346 443775
Blocks: 458507 459482 459483 459484 459485
  Show dependency treegraph
 
Reported: 2005-09-14 13:29 PDT by Scott MacGregor
Modified: 2009-12-17 05:03 PST (History)
35 users (show)
davida: blocking‑thunderbird3.0a2-
mozilla: blocking‑thunderbird3-
davida: wanted‑thunderbird3.0a2+
mozilla: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
strings for the growl alert (683 bytes, patch)
2006-12-03 22:08 PST, Scott MacGregor
no flags Details | Diff | Review
Starter patch (broken), see following comment 42 (16.76 KB, patch)
2007-08-24 02:49 PDT, Wayne Woods
no flags Details | Diff | Review
Adds growl notifications via sdwilsh's alerts work (18.92 KB, patch)
2007-11-27 10:18 PST, David Humphrey (:humph)
mozilla: review+
Details | Diff | Review
Patch with David's fixes applied (19.12 KB, patch)
2007-12-04 08:47 PST, David Humphrey (:humph)
no flags Details | Diff | Review
Same patch with named notification added (20.04 KB, patch)
2007-12-14 05:40 PST, David Humphrey (:humph)
no flags Details | Diff | Review
Same patch with named notification added (20.04 KB, patch)
2007-12-14 05:41 PST, David Humphrey (:humph)
no flags Details | Diff | Review
Patch with localized Growl Notification (21.69 KB, patch)
2008-05-04 14:29 PDT, David Humphrey (:humph)
no flags Details | Diff | Review
Patch with localized Growl Notification + Makefile fixes (23.09 KB, patch)
2008-05-04 15:58 PDT, David Humphrey (:humph)
mozilla: superreview+
Details | Diff | Review
r+/sr+ patch minus printfs (22.99 KB, patch)
2008-06-24 12:02 PDT, David Humphrey (:humph)
no flags Details | Diff | Review
r+/sr+ patch minus printfs, fixed bitrot (23.07 KB, patch)
2008-06-25 07:27 PDT, David Humphrey (:humph)
no flags Details | Diff | Review
Patch to fix SM bustage (2.69 KB, patch)
2008-07-04 02:18 PDT, Mark Banner (:standard8)
mnyromyr: review+
mozilla: superreview+
Details | Diff | Review
[checked in] Combined and updated (20.91 KB, patch)
2008-10-02 07:00 PDT, Mark Banner (:standard8)
mnyromyr: review+
mozilla: superreview+
Details | Diff | Review
[checked in] Fix growl registration (5.53 KB, patch)
2008-10-02 07:04 PDT, Mark Banner (:standard8)
mnyromyr: review+
mozilla: superreview+
Details | Diff | Review

Description Scott MacGregor 2005-09-14 13:29:37 PDT
Jason/Josh and I were talking about this the other day.

Would be really cool if new mail notifications on Mac OS X went out to growl.

Should be easy from a mozilla point of view because we already have all the
notifications set up for new mail:

http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMessengerOSXIntegration.cpp

and we do something very similar on windows with our native alert service.

Josh pointed out that the growl APIs may be in Objective C which might
complicate some of this.
Comment 1 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-09-14 13:40:27 PDT
I would note that we already have bugs (and patches!) on making the alert
service XP.
Comment 2 Josh Aas 2005-09-14 13:42:21 PDT
http://growl.info/documentation/developer/

This page indicates that there is a Carbon API. I'm going to whip something up
now...
Comment 3 Jo Hermans 2005-09-14 14:08:53 PDT
see also bug 282185
Comment 4 Josh Aas 2005-09-14 14:34:46 PDT
We'd have to include the growl framework in our app bundle or add the
communications code in that framework to our own codebase. I don't think either
approach is a good idea, so I'm not going to work on this any more right now.

Would be cool if someone made a nice extension for growl support though.
Comment 5 zug_treno 2005-09-18 02:06:03 PDT
Related to/duplicate of bug 303476?
Comment 6 Josh Aas 2005-12-30 14:25:08 PST
*** Bug 303476 has been marked as a duplicate of this bug. ***
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2006-04-03 16:07:13 PDT
(In reply to comment #4)
> We'd have to include the growl framework in our app bundle or add the
> communications code in that framework to our own codebase. I don't think either
> approach is a good idea, so I'm not going to work on this any more right now.

Is this really true? There are plenty of apps out there that use Growl (Adium, for instance) but don't require it. If the framework isn't installed, then the notifications just go unheeded.
Comment 8 Scott MacGregor 2006-05-23 16:31:52 PDT
It looks like someone has gotten growl notifications to work for Thunderbird via the YAMB extension. For more information, see:

http://www.neilturner.me.uk/2005/Nov/04/growl_and_thunderbird.html

Thanks to John Lilly for pointing that out.
Comment 9 Scott MacGregor 2006-08-01 13:48:47 PDT
cc'ing Chris over at the growl project who has commented on the Firefox bug for adding growl support.

Chris, I'm interested in kick starting this project for Thunderbird. I just started browsing over the growl documentation, but am not a Mac expert. We've already got a C++ object that's mac OS X specific and knows all the details about new mail: http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMessengerOSXIntegration.cpp

We just need to hook up the code to turn that into a growl notification.

1) How much of the growl framework do we need to include in our builds? 
2) How large is the framework? The download was 7 megs I believe, but I'm guessing applications don't really distribute themselves with the full package?

We are a carbon app if that helps. 

thanks Chris.
Comment 10 Chris Forsythe 2006-08-01 14:05:55 PDT
(In reply to comment #9)
> cc'ing Chris over at the growl project who has commented on the Firefox bug for
> adding growl support.
> 
> Chris, I'm interested in kick starting this project for Thunderbird. I just
> started browsing over the growl documentation, but am not a Mac expert. We've
> already got a C++ object that's mac OS X specific and knows all the details
> about new mail:
> http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMessengerOSXIntegration.cpp

I am far from a c++ guy.

> 
> We just need to hook up the code to turn that into a growl notification.
> 

You've got a few options actually, I'll explain below.

> 1) How much of the growl framework do we need to include in our builds? 

All of it? The framework is designed so that it gives you backwards compatibility should we upgrade to a new system in 1.1, or something like that. Ripping code out of the framework and putting it into (I hope I'm reading bugzilla right) Thunderbird would amount to very small space savings.

> 2) How large is the framework? The download was 7 megs I believe, but I'm
> guessing applications don't really distribute themselves with the full package?
> 

The source tarball is 9 mb.

This includes a bunch of images, and all the Extras code, and all the Growl code, and all the Framework code, and the Release directory, and so forth.

What you want is the SDK. The download itself includes 2 frameworks, Growl-Withinstaller.framework and Growl.Framework.

Growl.framework gives you what you need to send notifications.

Growl-Withinstaller.framework gives you what you need to send notifications, along with, as the name says, an installer to install Growl.

> We are a carbon app if that helps. 
> 

The framework can work with Carbon or Cocoa.




You folks do have another option, but it isn't guaranteed. Some application developers are hesitant to include Growl.framework in their application, that's fine, so they send notifications via Applescript.

Now, the problem is this:

1) The Applescript API is not guaranteed between releases. It hasn't changed much, but if we change it the API is for scripters, who by all rights should be able to change their scripts with minimal effort.

2) You can crash GrowlHelperApp via Applescript, or your own application, if you don't do it right. (I am not, nor have I ever been, an Applescripter. Please see our forums at Cocoaforge if you need references to this)

3) You do not get the nice stuff like ClickBack support, which notifies your application that a click was made on the Growl notification, along with other niceties.




On to a little history, and why we include the framework rather than putting it into a system location.

1) Back when we released .5, we got a lot of feedback saying that installing into /Library/Frameworks was bad mojo. I agreed, but the OS X installer can't install into ~/Library/Frameworks so we were left with no choice there.

2) If the Growl.framework framework wasn't in place on a system, and a developer did not weak link he application correctly, the application would crash.


2 happened a heck of a lot. Weak linking seemed like a bad idea.

So we hit upon the including the framework idea. It solved 2, and it solved 1. It was the only solution to 2 really, because if the framework was always there you'd never crash due to it not being where the application expected it.


So anyhow, point is. You folks have options, Applescript if you really really really don't want to use the framework, and the framework if you really want to give a better experience to users who want to use Thunderbird and Growl.
Comment 11 Chris Forsythe 2006-08-01 14:08:13 PDT
Oh ya, the sdk is over here:

http://growl.info/downloads_developers.php
Comment 12 Chris Forsythe 2006-08-01 14:32:22 PDT
(In reply to comment #9)
> 2) How large is the framework? The download was 7 megs I believe, but I'm
> guessing applications don't really distribute themselves with the full package?


kremit:/Volumes/Growl-SDK/Frameworks chris$ du -sh Growl.framework/
244K    Growl.framework/
kremit:/Volumes/Growl-SDK/Frameworks chris$ du -sh Growl-WithInstaller.framework/
1.6M    Growl-WithInstaller.framework/
Comment 13 Scott MacGregor 2006-08-04 17:38:44 PDT
Chris, thanks a lot for this information. It helps a lot!

Josh, after talking with bienvenu, I think I've got an idea for how we can do this without checking the growl SDK into CVS. We already do something very similar today for our address book palm sync conduit on Windows. It uses a Palm SDK which we don't check into the build either. If we want to release the palm sync conduit with a version of thunderbird, we install the palm SDK on the actual release machine, and set a build environment variable which defines the location of the palm SDK. If the build variable isn't defined, the code that uses the palmsync sdk doesn't get built. 

See an example: http://lxr.mozilla.org/seamonkey/source/mailnews/extensions/palmsync/conduit/Makefile.in

So something like:

1) build environment variable that points to the location of the growl SDK
2) Wrap all of the code in nsMessengerOSXIntegration related to sending off the growl notification with an ifdef that corresponds to this variable. 
3) Presumably we'll have some bundle packaging to make sure parts of the growl framework get backed into thunderbird.app as well based on this ifdef and the location of the SDK.

What do you think Josh? 
Comment 14 Mitar 2006-10-11 12:51:55 PDT
I made an extension which adds this feature to Thunderbird through growlnotify command line utility:

https://addons.mozilla.org/thunderbird/3448/
Comment 15 Scott MacGregor 2006-12-03 22:08:34 PST
Created attachment 247384 [details] [diff] [review]
strings for the growl alert

here are the strings we could use. Getting this on the branch now in case we add this feature to thunderbird after the l10n freeze.
Comment 16 Shawn Wilsher :sdwilsh 2006-12-03 22:13:57 PST
This will probably be a lot easier to do if Bug 362685 gets in.  What is the deadline for Thunderbird 2?  Seeing as how Linux and Windows use nsIAlertsService, having Mac use it kinda makes sense.

(Sadly, I'm betting it won't get done in time.)
Comment 17 Scott MacGregor 2006-12-03 22:36:06 PST
(In reply to comment #16)
> This will probably be a lot easier to do if Bug 362685 gets in.  What is the
> deadline for Thunderbird 2?  Seeing as how Linux and Windows use
> nsIAlertsService, having Mac use it kinda makes sense.
> 
> (Sadly, I'm betting it won't get done in time.)
> 

Shawn, that would totally rock. what are the plans in 362685 for integrating with the growl sdk? We were thinking of just hacking into growlnotify command line in the short term. Thunderbird beta 1 deadline is tomorrow. Beta 2 is early January. let me know if you start to come up with something! I would be very interested.
Comment 18 Shawn Wilsher :sdwilsh 2006-12-03 22:39:48 PST
(In reply to comment #17)
> Shawn, that would totally rock. what are the plans in 362685 for integrating
> with the growl sdk? We were thinking of just hacking into growlnotify command
> line in the short term. Thunderbird beta 1 deadline is tomorrow. Beta 2 is
> early January. let me know if you start to come up with something! I would be
> very interested.

Plan is to use the Coccoa API, so we'll have all the goodies of nsIAlertsService.  I think I could have this done by the end of December (my limiting factor right now is finals coming up) - so hopefully that won't be too late for this.

Comment 19 Shawn Wilsher :sdwilsh 2007-01-05 10:35:49 PST
Scott - do you guys use nsIAlertsService for this, or do you have your own interface for it?
Comment 20 Matt Dudziak 2007-01-30 15:22:43 PST
*** Bug 363762 has been marked as a duplicate of this bug. ***
Comment 21 Scott MacGregor 2007-01-30 15:27:31 PST
Shawn, we can use whatever we need to in http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMessengerOSXIntegration.cpp

to hook this up in the make including using the nsIAlertsService.
Comment 22 Scott MacGregor 2007-04-24 21:48:31 PDT
It looks like growl support has landed on toolkit for the trunk. It might be interesting to take a look into hooking up the code in nsMessengerOSXIntegration.cpp to use it via nsIAlertService. Maybe Wayne will be tempted :)
Comment 23 Wayne Woods 2007-04-24 22:35:03 PDT
Is Shawn already working on it? I don't want to muscle in.. especially when I currently have little clue. But if he's not, I'm more than happy to give it a go :)
Comment 24 Shawn Wilsher :sdwilsh 2007-04-25 05:10:57 PDT
I know very very little about mail code, so it'd take me a heck of a lot longer.  If you have questions, feel free to ask though.  (in other words, if you want to do it, I'd be happy to let you)

Doing this *right* will involve a bit of reworking, but it'd be just like I'm doing with the Growl Extension that I've made, so it won't be too terrible.
Comment 25 Shawn Wilsher :sdwilsh 2007-04-27 20:57:17 PDT
So, the question is this:  Do you want to have a named notification in growl, or is "General Notification" OK?

I'd argue that the right way to do this is to have your own named notification, but if you don't care, this is easy.

All you would have to do is call the static method of the mozGrowlDelegate:
http://mxr.mozilla.org/seamonkey/source/toolkit/components/alerts/src/mac/mozGrowlDelegate.h#54

If you want to have a named notification, we'd have to rework like I said, but I've already done the work in an extension, so it'd just be porting it over to trunk code.
Comment 26 Wayne Woods 2007-04-29 16:59:26 PDT
That looks great, Shawn. I agree we'd probably want our own named notification... best we do it right from the start. If you want me to port it over, would the source code for your extension be available? Thanks :)
Comment 27 Shawn Wilsher :sdwilsh 2007-04-29 17:08:54 PDT
I'm not sure if I'll have time, so I'll toss all this up here now.
http://trac.growl.info/browser/branches/moz-extension/trunk/src/

The filenames are very similar to our current setup.  mozGrowlDelegate is our controller (I kinda picked a bad name for our code - didn't find out until just recently).  I have two methods that let you add methods to the controller to register:
http://trac.growl.info/browser/branches/moz-extension/trunk/src/mozGrowlDelegate.h#L49
http://trac.growl.info/browser/branches/moz-extension/trunk/src/mozGrowlDelegate.h#L56

This is where I dispatch an observer to let other things add notifications:
http://trac.growl.info/browser/branches/moz-extension/trunk/src/grNotifications.mm#L134

And this is how I currently add a notification (this is a JS file, but it could be done in C++ as well):
http://trac.growl.info/browser/branches/moz-extension/trunk/components/grMailNotifications.js#L52

I think the code is fairly self explanatory, but if you have questions ask.

This is the second version of the api I came up with for this, and I think this is a very clean way to do it.  The plus side is that if we do it this way, extension authors can easily add their own as well.

This code is licensed under the Growl License, but I'm OK with relicensing it under the tri-license (I'm the original author and contributor for it).
Comment 28 Shawn Wilsher :sdwilsh 2007-04-29 17:24:01 PDT
Oh yeah, I have this method which lets me dispatch names messages.  You will either want to add a new component for this, or make it a part of nsIAlertsService  (it'd do nothing on other platforms I guess).
http://trac.growl.info/browser/branches/moz-extension/trunk/public/grINotifications.idl#L23

I'm not sure if you want to do that work in a separate bug though.
Comment 29 Wayne Woods 2007-04-30 03:07:22 PDT
Thanks Shawn! I have a few more questions, if you don't mind me bugging you, but instead of spamming everyone here with minor growl details, do you mind if I email you directly?
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-04-30 05:25:38 PDT
(In reply to comment #29)
> instead of spamming everyone here with minor growl details, do you mind if I
> email you directly?

If you're asking Growl questions, doing it in a Growl bug isn't "spam". People who don't want to get email about it can un-CC themselves :). I think it's a good idea to put any discussion about Growl in the bug rather than in private email, if possible, since it can help other people who have the same questions.
Comment 31 Chris Forsythe 2007-04-30 06:13:36 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > instead of spamming everyone here with minor growl details, do you mind if I
> > email you directly?
> 
> If you're asking Growl questions, doing it in a Growl bug isn't "spam". People
> who don't want to get email about it can un-CC themselves :). I think it's a
> good idea to put any discussion about Growl in the bug rather than in private
> email, if possible, since it can help other people who have the same questions.
> 

I was just about to type the same thing. I don't think it'd be annoying at all, and would be beneficial to anyone cc'd.
Comment 32 Wayne Woods 2007-05-01 04:46:08 PDT
Fair enough :) I just didn't want to bog the discussion down in minor technical questions.

(In reply to comment #28)
> Oh yeah, I have this method which lets me dispatch names messages.

I haven't looked at this closely yet, but I assume it's used for named notifications. If so, would this be wanted for most or all uses of Growl within Mozilla apps? And so would be regarded as a general inclusion, rather than just for Thunderbird?
Comment 33 Shawn Wilsher :sdwilsh 2007-05-01 06:04:07 PDT
(In reply to comment #32)
> I haven't looked at this closely yet, but I assume it's used for named
> notifications. If so, would this be wanted for most or all uses of Growl within
> Mozilla apps? And so would be regarded as a general inclusion, rather than just
> for Thunderbird?

Yes, and yes.  That's why I was saying you might want to spin it off into a new bug.
Comment 34 Wayne Woods 2007-05-13 00:38:37 PDT
Sorry for the slow progress on this. I know your code should be self-explanatory, but I'm not a programmer by trade, and I don't speak Cocoa at all, so I'm afraid it's been a bit beyond me trying to work out what I need to change. For the new method that requires a named notification, do I add it in parallel with the  current "general notification" method? or do I modify the current one so that all uses now require a notification name?
Comment 35 Shawn Wilsher :sdwilsh 2007-05-13 00:41:34 PDT
I'm not 100% sure I understand your question.  Are you talking about the static method on the mozGrowlDelegate?
Comment 36 Wayne Woods 2007-05-13 00:56:21 PDT
The fact I couldn't explain myself clearly probably speaks volumes. ;) Basically, do I modify the static method on the mozGrowlDelegate that you referred to earlier:
http://mxr.mozilla.org/seamonkey/source/toolkit/components/alerts/src/mac/mozGrowlDelegate.h#54
to have a name like the one in your extension:
http://trac.growl.info/browser/branches/moz-extension/trunk/src/mozGrowlDelegate.h#L28
Or do I create a new method for that, instead? So you'd still have a method that used a general notification name, and a new one that required a name for the notification. 
Comment 37 Shawn Wilsher :sdwilsh 2007-05-13 03:16:16 PDT
I would suggest that you rename it and update it.  You will then have to update the calls elsewhere (I think two places).
Comment 38 Shawn Wilsher :sdwilsh 2007-05-14 16:46:11 PDT
Bug 378527 does a large portion of the work you need to do.  I figured since I was there I would fix it.  That should land today.
Comment 39 Wayne Woods 2007-05-15 00:41:53 PDT
Thanks Shawn. I'll wait for it :)
Comment 40 Wayne Woods 2007-07-16 05:49:18 PDT
With that dependency landing, I assume this is ready to be worked on now? :)
Comment 41 Shawn Wilsher :sdwilsh 2007-07-16 08:47:48 PDT
That is correct.  You have to dispatch the notification from C++ though.  Here's how:

First, #include "nsAlertsService.h".  In that file, the function that you'll end up using is
NS_DispatchNamedNotification(const nsAString &aName,
                             const nsAString &aImage,
                             const nsAString &aTitle,
                             const nsAString &aMessage,
                             const nsAString &aCookie,
                             nsIObserver *aListener);
Now, to add a notification, you'll need to register an observer for "before-growl-registration" sometime before "final-ui-startup".  When your observer gets called with that topic, simply QI the aData parameter to nsINotificationsList, and using that interface, add the names of the notifications you might be dispatching.

Hopefully that's clear enough for you to do what you need to do :)
Comment 42 Wayne Woods 2007-08-24 02:49:38 PDT
Created attachment 278037 [details] [diff] [review]
Starter patch (broken), see following comment 42

Thanks for your advice Shawn. I'm not there yet, and haven't even got as far as the observer. I've run into a problem and finally decided I should stop messing about and ask for help. As you'll guess from below, my knowledge of C++ is extremely limited...

In order for NS_DispatchNamedNotification to work, I seem to need to add nsAlertsService to the nsMessengerOSXIntegration class  in the header file. Otherwise I get errors about undefined symbols related to that function.

However, I also want an nsIObserver class added so I can use nsMessengerOSXIntegration as an observer. However when I nsAlertsService and nsIObserver are there together, I get the error:
nsMessengerOSXIntegration.h:64: warning: direct base 'nsIObserver' inaccessible in 'nsMessengerOSXIntegration' due to ambiguity

And then a string of errors relating to this 'ambiguity'. I've tried adding and removing various things, but I can't work out what I'm doing wrong. Any thoughts? Help! :)
Comment 43 Shawn Wilsher :sdwilsh 2007-08-24 06:14:47 PDT
Comment on attachment 278037 [details] [diff] [review]
Starter patch (broken), see following comment 42

>+class nsIStringBundle;
>+
shouldn't need this since you are #including

> class nsMessengerOSXIntegration : public nsIMessengerOSIntegration,
>-                                  public nsIFolderListener
>+                                  public nsIFolderListener,
>+                                  public nsIObserver,
>+                                  public nsAlertsService
Don't want this, just the observer

> {
> public:
>   nsMessengerOSXIntegration();
>   virtual ~nsMessengerOSXIntegration();
>   virtual nsresult Init();
> 
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIMESSENGEROSINTEGRATION
>   NS_DECL_NSIFOLDERLISTENER
>+  NS_DECL_NSIOBSERVER
>+  NS_DECL_NSIALERTSSERVICE
and get rid of this last line

>Index: nsMessengerOSXIntegration.cpp
>+#include "nsIStringBundle.h"
>+#include "nsAlertsService.h"
Don't need these since you got them in the .h file

> NS_INTERFACE_MAP_BEGIN(nsMessengerOSXIntegration)
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIMessengerOSIntegration)
>    NS_INTERFACE_MAP_ENTRY(nsIMessengerOSIntegration)
>    NS_INTERFACE_MAP_ENTRY(nsIFolderListener)
>+   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIObserver)
NS_INTERFACE_MAP_ENTRY(nsIObserver)

I think that should get you compiling.
Comment 44 Wayne Woods 2007-08-24 17:23:05 PDT
Thanks Shawn,

(In reply to comment #43)
> > class nsMessengerOSXIntegration : public nsIMessengerOSIntegration,
> >-                                  public nsIFolderListener
> >+                                  public nsIFolderListener,
> >+                                  public nsIObserver,
> >+                                  public nsAlertsService
> Don't want this, just the observer

If I remove the |public nsAlertsService| line, I get the following error:
/usr/bin/ld: Undefined symbols:
NS_DispatchNamedNotification(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIObserver*)

But if I leave it in, I get (trimmed the paths for brevity):
nsMessengerOSXIntegration.h:63: warning: direct base 'nsIObserver' inaccessible in 'nsMessengerOSXIntegration' due to ambiguity
nsMessengerOSXIntegration.cpp: In constructor 'nsMessengerOSXIntegration::nsMessengerOSXIntegration()':
../../../dist/include/alerts/nsAlertsService.h:75: error: 'nsAlertsService::~nsAlertsService()' is private
nsMessengerOSXIntegration.cpp:74: error: within this context
nsMessengerOSXIntegration.cpp: In destructor 'virtual nsMessengerOSXIntegration::~nsMessengerOSXIntegration()':
nsAlertsService.h:75: error: 'nsAlertsService::~nsAlertsService()' is private
nsMessengerOSXIntegration.cpp:84: error: within this context
nsMessengerOSXIntegration.cpp: In member function 'virtual nsresult nsMessengerOSXIntegration::QueryInterface(const nsIID&, void**)':
nsMessengerOSXIntegration.cpp:100: error: 'nsIObserver' is an ambiguous base of 'nsMessengerOSXIntegration'
Comment 45 Shawn Wilsher :sdwilsh 2007-08-24 18:10:12 PDT
Ah, linking errors.  You want is this in your Makefile.in:
SHARED_LIBRARY_LIBS = $(DEPTH)/toolkit/components/alerts/src/mac/$(LIB_PREFIX)alerts_s.$(LIB_SUFFIX)
Comment 46 Wayne Woods 2007-08-27 07:19:29 PDT
(In reply to comment #45)
> Ah, linking errors.  You want is this in your Makefile.in:
> SHARED_LIBRARY_LIBS =
> $(DEPTH)/toolkit/components/alerts/src/mac/$(LIB_PREFIX)alerts_s.$(LIB_SUFFIX)

I added that to Makefile.in in the root directory for my trunk cvs. I added it to the very end of the file, so it was outside any ifs. Unfortunately I still get the same "Undefined symbols" error from comment 44. Even tried a distclean.

Comment 47 Shawn Wilsher :sdwilsh 2007-08-27 08:34:26 PDT
Sorry - I should have specified where to put it too.  It is generally found on the line above EXTRA_DSO_LDOPTS.  I think that you will need to place the line here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/build/Makefile.in&rev=1.80#96
Comment 48 Wayne Woods 2007-08-28 05:15:48 PDT
RCS file: /cvsroot/mozilla/mailnews/base/build/Makefile.in 
 SHARED_LIBRARY_LIBS = \
 		../src/$(LIB_PREFIX)msgbase_s.$(LIB_SUFFIX) \
 		../search/src/$(LIB_PREFIX)msgsearch_s.$(LIB_SUFFIX) \
+                $(DEPTH)/toolkit/components/alerts/src/mac/$(LIB_PREFIX)alerts_s.$(LIB_SUFFIX) \
 		$(NULL)


Should that work? It gives the same error. I tried adding it as a separate statement, too, and it also failed.
Comment 49 Shawn Wilsher :sdwilsh 2007-08-28 07:01:38 PDT
(In reply to comment #48)
> Should that work? It gives the same error. I tried adding it as a separate
> statement, too, and it also failed.
Yeah, I would expect that to work.  I suggest you hop on to irc #developers and ask about it.  bsmedberg would probably know.
Comment 50 Wayne Woods 2007-08-29 05:50:04 PDT
Ok, thanks anyway. I presume that SHARED_LIBRARY_LIBS thing is supposed to make it include the Mac-specific nsAlerts.h file instead of the cross-platform one?

It might take me a while to reach someone with the right know-how on #developers (living in Oz and all). In the meantime, would you happen to have any sample code or patches in which you have NS_DispatchNamedNotification() calls working? I'm afraid there's nothing in the Moz trunk to use as a reference.

Thanks!
Comment 51 Wayne Woods 2007-08-30 07:34:48 PDT
From #developers:
bsmedberg: sdwilsh|away: I'm not sure I understand why you're linking alerts into thunderbird anyway... can't you expose an XPCOM API to do whatever it is you're trying to do?
bsmedberg: sdwilsh|away: but, if that's really what you need to do, you'll have to compile alerts_s twice, once with internal linkage and once with external linkage

So is that what we want to do? Or are there reasons we maybe shouldn't be linking alerts into Tbird that way?
Comment 52 Shawn Wilsher :sdwilsh 2007-08-30 08:42:29 PDT
I was against making an xpcom interface for it since it was mac only, but that may be what we need to do :(
Comment 53 Shawn Wilsher :sdwilsh 2007-09-21 19:10:42 PDT
I think the new API should be much easier for you to use :)
Comment 54 Shawn Wilsher :sdwilsh 2007-10-10 11:43:52 PDT
Wayne - what's the status on this?  Any more questions?
Comment 55 Wayne Woods 2007-10-10 22:05:37 PDT
(In reply to comment #54)
> Wayne - what's the status on this?  Any more questions?

Sorry Shawn. Since Firefox 3 is shipping sooner, I was concentrating on some things I was trying to get in in time for that, and fix anything related to it. I promise I'll get to this soon :)

BTW, is there a milestone schedule for Thunderbird 3? How long before feature freeze?

Comment 56 Wayne Woods 2007-10-17 00:15:50 PDT
Actually, the point is moot. I just fried the cpu on my home computer and can only do this work from home, so this will have to be suspended indefinitely till I can afford a new one. A feature freeze deadline (or even a rough idea) for Tbird would still be handy if anyone knows it.
Comment 57 David Humphrey (:humph) 2007-11-27 10:18:36 PST
Created attachment 290410 [details] [diff] [review]
Adds growl notifications via sdwilsh's alerts work

This works, and I'd be interested for feedback about whether the current dock-based code should be changed (I've left it without knowing).  Also, I haven't used the growl strings in the earlier attachment, and would be interested to know if that's still needed (personally I like just getting the count, but realize you might want more).
Comment 58 David :Bienvenu 2007-11-27 14:57:15 PST
thx, David - I'll give this patch a try...
Comment 59 Wayne Woods 2007-11-27 14:59:37 PST
Thanks for taking this David! Unfortunately I still don't have a home machine to test your patch or help out with this, so my feedback is limited...

I think I'm indifferent as to whether the subjectNotificationTitle etc. are used. Just giving the count of new messages is probably a lot neater, which is what I assume you've done. And I don't think it's overkill to preserve the dock notification as well.

Cheers.
Comment 60 David :Bienvenu 2007-11-28 13:27:49 PST
David, what should I see when I run with the patch? Is this just for animating the doc icon with the number of new messages, as opposed to the full blown msg subject text that we do on windows?
Comment 61 Shawn Wilsher :sdwilsh 2007-11-28 13:38:59 PST
If you don't have Growl installed, you'll see nothing.  Otherwise, it all depends on how you have Growl configured as to what is displayed where.
Comment 62 David :Bienvenu 2007-12-03 08:29:41 PST
Comment on attachment 290410 [details] [diff] [review]
Adds growl notifications via sdwilsh's alerts work

+  if (strcmp(aTopic, "alertfinished") == 0)

as a matter of style, we prefer if (!strcmp...) It's way down the list of things I care about...

+nsresult nsMessengerOSXIntegration::GetStringBundle(nsIStringBundle **aBundle)
+{
+  nsresult rv = NS_OK;
+  NS_ENSURE_ARG_POINTER(aBundle);
+  nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
+  nsCOMPtr<nsIStringBundle> bundle;
+  if (bundleService && NS_SUCCEEDED(rv))
+    bundleService->CreateBundle("chrome://messenger/locale/messenger.properties", getter_AddRefs(bundle));
+  NS_IF_ADDREF(*aBundle = bundle);
+  return rv;
+}

This was probably cloned from somewhere, but:

nsresult rv should be declared right before it's used, and it doesn't have to be initialized.

Instead of NS_IF_ADDREF(*aBundle = bundle), you can use

bundle.swap(*aBundle);

+    if (mFoldersWithNewMail->IndexOf(weakFolder) == -1)

I prefer using kNotFound to -1.

r=bienvenu, with those nits addressed, thx, Dave!
Comment 63 David Humphrey (:humph) 2007-12-04 08:47:13 PST
Created attachment 291444 [details] [diff] [review]
Patch with David's fixes applied

I've made David's changes, and this is done now.  However, based on an irc conversation with David, there is more I can do, namely:

* 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

I'm going to work on these now, but probably it's best to SR and then check this patch in and work from there.  Should I continue working in this bug or start a new one for these enhancements?
Comment 64 Shawn Wilsher :sdwilsh 2007-12-12 12:58:25 PST
It also doesn't look like this patch actually registers a named notifications, which is the right thing to do.
Comment 65 David Humphrey (:humph) 2007-12-12 17:10:21 PST
Being new to Growl, I'm not sure how to assess the significance of this.  Unless it adds a lot of value, I'm not eager to rework this completely.  Thoughts?
Comment 66 David :Bienvenu 2007-12-12 17:22:10 PST
I know nothing about Growl, but I doubt adding named notifications would involve reworking this - we'd still be using the nsIAlertService, right?
Comment 67 Shawn Wilsher :sdwilsh 2007-12-13 14:06:58 PST
Yup.  It's really trivial to add with your existing code.  You need to do two things:
1) Pass the name you register as the last argument to ShowAlertNotification.
2) To register a named notification, you have to listen for the observer topic "before-growl-registration".  This page has some of the basics with sample code:
http://trac.growl.info/wiki/MozillaGrowlNotifications
Just note that anything that's grI* should be nsI*, and you'll use nsIAlertsService instead of grINotifications.  If you have any questions, I'll be happy to answer them.
Comment 68 David Humphrey (:humph) 2007-12-14 05:40:59 PST
Created attachment 293124 [details] [diff] [review]
Same patch with named notification added

The rest is unchanged.  To be clear, here is what I've added beyond what David reviewed:

+#include "nsINotificationsList.h"

+#define kGrowlNotification "New Mail"

 nsresult
 nsMessengerOSXIntegration::Init()
 {
+  // need to register a named Growl notification
   nsresult rv;
-    
+  nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1", &rv);
+  if (NS_SUCCEEDED(rv))
+    observerService->AddObserver(this, "before-growl-registration", PR_FALSE);


+NS_IMETHODIMP
+nsMessengerOSXIntegration::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aData)
+{
...
+  // register named Growl notification for new mail alerts.
+  if (!strcmp(aTopic, "before-growl-registration"))
+  {
+    nsresult rv;
+    nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1", &rv);
+    if (NS_SUCCEEDED(rv))
+      observerService->RemoveObserver(this, "before-growl-registration");
+
+    nsCOMPtr<nsINotificationsList> notifications = do_QueryInterface(aSubject, &rv);
+    if (NS_SUCCEEDED(rv))
+      notifications->AddNotification(NS_LITERAL_STRING(kGrowlNotification), PR_TRUE);
+  }
+
+  return NS_OK;
+}


+      rv = alertsService->ShowAlertNotification(NS_LITERAL_STRING(kNewMailAlertIcon), aAlertTitle, aAlertText, PR_TRUE, NS_ConvertASCIItoUTF16(aFolderURI),                            this, NS_LITERAL_STRING(kGrowlNotification));
Comment 69 David Humphrey (:humph) 2007-12-14 05:41:24 PST
Created attachment 293125 [details] [diff] [review]
Same patch with named notification added

The rest is unchanged.  To be clear, here is what I've added beyond what David reviewed:

+#include "nsINotificationsList.h"

+#define kGrowlNotification "New Mail"

 nsresult
 nsMessengerOSXIntegration::Init()
 {
+  // need to register a named Growl notification
   nsresult rv;
-    
+  nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1", &rv);
+  if (NS_SUCCEEDED(rv))
+    observerService->AddObserver(this, "before-growl-registration", PR_FALSE);


+NS_IMETHODIMP
+nsMessengerOSXIntegration::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aData)
+{
...
+  // register named Growl notification for new mail alerts.
+  if (!strcmp(aTopic, "before-growl-registration"))
+  {
+    nsresult rv;
+    nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1", &rv);
+    if (NS_SUCCEEDED(rv))
+      observerService->RemoveObserver(this, "before-growl-registration");
+
+    nsCOMPtr<nsINotificationsList> notifications = do_QueryInterface(aSubject, &rv);
+    if (NS_SUCCEEDED(rv))
+      notifications->AddNotification(NS_LITERAL_STRING(kGrowlNotification), PR_TRUE);
+  }
+
+  return NS_OK;
+}


+      rv = alertsService->ShowAlertNotification(NS_LITERAL_STRING(kNewMailAlertIcon), aAlertTitle, aAlertText, PR_TRUE, NS_ConvertASCIItoUTF16(aFolderURI),                            this, NS_LITERAL_STRING(kGrowlNotification));
Comment 70 Shawn Wilsher :sdwilsh 2007-12-14 07:49:37 PST
I hate to break it to you, but the string should be localized.  That's what makes this process so complicated to begin with.

Otherwise, this code will work, assuming Init is called before final-ui-startup.  I don't know mail well enough to comment on that though.
Comment 71 David Humphrey (:humph) 2007-12-14 12:59:32 PST
Where's the right way to put this localized notification name?  Should I do it like biffnotification_message in messenger.properties, or is there an I'm-only-on-Mac way to do localized strings?
Comment 72 Shawn Wilsher :sdwilsh 2007-12-17 13:34:42 PST
Comment on attachment 293125 [details] [diff] [review]
Same patch with named notification added

(In reply to comment #71)
> Where's the right way to put this localized notification name?  Should I do it
> like biffnotification_message in messenger.properties, or is there an
> I'm-only-on-Mac way to do localized strings?
I don't have a clue with regards to this - otherwise the patch looks right.
Comment 73 Greg K. 2008-02-06 07:22:51 PST
Note that TB3 nightlies already show a Growl notification when an application update has been downloaded and is ready to apply.
Comment 74 Gary Kwong [:gkw] [:nth10sd] 2008-03-13 12:33:12 PDT
Nominating wanted-thunderbird3 to keep it on the radar.
Comment 75 Shawn Wilsher :sdwilsh 2008-04-24 13:18:02 PDT
What else does this bug need to land?
Comment 76 Phil Ringnalda (:philor) 2008-04-24 13:29:17 PDT
According to sdwilsh, it needs to have a localized string for kGrowlNotification and someone needs to review whether it's getting initialized early enough, and after four months, someone needs to at least glance at alertsservice and notificationslist to be sure the APIs haven't changed.
Comment 77 Shawn Wilsher :sdwilsh 2008-05-01 13:09:25 PDT
They haven't - my goal was to see if anyone plans on doing the rest of the work needed.
Comment 78 David Humphrey (:humph) 2008-05-04 14:29:45 PDT
Created attachment 319294 [details] [diff] [review]
Patch with localized Growl Notification

Sorry it's taken so long to get back to this.  I've localized the growl notification in the attached patch, but can't test yet as I now get this error when trying to build (note: I've changed nothing in nsMessengerOSXIntegration.h):

nsMsgFactory.cpp
c++ -o nsMsgFactory.o -c -I../../../dist/include/system_wrappers -include /Users/dave/moz/tb-clean/mozilla/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -DOSTYPE=\"Darwin9.2.2\" -DOSARCH=Darwin -I/Users/dave/moz/tb-clean/mozilla/mailnews/base/build -I/Users/dave/moz/tb-clean/mozilla/mailnews/base/build/../src -I/Users/dave/moz/tb-clean/mozilla/mailnews/base/build/../search/src -I/Users/dave/moz/tb-clean/mozilla/mailnews/base/build/../util  -I/Users/dave/moz/tb-clean/mozilla/mailnews/base/build -I. -I../../../dist/include/xpcom -I../../../dist/include/string -I../../../dist/include/necko -I../../../dist/include/intl -I../../../dist/include/locale -I../../../dist/include/rdf -I../../../dist/include/content -I../../../dist/include/dom -I../../../dist/include/docshell -I../../../dist/include/widget -I../../../dist/include/layout -I../../../dist/include/mime -I../../../dist/include/uriloader -I../../../dist/include/mailnews -I../../../dist/include/addrbook -I../../../dist/include/mork -I../../../dist/include/txmgr -I../../../dist/include/pref -I../../../dist/include/msgcompose -I../../../dist/include/msgbaseutil -I../../../dist/include/msgdb -I../../../dist/include/appcomps -I../../../dist/include/toolkitcomps -I../../../dist/include/msgnews -I../../../dist/include/msgimap -I../../../dist/include/gfx -I../../../dist/include/webbrwsr -I../../../dist/include   -I../../../dist/include/msgbase -I../../../dist/include/nspr     -I/usr/X11/include   -fPIC  -I/usr/X11/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -I/Developer/Headers/FlatCarbon -pipe  -DDEBUG -D_DEBUG -DDEBUG_dave -DTRACING -g  -I/usr/X11/include -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsMsgFactory.pp /Users/dave/moz/tb-clean/mozilla/mailnews/base/build/nsMsgFactory.cpp
In file included from /Users/dave/moz/tb-clean/mozilla/mailnews/base/build/nsMsgFactory.cpp:121:
/Users/dave/moz/tb-clean/mozilla/mailnews/base/build/../src/nsMessengerOSXIntegration.h:52:30: error: nsIAlertsService.h: No such file or directory
make[6]: *** [nsMsgFactory.o] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_app] Error 2
make[2]: *** [tier_app] Error 2
make[1]: *** [alldep] Error 2
make: *** [alldep] Error 2

It's been a while since I was working in this code, so does anyone know of something that would have changed to cause me to break like this?
Comment 79 Mark Banner (:standard8) 2008-05-04 14:57:19 PDT
(In reply to comment #78)
> Sorry it's taken so long to get back to this.  I've localized the growl
> notification in the attached patch, but can't test yet as I now get this error
> when trying to build (note: I've changed nothing in
> nsMessengerOSXIntegration.h):
...
> It's been a while since I was working in this code, so does anyone know of
> something that would have changed to cause me to break like this?

I think you need to add "alerts" to the REQUIRES option of:

http://mxr.mozilla.org/seamonkey/source/mailnews/base/build/Makefile.in
and
http://mxr.mozilla.org/seamonkey/source/mailnews/build/Makefile.in

Preferably, ifdefed for mac only. I'm not sure what would have changed to cause this to regress in that time frame.
Comment 80 David Humphrey (:humph) 2008-05-04 15:58:15 PDT
Created attachment 319307 [details] [diff] [review]
Patch with localized Growl Notification + Makefile fixes

Thanks Mark, those changes fixed it.  I'd like to get this in, and enhancements to the notification (e.g., notification per message with subject and text) can be done in another follow-up bug.  Not sure the best r/sr path to get this done.  Dan, can you do it?
Comment 81 David Ascher (:davida) 2008-05-06 10:33:20 PDT
I think this is fine to have in an early alpha, but it won't block.  I'm making it block tb3 just to get it out of the way.
Comment 82 Wayne Mery (:wsmwk, NI for questions) 2008-06-06 06:59:57 PDT
*** Bug 332637 has been marked as a duplicate of this bug. ***
Comment 83 David :Bienvenu 2008-06-24 10:53:09 PDT
Comment on attachment 319307 [details] [diff] [review]
Patch with localized Growl Notification + Makefile fixes

Dmose said I could sr this if I didn't think it needed more eyes.  It looks OK, except for the printfs, which should be removed. Thx for working on this!~ I think we should get this in so mac users can try it out.
Comment 84 David Humphrey (:humph) 2008-06-24 12:02:00 PDT
Created attachment 326529 [details] [diff] [review]
r+/sr+ patch minus printfs

Removed printfs, sorry about that.
Comment 85 Mark Banner (:standard8) 2008-06-25 01:39:27 PDT
(In reply to comment #84)
> Created an attachment (id=326529) [details]
> r+/sr+ patch minus printfs
> 
> Removed printfs, sorry about that.
> 

Unfortunately this patch doesn't apply to HEAD cvs:

patching file mail/locales/en-US/chrome/messenger/messenger.properties
Hunk #1 FAILED at 434.
1 out of 1 hunk FAILED -- saving rejects to file mail/locales/en-US/chrome/messenger/messenger.properties.rej
patching file mailnews/base/build/Makefile.in
patching file mailnews/base/src/nsMessengerOSXIntegration.cpp
patch: **** malformed patch at line 221: @@ -152,30 +232,184 @@ PRInt32 nsMessengerOSXIntegration::Count

Please fix and add checkin-needed keyword again once a new patch is up.
Comment 86 David Humphrey (:humph) 2008-06-25 07:27:40 PDT
Created attachment 326691 [details] [diff] [review]
r+/sr+ patch minus printfs, fixed bitrot

This should fix it.
Comment 87 Mark Banner (:standard8) 2008-06-26 01:53:48 PDT
Checking in mail/locales/en-US/chrome/messenger/messenger.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messenger.properties,v  <--  messenger.properties
new revision: 1.58; previous revision: 1.57
done
Checking in mailnews/base/build/Makefile.in;
/cvsroot/mozilla/mailnews/base/build/Makefile.in,v  <--  Makefile.in
new revision: 1.86; previous revision: 1.85
done
Checking in mailnews/base/src/nsMessengerOSXIntegration.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMessengerOSXIntegration.cpp,v  <--  nsMessengerOSXIntegration.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in mailnews/base/src/nsMessengerOSXIntegration.h;
/cvsroot/mozilla/mailnews/base/src/nsMessengerOSXIntegration.h,v  <--  nsMessengerOSXIntegration.h
new revision: 1.4; previous revision: 1.3
done
Checking in mailnews/build/Makefile.in;
/cvsroot/mozilla/mailnews/build/Makefile.in,v  <--  Makefile.in
new revision: 1.30; previous revision: 1.29
done
Comment 88 Mark Banner (:standard8) 2008-06-26 04:13:52 PDT
I just had to back this out because the SeaMonkey is busted, and it didn't look like an easy/quick one-line fix:

/builds/tinderbox/SeaMonkey-Trunk/Darwin_8.7.2_Depend/mozilla/mailnews/base/src/nsMessengerOSXIntegration.cpp: In member function 'nsresult nsMessengerOSXIntegration::OnAlertClicked()':
/builds/tinderbox/SeaMonkey-Trunk/Darwin_8.7.2_Depend/mozilla/mailnews/base/src/nsMessengerOSXIntegration.cpp:397: error: 'GetFirstFolderWithNewMail' was not declared in this scope
/builds/tinderbox/SeaMonkey-Trunk/Darwin_8.7.2_Depend/mozilla/mailnews/base/src/nsMessengerOSXIntegration.cpp: In member function 'nsresult nsMessengerOSXIntegration::OnAlertFinished(const PRUnichar*)':
/builds/tinderbox/SeaMonkey-Trunk/Darwin_8.7.2_Depend/mozilla/mailnews/base/src/nsMessengerOSXIntegration.cpp:461: warning: 'FMGetFontFamilyFromName' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:741)
/builds/tinderbox/SeaMonkey-Trunk/Darwin_8.7.2_Depend/mozilla/mailnews/base/src/nsMessengerOSXIntegration.cpp:461: warning: 'FMGetFontFamilyFromName' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:741)
/builds/tinderbox/SeaMonkey-Trunk/Darwin_8.7.2_Depend/mozilla/mailnews/base/src/nsMessengerOSXIntegration.cpp:463: warning: 'FMGetFontFromFontFamilyInstance' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:875)
/builds/tinderbox/SeaMonkey-Trunk/Darwin_8.7.2_Depend/mozilla/mailnews/base/src/nsMessengerOSXIntegration.cpp:464: warning: 'FMGetFontFromFontFamilyInstance' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:875)

If possible, the equivalent locale file in suite/locales should also be updated to include the new string.
Comment 89 Dan Mosedale (:dmose) 2008-06-26 15:31:54 PDT
Was Thunderbird similarly busted?  If not, any idea why not?
Comment 90 Karsten Düsterloh 2008-06-26 16:04:56 PDT
(In reply to comment #89)
> Was Thunderbird similarly busted?

No.

> If not, any idea why not?

Well, the bustage takes place in the only ifdef section of the entire patch:

+nsresult nsMessengerOSXIntegration::OnAlertClicked()
+{
+#ifdef MOZ_THUNDERBIRD
+  nsresult rv;
+  nsCOMPtr<nsIMsgMailSession> mailSession = do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv);
+  NS_ENSURE_SUCCESS(rv,rv);
   
+  nsCOMPtr<nsIMsgWindow> topMostMsgWindow;
+  rv = mailSession->GetTopmostMsgWindow(getter_AddRefs(topMostMsgWindow));
+  if (topMostMsgWindow)
+  {
+    nsCOMPtr<nsIDOMWindowInternal> domWindow;
+    rv = topMostMsgWindow->GetDomWindow(getter_AddRefs(domWindow));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    domWindow->Focus();
+  }
+#else
+  nsCString folderURI;
+  GetFirstFolderWithNewMail(folderURI);
+  openMailWindow(folderURI);
+#endif
+  return NS_OK;
 }
Comment 91 Mark Banner (:standard8) 2008-06-27 00:09:38 PDT
(In reply to comment #90)
> +#else
> +  nsCString folderURI;
> +  GetFirstFolderWithNewMail(folderURI);
> +  openMailWindow(folderURI);
> +#endif
> +  return NS_OK;
>  }

It looks like GetFirstFolderWithNewMail is in the other nsMessenger*Integration files, but not OS X. I wasn't confident that just commenting out the lines, or copying and pasting would fix the problem enough, hence the back-out.
Comment 92 Mark Banner (:standard8) 2008-07-04 02:18:30 PDT
Created attachment 328098 [details] [diff] [review]
Patch to fix SM bustage

This patch (when applied after attachment 326691 [details] [diff] [review]) will fix the SM bustage. I just copied and pasted the code from the Windows version, we can try and find a way to clean this up later if we really want to.

However, I have to say that I have not been able to get the original patch to work with TB and growl version 1.1.4.

With the debugger I found that (in this order):

- In toolkit/components/alerts/src/mac/nsAlertsService.mm Init() growl is detected as being installed.
- In its Init function, nsAlertsService.mm hooks into the final-ui-startup notification from the observer service.
- A little bit later final-ui-startup is fired, which nsAlertsService.mm picks up and fires a "before-growl-registration" notification, nothing happens here.
- A little bit later still, nsMessengerOSXIntegration gets initialised (via its Init function) and registers for the "before-growl-registration" notification, which, of course, it never gets.

I tried this on a fully static build of TB as well, and again it didn't appear to work, but that wasn't a debug build, so I haven't been able to check to see if we get the same problems or not.

If we do want to take these patches and I'm not here, feel free to check in on my behalf.
Comment 93 Mark Banner (:standard8) 2008-07-04 02:53:11 PDT
(In reply to comment #92)
> I tried this on a fully static build of TB as well, and again it didn't appear
> to work, but that wasn't a debug build, so I haven't been able to check to see
> if we get the same problems or not.

Debugging of the static build revealed pretty much the same results.
Comment 94 David Humphrey (:humph) 2008-07-04 06:52:22 PDT
This worked with the previous versions of growl I developed against, but clearly something has changed in the new version.  After upgrading to 1.1.4 I was able to confirm Mark's findings, and this patch with 1.1.4 gives no alert :(

Shawn, any insight re: hooking-up on init?  Watching for before-growl-registration isn't soon enough any more.

Mark, thanks for that fix.
Comment 95 Shawn Wilsher :sdwilsh 2008-07-05 10:49:32 PDT
(In reply to comment #94)
> Shawn, any insight re: hooking-up on init?  Watching for
> before-growl-registration isn't soon enough any more.
Crap...that means our implementation is broken altogether now.  Can you file a new bug on that?
Comment 96 David Humphrey (:humph) 2008-07-09 19:18:02 PDT
(In reply to comment #92)
> I tried this on a fully static build of TB as well, and again it didn't appear
> to work, but that wasn't a debug build, so I haven't been able to check to see
> if we get the same problems or not.

I'm starting to think that this isn't a growl issue but is somehow tied to it being a debug build for me.  Shaver mentioned that he thought debug builds autoreg components every time, and wondered if I was doing anything magic at component registration.  I'm not (that I can see), but wonder if someone else with more knowledge above me in the stack here could comment.
Comment 97 Karsten Düsterloh 2008-07-11 12:35:00 PDT
Comment on attachment 328098 [details] [diff] [review]
Patch to fix SM bustage

Neglecting the overall Growl init problems, this looks okay (I do get Growl notifications in my SM debug build).
Comment 98 Karsten Düsterloh 2008-07-14 14:29:09 PDT
Comment on attachment 328098 [details] [diff] [review]
Patch to fix SM bustage

Huh, I spotted you forgot to patch suite/.../messenger.properties like Humphrey did for mail. 
r=me with that added
Comment 99 Phil Ringnalda (:philor) 2008-07-15 23:15:08 PDT
Thanks to Mnyromyr's hint from bug 443775 comment 22 (that removing the string makes it actually work in Tb), some thoughts based on finally having seen it working (many of which can be followups, but I'm leaving for a while, and don't want to file "once it lands, it will be broken this way" bugs):

* Not showing an(other) alert when we are already showing one, while the right thing for the other platforms, is probably the wrong thing for us. You can set Growl's persistence time up to 10 seconds, in which case biffing multiple accounts will skip notification for a timing-dependent number of them, or you can make alerts sticky (either always or on idle), in which case we'll do no more notification until you dismiss that first one.

* We don't seem to be notifying for the account with new mail, but for the first account which has new mail. I've got an IMAP account, and some POPs going into Local Folders, so when I get new mail in the IMAP account, we notify on it, and if I don't read it and later get new mail in Local Folders, we notify about the IMAP account again instead of what's actually new.

* Not doing the dock icon badging until after the alert is dismissed is not what we want to do: not only is it confusing to dismiss a sticky alert and only then have the icon bounce and get badged, it also opens us up to badging it with the unexciting but extremely persistent news that you have 0 new emails, if you have one and read it before the alert goes away.

* I'm not really clear (having been too lazy to actually look) on why we have separate actions for Tb and SM when you click the alert, but the Tb one doesn't seem to do what it wants to (which is probably Widget:Cocoa's fault): with Tb behind Fx, clicking the alert will generally get us to the top, without changing the selected folder the way the SM version does, but if we're behind a native app, we only come up to the top of the stack behind it.
Comment 100 David :Bienvenu 2008-08-15 10:44:20 PDT
moving from blocking to wanted
Comment 101 Mark Banner (:standard8) 2008-10-02 07:00:24 PDT
Created attachment 341438 [details] [diff] [review]
[checked in] Combined and updated

I've now got a solution for the registration problem, which I'll attach in the next patch. This patch is the combination of previous patches updated to latest trunk. This includes the SM fixes and string change from before. Apart from a change to the parameter of OnItemIntPropertyChanged (because of other mailnews changes) I've not changed anything else.
Comment 102 Mark Banner (:standard8) 2008-10-02 07:04:46 PDT
Created attachment 341439 [details] [diff] [review]
[checked in] Fix growl registration

This patch fixes the registration of the service with growl by additionally registering with the category manager to receive notification of app-startup. When it gets the app-startup notification it registers to receive the before-growl-registration, and then the registrations proceed as planned.

The nsMessengerOSXIntegration::Init() function will cause initialisation of the account manager, msg mail session and shut-down service to happen earlier, but I can see nothing in those initialisations that would do something bad at the wrong time.
Comment 103 Mark Banner (:standard8) 2008-10-02 07:07:30 PDT
(In reply to comment #99)
> Thanks to Mnyromyr's hint from bug 443775 comment 22 (that removing the string
> makes it actually work in Tb), some thoughts based on finally having seen it
> working (many of which can be followups, but I'm leaving for a while, and don't
> want to file "once it lands, it will be broken this way" bugs):

Given the amount of time that this code has been in the bug, I suggest we land it as is (reviews permitting), and file follow ups. I'll be happy to blog about the fact that we expect issues (or there are known issues) etc - but I think we're more likely to get those fixed if the code is in the code base rather than lying in this bug.
Comment 104 David :Bienvenu 2008-10-03 12:18:11 PDT
Comment on attachment 341438 [details] [diff] [review]
[checked in] Combined and updated

lose the printf here:

+  if (!strcmp(aTopic, "before-growl-registration"))
+  {
+      printf("\n\n\nbefore-growl-registration\n\n\n");

and here:

+      printf("...trying to get string bundle...\n");
+      nsCOMPtr<nsIStringBundle> bundle; 


typo:

+      // we are always going to remove the icon whenever we get our first no mai
+      // notification. 

It would be nice to share this code with the class it looks like its cloned from ;-)
Comment 105 Mark Banner (:standard8) 2008-10-04 06:17:25 PDT
(In reply to comment #104)
> (From update of attachment 341438 [details] [diff] [review])
> lose the printf here:
> 
> +  if (!strcmp(aTopic, "before-growl-registration"))
> +  {
> +      printf("\n\n\nbefore-growl-registration\n\n\n");
> 
> and here:
> 
> +      printf("...trying to get string bundle...\n");
> +      nsCOMPtr<nsIStringBundle> bundle; 

They have been removed in the second patch.

> typo:

Fixed locally, along with some whitespace 

> It would be nice to share this code with the class it looks like its cloned
> from ;-)

I filed bug 458507.
Comment 106 Nomis101 2008-10-04 11:34:42 PDT
I've tested this two patches (341438/341439) in my own build. I have multiple accounts in Thunderbird. If I receive new mails for only one account, than the Growl notification works perfect (notifies me about new mails and the amount of new mails). But if I receive new mails for more than one account, than it displays me only a Growl notification for one account.
But  theoretically it must be possible to display two (or more?) Growl notifications at the same time, I have an Applescript with growlnotify that doing this.

OS X 10.5.5, Growl 1.1.4, Thunderbird/3.0b1pre (de)
Comment 107 Phil Ringnalda (:philor) 2008-10-04 12:06:46 PDT
Nomis101: that would be comment 99 (and thus comment 103).
Comment 108 David Humphrey (:humph) 2008-10-08 11:45:57 PDT
So since this bug is "growl integration" and that's been done, can we land this and file new bugs on the comments above?  There are lots of things you *could* do with this, now that the base is in place.  But dragging this bug on even longer doesn't seem helpful to me.
Comment 109 Mark Banner (:standard8) 2008-10-08 14:13:13 PDT
(In reply to comment #108)
> So since this bug is "growl integration" and that's been done, can we land this
> and file new bugs on the comments above?  There are lots of things you *could*
> do with this, now that the base is in place.  But dragging this bug on even
> longer doesn't seem helpful to me.

I quite agree, we're just waiting on Karsten (Mnyromyr) to review the extra changes that were needed which should happen this week. When we land, I'll file follow-ups for the items mentioned in comment 99.
Comment 110 Karsten Düsterloh 2008-10-10 10:46:04 PDT
Comment on attachment 341439 [details] [diff] [review]
[checked in] Fix growl registration

>+  if (NS_FAILED(rv))
>+    return rv;

I'd prefer 
  NS_ENSURE_SUCCESS(rv, rv);

>+  if (NS_FAILED(rv)) return rv;

.. but either way, this is inimical to debuggers; at least put the return on its own line.


With both patches applied, this works for me (SM debug build), registering a "New Mail" notification and even activating exactly that on new mail (not just the general notification). Cool!
Comment 111 Mark Banner (:standard8) 2008-10-11 01:16:57 PDT
(In reply to comment #99)
> Thanks to Mnyromyr's hint from bug 443775 comment 22 (that removing the string
> makes it actually work in Tb), some thoughts based on finally having seen it
> working (many of which can be followups, but I'm leaving for a while, and don't
> want to file "once it lands, it will be broken this way" bugs):

I have filed new bugs for these. See bug 459482, bug 459483, bug 459484, bug 459485 - also in the list above of bugs that this bug blocks.
Comment 112 Mark Banner (:standard8) 2008-10-11 01:25:33 PDT
Comment on attachment 341438 [details] [diff] [review]
[checked in] Combined and updated

Checked in, changeset id: 585:a2226c07cc4d
Comment 113 Mark Banner (:standard8) 2008-10-11 01:26:12 PDT
Comment on attachment 341439 [details] [diff] [review]
[checked in] Fix growl registration

Checked in, changeset id 586:1f3987606410
Comment 114 Mark Banner (:standard8) 2008-10-11 01:28:47 PDT
This bug is now fixed, it will be included in today's Thunderbird nightlies (assuming all goes well) and tomorrow's SeaMonkey nightlies.. Please file follow up issues in separate bugs if they haven't already been filed (see comment 111).

Many thanks to David for doing the bulk of the work on this.
Comment 115 Mark Banner (:standard8) 2008-10-11 03:18:20 PDT
Note: I've just filed bug 459487 which prevents this from working when new mail notifications are received on IMAP folders which are also selected for downloading for offline use.
Comment 116 Jurek R. 2009-12-09 12:42:43 PST
Growl Notifications do not work in recent Shredder nightly...
Comment 117 David Humphrey (:humph) 2009-12-09 13:26:32 PST
(In reply to comment #116)
> Growl Notifications do not work in recent Shredder nightly...

Blake and I just tested this, and its WFM.  If you are indeed still seeing this, please file a new bug.
Comment 118 Jurek R. 2009-12-17 05:03:40 PST
Sorry, I had installed Growl notification plugin which is obsolete. It works fine without it.

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