1211261 - Activity Manager should report if downloaded POP messages were moved by "before junk" message filter

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

When you run filters manually on a folder, the activity manager reports what was done. I would like automatic moving by filters upon receipt of messages also to report the actions taken in the activity manager.

Background:
I receive a lot of e-mail. A good part of this e-mail is from mailing lists, so I have filters that move the messages to the right folder upon receipt. Given that I have a few folders with unread mail in them, I cannot find out where new mail has been moved to. For example: The activity manager shows me "2 messages downloaded", but I can't find them. One got moved to my BMO folder, but I have no idea where the other one went.

Comment 1

[Jorg K (CEST = GMT+2)]

Looks like this is what I need to look for:
http://mxr.mozilla.org/comm-central/ident?i=nsActEvent

Here we create the A/M entry "xx messages downloaded":
http://mxr.mozilla.org/comm-central/source/mail/components/activity/modules/pop3Download.js#107
And here we create the "Moved xx messages from yy to zz":
http://mxr.mozilla.org/comm-central/source/mail/components/activity/modules/moveCopy.js#165
(more A/M entries in the same file at lines 93, 210, 250, 289).

Now looking for the spot where the filters move messages.

Comment 2

[Jorg K (CEST = GMT+2)]

The feature I am requesting already works for IMAP. The A/M says:
---
<account> is up to date.
Total number of messages downloaded: 1
---
Moved 1 message from Inbox to <target>
<account>
---

Stack trace:
nsMsgFolderNotificationService::NotifyMsgsMoveCopyCompleted(bool aMove, ...) Line 126	C++
nsImapMailFolder::OnStopRunningUrl(nsIURI * aUrl, nsresult aExitCode) Line 5230	C++
nsMsgMailNewsUrl::SetUrlState(bool aRunningUrl, nsresult aExitCode) Line 97	C++
nsImapMailFolder::SetUrlState(nsIImapProtocol * aProtocol, ...) Line 6874	C++
`anonymous namespace'::SyncRunnable5<nsIImapMailFolderSink,... >::Run() Line 251 C++

So I guess it's a bug that it doesn't work for POP.

Comment 3

[Jorg K (CEST = GMT+2)]

Neil, can you please point me to the code that does the POP downloading, the download notify, the filtering and that sadly doesn't notify the result of the filtering.

Comment 4

[Jorg K (CEST = GMT+2)]

(Oops, I had two debuggers running and the breakpoints in the one that wasn't attached).
I tracked some stuff down:

nsMsgDBFolder::CallFilterPlugins() - In there it seemed to have processed one filter.
Or is this function only about SPAM filtering?
When I get to
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2644
if (!filterForOther && !filterForJunk && !filterPostPlugin)
it just returns.

It's weird: "Normal" filters and the ones run for IMAP report to the A/M, filters run at POP download time don't.

Comment 5

[Jorg K (CEST = GMT+2)]

This is getting more interesting:
If the message filter is set to run *after* junk classification, the move is reported in the A/M.

Only of the message filter is set to run *before* junk classification, it is not reported.

"Before" filters are skipped here:
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2631

"After" filters seem to be processed here:
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2793
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2316
which calls ApplyFilters() and also does some Notifying.

Still looking for the "Before" filter processing and why it doesn't notify.

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, I put a breakpoint on 
http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgFilterService.cpp#1096
nsMsgFilterService::ApplyFilters()
That only breaks on the "after" filters.

Neil, where are the "before" filters processed, so the ones that run before junk classification?

Comment 7

[Jorg K (CEST = GMT+2)]

When a message is processed by an "after" rule, it is first received into the Inbox, and then moved to the target folder.
When a message is processed by a "before" rule, it is received immediately into the target folder. So no real move takes place and therefore nothing is reported.

So maybe the aim should be to report
---
1 message downloaded to Inbox
--- or ---
1 message downloaded to <folder>
---

Comment 8

[Jorg K (CEST = GMT+2)]

Neil, when notifying the download complete here:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Sink.cpp#367
do we know into which folder the message got put?

I modified
http://mxr.mozilla.org/comm-central/source/mail/components/activity/modules/pop3Download.js#107
to dump out the folder name
  new nsActEvent(displayText + " (" + aFolder.URI + ")",
but I only see Inbox there.

Changing that is obviously a problem if multiple messages got received and moved into different folders.

What's a good way to approach this issue?

Comment 9

[[e-mail]]

(In reply to Jorg K from comment #6)
> Neil, where are the "before" filters processed, so the ones that run before
> junk classification?

In nsParseMailbox.cpp, specifically I think nsParseNewMailState::MoveIncorporatedMessage is probably where you want to be adding your activity.

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks! Or maybe one up in the callstack:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2069

nsParseNewMailState::ApplyFilterHit(nsIMsgFilter * filter, ...) Line 2069	C++
nsMsgFilterList::ApplyFiltersToHdr(int filterType, ...) Line 318	C++
nsParseNewMailState::ApplyFilters(bool * pMoved, ...) Line 1947	C++
nsParseNewMailState::PublishMsgHeader(nsIMsgWindow * msgWindow) Line 1870	C++
nsPop3Sink::IncorporateComplete(nsIMsgWindow * aMsgWindow, int aSize) Line 826	C++

I'll look for a good spot.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1).

This works. It also works when two messages are downloaded and moved to the same folder.

I'm not sure about the array stuff, I copied that from here:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalUndoTxn.cpp#243

In the (unlikely) event that it's OK like this, you could put an r+ instead of an f+.

Comment 12

[[e-mail]]

Comment on attachment 8669733 [details] [diff] [review]
Proposed solution (v1).

Strictly speaking the msgHdr becomes invalid if the move succeeds, so you shouldn't use it here.

Comment 13

[Jorg K (CEST = GMT+2)]

Hey, that's constructive criticism ;-(

So what should I be using? The notification function needs to be fed something.

Comment 14

[[e-mail]]

Comment on attachment 8669733 [details] [diff] [review]
Proposed solution (v1).

Oh, and there's another bug: this calls MoveNewlyDownloadedMessage, which in the case of maildir store, generates a notification of some sort, so you probably shouldn't generate a second one.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2).

OK, moved it to where you had said it should go in comment #9.

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to [e-mail] from comment #14)
> Oh, and there's another bug: this calls MoveNewlyDownloadedMessage, which in
> the case of maildir store, generates a notification of some sort, so you
> probably shouldn't generate a second one.

So the v2 patch solves this, right? The code is now in the spot you had said before.
I tested it, it works just the same.

Comment 17

[[e-mail]]

Comment on attachment 8669770 [details] [diff] [review]
Proposed solution (v2).

>+  // Notify the message was moved.
>+  if (notifier)
I'm confused; this notifier is used previously to notify for a message added, but normally moves don't notify that, they notify for a move completed. At this point I think I'm going to have to give up and suggest you find someone who knows more about this mechanism. Sorry about that.

Comment 18

[Jorg K (CEST = GMT+2)]

Correct, a few lines before the notifier is created and used like so:
nsCOMPtr<nsIMsgFolderNotificationService> notifier(do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID));
  if (notifier)
    notifier->NotifyMsgAdded(newHdr);
for the *new* header.

My addition is:
  nsCOMPtr<nsIMutableArray> messages = do_CreateInstance(NS_ARRAY_CONTRACTID);
  messages->AppendElement(mailHdr, false);
  notifier->NotifyMsgsMoveCopyCompleted(true, messages, destIFolder, nullptr);
so I notify for the *old* header.

I don't see this as a contradiction.

Notifying the addition doesn't mean that anything happens, if there is no listener for that event.

Who can review this? I thought /mailnews is reviewed by Neil, so if he can't do it, we can never change the code?

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8669770 [details] [diff] [review]
Proposed solution (v2).

Neil mentioned maildir. Would this affect maildir?

Comment 20

[Kent James (:rkent)]

"Who can review this?" typically rkent, jcranmer, neil or Ian. I suppose the mess around filters, notifications, and maildir is stuff I am closest to.

You're encountering some of my pet peeves. The msgstore has two different mechanisms for moves, one typically for mbox and one typicially for maildir. This has caused all sorts of bugs and issues. The msgStore abstraction somehow never managed to get designed to handle these issues properly.

I can review this, but it may be a few weeks before I can get to it.

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #20)
> I can review this, but it may be a few weeks before I can get to it.

It's a four line addition, so it would be great if it could land on TB 44, so before the end of October.

Comment 22

[[e-mail]]

(In reply to Jorg K from comment #19)
> Neil mentioned maildir.

Yes, you should totally try with a POP account using maildir too.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v3), now working for mbox and maildir.

(In reply to [e-mail] from comment #22)
> Yes, you should totally try with a POP account using maildir too.

Done. I've added this case to the patch. Instead of four lines it's now eight lines ;-)

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v3b), now working for mbox and maildir.

(oops, forgot hg qref before attaching the patch)

Comment 25

[Kent James (:rkent)]

Comment on attachment 8672219 [details] [diff] [review]
Proposed solution (v3b), now working for mbox and maildir.

Just for the record, I have about 4 hours invested in this little 8 line patch.

I'm going to have to give an r- to this I'm afraid. You are adjusting a low-level notification in an incorrect manner to try to get the UI to report something that you want it to report. But in the process you are taking an already complex, convoluted notification scheme and making it more so, violating its basic assumptions.

The paradigm here is that either a msgAdded notification is done, or if the message already exists then a msgsMoveCopyCompleted notification. Also, in the case of the before filters, the message is never really added to the system in the original location, so it does not make sense to claim that it moved. Incoming filters in theory operate before the messages ever reach the Inbox, at least in the overall mailnews paradigm.

You are violating that because you like the UI message that msgsMoveCopyCompleted gives, but there are other users of that notification, particularly search integration and gloda. Even if you could make them work (and frankly I see no evidence that they work badly after your change), you are changing the meaning of a low-level notification without really doing the legwork to make that possible (such as extensive tests and documentation).

If you want to see where incoming messages land in the activity manager, the proper way to do that with the current notifications is to add what you want to mail/ components/ activity/ modules/ pop3Download.js, less ideal but probably acceptable is to make the change in moveCopy.js

Gloda adapted the notification notifyMsgsClassified as the main way to note messages coming into a folder. You can either use that, or a notifyMsgAdded on each individual message to get the activity that you want. Perhaps you could add a notifyMsgsClassified message in the activity manager, that would show you where messages end up. But I'm not sure how that interacts with the other types. If you really wanted to see where POP3 messages end up, you would need to add a method to nsIPop3ServiceListener.

I really wish I could let you take the easy path that you want, but there shortcuts are what make the code so fragile. We have way too many already.

Comment 26

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #25)
> Just for the record, I have about 4 hours invested in this little 8 line
> patch.
Thanks!! (Actually, the complexity of the system is not my fault, I've spent hours for a one liner in the past.)

> I'm going to have to give an r- to this I'm afraid.
I can live with that. ;-)

> The paradigm here is that either a msgAdded notification is done, or if the
> message already exists then a msgsMoveCopyCompleted notification.
Indeed. Right above the 4 lines I wanted to add there is
   nsCOMPtr<nsIMsgFolderNotificationService> notifier(
     do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID));
   if (notifier)
     notifier->NotifyMsgAdded(newHdr);
Why doesn't that end up in as activity? Wouldn't that make more sense?

> If you want to see where incoming messages land in the activity manager, 
I do! Don't you? Right now, I've set all filters to "after spam" with the result that each morning 50+ messages get downloaded into the inbox and then get busily put elsewhere. That's really no good.

> the
> proper way to do that with the current notifications is to add what you want
> to mail/ components/ activity/ modules/ pop3Download.js, less ideal but
> probably acceptable is to make the change in moveCopy.js
In moveCopy.js I see
  msgAdded : function(aMsg) {
  },
so maybe that is the listener notified by NotifyMsgAdded(), but it doesn't do anything. I will investigate.

> Gloda adapted the notification notifyMsgsClassified as the main way to note
> messages coming into a folder. You can either use that, or a notifyMsgAdded
> on each individual message to get the activity that you want. Perhaps you
> could add a notifyMsgsClassified message in the activity manager, that would
> show you where messages end up. But I'm not sure how that interacts with the
> other types. If you really wanted to see where POP3 messages end up, you
> would need to add a method to nsIPop3ServiceListener.
I don't really follow this. I will look into adding the change to moveCopy.js first before coming back to this.

Thanks again for taking the time!!

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v4), doing something useful in moveCopyModule.msgAdded

OK, as suggested, I added code to moveCopyModule.msgAdded.

This has the advantage, that more activity is logged now, for example adding messages to the Sent folder (see picture attachment).

In absence of an "add mail" icon (mail/themes/<OS>/mail/activity) I used the one for move mail, and I think that's quite OK, Richard?

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of new behavior (Windows).

What you see in the picture (reverse chronological order):

Two messages were downloaded.
The downloaded messages were moved by "before junk" rules and added to HUHU folder.
Two messages were sent and added to the Sent folder.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of new behavior also for IMAP account (Windows).

In the picture you see the following.

Two messages downloaded for POP account [e-mail]. One stored in the Inbox, the other one, stored in HUHU.
Two messages downloaded for IMAP account [e-mail] and stored in the Inbox.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of new IMAP only

Grrrr.
If I configure a rule for an IMAP account, I get two notifications,
one for the move, and one for the addition.

To just change the POP behaviour, you suggested to add code to pop3Download.js (most likely onDownloadCompleted), but the folder passed in there is always "Inbox". So as far as I can see, that won't work.

Thoughts, Kent?

Comment 31

[Richard Marti (:Paenglab)]

Comment on attachment 8675264 [details] [diff] [review]
Proposed solution (v4), doing something useful in moveCopyModule.msgAdded

Looks good. But yeah on IMAP the double add and move looks a bit weird.

You could use the addItem class which uses a plus icon. This would better fit to the text "Added 2 messages...".

Comment 32

[Jorg K (CEST = GMT+2)]

To avoid changing the IMAP behaviour one could check the server type (folder.server.type) in the msgAdded processing, nice hack.

But worse than that: My addition to msgAdded now causes a crash when processing "after junk" filters:
When manually run:
Assertion failure: mNeedsRelease (OnEndExecution called a second time), at c:/mozilla-source/comm-central/mailnews/base/search/src/nsMsgFilterService.cpp:364
#01: nsMsgFilterAfterTheFact::OnSearchDone (c:\mozilla-source\comm-central\mailnews\base\search\src\nsmsgfilterservice.cpp:508)
#02: nsMsgSearchSession::NotifyListenersDone (c:\mozilla-source\comm-central\mailnews\base\search\src\nsmsgsearchsession.cpp:540)
#03: nsMsgSearchSession::TimerCallback (c:\mozilla-source\comm-central\mailnews\base\search\src\nsmsgsearchsession.cpp:483)

While receiving messages:
Assertion failure: mNeedsRelease (OnEndExecution called a second time), at c:/mozilla-source/comm-central/mailnews/base/search/src/nsMsgFilterService.cpp:364
#01: nsMsgApplyFiltersToMessages::RunNextFilter (c:\mozilla-source\comm-central\mailnews\base\search\src\nsmsgfilterservice.cpp:1093)
#02: nsMsgFilterAfterTheFact::ApplyFilter (c:\mozilla-source\comm-central\mailnews\base\search\src\nsmsgfilterservice.cpp:891)
#03: nsMsgFilterAfterTheFact::OnStopCopy (c:\mozilla-source\comm-central\mailnews\base\search\src\nsmsgfilterservice.cpp:1152)
#04: nsMsgMaildirStore::CopyMessages (c:\mozilla-source\comm-central\mailnews\local\src\nsmsgmaildirstore.cpp:1114)
#05: nsMsgLocalMailFolder::CopyMessages (c:\mozilla-source\comm-central\mailnews\local\src\nslocalmailfolder.cpp:1574)
#06: nsMsgCopyService::DoNextCopy (c:\mozilla-source\comm-central\mailnews\base\src\nsmsgcopyservice.cpp:320)

Hmm, not an easy problem after all :-(

Comment 33

[Jorg K (CEST = GMT+2)]

Thanks for the quick reply.

(In reply to Richard Marti (:Paenglab) from comment #31)
> Looks good. But yeah on IMAP the double add and move looks a bit weird.
Yes, working on the double-up. I've just detected that the solution doesn't work at all, see previous comment.

> You could use the addItem class which uses a plus icon. This would better
> fit to the text "Added 2 messages...".
Hmm, I saw the item, but I don't like it. If at all, we should do an "addMail" icon, some sort of arrow, perhaps diagonally. Let's not do anything now for the icon, first I have to have a solution that doesn't crash ;-(

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v5) - alternative, added onMsgAdded in nsIPop3ServiceListener.

OK, now I need more input from Kent.

I have two solutions:
1) Added stuff to moveCopyModule.msgAdded (attachment 8675264 [details] [diff] [review]).
   This has problems with IMAP, since we get double notifications.
   This could be worked/hacked around with checking "folder.server.type".
2) As Kent suggested in comment #25 (this attachment):
   Added onMsgAdded in nsIPop3ServiceListener.
   This works well, it only notifies the messages that get "added/move"
   to a mailbox other than the Inbox, just like the bug summary requests.

Kent, which do you prefer? I'd go with 2) since I've already invested the work.
As far as I can see, the patch would represent the final solution, only some comment needs to be added to nsIPop3Service.idl.

BTW, the crash mentioned in comment #32 is not my fault, see bug 1215807.

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of new *alternative* behavior

Three messages were downloaded, one went to the inbox, two were moved by "before junk" rules to folder HUHU.

Comment 36

[Jorg K (CEST = GMT+2)]

This bug here is important, since "after junk" filters have problems, see bug 1215817.

Comment 37

[Jorg K (CEST = GMT+2)]

Kent, perhaps the event shouldn't be called onMsgAdded but onMsgMoved? The notification could read "Moved xx message(s) to yy". Then the use of the move icon would also be justified. What do you think?

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v6): added onMsgMoved in nsIPop3ServiceListener.

Sadly there was no feedback, so I went ahead with what I suggested in comment #37 and I am presenting the solution for review here.

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of proposed new behaviour

This reports the moved messages just the same as the "after junk" filters do. In the test case, 3 messages were downloaded, of these, 2 were moved.

No UX review necessary, but I'm sure that Richard approves ;-)

Comment 40

[Kent James (:rkent)]

Comment on attachment 8675370 [details] [diff] [review]
Proposed solution (v6): added onMsgMoved in nsIPop3ServiceListener.

I hate to do this to you, but I'm going to suggest another alternative that is closer to your first plan.

What you are looking for is a notification that an unincorporated message has moved. So let's just add that notification.

nsIMsgFolderListener is designed to be extendable, so let's add a new notification that does what you want as an ItemEvent. You could call it "UnincorporatedMessageMoved". You two nsISupports you can pass, the first should be associated with the message source (maybe the source folder?) and the second could just be the new msgHdr. Then you just need to hookup the activity listener for this event. This will be fairly close to your original implementation.

The problem with the current patch is that introducing these new POP3 listener notifications in new places is an unacceptable complication to the listener structure, as those notifications now all happen in a single method.

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v7): Using NotifyItemEvent() with UnincorporatedMessageMoved

This should be what you've described in comment #40. Activity added in moveCopy.js.
Third time lucky, perhaps ;-)

No need to "mozilla/mach update-uuids" for IDL file if only comment was added, right?

Note:
The previous solution did what you had said in comment #25:
- add what you want to mail/components/activity/modules/pop3Download.js
- you would need to add a method to nsIPop3ServiceListener.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot for patch v7.

This now looks exactly as it if were moved by a "after junk" filter.
So maybe the best solution so far.

Comment 43

[Kent James (:rkent)]

Comment on attachment 8676296 [details] [diff] [review]
Proposed solution (v7): Using NotifyItemEvent() with UnincorporatedMessageMoved

Yes this is going to work. Thanks for your patience as I thought this all through.

Right, do not roll the IDL for comment-only changes.

One other quick comment:

+  // Notify the message was moved.
+  if (notifier) {
+    nsCOMPtr<nsIMsgFolder> folder;
+    nsresult rv = mailHdr->GetFolder(getter_AddRefs(folder));
+    NS_ENSURE_SUCCESS(rv, rv);

This notification is not critical, so failure should not result in aborting the method. Use NS_WARN_IF_FALSE and continue.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v7b): Using NotifyItemEvent() with UnincorporatedMessageMoved

Thanks for your patience ;-)
Can we land this before 45 comes around?
(I'm currently using those "after junk" filters, and that's just terrible.)

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: Another screenshot, this time the message moves to a local folder.

Comment 46

[Kent James (:rkent)]

Comment on attachment 8676377 [details] [diff] [review]
Proposed solution (v7b): Using NotifyItemEvent() with UnincorporatedMessageMoved

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

LGTM. Thanks for your patience!

Comment 47

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/7abe3eb0e95845e5ce52f3639cc65832725fe726
Bug 1211261 - Notify when messages are moved by 'before junk' filter. r=rkent