174441 - Implement multiple filter actions

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Navin Gupta]

This bug deals with all the backend issues for supporting multiple filter actions. 
The UI will be dealt with in a separate bug. I have made it so that it works with
exisiting UI. For multiple filter actions we are just going to have one action
after another in rules.dat, something like this....

name="Junk"
enabled="yes"
type="1"
action="Label"
actionValue="1"
action="Move to folder"
actionValue="imap://qatest37@nsmail-2/Junk"
condition="OR (subject,contains,Junk)"

It can work with existing rules.dat if someone adds more actions by hand since
there is no UI for it (at present).

Comment 1

[laurel]

I know there's an existing bug or two out there about this... will duplicate
those to this one and/or the UI one.

Comment 2

[Navin Gupta]

Attachment: proposed fix, v1

I am still testing this patch but in general looks ok. 

I have added an nsISupportsArray that can hold multiple filter action to
nsIMsgFilter. Also I had to move single action support from nsIMsgFilter to a
new nsIMsgRuleAction interface so that we can get/set action properties. So we
parse
the rules.dat and add actions to this array and when we apply actions we
iterate
through the array and apply them. MoveToFolder has to be the last action
because
we copy the original hdr to destination folder and delete original hdr (for
local) and we stop applying any more actions (*applyMore = PR_FALSE). I have
made this to work w/ exisiting UI so it can be checked in independently.

Comment 3

[Navin Gupta]

Attachment: proposed fix, v2

See above comment for general explanation. fixed some issues with found when
testing. Completed testing for all affected areas by the patch. 

Cavin, can you do the review ? thx.

Comment 4

[Navin Gupta]

Attachment: proposed fix, v1

This is the complete patch for implementing multiple filter actions. It
includes UI changes also. I will attach the screen-shot for UI and screenshot
as per jglick. The most important changes include adding an nsISupportsArray
that can hold multiple filter actions to nsIMsgFilter, moving single action
support from nsIMsgFilter to a new nsIMsgRuleAction interface so that we can
get/set action properties. We parse the rules.dat and add actions to this array
and when we apply actions we iterate through the array and apply them.
MoveToFolder has to be the last action because we copy the original hdr to
destination folder and delete original hdr (for local) and we stop applying any
more actions (*applyMore = PR_FALSE). I added a method GetSortedActionList to
get the action list that has MoveToFolder action (if present) as last action. 

For the UI, basically implemented a listbox that holds all the actions which
can be checked off. When we saveFilter we iterate thru the ones that are
checked off
and add them to actionsArray. The same thing happens when we are editing a
filter, we iterate thru the actions array and check off in the UI.

I have tested the feature, it works. One known issue that has to be resolved is
if we have changePriority and MoveToFolder for imap then changed priority
wouldn't show up in destination msg because we don't store the flag on the
server, so it gets lost.

Comment 5

[Navin Gupta]

Attachment: screenshot as per jglick

Robinf suggested New folder --> New Folder

Comment 6

[Navin Gupta]

Attachment: screenshot as per jglick

Robin suggested New folder.. --> New Folder..

Comment 7

[Navin Gupta]

David, Cavin, Can I get a review ? thx. I will post a screenshot of UI. 

Comment 8

[David :Bienvenu]

+mustSelectAction=You must select atleast one filter action.
typo - should be "at least"

NS_IMETHODIMP
+nsMsgFilter::CreateAction(nsIMsgRuleAction **aAction)
+{
+    nsMsgRuleAction *action = new nsMsgRuleAction;
+    NS_ENSURE_TRUE(action, NS_ERROR_OUT_OF_MEMORY);
+
for consistency, should we have NS_ENSURE_ARG_POINTER(aAction)?

I don't like NS_ERROR_FAILURE here:
-    if (NS_SUCCEEDED(rv) && msgHdr)
 
+  if (!msgHdr)
+    return NS_ERROR_FAILURE; //fatal error, cannot apply filters
+

can we use NS_ERROR_NULL_POINTER? NS_ERROR_FAILURE is so meaningless.
NS_ERROR_NULL_POINTER is at least slightly more meaningful.

+      {
+        if ((actionType == nsMsgFilterAction::MoveToFolder ||
+            (actionType == nsMsgFilterAction::Delete && deleteToTrash)) &&
!m_msgMovedByFilter)  
+        {  //don't log hit if move failed
+        }
+        else
+          (void)filter->LogRuleHit(filterAction, msgHdr); 

is this the same as 
// only log if successful move, or non-move action
if (m_msgMovedByFilter || (actionType != nsMsgFilterAction::MoveToFolder &&
(actionType != nsMsgFilterAction::Delete || !deleteToTrash))
  (void) filter->LogRuleHit(filterAction, msgHdr);

that would avoid the empty else clause and make the code a little more readable.

+         filterAction->GetTargetFolderUri(getter_Copies(actionTargetFolderUri));
+         if (actionTargetFolderUri.IsEmpty())
               return rv;
what's rv here? It looks unrelated to the actionTargetFolderUri being empty -
i.e., it's whatever rv was last time it was set. My guess is that it's always
going to be NS_OK, though that could be a limitation of the context diff, or I
could be reading it wrong. If you do want to return NS_OK, you should be
explicit about it.

Are we revving the filter file version (I don't see that happening, but I could
have missed it), or is there some reason we don't have to? If the latter, what
happens if the old filing code tries to read in the new multi-action filters?
Does it just ignore the extra actions and thus lose them when writing out the
filters again?

Comment 9

[Navin Gupta]

I don't know if your "if" statement will work. It is much more complicated to read.
I could add more comments for empty part. 

If you use older mozilla version with multiple filter actions then the last
action would be picked up. Yes, when writing back to the disk, only last action
would be written. Since we are going to fork rules.dat, we are going to be fine
as far as multiple filter actions are concerned. 

 

Comment 10

[(not reading, please use [e-mail] instead)]

another alternative to the if statement:

+        //don't log hit if move failed
+        if (!((actionType == nsMsgFilterAction::MoveToFolder ||
+            (actionType == nsMsgFilterAction::Delete && deleteToTrash)) 
+            && !m_msgMovedByFilter))
+          (void)filter->LogRuleHit(filterAction, msgHdr); 

keeps the code the same as navin, wraps it with a !, but removes the empty 
clause (I agree, remove that) 

Comment 11

[(not reading, please use [e-mail] instead)]

bienvenu's if statement works, too. he's just applying the ! through the 
boolean expression.

Comment 12

[Navin Gupta]

ok, I'll do as per bienvenu.

Comment 13

[Navin Gupta]

Attachment: screenshot of UI

Comment 14

[Navin Gupta]

Attachment: patch

changes as per bienvenu's comments. Also I added three actions to the table
reply, forward and stop execution.

Comment 15

[David :Bienvenu]

Comment on attachment 104413 [details] [diff] [review]
patch 

thx, sr=bienvenu

Comment 16

[Navin Gupta]

Cavin, Can I get r=? for last patch

Comment 17

[Cavin Song]

Comment on attachment 104413 [details] [diff] [review]
patch 

r=cavin. You should also check rv from GetSortedActionList() calls.

Comment 18

[Navin Gupta]

fix checked in. addressed cavin's last comment.

Comment 19

[laurel]

Marking verified using nov22 commercial trunk build: win98, mac OS 10.2, linux rh8.
Have not tested all action combinations, all account types, continuing execution
through the filter list, etc., but have tested that multiple filter actions can
be configured and execute on a filter match.
Any further problems found with multiple filter actions will be logged separately.

Comment 20

[Igor Velkov]

Maybe, better UI can be like that:

instead check on place with pre-defined action - possible to add new entry with
selectable actions?

Then we can make some similar actions a time - for example, forward message to 2
different people (to home, for example), and place it to two different mailboxes
- personal archive(local folder) and to shared workgroup box (imap folder,
differnt account).

Today's functionality is harder in understanding and lover usability - it's my
opinion