479823 - archive filter rules: when Archive function is used a special set of filters marked to activate at 'Archive time' should be invoked, not the built-in archive function

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Jens Müller (:tessarakt)]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.1b2) Gecko/20090211 Gentoo Firefox/3.1b2
Build Identifier: 

The archive function should support "archive filter rules". That means: If a message is covered by such a filter, it should be moved to a pre-specified folder when pressing "Archive", not to the general archive folder structure.

Reproducible: Always

Comment 1

[Magnus Melin [:mkmelin]]

Confirming RFE.

Comment 2

[Rolf Leggewie]

This is a general solution to my wishlist item to keep sent and received messages in separate archives.

Comment 3

[Matěj Cepl]

And after thinking long and hard about bug 93094, I think this is the only possible solution for my needs described in bug 93094 comment 27.

+1

Comment 4

[Daniela Waranie]

+1

Yeah, please add a new "action" to the filter dialog. This action should trigger the build-in archive function for the message, that is covered by the filter.

Archive function should use the archive setting of the relevant account in with the e-mail resists (which is covered by the filter).

Comment 5

[Daniela Waranie]

At least a addon for that would help (TB 5.x).

Comment 6

[Wayne Mery (:wsmwk)]

Sounds like this is nothing more than run filters against the message. Is that correct?

Comment 7

[:aceman]

I think I understand what comment 4 wants, which could be interesting feature and not that hard.

But I am not sure what comment 0 wants.

Comment 8

[Simon Woods]

Also not entirely sure what comment 0 wants but started following this because I was wanting something like the following:

I work on several projects and try to archive my mails after reading them according to projects - at least the most important mails. The rest gets archived by date.

I would set up a number of filters which try to identify the mail based on the subject or the person that I am sending or receiving the mail from. If the filter matches for a particular project it would move the mail to the appropriate folder. This would be using the standard mail filter mechanisms.

The problem is running this set of specific mail filters with a single click on the message. If the filter allowed the Archive button to be used as a trigger then that would be a considerable motivation to make more use of the filters and the archive function.

I think that this is probably close to what comment 0 wants.
Comment 4 seems to be the other way around: A message filter should call the archive function. The only advantage I see here is that it would then be possible to move the mail to a "dynamic" folder <year>/<month> which is probably only possible otherwise using an addon. I would still need to select the message, then tools -> Run filters on message, and hope that the other filters which I have defined don't do something with the message that I am not expecting.

Comment 9

[Simon Woods]

Attachment: An idea how the Filter Rules could look

This shows an additional menu item "When Archiving Mail".

Comment 10

[Kent James (:rkent)]

Comment on attachment 643864 [details]
An idea how the Filter Rules could look

This would be a useful feature, as would the similar filter on send.

The issue to me is the UI above. What you have done is the natural extension of this, and exactly what I did ("Checking mail (after classification) and "Checking Mail (after classification or Manually Run" are my additions).

But we are running into a combinatorial explosion here. The original mistake was the first change, with taking two features ("Incoming" and "Manual") and adding three options for that when what we should have had was checkboxes. The same filter should be able to applied in any valid combination of contexts without requiring 4,8,16, or 32 choices as we add features.

I suppose what would be useful is another bug to change this UI, and make the current bug and the "Filter on Send" bug dependent on it.

I have an aversion to opening enhancement requests that I am not going to undertake myself, but aceman if you would like to take a stab at the UI change, then we might be able to move forward with these other very important bugs.

Comment 11

[:aceman]

Yes I can look at the UI, depends on how the backend uses the values.

Comment 12

[:aceman]

Can you file the bug? Basically copy your comment 10 :)

Comment 13

[Wayne Mery (:wsmwk)]

bug 543445 and bug 700717 are dupes?

Comment 14

[:aceman]

I think bug 700717 is a dupe, but bug 543445 seems broader than this one here. I suggest making this one block 543445.

Comment 16

[Wayne Mery (:wsmwk)]

bug 661271?

Comment 17

[:aceman]

That one seems to be comment 4 here, but that does not correspond to comment 0. I do not think it is a dupe and I have confirmed it as RFE (until we find a dupe for it, but this one it is not) and moved commenter of comment 4 there.

Comment 18

[Kent James (:rkent)]

It should not be hard to add an archive filter context after aceman did the heavy lifting on the UI, so I'd like to squeeze this into TB 24

Comment 19

[Kent James (:rkent)]

Attachment: WIP rev 1

This basically works (it needs the patch from bug 69567).

Remaining to be done:

1) I need the equivalent changes in suite.

2) FilterAfterTheFact is capable of chaining together multiple async actions, but this version does not allow for that. So for example it would probably fail if there was both a copy and move filter action both applied.

But this is only a few hours of work. With the UI issues resolved, and issues fixed in nsMsgFilterAfterTheFact, the backend work is pretty straightforward for applying filter in different contexts to existing messages.

I think what I need to do is to add a callback listener to the call to applyFilters that would return when the filters are done. I could then use that callback to kick off the actual archive.

Comment 20

[:aceman]

   const long Incoming         = Inbox | News;
   const long Manual           = 0x10;
   const long PostPlugin       = 0x20; // After bayes filtering
+  const long Archive          = 0x40; // Before archiving
   const long All              = Incoming | Manual;

Did you intentionally not include Archive in All ? But it seems to not be used anywhere.

Comment 21

[Kent James (:rkent)]

Attachment: With better async support

This is now pretty complete.

I'm following here a number of conventions that I do in my own code, namely:

1) Using error management loops with BREAK_IF .. or CONTINUE_IF .. macros for error checks.

2) When an async return is expected, the return cannot be done before the original call returns. This requires a timer in the C++ to handle returns.

3) I make a lot of use of very simple listeners for async processes. In my own code, I have mostly used a nsIUrlListener for that purpose, but that is sort of silly since the url is typically not defined. Here I defined what I really want, just a really simple operation listener interface that reliably returns when an async operation is complete.

This patch requires the recently updated patch to bug 695671.

Comment 22

[Kent James (:rkent)]

Attachment: With suite support

This is now complete, so let's get both Thunderbird (Irving) and SeaMonkey (Neil) reviews done. Note you still need this patch:

https://bugzilla.mozilla.org/attachment.cgi?id=739846

from bug 695671

Comment 23

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 741999 [details] [diff] [review]
With suite support

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

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +264,5 @@
> +  if (mListener)
> +    mListener->OnDone(mResult);
> +  Release();
> +  return NS_OK;
> +}

Is this the usual pattern for deferring a callback until later? I've also seen code that uses ThreadManager to put a runnable on the queue directly, rather than indirectly through a timer.

nsThreadUtils.h gives us NS_DispatchToCurrentThread(nsIRunnable *event) and  NS_DispatchToMainThread(nsIRunnable *event); I think I'd prefer that approach.

@@ +962,5 @@
>    virtual   nsresult  RunNextFilter();
>  
>    nsCOMArray<nsIMsgDBHdr> m_msgHdrList;
>    nsMsgFilterTypeType     m_filterType;
> +

Nit: extra blank line

Comment 24

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 741999 [details] [diff] [review]
With suite support

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

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +232,5 @@
>    return rv;
>  }
>  
> +// runs onDone async to prevent running before return of initiating method
> +class DoLater: public nsITimerCallback

Needs MOZ_FINAL to fix

/Users/ireid/tbird/comm-central/mailnews/base/search/src/nsMsgFilterService.cpp:248:1: warning: delete called on 'DoLater' that has virtual functions but non-virtual destructor [-Wdelete-non-virtual-dtor]
NS_IMPL_ISUPPORTS1(DoLater, nsITimerCallback)
^
../../../../mozilla/dist/include/nsISupportsImpl.h:1227:3: note: expanded from macro 'NS_IMPL_ISUPPORTS1'

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 741999 [details] [diff] [review]
With suite support

Yep, while I'm not a giant fan of a billion checkboxes, that one does fit in to the design of the page.  :)  ui-r=me.

Comment 26

[Kent James (:rkent)]

I was hoping to get bug 695671 landed before this, then get this in before TB 24. But I am running out of time with the long review delays. So at this point, I am considering the following:

1) Putting suite support in a separate bug
2) Landing this without bug 695671. It does not really have to have that bug, and this bug (with a string) needs to get in before TB 24, while bug 695671 could be landed in aurora.

I don't think I need another review to do that (though I have not really looked at this in detail yet), but if there are objections I'll get one.

Comment 27

[Kent James (:rkent)]

Attachment: Patch to land

I've got the main review done on this, I just need to get the suite piece approved.

Comment 28

[Ian Neal]

Comment on attachment 764901 [details] [diff] [review]
Patch to land

Similar comments to the periodic filter patch.
Nit: Comments starting with a capital letter should end with a period/full stop.
The filter editor dialog is looking a bit top heavy, but can be revisited as a followup.
New uuids usually differ by more than a single character.
I have done limited testing but I do not tend to use the Archiving functionality (though perhaps I should).
r=me with the comments issue fixed and the uuid issue fixed/responded to.

Comment 29

[[e-mail]]

Comment on attachment 764901 [details] [diff] [review]
Patch to land

I skimmed this just for completeness sake, since the request is somewhat overdue...

>+  /**
>+   * Operation is done (possibly with errors)
>+   *
>+   * @param aResult     Success or failure of the operation
>+   */
>+    void onDone(in nsresult aResult);
I have to say that I think this is a very boring name. I can't actually decide on an exact name but I think it should contain the word "Operation" e.g. onOperationEnded, onStopOperation, onOperationDone.

>+nsresult DoLater::OnDone(nsIMsgOperationListener *aListener, nsresult aResult)
>+{
>+  mListener  = aListener;
>+  mResult = aResult;
Ideally you would set these in the constructor, and maybe the caller could explicitly dispatch the object.

>-nsMsgApplyFiltersToMessages::nsMsgApplyFiltersToMessages(nsIMsgWindow *aMsgWindow, nsIMsgFilterList *aFilterList, nsIArray *aFolderList, nsIArray *aMsgHdrList, nsMsgFilterTypeType aFilterType)
>+nsMsgApplyFiltersToMessages::nsMsgApplyFiltersToMessages(
>+  nsIMsgWindow *aMsgWindow, nsIMsgFilterList *aFilterList,
>+  nsIArray *aFolderList, nsIArray *aMsgHdrList,
>+  nsMsgFilterTypeType aFilterType, nsIMsgOperationListener *aListener)
> : nsMsgFilterAfterTheFact(aMsgWindow, aFilterList, aFolderList),
>   m_filterType(aFilterType)
> {
>   nsCOMPtr<nsISimpleEnumerator> msgEnumerator;
>+  mApplyFilterListener = aListener;
mApplyFilterListener is a member of nsMsgFilterAfterTheFact, so ideally you would construct this through nsMsgFilterAfterTheFact's constructor.

>+    // This function implements nsIMsgOperationListener
>+    Components.utils.import("resource:///modules/XPCOMUtils.jsm", this);
>+    function FilterListener(aSrcFolder, aDstFolder, aMsgs, aParent)
Strictly speaking I would make this a top-level function as it doesn't depend on function scope.

>+      if (aResult != Components.results.NS_OK)
You should use !Components.isSuccessCode(aResult) here.

I also had a look at the dialog and the only space-saving idea I could come up with was to turn it into tabs (General, Conditions, Actions).

Comment 30

[Ian Neal]

This patch has bitrotted now.

Comment 31

[Kent James (:rkent)]

We'll do bug 11039 first since there are some common issues.

Comment 32

[Kent James (:rkent)]

Attachment: Part 1, core changes

Comment 33

[Kent James (:rkent)]

Attachment: Part 2, suite implementation

Comment 34

[Kent James (:rkent)]

Attachment: Part 3, mail implementation

Comment 35

[Kent James (:rkent)]

With the OnStopOperation implementation done in bug 11039, this bug needed updating. Bug 695671 has also been completed which was a prerequisite.

Comment 36

[[e-mail]]

Comment on attachment 8543532 [details] [diff] [review]
Part 2, suite implementation

>+    if (this._batches) {
The reindentation added by this check makes the patch very difficult to read. There's obviously a bug in the code because it sets _batches to null on failure although the constructor initialises _batches to {} instead. If you fix those failure cases you can avoid the extra check and reindentation. (Extra brownie points for switching to a Map!)

Comment 37

[[e-mail]]

Ah no, it's some other logic change that is messing up the indentation.

Comment 38

[[e-mail]]

Comment on attachment 8543532 [details] [diff] [review]
Part 2, suite implementation

>-    for (let key in this._batches)
>-    {
>-      this._currentKey = key;
>-      let batch = this._batches[key];
>+    // get the first defined key and value
>+    if (this._batches) {
>+      let keys = Object.keys(this._batches);
>+      if (keys.length) {
>+        this._currentBatch = this._batches[keys[0]];
>+        delete this._batches[keys[0]];
>+        return this.filterBatch();
So, as it happens, for in loops don't mind null values, which is why the old code works. As you're returning anyway, you might as well keep the old style, i.e.
this._currentBatch = this._batches[key];
delete this._batches[key];
return this.filterBatch();

>+    // all done
>+    this._batches = null;
[Not strictly necessary because we've already deleted all the batches.]

>+          return; // continues wiht OnStopRunningUrl
Typo.

Comment 39

[Magnus Melin [:mkmelin]]

Comment on attachment 8543533 [details] [diff] [review]
Part 3, mail implementation

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

Haven't tested it yet, but some nits

::: mail/base/content/mailWindowOverlay.js
@@ +1679,5 @@
>    processNextBatch: function BatchMessageMover_processNextBatch() {
> +    // get the first defined key and value
> +    if (this._batches) {
> +      let keys = Object.keys(this._batches);
> +      if (keys.length) {

I prefer (keys.length > 0) for numeric checks

@@ +1692,5 @@
> +    // all done
> +    return;
> +  },
> +
> +  filterBatch: function BatchMessageMover_filterBatch() {

the anonymous function naming is no longer needed, so we shouldn't add any more of it. (here and a few other places)

@@ +1697,5 @@
> +    let batch = this._currentBatch;
> +
> +    let filterArray = Components.classes["@mozilla.org/array;1"]
> +                                .createInstance(Components.interfaces.nsIMutableArray);
> +    for (let message of batch.messages)

please use braces for all for loops

@@ +1700,5 @@
> +                                .createInstance(Components.interfaces.nsIMutableArray);
> +    for (let message of batch.messages)
> +      filterArray.appendElement(message, false);
> +
> +    // Apply filters to this batch

add .

@@ +1710,5 @@
> +
> +  onStopOperation: function BatchMessageMover_onStopOperation(aResult) {
> +    if (!Components.isSuccessCode(aResult))
> +    {
> +      Components.utils.reportError("Archive filter failed");

these might want to be 
  ... failed: " + aResult);

so you'd at least get a better clue on what went wrong

@@ +1734,5 @@
> +      for (let item of batch.messages) {
> +        if (srcFolder.msgDatabase.ContainsKey(item.messageKey) &&
> +            !(srcFolder.getProcessingFlags(item.messageKey) &
> +              Components.interfaces.nsMsgProcessingFlags.FilterToMove))
> +          moveArray.appendElement(item, false);

long enough if to warrant braces for readability?

@@ +1737,5 @@
> +              Components.interfaces.nsMsgProcessingFlags.FilterToMove))
> +          moveArray.appendElement(item, false);
> +      }
> +
> +      if (!moveArray.length)

== 0

Comment 40

[Kent James (:rkent)]

Attachment: Part 1, core changes v1

Comment 41

[Kent James (:rkent)]

Attachment: Part 3, mail implementation v1

Comment 42

[Kent James (:rkent)]

Attachment: Part 2, suite implementation v1

Comment 43

[[e-mail]]

Comment on attachment 8549140 [details] [diff] [review]
Part 2, suite implementation v1

Not sure what happened here...

Comment 44

[Kent James (:rkent)]

Maybe I moved up the wrong patch. At one point I was hoping to just copy and paste from the mail to suite, but then decided there were too many differences. I must have given you that file instead.

I'll check again.

Comment 45

[[e-mail]]

Comment on attachment 8549138 [details] [diff] [review]
Part 1, core changes v1

>diff --git a/mailnews/base/search/content/FilterEditor.xul b/mailnews/base/search/content/FilterEditor.xul
[Needs ui-r]

>+  // bail early if nothing to do
[Probably a good idea, but how does this happen?]

Comment 46

[Kent James (:rkent)]

Attachment: Part 2, suite implementation v1a (correct patch this time)

OK this time I actually compiled SM and tested that it works :)

Comment 47

[Magnus Melin [:mkmelin]]

Comment on attachment 8549139 [details] [diff] [review]
Part 3, mail implementation v1

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

Seems to work fine. This can be very useful I think! r=mkmelin

::: mail/base/content/mailWindowOverlay.js
@@ +1828,5 @@
>      // this will always be a create folder url, afaik.
>      if (Components.isSuccessCode(exitCode))
> +      this.continueBatch();
> +    else {
> +      Components.utils.reportError("Archive failed to create folder");

: + exitCode for this too

@@ +1845,5 @@
> +    if (Components.isSuccessCode(aStatus))
> +      return this.processNextBatch();
> +
> +    // stop on error
> +    Components.utils.reportError("Archive failed to copy");

+ aStatus here

Comment 48

[[e-mail]]

Comment on attachment 8549284 [details] [diff] [review]
Part 2, suite implementation v1a (correct patch this time)

>+          return; // continues wiht OnStopRunningUrl
Nit: typo

Comment 49

[Kent James (:rkent)]

Attachment: Filter editor with Archiving option

Neil asked for a UI review. The "Archiving" filter context is new.

Comment 50

[Kent James (:rkent)]

Comment on attachment 8549985 [details]
Filter editor with Archiving option

I am removing the request for ui approval, because we already received that in comment 25. Although the code changed substantially since then due to bit rot, the UI design did not.

Comment 51

[Kent James (:rkent)]

(In reply to [e-mail] from comment #45)
> Comment on attachment 8549138 [details] [diff] [review]
> Part 1, core changes v1
> 
> >diff --git a/mailnews/base/search/content/FilterEditor.xul b/mailnews/base/search/content/FilterEditor.xul
> [Needs ui-r]
> 
> >+  // bail early if nothing to do
> [Probably a good idea, but how does this happen?]

The section after this comment does this, by calling OnStart/StopCopy and returning if there are no messages:

  // bail early if nothing to do
  messages->GetLength(&cnt);
  if (!cnt)
  {
    if (listener)
    {
      listener->OnStartCopy();
      listener->OnStopCopy(NS_OK);
    }
    return NS_OK;
  }

Comment 52

[Kent James (:rkent)]

Checked in
https://hg.mozilla.org/comm-central/rev/ad0936add555
https://hg.mozilla.org/comm-central/rev/f89c66e48638
https://hg.mozilla.org/comm-central/rev/dc73fedb8caf

Comment 54

[Matt]

Currently updating the user documentation.
On a factual note.  The filter action here applies only to the mail involved in the current archive operation,  not all mail.  Correct?

Comment 55

[Kent James (:rkent)]

(In reply to Matt from comment #54)
> Currently updating the user documentation.
> On a factual note.  The filter action here applies only to the mail involved
> in the current archive operation,  not all mail.  Correct?

Correct.