119344 - add icon to system tray when biff goes off

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P1)

Description

[Scott MacGregor]

Part of the spec for new mail notification is to place an icon in the system
tray (just like 4.x) on windows when new mail is available. 

I have already filed Bug #119328 to get a new icon to use for this. My current
patch uses the icon from nsotify in 4.x (a white mailbox with a red flag over it).

Behavior:
when biff goes off and we have new mail available for one or more accounts, the
icon is placed in the system tray. 

If you read one of these new messages the icon is removed from the system tray. 

If you mouse over the icon in the system tray you get the following text
"username has # new messages"
If multiple accounts have new mail, the tooltip text shows multiple lines of
information (one account per line). Up to 128 characters. Windows only allows a
tooltip to have 128 characters in it. If you have many accounts with new mail,
we'll show as many accounts as will comfortably fit in the tooltip without
causing truncation. 

This is just the first step in our efforts to improve new mail notification.
This feature is not done yet nor is it finished. 

Patch coming up.

Comment 1

[Scott MacGregor]

setting Sheela as the QA contact. 

I know there are other bugs out there which kind of talk about the same thing
but I  didn't want to go there. 

Comment 2

[Scott MacGregor]

Attachment: changes to nsMessengerWinIntegration to add sys tray icon for biff

Comment 3

[Scott MacGregor]

Attachment: 2nd part of the patch: add a deep version of GetNumNewMessages to nsIMsgFolder

Comment 4

[Scott MacGregor]

I posted 2 patches. The first patch shows the changes I made to
nsMessengerWinIntegration to add support of a system tray icon when biff goes
off. Most of the code is localization related stuff to properly con together the
tooltip text. We register ourselves as a property flag listener on each root
folder. That allows us to get new mail notifications from the folder. 

We keep an array of weak references to each root folder containing new mail. The
tooltip info is generated by iterating over all of these folders and getting the
number of new unread messages for each root folder. A separate line of tooltip
data is generated for each one. The icon is removed from the system tray when
one of the  folders says it no longer has new mail (implying the user took
action and has read one of the messages). 

The 2nd part of the patch was a change to nsIMsgFolder::GetNumNewMessages. I
added a boolean to make it deep so I could ask the root folder how many new
messages this account has. 

I did not post the patch which shows the following
msgBiffIcon.ico  // the windows ico file for the icon going into the system tray
mail.rc          // the resource file which bundles the ico file and gets bundled
                 // into msgbase.dll
resource.h       // contains the resource ID for the mail biff icon used by
                 // mail.rc

Comment 5

[racham]

1. You might want to remove
   +  //mBiffIconData.uTimeout = 10;
   +//  mBiffIconData.szInfo = "You've Got mail";
   +  //mBiffIconData.szInfoTitle = "New Mail Notification"; 
   +//  mBiffIconData.dwInfoFlags = NIIF_INFO;

from the first patch.

2. Also in the WinIntegration's Init() routine,
http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMessengerWinIntegration.cpp#118
we have code that bails out of whole operation, if we detect the timer pref
value is negative.

132 if (timerInterval <= 0) {
133 return NS_OK;
134 }

Though timer pref;s (mail.windows_xp_integration.unread_count_interval) default
value is set to be 5 minutes
(http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/win/winpref.js#173),
it is possible for some ome to customize this to be a negative value. Then the
we simply don't anything on biff front also. So, I think we need couple of
changes like,

mIntervalTime = 0; (in the constructor)

then in Init(), after getting the pref timerInterval (at line 130),

if (timerInterval > 0) 
  mIntervalTime = timerInterval * 1000; 

and later, alter a line in your patch from 

+  if (mSHSetUnreadMailCount && mSHEnumerateUnreadMailAccounts)
+    mStoreUnreadCounts = PR_TRUE; 

to 

+  if (mSHSetUnreadMailCount && mSHEnumerateUnreadMailAccounts && mIntervalTime > 0)
+    mStoreUnreadCounts = PR_TRUE; 

Does that sound right..?

It would have been nicer if we had left a mailbox-with-no-flag instead of
completely removing it. When we get nicer icons, I guess we should do that.

Finally, system tray notification is a reality. Great & Cool..!!

Besides the comments above, patch looks good. r=bhuvan. 

Comment 6

[racham]

I meant patches. Besides my comments, both patches look good. r=bhuvan.

Comment 7

[scottputterman]

+'ing with enthusiasm.

Comment 8

[Scott MacGregor]

Attachment: updated nsMessengerWinIntegration files with Bhuvan's feedback.

Bhuvan, I left the commented out code in there because those are the fields
used on win2k and winxp to use balloon tooltips. Once I figure out how to
incorporate those into the build I'm going to use them. I want to leave them
for reference reasons right now since this is still a work in progress. Made
the other changes you suggested in this patch.

Comment 9

[Scott MacGregor]

Comment on attachment 64392 [details] [diff] [review]
2nd part of the patch: add a deep version of GetNumNewMessages to nsIMsgFolder

r=bhuvan (I had an r=bhuvan on this)

Comment 10

[Scott MacGregor]

Comment on attachment 64886 [details] [diff] [review]
updated nsMessengerWinIntegration files with Bhuvan's feedback.

I had an r=bhuvan on this

Comment 11

[Scott MacGregor]

We'll land this in 0.9.9. I didn't want to check it in right before the 098
deadline in case there are problems with the behavior. 

Comment 12

[Scott MacGregor]

In the win integration patch I removed the extra field I had added to the the
mail session folder listener. We were already getting the notification from the
root folder I didn't need to get it from the mail session as well. So the
following part of the diff is no longer present. I also removed the commented
out lines I was using to get balloon tooltips working (which is not part of this
patch)

-  rv = mailSession->AddFolderListener(this,
nsIFolderListener::boolPropertyChanged | nsIFolderListener::intPropertyChanged);

+  rv = mailSession->AddFolderListener(this,
nsIFolderListener::boolPropertyChanged | nsIFolderListener::intPropertyChanged |
nsIFolderListener::propertyFlagChanged);

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 64392 [details] [diff] [review]
2nd part of the patch: add a deep version of GetNumNewMessages to nsIMsgFolder

sr=sspitzer for the first patch.

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 64886 [details] [diff] [review]
updated nsMessengerWinIntegration files with Bhuvan's feedback.

looks good.  I've got a few nits, the only non-nit is the comment containing
"msft APIs are wstring friendly"
sr=sspitzer once you address them.

nsMessengerWinIntegration is a service, so why not make this atom a member
variable

nsCOMPtr<nsIAtom> mBiffStateAtom;

>+  kBiffStateAtom = NS_NewAtom("BiffState");
mBiffStateAtom = getter_AddRefs(NS_NewAtom("BiffState"));

>+nsIAtom * nsMessengerWinIntegration::kBiffStateAtom = nsnull;
>+  NS_IF_RELEASE(kBiffStateAtom);
can be removed

>+  nsCOMPtr<nsIAppShellService> appService = do_GetService( "@mozilla.org/appshell/appShellService;1", &rv);
>+  if (NS_FAILED(rv)|| (!appService) ) return;

You don't need to check appService, as do_GetService() won't return NS_OK
without appService being non-null;

>+  //mBiffIconData.uTimeout = 10;
>+//  mBiffIconData.szInfo = "You've Got mail";
>+  //mBiffIconData.szInfoTitle = "New Mail Notification";
>+//  mBiffIconData.dwInfoFlags = NIIF_INFO;

should these comments be removed?

>   // return if the timer value is negative or ZERO
>-  if (timerInterval <= 0) {
>-    return NS_OK;
>-  }
>-  else {
>+  if (timerInterval > 0) 
>+  { 

the logic is different here, and my old eyes can't tell if you've preserved the
behaviour when timerInterval
is <= 0 (I think you did, but just double check)

>   // because we care if the unread total count changes
>-  rv = mailSession->AddFolderListener(this, nsIFolderListener::boolPropertyChanged | nsIFolderListener::intPropertyChanged);
>+  rv = mailSession->AddFolderListener(this, nsIFolderListener::boolPropertyChanged | nsIFolderListener::intPropertyChanged | nsIFolderListener::propertyFlagChanged);

the comment should be updated to reflected that we care about unread and new,
right?

>+nsresult nsMessengerWinIntegration::GetStringBundle(nsIStringBundle **aBundle)
>+{
>+  nsresult rv = NS_OK;

you don't need to initialize rv (brendan's gives me flack for that)

>+  *aBundle = bundle;
>+  NS_IF_ADDREF(*aBundle);

instead do the old dmose trick:

NS_IF_ADDREF(*aBundle = bundle);


>+        nsXPIDLString finalText; 
>+        bundle->FormatStringFromName(NS_ConvertASCIItoUCS2("biffNotification").get(), formatStrings, 3, getter_Copies(finalText));

since "biffNotification" is a constant, do
NS_LITERAL_STRING("biffNotification").get(), so that the conversion happens at
compile time.

>+
>+        asciiFinalText.AssignWithConversion(finalText);

since the format comes from a .properties file, isn't this going to be a
problem in localized builds?
usually, the msft APIs are wstring friendly.  or am I missing something?

>+  nsresult rv = NS_OK;

initialization not needed

>+  if (!mStoreUnreadCounts) return NS_OK; // don't do anything here if we aren't storing unread counts...

for debugging sake, do waterson nit so you can set a breakpoint on the return;

if (!mStoreUnreadCounts) 
  return NS_OK; // don't do anything here if we aren't storing unread counts...

>+  if (!mStoreUnreadCounts) return NS_OK; // don't do anything here if we aren't storing unread counts...

waterson nit.

>+  if (!mStoreUnreadCounts) return NS_OK; // don't do anything here if we aren't storing unread counts...

waterson nit.

>+  if (!mStoreUnreadCounts) return NS_OK; // don't do anything here if we aren't storing unread counts...

waterson nit.

>+  if (!mStoreUnreadCounts) return NS_OK; // don't do anything here if we aren't storing unread counts...

waterson nit.

>+  if (!mStoreUnreadCounts) return NS_OK; // don't do anything here if we aren't storing unread counts...

waterson nit.

Comment 15

[Scott MacGregor]

I just checked this in after incorporating Seth's changes (including using a
different shell structure called: NOTIFYICONDATAW which takes a wide string. 

I already have a separate bug on the UE group for getting a final icon for biff. 

Comment 16

[Caspy7]

Not seeing this working on build 2002012605 in win98.  Its not really intended
only for win2k as the OS for the bug might indicate is it?

Comment 17

[Martin Poirier]

Doesn't work for me either, Win98SE, mozilla trunk builds 20020126, 20020127 and
20020128

Comment 18

[jlarsen]

I am seeing this for win2000 and win98 win 20020129, But the icon is not
clickable to run mail.

Comment 19

[Sheela Ravindran]

2002-01-29-06 build on win98. Mail Notification in system tray is not working on 
win98.  See bug that has been logged by mscott # 122254 which is related and 
which is spun off of bug bug # 122228

Comment 20

[Sheela Ravindran]

Verifying after testing the following:
Tested on imap account, POP account, webmail, aol account in a single profile
Tested on each account as default account in a single profile
Works when biff goes off as well as when user clicks on get message button.
Works with Get all messages on multiple accounts as well.
Using build: 2002-01-29-06
WinNT-mailnotification icon appears.  Disappears when mouse over- See bug 
#122351
Win2k-works fine
winXP-works fine

Mail notification does not work on the following version.  This could be 
resolved when bug 122351 gets resolved.  
win95
winME
win98

Verifying this bug since the other platforms are tracked on a different bug as 
mentioned above. 

Comment 21

[John Fredlund]

This bug does not appear to be fixed for me, WIN98 using build 2002020803. Why
is it closed if it still is not working for WinME, WIn95, and Win98? I could
understand a dependency on some other bug, but the one mentioned in comment #20
says it is about a seemingly unrelated problem.