Closed Bug 555051 Opened 10 years ago Closed 10 years ago

Support "needs body" for custom filter actions

Categories

(MailNews Core :: Filters, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.1b2

People

(Reporter: rkent, Assigned: rkent)

Details

Attachments

(1 file, 1 obsolete file)

Custom searches can request "needs body" which results in the delay of the filter until after messages are downloaded. Custom filter actions need the same capability in nsIMsgFilterCustomAction
I have a patch for this, but I'm having second thoughts about whether I really need this.
Target Milestone: Thunderbird 3.1b2 → ---
Summary: Support needs body for custom filter actions → Support "needs body" for custom filter actions
I implemented today a Body Regular Expression search in FiltaQuilla using the equivalent needsBody attribute in custom search terms, and I am very pleased with that (see also bug 19442 with 104 votes!). Without needsBody, it hung on imap. Not sure why, maybe trying to start an IMAP url to download the body while we are in the middle of header processing doesn't work.

But the upshot is that I DO want this feature for custom actions as well, as many actions may also want the body available. So I will go ahead and post the patch and review request.
Attachment #435423 - Flags: superreview?(bienvenu)
Attachment #435423 - Flags: review?(bienvenu)
Whiteboard: [needs r/sr bienvenu]
Bienvenu: this turns out to be an important bug for me for TB 3.1, as without it I can't really do any filter actions that need the body (such as save attachment) in IMAP. It might also affect Calendar uses that need to process message attachments.
Target Milestone: --- → Thunderbird 3.1b2
Comment on attachment 435423 [details] [diff] [review]
needsBody attribute and unit test

+          nsCOMPtr<nsIMsgRuleAction> action;
+          rv = actionList->QueryElementAt(actionIndex,
+                                          NS_GET_IID(nsIMsgRuleAction),
+                                          getter_AddRefs(action));

can't this be

nsCOMPtr<<nsIMsgRuleAction> action(do_QueryElementAt(actionList, actionIndex, &rv));

I know you're just cloning tests, but I don't think this part is true or needed:

+  // these hacks are required because we've created the inbox before
+  // running initial folder discovery, and adding the folder bails
+  // out before we set it as verified online, so we bail out, and
+  // then remove the INBOX folder since it's not verified.
+  msgImapFolder.hierarchyDelimiter = '/';
+  msgImapFolder.verifiedAsOnlineFolder = true;
+

Other than that, it looks fine, and I'll give the patch and unit test a try.
Comment on attachment 435423 [details] [diff] [review]
needsBody attribute and unit test

Oh, the fact that you need this change means a caller is not checking the rv correctly. But it looks to me like you're using a comptr in the calling code so you shouldn't need this change?
Comment on attachment 435423 [details] [diff] [review]
needsBody attribute and unit test

r/sr=me, modulo previous comments.
Attachment #435423 - Flags: superreview?(bienvenu)
Attachment #435423 - Flags: superreview+
Attachment #435423 - Flags: review?(bienvenu)
Attachment #435423 - Flags: review+
Whiteboard: [needs r/sr bienvenu]
(In reply to comment #6)
> (From update of attachment 435423 [details] [diff] [review])
> Oh, the fact that you need this change means a caller is not checking the rv
> correctly. But it looks to me like you're using a comptr in the calling code so
> you shouldn't need this change?

If you mean:
-      NS_ERROR("Need to set CustomId");
+      *aCustomAction = nsnull;

I needed that change so that I could simply do the following:

+          nsCOMPtr<nsIMsgFilterCustomAction> customAction;
+          action->GetCustomAction(getter_AddRefs(customAction));
+          if (!customAction)
+            continue;

that checks for the customAction. The NS_ERROR caused some issue, perhaps it
caused the unit test to fail, can't recall. I don't see that I need it, if you
want I could change it to an NS_WARNING though. I still want to set the null
return though, as that just makes it easier to check for the customId in a
single operation. This is a critical point in the message processing, and I am
not going to do anything other than continue regardless of the error, so I
really don't need to consider the rv itself.
it's bad xpcom style, unfortunately. If there's an error, you shouldn't make any assumptions about the out params. NS_WARNING would be good.
Attached patch Fixed nitsSplinter Review
Attachment #435423 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.