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.

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

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

+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).

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?

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.

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.

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

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.

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

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

bug 543445 and bug 700717 are dupes?

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

bug 661271?

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.

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

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.

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.

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.

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 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 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.

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.

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

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 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).

This patch has bitrotted now.

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

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

Comment on attachment 8549140 [details] [diff] [review] Part 2, suite implementation v1 Not sure what happened here...

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?]

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

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 on attachment 8549284 [details] [diff] [review] Part 2, suite implementation v1a (correct patch this time) >+ return; // continues wiht OnStopRunningUrl Nit: typo

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

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.

(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; }

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

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?

(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.