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.

RESOLVED FIXED in Thunderbird 65.0

Status

defect
RESOLVED FIXED
7 years ago
6 months ago

People

(Reporter: bugzilla, Assigned: jorgk)

Tracking

(Blocks 3 bugs)

Thunderbird 65.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr6064+ fixed, thunderbird65 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

7 years ago
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.
Blocks: tb-drafts
let's assume this is same as bug 767051
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 767051
Reporter

Comment 2

7 years ago
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.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---

Updated

6 years ago
Depends on: 767051
(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.
(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.
Status: UNCONFIRMED → NEW
Component: General → Backend
Ever confirmed: true
Product: Thunderbird → MailNews Core
Version: Trunk → 17
Blocks: 767051
No longer depends on: 767051
FYI.
"new mail" in this bug :
      mail of larger UID than HighestUID which Tb knows,
  and mail of no \Seen flag, no \Deleted flag.
Summary: Drafts appear as a new message when a new email arrives → Drafts appear as a new message in New Mail Alert by Biff when a new email arrives at Inbox
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"?
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
Summary: Drafts appear as a new message in New Mail Alert by Biff when a new email arrives at Inbox → 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.

Updated

6 years ago
Duplicate of this bug: 901929

Comment 9

5 years ago
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

4 years ago
This problem occurs with Seamonkey on Linux as well.
Assignee

Updated

Last year
Duplicate of this bug: 1442816

Comment 12

Last year
Posted patch drafts-no-notifiy.diff (obsolete) — Splinter Review
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.
Attachment #8956688 - Flags: feedback?(jorgk)

Comment 13

Last year
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.
Attachment #8956710 - Flags: feedback?(jorgk)
Assignee

Comment 14

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

Thanks for the research, the other patch is more comprehensive.
Attachment #8956688 - Flags: feedback?(jorgk)
Assignee

Comment 15

Last year
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.
Attachment #8956710 - Flags: feedback?(jorgk)
Attachment #8956710 - Flags: feedback?(acelists)
Attachment #8956710 - Flags: feedback+
Assignee

Comment 16

Last year
Aceman, can we please move this bug forward.
Flags: needinfo?(acelists)
Assignee

Updated

Last year
Attachment #8956710 - Flags: feedback?(acelists) → review?(acelists)

Comment 17

11 months ago
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)
Attachment #8956710 - Flags: feedback?(vseerror)

Comment 18

11 months ago
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
Flags: needinfo?(acelists)
Attachment #8956710 - Flags: feedback?(vseerror)

Comment 19

11 months ago
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

11 months ago
Gene, can you try finishing the patch?
Assignee: nobody → gds
Status: NEW → ASSIGNED
Flags: needinfo?(gds)
OS: Windows 7 → All
Hardware: x86 → All

Comment 21

11 months ago
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.)
Flags: needinfo?(gds)

Comment 22

11 months ago
(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

11 months ago
(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

11 months ago
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

11 months ago
(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

11 months ago
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

11 months ago
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

10 months ago
Posted patch 809513-review-changes0.patch (obsolete) — Splinter Review
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.
Attachment #8956688 - Attachment is obsolete: true
Attachment #8956710 - Attachment is obsolete: true
Attachment #8956710 - Flags: review?(acelists)
Attachment #8998077 - Flags: review?(acelists)
Attachment #8998077 - Flags: feedback?(jorgk)
Assignee

Comment 29

10 months ago
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'.
Attachment #8998077 - Flags: feedback?(jorgk) → feedback+

Comment 30

10 months ago
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
Assignee

Updated

8 months ago
Duplicate of this bug: 1498341

Comment 32

7 months ago
(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.

Updated

7 months ago
Flags: needinfo?(vseerror)
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".
Flags: needinfo?(vseerror)

Comment 34

7 months ago
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

7 months ago
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.
Attachment #8998077 - Flags: review?(acelists) → review+
Assignee

Comment 36

7 months ago
Please address my comments in comment #29.

Comment 37

7 months ago
(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".
Assignee

Comment 38

7 months ago
(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.
Keywords: checkin-needed

Comment 39

7 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6d2e32bd9729
Prevent unread Drafts from showing in new mail notification. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Assignee

Comment 40

7 months ago
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.
Attachment #8998077 - Flags: approval-comm-esr60?
Assignee

Comment 41

7 months ago
Posted patch 809513-follow-up2.patch (obsolete) — Splinter Review
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

7 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9063f5e11b9e
Don't notify for new mail in Sent folder. r=me
Assignee

Comment 43

7 months ago
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
Flags: needinfo?(gds)
Flags: needinfo?(acelists)
Assignee

Comment 44

7 months ago
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

7 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/95ff25a433f1
Backed out 2 changesets to make way for a correct solution. a=backout DONTBUILD
Assignee

Comment 46

7 months ago
Posted patch 809513-take-3.patch (obsolete) — Splinter Review
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
Assignee: gds → jorgk
Attachment #8998077 - Attachment is obsolete: true
Attachment #9028815 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Attachment #8998077 - Flags: approval-comm-esr60?
Flags: needinfo?(gds)
Flags: needinfo?(acelists)
Attachment #9028846 - Flags: review?(acelists)
Resolution: FIXED → ---

Comment 47

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

Comment 48

7 months ago
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?
Assignee

Comment 49

7 months ago
The reviewer can test that ;-)
Assignee

Comment 50

7 months ago
Removed trailing spaces to avoid complaints.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e7da6e08d6e6dc3094f1683651723d7bf69ad7a6
Attachment #9028846 - Attachment is obsolete: true
Attachment #9028846 - Flags: review?(acelists)
Attachment #9028895 - Flags: review?(acelists)
Assignee

Comment 51

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

Comment 52

7 months ago
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

7 months ago
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.
Attachment #9028895 - Flags: review?(acelists) → review+
Assignee

Comment 54

7 months ago
That would defy the purpose of explaining the code in detail.

Comment 55

7 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fa17a20f5491
Only notify for new mail in Inbox and non-special/virtual folders. r=aceman
Status: REOPENED → RESOLVED
Closed: 7 months ago7 months ago
Resolution: --- → FIXED
Assignee

Updated

7 months ago
Attachment #9028895 - Flags: approval-comm-esr60+
Attachment #9028895 - Flags: approval-comm-beta+
Assignee

Updated

6 months ago
Target Milestone: --- → Thunderbird 65.0
Assignee

Comment 56

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

Missed TB 64 beta 4.
Attachment #9028895 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.