:Irving Reid (No longer working on Firefox) Reporter
	Description • 13 years ago

Each OS integration currently contains its own implementation of how to determine whether to notify the user about new messages.

This bug is to track implementation of a new mail notification service to provide one place for the logic for counting, filtering, etc.

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 1 • 13 years ago

To do:

Clean up dump() statements and use log4moz where appropriate
Convert OS X integration to use NewMailListener callbacks for unread count
Implement New vs. Unread message count preference in new service
Implement callbacks with message contents for popup notifications (e.g. Growl)

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 2 • 13 years ago

Adds log4moz logging (with a couple of tiny fixes to the TB l4m implementation)
Switch OS X icon badge to use unread count from new mail notification service

Next:
Implement badging with count of new messages (instead of unread)
Implement popup notification with message details (probably based on a mixture of existing Windows and Linux algorithms)

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 3 • 13 years ago

Also: I'm considering a couple of framework level changes:

1) Switch the OS integration components to initialize in later categories (profile-after-change or mail-startup-done) rather than initializing in app-startup and then registering themselves for those later events to finish setting up (https://developer.mozilla.org/en/XPCOM/Receiving_startup_notifications) - mostly to make it simpler to prevent them from trying to initialize NewMailNotificationService and log4moz before the prefs are available.

2) Have the 'mail-startup-done' notification, currently sent through the Observer service, also instantiate any objects registered with the category service, like the Firefox platform does with profile-after-change (http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/xre/nsXREDirProvider.cpp#753). This would make it simpler for both core and extension components to start up after everything they need is ready.

maybe) Convert the OS integration components (ie nsMessengerOSXIntegration) to load up using the Gecko 2 style manifest file rather than hard coded in nsMailModule.cpp

	Mark Banner (:standard8)
	Comment 4 • 13 years ago

Comment on attachment 587932 [details] [diff] [review]
Second WIP patch

Review of attachment 587932 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start. Comments below.

::: mailnews/base/public/mozINewMailListener.idl
@@ +38,5 @@
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(e15f104f-a16d-4e51-a362-4b4c5efe05b9)]
> +interface mozINewMailListener : nsISupports {
> +  void onUnreadCountChanged(in unsigned long unreadCount);

We'll need to add doxygen documentation to these as found with other interfaces that are about. Should add a general description as well.

::: mailnews/base/src/Makefile.in
@@ +125,5 @@
>  
>  EXTRA_COMPONENTS = \
>  		nsMailNewsCommandLineHandler.js \
>  		msgAsyncPrompter.js \
> +		newMailNotificationService.js \

This will also need adding to the mail and suite package-manifest.in files.

::: mailnews/base/src/newMailNotificationService.js
@@ +62,5 @@
> +let gNotificationInstance = null;
> +
> +// constructor
> +function NewMailNotificationService() {
> +  // Implement as a singleton with access to JS object

I think you can now do this in a much better way - see bug 722254

@@ +67,5 @@
> +  if (gNotificationInstance) {
> +    return gNotificationInstance;
> +  }
> +  gNotificationInstance = this;
> +  this.wrappedJSObject = this;

We shouldn't be needing to wrap this object. Components should work without wrapping.

@@ +100,5 @@
> +        Services.obs.addObserver(this, "mail-startup-done", false);
> +      }
> +      else if (aTopic == "mail-startup-done") {
> +        MailServices.mailSession.AddFolderListener(this, Ci.nsIFolderListener.intPropertyChanged
> +                                                         | Ci.nsIFolderListener.added

nit: we normally prefer | (and other operators) on the end of lines.

@@ +113,5 @@
> +
> +  initUnreadCount: function NMNS_initUnreadCount() {
> +    let total = 0;
> +    let allServers = MailServices.accounts.allServers;
> +    for (let i = 0 ; i < allServers.Count() ; i++) {

nit: no space before ;

@@ +115,5 @@
> +    let total = 0;
> +    let allServers = MailServices.accounts.allServers;
> +    for (let i = 0 ; i < allServers.Count() ; i++) {
> +      let currentServer = allServers.GetElementAt(i)
> +                                    .QueryInterface(Ci.nsIMsgIncomingServer);

Use QueryElementAt(index, Ci...)

@@ +162,5 @@
> +    else if (aFolder.flags & Ci.nsMsgFolderFlags.Virtual)
> +      shouldCount.data = false;
> +
> +    // if we're only counting inboxes and it's not an inbox...
> +    else try {

try on the next line please.

::: mailnews/base/src/nsMessengerOSXIntegration.mm
@@ +270,5 @@
> +    rv = newmail->AddListener(this, mozINewMailNotificationService::unreadCount);
> +    NS_ENSURE_SUCCESS(rv, rv); // This should really be an assert with a helpful message
> +
> +    // Get the initial unread count. Ignore return value; if code above didn't fail, this won't
> +    rv = newmail->GetUnreadMessageCount(&mUnreadTotal);

So the new mail service won't have set this up until mail-startup-done. Hence in theory, this will always be zero. Probably not a problem for initial startup, but we could potentially avoid badging the dock icon until we get the initial count if the notification services notifies us when it does its init.

::: mailnews/base/test/unit/test_newMailNotification.js
@@ +41,5 @@
> + * information to platform-specific notification modules */
> +
> +Components.utils.import("resource:///modules/mailServices.js");
> +
> +const iNMNS = Ci.mozINewMailNotificationService;

It might be nicer to use MailServices.newMailNotification instead of the const.

@@ +54,5 @@
> +/*
> + * Register listener for a particular event, make sure it shows up in the right lists
> + * of listeners (and not the wrong ones) and doesn't show up after being removed
> + */
> +function testListeners() {

I'm not sure we need these listener specific tests here, I think you could do it implied via testNotifyInbox, then you wouldn't need to expose the service via wrappedJSObject e.g. see what I did in this test:

http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_nsIMsgFolderListener.js

::: mailnews/base/util/mailServices.js
@@ +100,5 @@
>  XPCOMUtils.defineLazyServiceGetter(MailServices, "junk",
>                                     "@mozilla.org/messenger/filter-plugin;1?name=bayesianfilter",
>                                     "nsIJunkMailPlugin");
> +
> +XPCOMUtils.defineLazyServiceGetter(MailServices, "newMailNotification",

I wonder if we shouldn't go for something shorter like just "newmail"

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 5 • 12 years ago

(In reply to Mark Banner (:standard8) from comment #4)

Updated WIP coming that addresses all the comments I'm not responding to below...

> ::: mailnews/base/src/nsMessengerOSXIntegration.mm
> @@ +270,5 @@
> > +    rv = newmail->AddListener(this, mozINewMailNotificationService::unreadCount);
> > +    NS_ENSURE_SUCCESS(rv, rv); // This should really be an assert with a helpful message
> > +
> > +    // Get the initial unread count. Ignore return value; if code above didn't fail, this won't
> > +    rv = newmail->GetUnreadMessageCount(&mUnreadTotal);
> 
> So the new mail service won't have set this up until mail-startup-done.
> Hence in theory, this will always be zero. Probably not a problem for
> initial startup, but we could potentially avoid badging the dock icon until
> we get the initial count if the notification services notifies us when it
> does its init.

I did this on purpose so that it doesn't matter which order the two services initialize; if the OSX integration is first it will receive the initial count by callback when the notification service initializes, and if the notification service is first the OSX integration gets the initial count from the attribute and sees later changes through the callback.


> ::: mailnews/base/test/unit/test_newMailNotification.js
> @@ +41,5 @@
> > + * information to platform-specific notification modules */
> > +
> > +Components.utils.import("resource:///modules/mailServices.js");
> > +
> > +const iNMNS = Ci.mozINewMailNotificationService;
> 
> It might be nicer to use MailServices.newMailNotification instead of the
> const.

The const is only used for the flag constants defined in the interface; are those values also available through the service object itself?


> I wonder if we shouldn't go for something shorter like just "newmail"

I probably thought up a bunch of scenarios where people invented deliberate mis-interpretations of the name and then argued about them, to convince myself to be longer and overly specific. Happy to change if others prefer these names to be shorter.

	David :Bienvenu
	Comment 6 • 12 years ago

(In reply to Irving Reid (:irving) from comment #5)

> > I wonder if we shouldn't go for something shorter like just "newmail"
> 
> I probably thought up a bunch of scenarios where people invented deliberate
> mis-interpretations of the name and then argued about them, to convince
> myself to be longer and overly specific. Happy to change if others prefer
> these names to be shorter.

I think you were right - newMail is ambiguous - the service is just about notifications, right?

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 7 • 12 years ago

I was sure I had more progress on this patch (hooking up the "new" mail count along with the unread) but I can't find it; I think it got lost in my switch from hg pbranch to mq.

I didn't remove the "wrapped JS object" hook that I'm using for testing, because I haven't taken the time to rewrite the other tests to check that functionality from outside the XPCOM api. Is it feasible to just use the JS object directly for that sort of low level tests, rather than instantiating through the service factory?

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 8 • 12 years ago

This has (as far as I can tell) feature identical implementation of new and unread mail counting as used by the Mac OS X notification implementation, moved to a platform independent Javascript service. This could be reviewed and landed, or held until it also includes popup notifications.

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 9 • 12 years ago

Mark wants to start landing chunks of this work incrementally. This should be completely compatible with the existing OS X unread mail / new mail count, and shouldn't affect any other platforms.

	Mark Banner (:standard8)
	Comment 10 • 12 years ago

Comment on attachment 634988 [details] [diff] [review]
Complete handling of Mac new/unread mail count and icon badging

Review of attachment 634988 [details] [diff] [review]:
-----------------------------------------------------------------

Some initial comments, mainly style. I've got a few more coming.

::: mailnews/base/public/mozINewMailListener.idl
@@ +11,5 @@
> + * NOTE: THIS INTERFACE IS UNDER ACTIVE DEVELOPMENT AND SUBJECT TO CHANGE,
> + * see https://bugzilla.mozilla.org/show_bug.cgi?id=715799
> + */
> +interface mozINewMailListener : nsISupports {
> +  /** The new mail notification service will call this when the number of interesting

nit: we generally start the text of the comment on the line after this.

::: mailnews/base/src/newMailNotificationService.js
@@ +29,5 @@
> +// When we go cross-platform we should migrate to
> +// const countNewMessagesPref = "mail.notification.count.new";
> +
> +// Helper function to retrieve the "are we counting new messages" pref
> +function countNew() {

I think you should make this an attribute of NewMailNotificationService, so you can just do if (this.countNew)... etc. I think it also wouldn't hurt to make this a read-on startup pref as it is a hidden one - you can then use the once-only initialisation seen here: http://hg.mozilla.org/comm-central/annotate/28b099d53998/mailnews/base/src/nsMailNewsCommandLineHandler.js#l19

@@ +72,5 @@
> +    // Set up to catch updates to unread count
> +    this._log.info("NMNS_Observe: " + aTopic);
> +
> +    try {
> +      if (aTopic == "mail-startup-done") {

We should remove the mail-startup-done observer here.

@@ +225,5 @@
> +    }
> +  },
> +
> +  _newMailReceived: function NMNS_newMailReceived(folder, oldValue, newValue) {
> +    if (this.confirmShouldCount(folder)) {

I think you can negate the if and return early.

@@ +268,5 @@
> +                                                                    property,
> +                                                                    oldFlag,
> +                                                                    newFlag) {
> +    if (item instanceof Ci.nsIMsgDBHdr) {
> +      if((oldFlag & Ci.nsMsgMessageFlags.New)

nit: space before the first (

	Mark Banner (:standard8)
	Comment 11 • 12 years ago

Comment on attachment 634988 [details] [diff] [review]
Complete handling of Mac new/unread mail count and icon badging

Review of attachment 634988 [details] [diff] [review]:
-----------------------------------------------------------------

This is generally looking good, but in testing I found a couple issues:

- I think you're missing doing the unread count on the inboxes at startup. Looking at _countUnread, it looks as if you're not counting the unread of the root folder, like the previous GetTotalUnread function was.

::: mailnews/base/src/newMailNotificationService.js
@@ +287,5 @@
> +  
> +
> +  // Implement mozINewMailNotificationService
> +
> +  getMessageCount: function NMNS_getMessageCount() {

You definitely need to use the "get messageCount() {" form here, as otherwise xpcom doesn't pick up the function and we might not give the dock icon total on start up (depending on order of registrations for the mail-startup-done notification).

	David :Bienvenu
	Comment 12 • 12 years ago

Comment on attachment 634988 [details] [diff] [review]
Complete handling of Mac new/unread mail count and icon badging

clearing sr request based on r-. I did run with the patch a bit. The first thing I noticed is that the badge count isn't cumulative (it wasn't before this patch either, but it feels like it should be). In other words, on startup, I got 6 new messages in one account, and 1 new message in an other, but the badge count was 6.

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 13 • 12 years ago

Fixed one-based array index bug (bah!) that was often skipping INBOX, cleaned up nits, added checking the count on the root folder of the server.

I still occasionally see crazy results with the "count new messages" pref set, but based on the logging it's because the nsMsgDBFolder is notifying with a crazy number rather than because of an issue in this code.

	Mark Banner (:standard8)
	Comment 14 • 12 years ago

I'm still testing, but I've just noticed that if I toggle unread/read on an RSS message feed item, then that changes the count in the dock icon when you're counting all folders and unread messages.

	David :Bienvenu
	Comment 15 • 12 years ago

Comment on attachment 637151 [details] [diff] [review]
Unread new new mail counts on Mac, with review nits cleaned

that works better for me...I worry about adding extra notifications from c++ to js, but we already do similar things for the folder pane. And since we're hiding things behind a new notification service, we can probably be clever about which notifications it gets if it turns out to be a performance issue.

	Mark Banner (:standard8)
	Comment 16 • 12 years ago

Comment on attachment 637151 [details] [diff] [review]
Unread new new mail counts on Mac, with review nits cleaned

Review of attachment 637151 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately this has bitrotted slightly, but I tested it pre-bitrot and didn't see any issues apart from the rss one I previously mentioned. I've included a note below as to what I think you should do with it.

I think I'd just like to give that change a quick once-over and then it'll be r+.

::: mailnews/base/src/newMailNotificationService.js
@@ +159,5 @@
> +  },
> +
> +  // Filter out special folders and then ask for observers to see if
> +  // we should monitor unread messages in this folder
> +  confirmShouldCount: function NMNS_confirmShouldCount(aFolder) {

Based on my previous comment, I think this function should try and see if the folder is nntp or rss and not try to count it.

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 17 • 12 years ago

Removed the bitrot, but now needs to be extended to include Florian's unread IM handling - for now, I left that in the C++ module.

Added a specific check for RSS folders to default to not counting new/unread messages in them.

	Mark Banner (:standard8)
	Comment 18 • 12 years ago

Comment on attachment 639169 [details] [diff] [review]
[checked in] Unread new new mail counts on Mac, no longer counts RSS

Review of attachment 639169 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, looks good. r=me

	Mark Banner (:standard8)
	Comment 19 • 12 years ago

Comment on attachment 639169 [details] [diff] [review]
[checked in] Unread new new mail counts on Mac, no longer counts RSS

Checked in:

https://hg.mozilla.org/comm-central/rev/53f0b1dc8aab

	Mark Banner (:standard8)
	Comment 20 • 12 years ago

(In reply to Mark Banner (:standard8) from comment #19)
> Comment on attachment 639169 [details] [diff] [review]
> [checked in] Unread new new mail counts on Mac, no longer counts RSS
> 
> Checked in:
> 
> https://hg.mozilla.org/comm-central/rev/53f0b1dc8aab

I had to do a follow-up to disable the test on non-Mac platforms, I'm suspecting that the service isn't installed into those fully or something, so I didn't look in detail:

https://hg.mozilla.org/comm-central/rev/263cf289bce4

	Wayne Mery (:wsmwk)
	Comment 21 • 12 years ago

Was this deemed successful on Mac?
What is next step?
(fixes real bugs, so changing sev)

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 22 • 12 years ago

I have a tiny bit of WIP patch beyond what was checked in, just starting to implement gathering all the new message headers in order to display a summary of new message details in the Growl notification. Having that would be the next major step along the way to merging with the Windows and Linux implementations.

Because the Linux and Windows notification implementations behave quite differently (both in terms of bugs and in terms of configurable desired behaviours) there might be some disagreement about exactly how to merge the platforms, but I think it's the right thing to do in the long run.

I'm not likely to get back to it soon, so I'll post my WIP patch and flag it helpwanted.

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 23 • 12 years ago

The intent for this patch was to reimplement as much as possible of the code around nsMessengerOSXIntegration::FillToolTipInfo (http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessengerOSXIntegration.mm#289) and its helper methods in JavaScript

	Marvin
	Comment 24 • 10 years ago

Will this be fixed in a later version (sorry, I didn't read all the messages)?
I just upgraded from TB 2 (yes, really) to TB 24, and the "new" behavior (even though it may have been around for sometime) is extremely annoying.

I WROTE the draft, so of course I have *READ* it as well.
There's no logic in marking/treating it as unread.