138095 - System tray mail notification tooltip should 'listen' for additional incoming mail.

Status: NEW

(MailNews Core :: Backend, enhancement)

Description

[[e-mail] (gone - use [e-mail] instead)]

Build ID: 2002-04-16, trunk and branch commercial bits on Windows 2000/NT
4.0/Windows XP/Windows 98.

Summary: System tray mail notification tooltip should 'listen' for additional
incoming mail.

Steps to Reproduce:

1.  Enable 'Check for new messages every x minutes' set in Server Settings
(Account Manager).
2.  Restart.
3.  Send yourself a message, wait for mail notification to go off.
4.  When the system tray notification appears, mouse over it and look at the
tooltip. (It should say 'username has 1 new message').
5.  Send yourself another message, without reading the 1st.
6.  Wait for the specified biff interval to fire.
7.  Repeat step #4.

Expected Results:

Tooltip now says 'username has 2 new messages'.

Actual Results:

Tooltip says 'username has 1 new message'.

This was spun off from bug 123104.

Comment 1

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 141573 has been marked as a duplicate of this bug. ***

Comment 2

[René Stach]

I can confirm this behaviour for all major releases in the past. I saw it in the 0.9x versions and the releases 1.0.x, 1.2.1. I always said to myself: This bug is so obvious, they cannot afford to leave it unfixed.

Comment 3

[Ray Birks]

Confirming on FireBird 0.6.1+ build 20030906 in WinME.

Comment 4

[Mike Cowperthwaite]

*** Bug 268449 has been marked as a duplicate of this bug. ***

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 300213 has been marked as a duplicate of this bug. ***

Comment 6

[Ted Wray]

I believe the original anaysis of this bug is incorrect.  I believe that each
time mail is recieved the number of new emails replaces the prior count in the
system tray.  It should be added to the prior total, then zeroed when the
notification is removed from the tray.  This may be verified by exiting the
system long enough for several messages to accumulate.  Upon restarting, one is
notified that, say, 18 new messages have arrived.  Then wait for the next d/l. 
The message count will now be the same as the number of messages most recently
downloaded.

If my analysis is correct, then this would be a simple fix of only the display
routine.

Comment 7

[Ted Wray]

Attachment: I think this is the problem code with a suggested fix.

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: CVS diff of suggested changes

Note that I'm totally unable to test this patch, just attaching it to make
potential reviews easier.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Ted, as you can see, I've attached a diff of suggested changes.  If you want
this in, set the appropriate review (mscott, exclusively, since he wrote the
original code)/super-review flags on the attachment, but note that I can't test
or help out.  Thanks!

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #8)
> Created an attachment (id=189945) [edit]
> CVS diff of suggested changes
> 
> Note that I'm totally unable to test this patch, just attaching it to make
> potential reviews easier.

This patch doesn't build:

c:\mozilla\mozilla\mailnews\base\src\nsMessengerWinIntegration.cpp(675) : error
C2065: 'numNewMessages' : undeclared identifier

Comment 11

[Ted Wray]

(In reply to comment #10)
> (In reply to comment #8)
> > Created an attachment (id=189945) [edit] [edit]
> > CVS diff of suggested changes
> > 
> > Note that I'm totally unable to test this patch, just attaching it to make
> > potential reviews easier.
> 
> This patch doesn't build:
> 
> c:\mozilla\mozilla\mailnews\base\src\nsMessengerWinIntegration.cpp(675) : error
> C2065: 'numNewMessages' : undeclared identifier
> 

(In reply to comment #10)
> (In reply to comment #8)
> > Created an attachment (id=189945) [edit] [edit]
> > CVS diff of suggested changes
> > 
> > Note that I'm totally unable to test this patch, just attaching it to make
> > potential reviews easier.
> 
> This patch doesn't build:
> 
> c:\mozilla\mozilla\mailnews\base\src\nsMessengerWinIntegration.cpp(675) : error
> C2065: 'numNewMessages' : undeclared identifier
> 


Not at all surprising, since this was NOT a patch.  Rather, it was a text
illustration of the bug and a suggested fix.  Someday, when I have the time and
free disk space, if no one fixes this first, I may be able to d/l the various
protocols and other necessaries and procede to try to move this along the path.
 In the meanwhile, anyone familiar with those procedures might actually effect a
fix.  Note, though, that this is just a SUGGESTED fix that involves changeing a
subroutine to ACCUMULATE a value rather than return just an addend.  THEREFORE,
it would be ESSENTIAL to examine every other routine that calls this subroutine
to determine that this change would do no harm.  (The calling routine in
question unnecessarily zeros this variable priod to the call.)

Comment 12

[OstGote!]

related to bug 210148

Comment 13

[Lee_Dailey]

howdy y'all,

[1] my tb info ...
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051201 Thunderbird/1.5 - Build ID: 2005120115

[2] REQUEST = change "Product" from "mozilla application suite" to "core"
since the tbird bug ...
- New Messages Counter displays "you have 1 new message" event if I have 2 or 3
https://bugzilla.mozilla.org/show_bug.cgi?id=268449

... was resolved as a dupe of this one, shouldn't the product be changed?

[3] just to make it painfully clear, the problem still exists in tb15.

take care,
lee

Comment 14

[Magnus Melin [:mkmelin]]

Assuming that patch only needs something variable declaration to work, someone on windows want to give it some love?

Comment 15

[Casper Gripenberg]

I've spent the past two days trying to track this bug down. There
seems to be many reports on it all over bugzilla. To start to branch
out into the "bug tree" of this problem you can start by looking at
bug 421542. They are all related to the same problem.

I'm going to use the term "more new mail" indicating you 
already have the tray icon up and visible and then you 
receive additional new mail, triggering all the bugs.

The bugs are:

1: The tray text for the tray icon does not show the
   total amount of new mail you have. It only shows
   the amount of new mail you get the first time the
   tray icon is made visible. 

2: The new mail sound and new mail popup is not shown
   for more new mail.

The patch above does not fix these problems. The patch 
in itself will also introduce another bug into 
GetNumNewMessages().

The problem is much harder to fix than that (as far as I can see).

The patch was for the method FillToolTipInfo(). Problem number one
is FillToolTipInfo() is not even called when more new mail arrives.

Ok, so where is the problem then? It's in several locations. 

First:

There seems to be something called the "BiffState" that is sprinkled
all over the Thunderbird code. Many files access the biff state,
not only the Windows integration code, but both the generic IMAP and
POP protocol code manipulate the biff state.

The term "biff" comes from a command line tool on Unix called biff
that simply tells you "y" or "n" depending on if you have new mail
(y), or no new mail (n).

So this is simply a boolean value saying "you have new mail", or "you
don't have any new mail". It never tells you anything about how much
new mail you have.

The problem is the windows integration code is depending on this
variable to determine if the tray icon should be updated and if the
"you have new mail" popup should be shown and the mail sound played.

To me it looks like the biff state is only updated ONCE..when you get
new mail..it is never updated again when you get MORE new mail -> none
of the new mail updating code ever gets called for more new mail.

Here's the windows integration code that does this check:

NS_IMETHODIMP
nsMessengerWinIntegration::OnItemIntPropertyChanged(nsIRDFResource *aItem, nsIAtom *aProperty, PRInt32 aOldValue, PRInt32 aNewValue)
{
  // if we got new mail show a icon in the system tray
  if (mBiffStateAtom == aProperty && mFoldersWithNewMail)
  {
     // This branch never gets called on more new mail
     // 
     // mBiffStateAtom is the "BiffState" propery that the if
     // statement is trying to check for an update on. But BiffState
     // is never updated when more new mail arrives..
    
     ...code that updates the tooltip text and displays new mail popup...
  }

  ...
}

So..what's the fix for this? The biff state clearly provides 
insufficient information to determine if more new mail has 
arrived. I updated the code to look at the count of unread
messages instead. If the count of unread messages is going
up, it means we have more new mail. This fixes the problem
with FillToolTipInfo() not being called.

  if ((aProperty == mBiffStateAtom || 
      (aProperty == mTotalUnreadMessagesAtom && aNewValue > aOldValue)) &&
      mFoldersWithNewMail) 
  {
    ...
    // The following two if statements also need to be updated to:
    if ((aProperty == mBiffStateAtom && 
         aNewValue == nsIMsgFolder::nsMsgBiffState_NewMail) ||
        aProperty == mTotalUnreadMessagesAtom) 
    {
      // if the icon is not already visible, only show a system tray icon iff 
      // we are performing biff (as opposed to the user getting new mail)
      ...
    }
    else if (aProperty == mBiffStateAtom && 
             aNewValue == nsIMsgFolder::nsMsgBiffState_NoMail)
    {
      // we are always going to remove the icon whenever we get our
      // first no mail notification.
      ...
    
Ok..this will get us a little further, as FillToolTipInfo()
is now called correctly when we have more new mail. But we
still run into further problems.

1: I couldn't figure out where to get the total number
   of new messages from. The biff state and the folder
   itself only reports the count of new messages from
   the latest IMAP/POP polling cycle, not the totals.

2: The tool tip now gets updated with each polling cycle,
   which is some progress, but the text in the tooltip is 
   still wrong, and sometimes seems to show the folder 
   name twice (one for "Inbox", and one for the account name).

3: The popup notification window still doesn't come up.
   The problem seems to be at the very end of FillToolTipInfo():

     if (!mBiffIconVisible)
     {
   #ifndef MOZ_THUNDERBIRD
       ShowAlertMessage(accountName, animatedAlertText.get(), "");
   #else
       ShowNewAlertNotification(PR_FALSE);
   #endif
     }
     else
       GenericShellNotify( NIM_MODIFY);

   ..but even fiddling with that code never got me the popup
   to show up correctly.

At this point I'm throwing in the towel :( The code is a bundle 
of state variables and callbacks, that I think only the original 
authors can unravel (or people with more extra time than me). 

I wish the original authors would fix their code.

Comment 16

[Mike Cowperthwaite]

(In reply to comment #15)
> The bugs are:
> 
> 1: The tray text for the tray icon does not show the
>    total amount of new mail you have. It only shows
>    the amount of new mail you get the first time the
>    tray icon is made visible. 
> 
> 2: The new mail sound and new mail popup is not shown
>    for more new mail.

As you've already been told, (2) is another issue -- bug 210148 / bug 378150.
I'll add a comment at 210148.


> The patch above does not fix these problems. The patch in itself will also
> introduce another bug into GetNumNewMessages().

OK.


> The problem is much harder to fix than that (as far as I can see).

That's a big part of why this bug is still open.


> There seems to be something called the "BiffState" that is sprinkled
> all over the Thunderbird code. Many files access the biff state,
> not only the Windows integration code, but both the generic IMAP and
> POP protocol code manipulate the biff state.
> 
> The term "biff" comes from a command line tool on Unix called biff
> that simply tells you "y" or "n" depending on if you have new mail
> (y), or no new mail (n).
> 
> So this is simply a boolean value saying "you have new mail", or "you
> don't have any new mail". It never tells you anything about how much
> new mail you have.
> 
> The problem is the windows integration code is depending on this
> variable to determine if the tray icon should be updated and if the
> "you have new mail" popup should be shown and the mail sound played.
> 
> To me it looks like the biff state is only updated ONCE..when you get
> new mail..it is never updated again when you get MORE new mail -> none
> of the new mail updating code ever gets called for more new mail.

That sounds like a reasonable analysis.


> At this point I'm throwing in the towel :( The code is a bundle 
> of state variables and callbacks, that I think only the original 
> authors can unravel (or people with more extra time than me). 

I understand this -- I've submitted a few patches, but only simple ones in the JS frontend.  The code is Byzantine, no doubt about it.  Still, it sounds like you've actually run code changes that come closer to fixing this bug than has been done before; I hope you stick with it, as I'd really like to see this one fixed.


> I wish the original authors would fix their code.

The original authors have gone on to work for companies that will actually pay them for their work.  Also, you may have a hard time convincing them their code was broken.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 189945 [details] [diff] [review]
CVS diff of suggested changes

Not meant to be a working patch per comments #8 to #11.