465381 - os integration new mail hooks don't get notifications if biff hasn't cleared

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

nsMsgDBFolder::SetBiffState doesn't notify about biff state changes if the biff state hasn't changed. It checks the server biff state, and if that's the same, it doesn't notify.  (e.g., the user got new mail on an account, but didn't read any of the mail, before more mail came in)

Here's a stack trace. nsMsgDBFolder::SetBiffState is where this choice is made. 	msgbase.dll!nsMessengerWinIntegration::OnItemIntPropertyChanged(nsIMsgFolder * aItem=0x04cd3ac4, nsIAtom * aProperty=0x02697a30, int aOldValue=0x00000001, int aNewValue=0x00000000)  Line 898	C++
 	msgbsutl.dll!nsMsgDBFolder::NotifyIntPropertyChanged(nsIAtom * aProperty=0x02697a30, int aOldValue=0x00000001, int aNewValue=0x00000000)  Line 4338 + 0x76 bytes	C++
 	msgbsutl.dll!nsMsgDBFolder::SetBiffState(unsigned int aBiffState=0x00000000)  Line 4019 + 0x1f bytes	C++
>	msgbsutl.dll!nsMsgDBFolder::SetBiffState(unsigned int aBiffState=0x00000000)  Line 4015 + 0x20 bytes	C++
 	msgimap.dll!nsImapMailFolder::HeaderFetchCompleted(nsIImapProtocol * aProtocol=0x063865e0)  Line 5141	C++

We could simply always send this notification if we've received new mail, or we could invent a new notification - new mail received, or something. The OS integration-specific code will probably need to change the check about mBiffIconVisible, but we will need to be careful not to put up the alert over and over again quickly (Growl doesn't care as much, but the sliding alert on windows is slow...)

Comment 1

[David Humphrey (:humph)]

I like your second idea.  I'm feeling like in the interest of not breaking the way we do things now on all platforms, I should add a new atom, NewMailReceived.  We would still do BiffState identically, but for the second, third, ..., cases, we'd send a NewMailReceived with an updated count.  When the biff state gets reset, the next notification would be a new BiffState, with subsequent ones being NewMailReceived.  

This way you could choose to do things as you do now, or you could add this to the mix.  This also keeps the traditional and server-side meaning of biff.

David, if you're OK with this, I'll do the patch this way.

Comment 2

[David :Bienvenu]

humph, yes, that sounds good

Comment 3

[David Humphrey (:humph)]

Attachment: [WIP] adding NewMailReceivedAtom

David, this is working, but I have a few questions:

1) Should I send a NewMailReceivedAtom at the same time I do BiffState + NewMail vs. on subsequent ones?  As I have it now (which is only subsequent) it means that you don't have to keep state in the nsMessenger*Integration class (i.e., you can do one thing in BiffState, something else in NewMailReceived without flipping a flag about whether this if biff or not).  It would also mean that you wouldn't need to dig for the first folder with new mail, as we do now, when the root folder reports biff.

2) Should I report for the root or the INBOX (or whatever the folder is)?  I decided to do the root, since that what biff does, and for consistency of folder name, it made more sense.  However, the answer to 1) will influence this.  I could always send the folder itself, and then reach up to get the parent for building the notification string: "AccountName (Foldername) has 1 new mail."

I need to change the int I send (just 0 for now, but should be a count).  I suspect that pushing on this after we fix the imap-biffstate bug is wise.

Other thoughts?

Comment 4

[David Humphrey (:humph)]

David, I've thought about these a bit more, and I'm starting to think that where biff is really for the server/root itself, this atom should be about the folders.

> 1) Should I send a NewMailReceivedAtom at the same time I do BiffState +
> NewMail vs. on subsequent ones?  As I have it now (which is only subsequent) it
> means that you don't have to keep state in the nsMessenger*Integration class
> (i.e., you can do one thing in BiffState, something else in NewMailReceived
> without flipping a flag about whether this if biff or not).  It would also mean
> that you wouldn't need to dig for the first folder with new mail, as we do now,
> when the root folder reports biff.

I'm starting to think that doing this every time vs. the biff+n time is right.  In the os integration we can manage what to do with that info, and combine it with what we know about biff, since I'm not altering how we biff. 

> 2) Should I report for the root or the INBOX (or whatever the folder is)?  I
> decided to do the root, since that what biff does, and for consistency of
> folder name, it made more sense.  However, the answer to 1) will influence
> this.  I could always send the folder itself, and then reach up to get the
> parent for building the notification string: "AccountName (Foldername) has 1
> new mail."

I've been thinking about this more, and I'm now convinced I should report for the folder vs. the root, since I can nsMsgDBFolder::GetRootFolder in order to get/print the root name.

I'll work up a new patch, and test it with my other pending mac os integration patches.

Comment 5

[David Humphrey (:humph)]

Attachment: NewMailReceivedAtom for folder with proper count

After working on this for a bit more, I think our original plan was best, and this patch has NewMailReceived atoms occur after BiffState has gone through (i.e., the second, third etc. mails will trigger it, if biff is still set).

I'm also reporting for the folder itself vs. the root, and letting those that deal with this notification call GetRootFolder if they need to.  I've got a series of patches that use this to fix things in nsMessengerOSXIntegration based on this, so I've tested it as a consumer.

This does not break or change how we do biff in any way, so other consumers won't be affected, and we can add this if it makes sense down the road.

Comment 6

[David :Bienvenu]

Comment on attachment 349704 [details] [diff] [review]
NewMailReceivedAtom for folder with proper count

should we be always sending this notification, even if biff isn't already set? that being said, this looks good.

Comment 7

[David Humphrey (:humph)]

(In reply to comment #6)
> (From update of attachment 349704 [details] [diff] [review])
> should we be always sending this notification, even if biff isn't already set?
> that being said, this looks good.

I forgot that this isn't really a binary state, given the Unknown state.  What about this?  

+  }
+  else if (aBiffState == aOldBiffState && aBiffState == nsMsgBiffState_NewMail)
+  {
+    // biff is already set, but notify that there is additional new mail for the folder
+    NotifyIntPropertyChanged(kNewMailReceivedAtom, 0, mNumNewBiffMessages);

Comment 8

[David :Bienvenu]

yes, that would work.

Comment 9

[David Humphrey (:humph)]

Attachment: r+ patch with extra check on aBiffState == oldBiffState

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 350092 [details] [diff] [review]
r+ patch with extra check on aBiffState == oldBiffState

>@@ -4005,6 +4007,7 @@
>   if (NS_SUCCEEDED(rv) && server)
>     rv = server->GetBiffState(&oldBiffState);
>     // Get the server and notify it and not inbox.
>+
>   if (oldBiffState != aBiffState)
>   {
>     if (!mIsServer)

I think if you really want to put the blank line in, then you should put the comment in the right place which is probably just before the if (!mIsServer).

sr=me with that fixed.

Comment 11

[David Humphrey (:humph)]

Attachment: r+/sr+ patch with sr comment addressed

Thanks for the review, Mark.  I've corrected that line/comment as per your comment.  This is ready to go in.

Comment 12

[Mark Banner (:standard8)]

Patch checked in, please mark fixed if appropriate.

http://hg.mozilla.org/comm-central/rev/315ac410eb2b