809513 - Drafts appear as a new message in New Mail Alert by Biff when a new email arrives at Inbox. "New mail" state of saved draft mail is incorrect, even if draft is saved as Unread.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wesley Hardman]

If you are composing a draft message and the email has been saved to the drafts folder, the draft message will be displayed as a new message when Thunderbird receives an new message from the server.

Tested on the latest Daily (2012-11-06).

STR:
1. Create a new draft.
2. Save the draft.
3. Wait for Thunderbird to download a new message from the server.
4. Notice what the notification displays.

Expected Result:
A notification appears showing the new message that was received.

Actual Result:
A notification appears showing the new message and the draft that was saved.


Other email clients do not show a notification for the draft email.

Comment 1

[Wayne Mery (:wsmwk)]

let's assume this is same as bug 767051

Comment 2

[Wesley Hardman]

Re-opening this as I don't feel it is a duplicate of the bug 767051.

As I stated in bug 767051 comment 11
I specifically only see this when I receive a new message (other than the draft), and EVERY time I see a new message, until there is no longer a new draft message.  The other bug appears to be for receiving a notification for the draft message, when the ONLY email is the draft message.

Comment 3

[WADA:World Anti-bad-Duping Agency]

(A) Is Draft included in periodical new mail check? (Folder Properties/General of Drafts)
(B) Is IDLE command use enabled? (Server Settings, Advanced)
(C) Is max cached connection > 1? (Server Settings, Advanced)

If A=No and B=Yes/C=Yes, phenomenon may be following.
(i) When new draft mail in Draft(no \Seen by bug 673400) is notified via IDLE, it's excluded from new mail alert by A=No.
(ii) However, because "new mail alert"(Biff) is Tb wide feature and is not per folder feature, "new draft mail in Draft(no \Seen by bug 673400) notified via IDLE" is included in new mail alert by Biff which was invoked for other than Drafts.

If so, I think workaround of "Disable IDLE", which is already indicated in some already pointed bugs, is effective.
Have you tried it? 

Note: 
Bug 767051 is perhaps;
Because namespace="INBOX." is used and server forces "All folders under inbox", Drafts is sub folder of Inbox. Check of "SpecialUSE flag of Inbox == On" of Drafts folder is done with "inherit from parent folder, search flag of parent" mode, in order to support "namespace="INBOX." case without additional setting by uer. And, "Inbox" is always folder of "SpecialUSE flag of Inbox == On". So, when automatic new mail check on Inbox is enabled, new mail check(Biff) is invoked on INBOX.Drafts or INBOX/Drafts too. Thus, due to "no \Seen flag by bug 673400, draft mail is always shown in new mail alert.

Comment 4

[WADA:World Anti-bad-Duping Agency]

(In reply to Wesley Hardman from comment #2)
> I specifically only see this when I receive a new message (other than the draft), 
> and EVERY time I see a new message, until there is no longer a new
> draft message.

Thanks for re-opening this bug for clear and pure case.

I could see it, with IDLE is disabled, no new mail check except Inbox, and with auto-sync disabled.
(0) IDLE is disabled, no new mail check except Inbox,
    with auto-sync disabled.
(1) Select IMAP account, go Work Offline, go back to Work Online
    => Nothing is selected at any cached connection.
(2) Save As Draft
    => Drafts is selected at a cached connection.
       At first cached connection : select Drafts, append with no \Seen
       "uid store -Flags(\Deleted)" for old draft
    => "uid fetch yy Flags() body.peek[headers(Subject ...)"
       for new draft mail.
    New mail in Drafts is detected by message header fetch after Save.
(3) Send a mail to this account.
    => New mail is detected by Biff(new mail check) at Inbox
(4) New mail alert is shown by Biff.
    New mail in Drafts folder is shown by "New Mail Alert by Biff".

Confirming.

This is not Drafts only phenomenon.
If new mail is detected at an Mbox by "New mail check by Biff", "Periodical new mail check by auto-sync", or by "via IDLE", status of "Mbox has new mails" is correctly set for the Mbox by Tb.
If "task-tray icon for new mail" is closed by "click of Inbox" when new mail arrives at Inbox, "New mail Alert" is shown by Biff.
Because status of "Mbox has new mail" of Mbox is not cleared until all new mails is viewed or Mbox is re-opened(click other Mbox, then click Mbox again), and because Biff shows "new mail in any Mbox" upon new mail alert, phenomenon you see occurs.

A possible solution of this bug.
  Show new mails of an Mbox in new mail alert of Biff,
  only when "new mail check by Biff" is enabled for the Mbox.
However, "new mail detected via IDLE" is also a valid "New mail".

As for "auto-saved draft mail with no \Seen" case, using add-on like following is better.
- Different draft folder for auto-save
  See Bug 562748 Comment #24.
By the add-on, user can use any folder of "Local Folders" as "Drafts for auto-save". So, new mail alert shows "new draft mail in IMAP Drafts" only when you intensionally saved draft mail to IMAP Drafts in order to share draft mail with other IMAP client.
Main purpose of auto-save is protection from problem like "crash whle mail composition". Such add-on is sufficient for such purpose.

Comment 5

[WADA:World Anti-bad-Duping Agency]

FYI.
"new mail" in this bug :
      mail of larger UID than HighestUID which Tb knows,
  and mail of no \Seen flag, no \Deleted flag.

Comment 6

[WADA:World Anti-bad-Duping Agency]

As for "newly saved draft mail by Tb himself", as seen in STR of comment #4, "New Mail" in Drafts is never detected by "new mail check by Biff/auto-sync" nor "via IDLE". "New Mail" is detected by "message header fetch for newly saved draft mail" which is invoked by Tb after draft save.
So, if "New Mail" state is not set for "newly saved draft mail", this bug on draft mail won't occur.

If UIDPLUS is supported, UID for newly appended draft mail is returned from server to IMAP "APPEND" command. So, if UIDPLUS is supported, "prohibiting Mew mail state of newly save draft" is possible and is not so hard. However, if UIDPLUS is not supported, "SEARCH for messageg-ID, Subject etc." is needed to know UID of the newly saved draft mail. It'll make program code complex.

IIRC, "Copy of Unread mail from other account" won't produce "New mail state" of the copied mail. What is difference between "draft save with no \Seen" and "copy Unread mail from other account"?

Comment 7

[WADA:World Anti-bad-Duping Agency]

To bug opener:

When a "New Mail" in an Mbox is generated by server(newly arrived mail to Inbox) or by other IMAP client(append/upload/copy/move of Unread mail to an Mbox), the "New Mail" is detected by Thunderbird by "Folder open", "New mail check by Biff", "New mail check by auto-ssync", and "via IDLE".

When the "New Mail" should be shown in new mail alert?
When the "New Mail" shouldn't be shown in new mail alert?
  - mail in folder used as Draft folder
  - mail which has \Draft flag
  - mail placed in Mbox for which new mail check is not requested
  - other than above

Comment 9

[Mic]

I tried to fix this problem with this addon (waiting for a review at the moment): https://addons.mozilla.org/en-US/thunderbird/addon/imap-draft-unread/

It works for me, so now the drafts are unread but doesn't trigger a mail alert.
There is an option also to set the drafts as read.

Comment 10

[TT]

This problem occurs with Seamonkey on Linux as well.

Comment 12

[gene smith]

Attachment: drafts-no-notifiy.diff

Here's what seems like a quick fix for this (WIP). Skips over folders that have the "Virtual" and now "Drafts" flag set. Not sure, but maybe all "SpecialUse" flagged folders except INBOX (i.e., junk, sent, trash and templates and other?) should not notify when new mail is present. Anyhow, with this change, there is now no notification for new/unseen items in Drafts when new mail arrives in INBOX.

Comment 13

[gene smith]

Attachment: drafts-specialuse-no-notifiy.diff

This is another way that skips most of the special-use mailboxes instead of skipping explicitly Drafts. Note that it does not skip Inbox (where new messages will most typically be) since Inbox is also a member of the special-use mailbox set.

So any new/unseen mail in the special use folders, except for Inbox, will not produce a pop-up notification.

These are the special-use mailboxes, from  comm-central/mailnews/base/public/nsMsgFolderFlags.idl:

/// Special-use folders
const nsMsgFolderFlagType SpecialUse = Inbox|Drafts|Trash|SentMail|
                                       Templates|Junk|Archive|Queue;

Note also that the actual folder names for most of these can be assigned other names than the generic names show, i.e., customized or localized, and this patch will still work.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8956688 [details] [diff] [review]
drafts-no-notifiy.diff

Thanks for the research, the other patch is more comprehensive.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8956710 [details] [diff] [review]
drafts-specialuse-no-notifiy.diff

Thanks again for looking at this. This seems reasonable. However, I'm no expert here, so let's get some other feedback.

Comment 16

[Jorg K (CEST = GMT+2)]

Aceman, can we please move this bug forward.

Comment 17

[:aceman]

Comment on attachment 8956710 [details] [diff] [review]
drafts-specialuse-no-notifiy.diff

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

Thanks, this looks interesting.
Checking for Inbox first seems to also cover cases where the same folder is used for Inbox and Sent (part of SpecialUse) simultaneously.

What about other non-Inbox folders? If user created some more folders and uses one of them as Sent, can messages be delivered into those folders? And may the user expect to be notified about those? I am not familiar with IMAP here.

::: mailnews/base/content/newmailalert.js
@@ +57,5 @@
>    for (let folder of fixIterator(allFolders, Components.interfaces.nsIMsgFolder))
>    {
> +    if (folder.hasNewMessages && (folder.getFlag(Ci.nsMsgFolderFlags.Inbox) ||
> +          !(folder.getFlag(Ci.nsMsgFolderFlags.Virtual) ||
> +             folder.getFlag(Ci.nsMsgFolderFlags.SpecialUse))))

As SpecialUse is already a composition of multiple flags, maybe this could be written as folder.getFlag(Ci.nsMsgFolderFlags.Inbox) || !folder.getFlag(Ci.nsMsgFolderFlags.SpecialUse | Ci.nsMsgFolderFlags.Virtual)

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 8956710 [details] [diff] [review]
drafts-specialuse-no-notifiy.diff

(In reply to :aceman from comment #17)
> ...
> What about other non-Inbox folders? If user created some more folders and
> uses one of them as Sent, can messages be delivered into those folders? And
> may the user expect to be notified about those? I am not familiar with IMAP
> here.

I think yes. But that would be more of an edge case than Inbox being set as sent.  And I can offer that I have sent -> Inbox for almost all my accounts.

Clearing feedback, as I don't really have an opinion on the code itself

Comment 19

[Wayne Mery (:wsmwk)]

Note there a quite a few other biff bug reports, perhaps most notable is bug 479282.  https://mzl.la/2J3Zo6X is a partial list

Comment 20

[:aceman]

Gene, can you try finishing the patch?

Comment 21

[gene smith]

For some reason I can't seem to duplicate this now. I have tried several test accounts and started writing and then saved the message to Drafts without accessing it so it remains unread in Drafts. Then I send a message to the account (from another machine) and the pop-up only contains the info for the new message in Inbox and nothing about the unread message in Drafts.

I'm pretty sure I duplicated this before! Otherwise, I wouldn't have said the patch seemed to fix the problem.

I also double/triple checked and my proposed patch is definitely not applied (newmailalert.js is unchanged).

(The only issue I see is that sometimes I don't see the pop-up notification at all when the message comes in; I just hear a sound.)

Comment 22

[gene smith]

(In reply to gene smith from comment #21)
> For some reason I can't seem to duplicate this now. 

I can only duplicate this on windows. However bug 1442816 indicates it has the problem on linux and windows.

But bug 661239 says it was fixed, but probably just on linux/unix. This may explain why I don't see the problem on linux, but don't know why others do report the problem.

(In reply to :aceman from comment #17)
> 
> What about other non-Inbox folders? If user created some more folders and
> uses one of them as Sent, can messages be delivered into those folders?

You can create a new folder, AAA, and assigned AAA as the Sent destination. I just tested it and sent mail goes to AAA. (I assume that's what is meant by "delivered into those folders.)

> And may the user expect to be notified about those? I am not familiar with IMAP
> here.

There should be no notification. Sent mail would be placed into AAA with the \Seen flag set so it would not look like new mail. 

> 
> As SpecialUse is already a composition of multiple flags, maybe this could
> be written as folder.getFlag(Ci.nsMsgFolderFlags.Inbox) ||
> !folder.getFlag(Ci.nsMsgFolderFlags.SpecialUse | Ci.nsMsgFolderFlags.Virtual)

Seem reasonable.

(In reply to Wayne Mery (:wsmwk) from comment #18)
>
> And I can offer that I have sent -> Inbox for almost all my accounts.

Also, not a problem. Sent mail stored into Inbox will also have the \Seen flag set so won't trigger a notification.

If I understand the previous bugs and changes, at one time messages save into Drafts were not flagged as \Seen and that was deemed a bug since they were not highlighted in bold. Now that \Seen is set on new messages saved to Drafts, it (sometimes?) causes them to be detected as new and triggers a notification.

Comment 23

[:aceman]

(In reply to gene smith from comment #22)
> (In reply to :aceman from comment #17)
> > 
> > What about other non-Inbox folders? If user created some more folders and
> > uses one of them as Sent, can messages be delivered into those folders?
> 
> You can create a new folder, AAA, and assigned AAA as the Sent destination.
> I just tested it and sent mail goes to AAA. (I assume that's what is meant
> by "delivered into those folders.)

I meant that also incoming mail could be delivered into AAA in addition to sent, and then we would get NO notification for all of them (not even the incoming ones)? That may be irritating again.

Comment 24

[gene smith]

Well, apparently I never tested the original patch since I now don't see any of the javascript code actually running based on some inserted dump() calls. Also put in some intentional bad syntax and nothing happened. (I did do a build to install the symlink to the file in comm-central/obj-x86_64-pc-linux-gnu. Is there some else I need to do when changing a .js file?)

Comment 25

[gene smith]

(In reply to :aceman from comment #23)
> (In reply to gene smith from comment #22)
> > (In reply to :aceman from comment #17)
> > > 
> > > What about other non-Inbox folders? If user created some more folders and
> > > uses one of them as Sent, can messages be delivered into those folders?
> > 
> > You can create a new folder, AAA, and assigned AAA as the Sent destination.
> > I just tested it and sent mail goes to AAA. (I assume that's what is meant
> > by "delivered into those folders.)
> 
> I meant that also incoming mail could be delivered into AAA in addition to
> sent, and then we would get NO notification for all of them (not even the
> incoming ones)? That may be irritating again.

That may be what would happen. I would have to test it to be sure. But, at least for me on linux, my proposed code is never reached, so this may be moot. Any idea why I don't see the the code running?

Comment 26

[gene smith]

Bug 687140 has a patch that fixes this for windows. But it was never accepted due to failing unit test, I think. So probably what I proposed in patch above is totally wrong and the patch from Bug 687140 should be completed. There may also be an adaptation for OSX too that may be needed. Then again, there was an effort to unify the new mail notification so OS agnostic but was never completed AFAICT: Bug 715799.

Comment 27

[gene smith]

I set up a tb build environment on windows and tried the patch from Bug 687140 and it didn't work. In fact the code was never reached. However, with windows the code patch above *is* reached and fixes the problem. 

Regarding the case where there is an alternate Sent folder with sent messages stored there and also new mail delivered to it, with the patch as is the new mail will not produce a notification. A fix for this might be to treat mailbox with flag SentMail the same as the mailbox with flag Inbox:

diff -r 763cca5220a9 mailnews/base/content/newmailalert.js
--- a/mailnews/base/content/newmailalert.js	Sat Aug 04 12:08:11 2018 +0200
+++ b/mailnews/base/content/newmailalert.js	Sun Aug 05 03:12:01 2018 -0400
@@ -51,18 +51,23 @@
 
   // This is really the root folder and we have to walk through the list to
   // find the real folder that has new mail in it...:(
+  dump("gds: are we here?\n");
   let allFolders = rootFolder.descendants;
   var folderSummaryInfoEl = document.getElementById('folderSummaryInfo');
   folderSummaryInfoEl.mMaxMsgHdrsInPopup = gNumNewMsgsToShowInAlert;
   for (let folder of fixIterator(allFolders, Ci.nsIMsgFolder))
   {
-    if (folder.hasNewMessages && !folder.getFlag(Ci.nsMsgFolderFlags.Virtual))
+    if (folder.hasNewMessages && (folder.getFlag(Ci.nsMsgFolderFlags.Inbox | Ci.nsMsgFolderFlags.SentMail) ||
+        !(folder.getFlag(Ci.nsMsgFolderFlags.Virtual | Ci.nsMsgFolderFlags.SpecialUse))))
     {
       var asyncFetch = {};
       folderSummaryInfoEl.parseFolder(folder, new urlListener(folder), asyncFetch);
       if (asyncFetch.value)
         gPendingPreviewFetchRequests++;
+      dump("gds: notify new message\n");
     }
+    else
+      dump("gds: don't notify new message\n");
   }
 }

Comment 28

[gene smith]

Attachment: 809513-review-changes0.patch

I think this solves the problem where an active unread draft triggers an unwanted notification. I personally only see it on windows and have been unable to cause it on linux. There appears to already exist code for linux/unix that prevents this in the unix integration

The fix I added is to a js file that I only see running on windows. However, to ensure that the issue identified by aceman (folder marked as SentMail unable to notify new mail) I have made a similar change to the unix integration c++ file.

I have tested this on windows and linux. I can't test this on apple/OSX since I don't have a TB build environment on my very limited and old macbook air.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8998077 [details] [diff] [review]
809513-review-changes0.patch

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

Hmm, the existing code isn't really good, is it? Now it looks much better.

I don't know who would place sent messages into the inbox, but I guess that's an existing configuration option (like "place sent e-mail into the original folder or some such").

::: mailnews/base/content/newmailalert.js
@@ -55,5 @@
>    var folderSummaryInfoEl = document.getElementById('folderSummaryInfo');
>    folderSummaryInfoEl.mMaxMsgHdrsInPopup = gNumNewMsgsToShowInAlert;
>    for (let folder of fixIterator(allFolders, Ci.nsIMsgFolder))
>    {
> -    if (folder.hasNewMessages && !folder.getFlag(Ci.nsMsgFolderFlags.Virtual))

This does NOT match the C++ code for Linux.

@@ +61,5 @@
> +    // includes Inbox and SentMail as well as Drafts, Trash, Junk, Archive, Templates
> +    // and Queue.
> +    if (folder.hasNewMessages &&
> +        !folder.getFlag((Ci.nsMsgFolderFlags.SpecialUse | Ci.nsMsgFolderFlags.Virtual) &
> +                       ~(Ci.nsMsgFolderFlags.Inbox | Ci.nsMsgFolderFlags.SentMail)))

This now matches the C++ code for Linux.

::: mailnews/base/src/nsMessengerUnixIntegration.cpp
@@ -610,5 @@
>  
> -      // Unless we're dealing with an Inbox, we don't care
> -      // about Drafts, Queue, SentMail, Template, or Junk folders
> -      if (!(flags & nsMsgFolderFlags::Inbox) &&
> -           (flags & (nsMsgFolderFlags::SpecialUse & ~nsMsgFolderFlags::Inbox)))

I've been staring at this for while and it doesn't make sense. It says:
Skip if not inbox and special folder other than inbox.
The first part is redundant, no?

@@ +612,5 @@
> +      // or SentMail. Note: SpecialUse includes Inbox and SentMail as well as
> +      // Drafts, Trash, Junk, Archive Templates and Queue. A notification of new
> +      // mail will not be produced for folders that are skipped.
> +      if (flags & ((nsMsgFolderFlags::SpecialUse | nsMsgFolderFlags::Virtual) &
> +          ~(nsMsgFolderFlags::Inbox | nsMsgFolderFlags::SentMail)))

This says:
Skip special use/virtual folder other than inbox and sent, right? I can understand that. Basically the second part of the previous condition enhanced by 'virtual' and 'sent'.

Comment 30

[Wayne Mery (:wsmwk)]

Thanks for the patch. I know many users "depend" on notifications (like it is one of their "must haves", similar to coffee), it enabled by default, but sadly we have several old bugs in this area.


(In reply to Jorg K (GMT+2) from comment #29)
> ...
> I don't know who would place sent messages into the inbox, but I guess
> that's an existing configuration option (like "place sent e-mail into the
> original folder or some such").

Definitely non-standard.  However, I am one such datapoint - because I want a reasonably complete thread of messages, but I don't buy into a) gmail-like conversation view b) the behavior of deleting an entire thread when collapsed that comes with that (default) preference

Comment 32

[:aceman]

(In reply to Wayne Mery (:wsmwk) from comment #30)
> Definitely non-standard.  However, I am one such datapoint - because I want
> a reasonably complete thread of messages, but I don't buy into a) gmail-like
> conversation view b) the behavior of deleting an entire thread when
> collapsed that comes with that (default) preference

But do you have set to store copies of sent mail in the Inbox of that acocunt in account settings (copies & folders)? Or do you have it set to 'Sent'? But do you have the setting "store replies in the folder of the original message" set?
That would be a difference now.

Comment 33

[Wayne Mery (:wsmwk)]

The first.  copies & folders > Place copy > Other > "Inbox on" <account>
I ALSO have set "Place replies in the folder of the message being replied to".

Comment 34

[:aceman]

And I have "Place replies in the folder of the message being replied to" true, but otherwise save copy of Sent into "Sent messages"

So your Inbox will have the Ci.nsMsgFolderFlags.SentMail flag set, while mine won't.

Comment 35

[:aceman]

Comment on attachment 8998077 [details] [diff] [review]
809513-review-changes0.patch

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

But if I read the patch correctly, there will be notification even if the folder is destination for Sent mail (whether it is Inbox or normal folder).
We only have a problem if a folder is pure Sent mail and no messages arrive into it, there will be an unneeded notification for messages we just sent. But we probably can't distinguish this case.

Comment 36

[Jorg K (CEST = GMT+2)]

Please address my comments in comment #29.

Comment 37

[gene smith]

(In reply to Jorg K (GMT+1) from comment #36)
> Please address my comments in comment #29.

(In reply to Jorg K (GMT+1) from comment #29)
> Comment on attachment 8998077 [details] [diff] [review]
> 809513-review-changes0.patch
> 
> Review of attachment 8998077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, the existing code isn't really good, is it? Now it looks much better.
> 
> I don't know who would place sent messages into the inbox, but I guess
> that's an existing configuration option (like "place sent e-mail into the
> original folder or some such").
> 
> ::: mailnews/base/content/newmailalert.js
> @@ -55,5 @@
> >    var folderSummaryInfoEl = document.getElementById('folderSummaryInfo');
> >    folderSummaryInfoEl.mMaxMsgHdrsInPopup = gNumNewMsgsToShowInAlert;
> >    for (let folder of fixIterator(allFolders, Ci.nsIMsgFolder))
> >    {
> > -    if (folder.hasNewMessages && !folder.getFlag(Ci.nsMsgFolderFlags.Virtual))
> 
> This does NOT match the C++ code for Linux.

Right, doesn't match the original Unix Integration code. Since it only excludes virtual folders from new mail notification, it causes new mail in Drafts, Templates etc. to be notified.

> 
> @@ +61,5 @@
> > +    // includes Inbox and SentMail as well as Drafts, Trash, Junk, Archive, Templates
> > +    // and Queue.
> > +    if (folder.hasNewMessages &&
> > +        !folder.getFlag((Ci.nsMsgFolderFlags.SpecialUse | Ci.nsMsgFolderFlags.Virtual) &
> > +                       ~(Ci.nsMsgFolderFlags.Inbox | Ci.nsMsgFolderFlags.SentMail)))
> 
> This now matches the C++ code for Linux.

Yes, they now have the same effect, but in the opposite way (.js enables check, .cpp skips it).

> 
> ::: mailnews/base/src/nsMessengerUnixIntegration.cpp
> @@ -610,5 @@
> >  
> > -      // Unless we're dealing with an Inbox, we don't care
> > -      // about Drafts, Queue, SentMail, Template, or Junk folders
> > -      if (!(flags & nsMsgFolderFlags::Inbox) &&
> > -           (flags & (nsMsgFolderFlags::SpecialUse & ~nsMsgFolderFlags::Inbox)))
> 
> I've been staring at this for while and it doesn't make sense. It says:
> Skip if not inbox and special folder other than inbox.
> The first part is redundant, no?

I agree, that doesn't make sense in the original code. But it will skip check for Drafts, Templates, etc. So this bug doesn't seem to apply to Linux.


> 
> @@ +612,5 @@
> > +      // or SentMail. Note: SpecialUse includes Inbox and SentMail as well as
> > +      // Drafts, Trash, Junk, Archive Templates and Queue. A notification of new
> > +      // mail will not be produced for folders that are skipped.
> > +      if (flags & ((nsMsgFolderFlags::SpecialUse | nsMsgFolderFlags::Virtual) &
> > +          ~(nsMsgFolderFlags::Inbox | nsMsgFolderFlags::SentMail)))
> 
> This says:
> Skip special use/virtual folder other than inbox and sent, right? I can
> understand that. Basically the second part of the previous condition
> enhanced by 'virtual' and 'sent'.

Yes, this make both windows and unix act the same and incorporates aceman's concern. Not sure how this affects OSX "unix".

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #36)
> Please address my comments in comment #29.
Thanks Gene, my mistake, I've just noticed that the negative comments related to the code that was removed and there was nothing to address. I'll get this landed.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6d2e32bd9729
Prevent unread Drafts from showing in new mail notification. r=aceman

Comment 40

[Jorg K (CEST = GMT+2)]

Comment on attachment 8998077 [details] [diff] [review]
809513-review-changes0.patch

I think we should consider this for ESR uplift since seeing drafts in the notification is rather annoying.

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: 809513-follow-up2.patch

OK, this caused a test failure in test-notification.js which ensures that Sent is not notified. I discussed with Aceman on IRC and I'm taking Sent out again.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9063f5e11b9e
Don't notify for new mail in Sent folder. r=me

Comment 43

[Jorg K (CEST = GMT+2)]

Aceman, the test is still failing:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_no_notification_for_uninteresting_folders
Can you fix this. Or Gene, you might try it. Run this from your object directory:
./make mozmill-one SOLO_TEST=notification/test-notification.js

Comment 44

[Jorg K (CEST = GMT+2)]

The failure before with the first patch was:
https://taskcluster-artifacts.net/AW_mkDleT32Wew-Y0j8lOQ/0/public/logs/live_backing.log
EXCEPTION: Showed alert notification.
So we most likely showed an alert for a Sent box.

Now
https://taskcluster-artifacts.net/aMBHyuUXTLS5bea5PquzcA/0/public/logs/live_backing.log
the error is:
EXCEPTION: Did not show alert notification.
So we most likely skipped a notification on a folder that was both Inbox and Sent.

The original code was:
-      if (!(flags & nsMsgFolderFlags::Inbox) &&
-           (flags & (nsMsgFolderFlags::SpecialUse & ~nsMsgFolderFlags::Inbox)))
         continue;
This was basically correct, it notified for inbox and any other folder with new messages, unless this folder was special but excluding inbox.

I think it's more easy to read like this:
         // Any folder which is an inbox ...
notify = flags & nsMsgFolderFlags::Inbox ||
         // any non-special or non-virtual folder.
         !(flags & (nsMsgFolderFlags::SpecialUse | nsMsgFolderFlags::Virtual))

Do we agree on this? I think I'll back it all out and we start again :-(

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/95ff25a433f1
Backed out 2 changesets to make way for a correct solution. a=backout DONTBUILD

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 809513-take-3.patch

I think one bug in the original patch was the
folder.getFlag((Ci.nsMsgFolderFlags.SpecialUse | Ci.nsMsgFolderFlags.Virtual) & ...)
since that function only takes a single flag, right, see:
https://hg.mozilla.org/comm-central/rev/86c5e6686ff3

Comment 47

[gene smith]

I thought I was following aceman's example in comment 17. No compile error (but guess .js doesn't compile?).

Comment 48

[:aceman]

It does take multiple flags, but you must get the bit expression right. So it may be we messed it up. The ~ operator may cause problems from JS. That is why I don't like passing multiple flags into getFlag(). YEah, int comment 17 I suggested folding multiple flags into the call, but at least there was no ~ :)

Does the new version pass the test?

Comment 49

[Jorg K (CEST = GMT+2)]

The reviewer can test that ;-)

Comment 50

[Jorg K (CEST = GMT+2)]

Attachment: 809513-take-3.patch

Removed trailing spaces to avoid complaints.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e7da6e08d6e6dc3094f1683651723d7bf69ad7a6

Comment 51

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #48)
> Does the new version pass the test?
Yes.

Comment 52

[Jorg K (CEST = GMT+2)]

Note, the original Linux code

-      if (!(flags & nsMsgFolderFlags::Inbox) &&
-           (flags & (nsMsgFolderFlags::SpecialUse & ~nsMsgFolderFlags::Inbox)))

was actually correct.

I've just formed the negation, added the virtual folders and removed the unnecessary ~nsMsgFolderFlags::Inbox since that is already covered in the first part of the clause, hence:

+      bool notify =
+        // Any folder which is an inbox or ...
+        flags & nsMsgFolderFlags::Inbox ||
+        // any non-special or non-virtual folder. In other words, we don't
+        // notify for Drafts|Trash|SentMail|Templates|Junk|Archive|Queue or virtual.
+        !(flags & (nsMsgFolderFlags::SpecialUse | nsMsgFolderFlags::Virtual));

Comment 53

[:aceman]

Comment on attachment 9028895 [details] [diff] [review]
809513-take-3.patch

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

Thanks, this seems to work.

::: mailnews/base/content/newmailalert.js
@@ +61,5 @@
> +        // Any folder which is an inbox or ...
> +        folder.getFlag(Ci.nsMsgFolderFlags.Inbox) ||
> +        // any non-special or non-virtual folder. In other words, we don't
> +        // notify for Drafts|Trash|SentMail|Templates|Junk|Archive|Queue or virtual.
> +        !(folder.flags & (Ci.nsMsgFolderFlags.SpecialUse | Ci.nsMsgFolderFlags.Virtual));

Can the comments be placed differently so they do not split the expression in this way? The same in the .cpp file.

Comment 54

[Jorg K (CEST = GMT+2)]

That would defy the purpose of explaining the code in detail.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fa17a20f5491
Only notify for new mail in Inbox and non-special/virtual folders. r=aceman

Comment 56

[Jorg K (CEST = GMT+2)]

Comment on attachment 9028895 [details] [diff] [review]
809513-take-3.patch

Missed TB 64 beta 4.

Comment 57

[Jorg K (CEST = GMT+2)]

TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/bf0d776d3044be78117b7d198e44299ed954de43