Closed Bug 308552 Opened 19 years ago Closed 16 years ago

Growl Integration for Mail Alerts on Mac OS X

Categories

(Thunderbird :: Mail Window Front End, defect, P3)

Product:
Thunderbird
Component:
Mail Window Front End
Platform:
All
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 3.0b1

People

(Reporter: mscott, Assigned: humph)

References

(
URL
)

Details

Attachments

(2 files, 11 obsolete files)

[checked in] Combined and updated
20.91 KB, patch
mnyromyr
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
[checked in] Fix growl registration
5.53 KB, patch
mnyromyr
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
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.
I would note that we already have bugs (and patches!) on making the alert service XP.
http://growl.info/documentation/developer/ This page indicates that there is a Carbon API. I'm going to whip something up now...
see also bug 282185
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.
Related to/duplicate of bug 303476?
OS: Windows XP → MacOS X
Hardware: PC → Macintosh
*** Bug 303476 has been marked as a duplicate of this bug. ***
(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.
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.
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.
(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.
Oh ya, the sdk is over here: http://growl.info/downloads_developers.php
(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/
Assignee: joshmoz → nobody
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?
I made an extension which adds this feature to Thunderbird through growlnotify command line utility: https://addons.mozilla.org/thunderbird/3448/
Attached patch strings for the growl alert (obsolete) — — Splinter Review
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.
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.)
(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.
(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.
Scott - do you guys use nsIAlertsService for this, or do you have your own interface for it?
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.
Depends on: 362685
Summary: Growl Integration for Mac OS X → Growl Integration for Mail Alerts on Mac OS X
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 :)
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 :)
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.
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.
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 :)
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).
QA Contact: front-end
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.
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?
(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.
(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.
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?
(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.
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?
I'm not 100% sure I understand your question. Are you talking about the static method on the mozGrowlDelegate?
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.
I would suggest that you rename it and update it. You will then have to update the calls elsewhere (I think two places).
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.
Thanks Shawn. I'll wait for it :)
Depends on: 382694
With that dependency landing, I assume this is ready to be worked on now? :)
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 :)
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 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.
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'
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)
(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.
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
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.
(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.
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!
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?
I was against making an xpcom interface for it since it was mac only, but that may be what we need to do :(
Depends on: 394346
I think the new API should be much easier for you to use :)
Wayne - what's the status on this? Any more questions?
(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?
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.
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).
Assignee: nobody → david.humphrey
Attachment #278037 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #290410 - Flags: review?(bienvenu)
thx, David - I'll give this patch a try...
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.
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?
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 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!
Attachment #290410 - Flags: review?(bienvenu) → review+
Attached patch Patch with David's fixes applied (obsolete) — — Splinter Review
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?
Attachment #290410 - Attachment is obsolete: true
Attachment #291444 - Flags: superreview?(shaver)
It also doesn't look like this patch actually registers a named notifications, which is the right thing to do.
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?
I know nothing about Growl, but I doubt adding named notifications would involve reworking this - we'd still be using the nsIAlertService, right?
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.
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));
Attachment #291444 - Attachment is obsolete: true
Attachment #293124 - Flags: review?(comrade693+bmo)
Attachment #291444 - Flags: superreview?(shaver)
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));
Attachment #291444 - Attachment is obsolete: true
Attachment #293125 - Flags: review?(comrade693+bmo)
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.
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?
Hardware: Macintosh → All
Attachment #293124 - Attachment is obsolete: true
Attachment #293124 - Flags: review?(comrade693+bmo)
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.
Attachment #293125 - Flags: review?(comrade693+bmo)
Note that TB3 nightlies already show a Growl notification when an application update has been downloaded and is ready to apply.
Nominating wanted-thunderbird3 to keep it on the radar.
Flags: wanted-thunderbird3?
What else does this bug need to land?
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.
They haven't - my goal was to see if anyone plans on doing the rest of the work needed.
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3.0a2?
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?
Attachment #293125 - Attachment is obsolete: true
Attachment #319294 - Flags: review?(sdwilsh)
(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.
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?
Attachment #319294 - Attachment is obsolete: true
Attachment #319307 - Flags: superreview?(dmose)
Attachment #319294 - Flags: review?(sdwilsh)
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.
Flags: wanted-thunderbird3?
Flags: wanted-thunderbird3.0a2+
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3.0a2?
Flags: blocking-thunderbird3.0a2-
Flags: blocking-thunderbird3+
Flags: wanted-thunderbird3+
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.
Attachment #319307 - Flags: superreview?(dmose) → superreview+
Attached patch r+/sr+ patch minus printfs (obsolete) — — Splinter Review
Removed printfs, sorry about that.
Keywords: checkin-needed
(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.
Keywords: checkin-needed
This should fix it.
Attachment #326529 - Attachment is obsolete: true
Keywords: checkin-needed
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
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 3
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.
Was Thunderbird similarly busted? If not, any idea why not?
(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; }
(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.
Attached patch Patch to fix SM bustage (obsolete) — — Splinter Review
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.
Attachment #328098 - Flags: superreview?(bienvenu)
Attachment #328098 - Flags: review?(mnyromyr)
(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.
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.
Attachment #328098 - Flags: superreview?(bienvenu) → superreview+
(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?
Depends on: 443775
(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 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).
Attachment #328098 - Flags: review?(mnyromyr) → review+
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
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.
moving from blocking to wanted
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Target Milestone: Thunderbird 3 → Thunderbird 3.0b2
Priority: -- → P3
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.
Attachment #247384 - Attachment is obsolete: true
Attachment #319307 - Attachment is obsolete: true
Attachment #326691 - Attachment is obsolete: true
Attachment #328098 - Attachment is obsolete: true
Attachment #341438 - Flags: superreview?(bienvenu)
Attachment #341438 - Flags: review?(mnyromyr)
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.
Attachment #341439 - Flags: superreview?(bienvenu)
Attachment #341439 - Flags: review?(mnyromyr)
(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 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 ;-)
Attachment #341438 - Flags: superreview?(bienvenu) → superreview+
Attachment #341439 - Flags: superreview?(bienvenu) → superreview+
Blocks: 458507
(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.
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)
Nomis101: that would be comment 99 (and thus comment 103).
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.
(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.
Attachment #341438 - Flags: review?(mnyromyr) → review+
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!
Attachment #341439 - Flags: review?(mnyromyr) → review+
Blocks: 459482
Blocks: 459484
Blocks: 459485
Blocks: 459483
(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 on attachment 341438 [details] [diff] [review] [checked in] Combined and updated Checked in, changeset id: 585:a2226c07cc4d
Attachment #341438 - Attachment description: Combined and updated → [checked in] Combined and updated
Comment on attachment 341439 [details] [diff] [review] [checked in] Fix growl registration Checked in, changeset id 586:1f3987606410
Attachment #341439 - Attachment description: Fix growl registration → [checked in] Fix growl registration
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.
Growl Notifications do not work in recent Shredder nightly...
(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.
Sorry, I had installed Growl notification plugin which is obsolete. It works fine without it.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: