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

RESOLVED FIXED in Thunderbird 38.0

Status

enhancement
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: tessarakt, Assigned: rkent)

Tracking

(Blocks 1 bug, {feature, user-doc-needed})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 8 obsolete attachments)

57.14 KB, image/png
Details
9.44 KB, patch
neil
: review+
Details | Diff | Splinter Review
10.45 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
12.60 KB, patch
neil
: review+
Details | Diff | Splinter Review
53.93 KB, image/jpeg
Details
(Reporter)

Description

10 years ago
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
Confirming RFE.
Status: UNCONFIRMED → NEW
Component: General → Mail Window Front End
Ever confirmed: true
QA Contact: general → front-end

Comment 2

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

Comment 3

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

8 years ago
+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

8 years ago
At least a addon for that would help (TB 5.x).
Sounds like this is nothing more than run filters against the message. Is that correct?

Comment 7

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

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

7 years ago
This shows an additional menu item "When Archiving Mail".
(Assignee)

Comment 10

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

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

Comment 12

7 years ago
Can you file the bug? Basically copy your comment 10 :)
(Assignee)

Updated

7 years ago
Depends on: 775665
bug 543445 and bug 700717 are dupes?

Comment 14

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

Updated

7 years ago
Duplicate of this bug: 700717

Comment 17

7 years ago
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.
Component: Mail Window Front End → Filters
Product: Thunderbird → MailNews Core
Summary: Archive function: archive filter rules → 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
Version: unspecified → Trunk
(Assignee)

Comment 18

6 years ago
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
Assignee: nobody → kent
Status: NEW → ASSIGNED
Depends on: 695671
(Assignee)

Comment 19

6 years ago
Posted patch WIP rev 1 (obsolete) — Splinter Review
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

6 years ago
   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.
(Assignee)

Comment 21

6 years ago
Posted patch With better async support (obsolete) — Splinter Review
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.
Attachment #738733 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
Posted patch With suite support (obsolete) — Splinter Review
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
Attachment #739850 - Attachment is obsolete: true
Attachment #741999 - Flags: superreview?(neil)
Attachment #741999 - Flags: review?(irving)
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
Attachment #741999 - Flags: ui-review?(bwinton)
Attachment #741999 - Flags: review?(irving)
Attachment #741999 - Flags: review+
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 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.
Attachment #741999 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 26

6 years ago
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.
(Assignee)

Comment 27

6 years ago
Posted patch Patch to land (obsolete) — Splinter Review
I've got the main review done on this, I just need to get the suite piece approved.
Attachment #741999 - Attachment is obsolete: true
Attachment #741999 - Flags: superreview?(neil)
Attachment #764901 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #764901 - Flags: review? → review?(iann_bugzilla)

Comment 28

6 years ago
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.
Attachment #764901 - Flags: review?(iann_bugzilla) → review+
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

6 years ago
This patch has bitrotted now.

Updated

5 years ago
See Also: → 996130
(Assignee)

Updated

4 years ago
Blocks: 11039
(Assignee)

Comment 31

4 years ago
We'll do bug 11039 first since there are some common issues.
No longer blocks: 11039
Depends on: 11039
(Assignee)

Comment 32

4 years ago
Posted patch Part 1, core changes (obsolete) — Splinter Review
Attachment #764901 - Attachment is obsolete: true
Attachment #8543531 - Flags: review?(neil)
(Assignee)

Comment 33

4 years ago
Posted patch Part 2, suite implementation (obsolete) — Splinter Review
Attachment #8543532 - Flags: review?(neil)
(Assignee)

Comment 34

4 years ago
Posted patch Part 3, mail implementation (obsolete) — Splinter Review
Attachment #8543533 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 35

4 years ago
With the OnStopOperation implementation done in bug 11039, this bug needed updating. Bug 695671 has also been completed which was a prerequisite.
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!)
Ah no, it's some other logic change that is messing up the indentation.
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 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
(Assignee)

Comment 40

4 years ago
Attachment #8543531 - Attachment is obsolete: true
Attachment #8543531 - Flags: review?(neil)
Attachment #8549138 - Flags: review?(neil)
(Assignee)

Comment 41

4 years ago
Attachment #8543533 - Attachment is obsolete: true
Attachment #8543533 - Flags: review?(mkmelin+mozilla)
Attachment #8549139 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 42

4 years ago
Attachment #8543532 - Attachment is obsolete: true
Attachment #8543532 - Flags: review?(neil)
Attachment #8549140 - Flags: review?(neil)
Comment on attachment 8549140 [details] [diff] [review]
Part 2, suite implementation v1

Not sure what happened here...
Attachment #8549140 - Flags: review?(neil) → review-
(Assignee)

Comment 44

4 years ago
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 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?]
Attachment #8549138 - Flags: review?(neil) → review+
(Assignee)

Comment 46

4 years ago
OK this time I actually compiled SM and tested that it works :)
Attachment #8549140 - Attachment is obsolete: true
Attachment #8549284 - Flags: review?(neil)
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
Attachment #8549139 - Flags: review?(mkmelin+mozilla) → review+
Keywords: feature
Comment on attachment 8549284 [details] [diff] [review]
Part 2, suite implementation v1a (correct patch this time)

>+          return; // continues wiht OnStopRunningUrl
Nit: typo
Attachment #8549284 - Flags: review?(neil) → review+
(Assignee)

Comment 49

4 years ago
Neil asked for a UI review. The "Archiving" filter context is new.
Attachment #8549985 - Flags: ui-review?(josiah)
(Assignee)

Comment 50

4 years ago
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.
Attachment #8549985 - Flags: ui-review?(josiah)
(Assignee)

Comment 51

4 years ago
(In reply to neil@parkwaycc.co.uk 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;
  }
(Assignee)

Comment 52

4 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Duplicate of this bug: 1171965

Comment 54

3 years ago
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?
(Assignee)

Comment 55

3 years ago
(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.
You need to log in before you can comment on or make changes to this bug.