864187 - Run filters periodically

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Kent James (:rkent)]

Add the ability to run filters periodically on particular folders. This is anticipated in the more general bug 59365.

Comment 1

[Kent James (:rkent)]

Attachment: WIP

This must be applied on top of patches for bug 479823 and bug 695671

Comment 2

[Kent James (:rkent)]

This bug could easily also be done as an extension. But I think that we need additional filter contexts ("triggers" per Outlook and bug 59365) in core, and this particular one could server a variety of purposes.

This is my use case: I want to be able to process messages on my mobile as well as from Thunderbird. I used to use tags, but since my mobile does not support tags, I have started to "Archive" messages that need no further actions from me, which means moving to a mailbox Archives/2013.

But I want to archive messages locally long-term to reduce online disk usage. So if I move a message to Archives/2013 on the phone, after the message has aged a little I want to then move it to a local Archives/2013 folder. That would use a periodic filter.

Comment 3

[:aceman]

+<!ENTITY folderApplyPeriodicFilters.label         "When getting new messages for this account, apply periodic filters">

How are the periodic triggers dependent on "getting new messages"? Will they not trigger on some account where mail checking is disabled?

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to :aceman from comment #3)
> How are the periodic triggers dependent on "getting new messages"? Will they
> not trigger on some account where mail checking is disabled?

I think you're right, and that it's a good thing: I (can) never get new messages for local folders, but running periodic filters against them sounds like a great way of enabling auto-archiving (well, at least per folder, that is).

Comment 5

[:aceman]

So you also mean they should be independent on getting new mail into the account?

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 740125 [details] [diff] [review]
WIP

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

I know this is a WIP patch, but before I forget let's document some drive-by nits.

::: mailnews/base/search/content/FilterEditor.js
@@ +291,5 @@
>        this.checkBoxIncoming.checked = aType & (nsMsgFilterType.PostPlugin |
>                                                 nsMsgFilterType.Incoming);
>  
>        this.checkBoxArchive.checked  = aType & nsMsgFilterType.Archive;
> +      this.checkBoxPeriodic.checked  = aType & nsMsgFilterType.Periodic;

= could be lined up

::: mailnews/base/src/periodicFilterManager.js
@@ +26,5 @@
> +  init: function _init()
> +  {
> +    // set the next filter time
> +    let servers = MailServices.accounts.allServers;
> +    for each (let server in fixIterator(servers, Ci. nsIMsgIncomingServer))

space after Ci.

@@ +49,5 @@
> +  notify: function _notify(aTime)
> +  {
> +    let servers = MailServices.accounts.allServers;
> +    let nowTime = Date.now();
> +    for (let server in fixIterator(servers, Ci. nsIMsgIncomingServer))

space after Ci.

@@ +62,5 @@
> +      if (!foldersToFilter.length)
> +        continue;
> +      MailServices.filters.applyFiltersToFolders(server.getFilterList(null), foldersToFilter, null);
> +    }
> +  },

comma after last object property/function

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to :aceman from comment #5)
> So you also mean they should be independent on getting new mail into the
> account?

Debatable. Let's see what rkent's intention was.

Comment 8

[Kent James (:rkent)]

I was originally going to add the periodic filters to the biff manager, so initial comments mentioned "getting new messages". But I changed my mind, and decided to implement the independent periodicFilterManager instead. So any references to "getting new messages" in the strings have been removed.

Thanks for the drive-by reviews BTW, they are helpful.

It seems a little odd to me to implement this as the next filter context, since it has not been specifically requested. But it is easy enough to do, and can be used (albeit rather inefficiently) to solve a variety of other problems, for example:

1) Since incoming IMAP filters do not fire on read messages, a periodic filter could be used to process the inbox after it has been read by a mobile device.

2) You could actually do a variant of filter on send my running a periodic filter on the Sent folder.

But really my main goal personally is to allow me to manage storage between a two different accounts. On my mobile phone, I want to mark messages as Done by moving them to a folder Archives/2013. But I also want to limit the storage in that folder, moving messages older than 3 months to a local folder with the same name.

So one of my main questions is, given that this filter context is relatively easy to do, is it worth putting in core? I could easily also add it to FiltaQuilla instead.

Comment 9

[:aceman]

Was it already mentioned when do these filters actually trigger? What is the period between invocations? Is it simply going by the pref value ?
+pref("mail.server.default.periodicFilterRateSeconds", 600);

Comment 10

[Kent James (:rkent)]

(In reply to :aceman from comment #9)
> Was it already mentioned when do these filters actually trigger? What is the
> period between invocations? Is it simply going by the pref value ?
> +pref("mail.server.default.periodicFilterRateSeconds", 600);

My current plan is to not provide UI for that in this bug, but leave it for a subsequent bug or extension.

Comment 11

[Magnus Melin [:mkmelin]]

(In reply to Kent James (:rkent) from comment #8)
> So one of my main questions is, given that this filter context is relatively
> easy to do, is it worth putting in core? I could easily also add it to
> FiltaQuilla instead.

The use cases sound useful to me, so i'd opt for core.

Comment 12

[Kent James (:rkent)]

Attachment: with suite support

Comment 13

[Kent James (:rkent)]

Comment on attachment 740125 [details] [diff] [review]
WIP

I'm going to try some different reviewers on this to cover both the core, SM, and TB portions.

This patch relies on changes in https://bugzilla.mozilla.org/attachment.cgi?id=739846

It also assumes that patch https://bugzilla.mozilla.org/attachment.cgi?id=741999 is loaded, since that patch acts on the same portion of code so would bitrot this one. But there is no functionality from that patch needed, so in theory they could land separately.

Comment 14

[Ian Neal]

Comment on attachment 742030 [details] [diff] [review]
with suite support

Just a drive by comment:
>+++ b/mail/locales/en-US/chrome/messenger/FilterEditor.dtd
>+<!ENTITY contextPeriodic.label "Periodic Trigger Runs">
>+<!ENTITY contextPeriodic.accesskey "E">
Should be lower case, so "e"

>+++ b/mail/locales/en-US/chrome/messenger/folderProps.dtd
>+<!ENTITY folderApplyPeriodicFilters.label         "Apply periodic filters to messages in this folder">
>+<!ENTITY folderApplyPeriodicFilters.accesskey     "P">
Should be lower case, but maybe a better choice would be "m"
The two entries should probably be with the majority of the entities for the "General Information" tab (folderCheckForNewMessages2.label and folderCheckForNewMessages2.accesskey seem to be out of place)

>+++ b/suite/locales/en-US/chrome/mailnews/FilterEditor.dtd
>+<!ENTITY contextPeriodic.label "Periodic Trigger Runs">
>+<!ENTITY contextPeriodic.accesskey "E">
Should be lower case, so "e"

>+++ b/suite/locales/en-US/chrome/mailnews/folderProps.dtd
>+<!ENTITY folderApplyPeriodicFilters.label        "Apply periodic filters to messages in this folder">
>+<!ENTITY folderApplyPeriodicFilters.accesskey    "P">
Should be lower case, but maybe a better choice would be "m"
The two entries should probably be with the majority of the entities for the "General Information" tab (folderCheckForNewMessages2.label and folderCheckForNewMessages2.accesskey seem to be out of place)

For reference https://developer.mozilla.org/en/docs/XUL_Accesskey_FAQ_and_Policies

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 742030 [details] [diff] [review]
with suite support

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

::: mail/locales/en-US/chrome/messenger/FilterEditor.dtd
@@ +24,5 @@
>  <!ENTITY contextBeforeCls.label "Filter before Junk Classification">
>  <!ENTITY contextAfterCls.label "Filter after Junk Classification">
>  <!ENTITY contextArchive.label "Archiving">
>  <!ENTITY contextArchive.accesskey "A">
> +<!ENTITY contextPeriodic.label "Periodic Trigger Runs">

Should this be "Periodically Triggered Runs", or something?

::: mailnews/base/src/periodicFilterManager.js
@@ +29,5 @@
> +// error handling function
> +function re(e) {
> +  log.reportError("periodicFilterManager error: " + e);
> +  throw e;
> +}

I'm not a big fan of these kind of helper functions.

@@ +45,5 @@
> +    {
> +      this._initialized = true;
> +      Services.obs.addObserver(this, "mail-startup-done", false);
> +    }
> +  } catch (e) {re(e);}},

what kind of exceptions are you expecting?
Running everything inside try-catch for no reason should definitely be avoided.

::: suite/locales/en-US/chrome/mailnews/folderProps.dtd
@@ +37,5 @@
>  
>  <!ENTITY folderSynchronizationTab.label          "Synchronization">
>  <!ENTITY folderCheckForNewMessages2.label        "When getting new messages for this account, always check this folder">
>  <!ENTITY folderCheckForNewMessages2.accesskey    "c">
> +<!ENTITY folderApplyPeriodicFilters.label        "Apply periodic filters to messages in this folder">

If we're gonna expose filters in the folder properties, could we make it "Apply applicable filters ...." ?

Comment 16

[Kent James (:rkent)]

1) 'Should this be "Periodically Triggered Runs", or something?'?

The phrase completes the sentence "Apply filter when ..." I have to admit that I struggled to find something that worked well here. But I don't see how your grammar works. "Periodically Triggered" is either an adjectival phrase, forcing "Runs" to be a noun (which makes no sense. Also "The runs" means diarrhea), or is a passive construction with "Triggered" the verb, leaving "Runs" as invalid. In my version "Periodic" is an adjective, "Trigger" is a noun, and "Runs" is a verb, so this is a valid clause.

You could use simply "Periodically Triggered", but that makes me ask the question "Triggered by what? Is that done manually?" I think my phrase is less ambiguous, though I don't love it.

"I'm not a big fan of these kind of helper functions." Why not? In centralizes control of standard error reporting, and plays the same role as NS_WARNING() in C++ code

"what kind of exceptions are you expecting? Running everything inside try-catch for no reason should definitely be avoided."

This is done primarily to catch errors that I am NOT expecting, not those that I am. It is no different from the default for non-module code, where any js error generates a message to the error console. I have repeatedly been burned in module development by the paucity of error reporting in modules, finding code that silently fails to work because of unreported errors in modules. What do you have against routine standardized handling of errors, particularly when this just duplicates what is available in non-module code?

'If we're gonna expose filters in the folder properties, could we make it "Apply applicable filters ...." ?'

Just to clear of what is going on here, periodic filters are likely to be resource intensive, so we do not want to apply them to all folders, but only those to which the user has specifically enabled them. The folder properties is where this is done. From a technical writing perspective, you need a word to link this enabling in the folder properties with the similar enabling in the filter editor. I'm using the word "Periodic" to do that. I don't see how it makes sense to use "Applicable" in one place, and "Periodic" in the other. I don't see how "Applicable" makes sense in any way.

Comment 17

[Magnus Melin [:mkmelin]]

(In reply to Kent James (:rkent) from comment #16)
> You could use simply "Periodically Triggered", but that makes me ask the
> question "Triggered by what? Is that done manually?" I think my phrase is
> less ambiguous, though I don't love it.

Sorry, I didn't actually compile and test yet, so my suggestion was a bit off. If we don't care about it being a gramatically correct sentence it could simply be "Periodically".

> "I'm not a big fan of these kind of helper functions." Why not? In
> centralizes control of standard error reporting, and plays the same role as
> NS_WARNING() in C++ code

Well for one thing the exception you'd see in the error console would lead you to the line number of where the *helper function* threw... (yes, with effort you get the original line too). 

I don't think you can compare it to NS_WARNING, which would report exactly what line the problem occurred. That is also a standard macro used everywhere, so you need to know what it is. For someone unfamiliar with the code a local helper is just more info to keep in mind.

> "what kind of exceptions are you expecting? Running everything inside
> try-catch for no reason should definitely be avoided."
> 
> This is done primarily to catch errors that I am NOT expecting, not those
> that I am. It is no different from the default for non-module code, where
> any js error generates a message to the error console. I have repeatedly

I'd really expect them to be reported..!? 

Generally running everything in try-catch could hide really strange errors. Of course in this case you're re-throwing the errors. 
Try-catch also have some performance hit - i didn't really find good numbers for javascript, but at least someone made this very small test: http://jsfiddle.net/Mct5N/

> 'If we're gonna expose filters in the folder properties, could we make it
> "Apply applicable filters ...." ?'
> 
> Just to clear of what is going on here, periodic filters are likely to be
> resource intensive, so we do not want to apply them to all folders, but only
> those to which the user has specifically enabled them. The folder properties
> is where this is done. From a technical writing perspective, you need a word
> to link this enabling in the folder properties with the similar enabling in
> the filter editor. I'm using the word "Periodic" to do that. I don't see how
> it makes sense to use "Applicable" in one place, and "Periodic" in the
> other. I don't see how "Applicable" makes sense in any way.

I was hoping to get the Incoming filters to apply to other folders too, with the same checkbox (now or in the future, isn't there even an existing inherited folder prop for it?). Maybe applicable is redundant though.

Comment 18

[Kent James (:rkent)]

"I was hoping to get the Incoming filters to apply to other folders too, with the same checkbox (now or in the future, isn't there even an existing inherited folder prop for it?). Maybe applicable is redundant though."

Applying incoming filters is really a separate issue than running filters periodically, so they cannot be combined into a standard checkbox. FiltaQuilla adds this filter property, but perhaps it should be in core as well.

Personally I like inherited properties, but I seem to be the only one who ever adds them to code, so I feel like I have lost that battle. We don't have decent UI to manage inherited properties unfortunately. I've posted an image of what the "Apply incoming Filters" inherited UI looks like from FiltaQuilla as an example. I'd be happy to make Periodic Filter inherited if anyone wanted.

'If we don't care about it being a gramatically correct sentence it could simply be "Periodically".'

I actually started with something like that, I think I used "Periodic" instead. I don't have a strong opinion, but I think I like the original version better.

"Well for one thing the exception you'd see in the error console would lead you to the line number of where the *helper function* threw... (yes, with effort you get the original line too) ... I'd really expect them to be reported..!?  "
 
You are correct that the line number gets hidden, which is less than optimal. I'm actually not sure of what exactly causes the module reporting to fail in some cases, but I am pretty sure that the notify from a module timer will just fail silently without explicit error reporting.

In my own code, my re() function is more complex, where I unwind the entire js stack and report it (this is the string that is generated from an error, and re() reports the string):

function se(error) // string error
{
  let jsFrame;
  let str = 'javascript error: ' + error + '\n';
  if (error)
  {
    str += ' file: ' + error.fileName + ' line: ' + error.lineNumber + ' \n';
    jsFrame = error.stack;
  }
  if (!jsFrame)
    jsFrame = Components.stack;
  while (jsFrame)
  {
    let addString = jsFrame.toString() + ' \n';
    // skip re() and se() calls
    if ( (addString.indexOf(":: se ::") == -1) &&
         (addString.indexOf(":: re ::") == -1))
      str += addString;
    jsFrame = jsFrame.caller;
  }
  return str;
}

But clearly the issue of error reporting from modules is a non-trivial issue that is poorly handled currently in mozilla code. I feel pretty strongly that module errors should be reported somehow (which they are not by default as I understand it). What would you suggest?

Comment 19

[Kent James (:rkent)]

Attachment: Inherited property UI from FiltaQuilla

Comment 20

[Magnus Melin [:mkmelin]]

AFAIKT, module errors are supposed to be reported, if you have some test case i'm sure we could try to fix the case where they aren't. (I found bug 564293, but that's going nowhere since he can't reproduce.) I performed a small test and at least very basic syntax errors, and errors in a timeout where reported as expected.

Re inherited folder props, how about an UI like this:

 Enable Filters for this Folder:
  [o] If Enabled for Parent Folder [ ] Yes [ ] No

This is intentionally not showing what the parent value is, as i think thats' what makes things difficult to understand.

I wouldn't mind doing it as an inherited folder prop, but i don't feel strongly it should be either.

> Applying incoming filters is really a separate issue than running filters periodically, 
> so they cannot be combined into a standard checkbox.

Why is it (UI wise) a separate issue? Filters know the context whey they should apply to.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 742030 [details] [diff] [review]
with suite support

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

Getting this of my review queue. minusing as i think there's stuff to sort out yet, per previous comments

Comment 22

[Kent James (:rkent)]

Just as a FYI I have been ignoring this until we get the review for bug 695671 landed. Then I will return and sort out the review issues.

Comment 23

[Kent James (:rkent)]

Attachment: Review fixes

I've incorporated requested changes from the reviews.

Except: I cannot see combining the folder prop for incoming filters and periodic filters. Example of why: if a server has both periodic and incoming filters, then you might want the incoming filter to apply to a high-volume list with a server side filter (like a mailing list), but not have the periodic filter apply to that folder (since it is resource intensive). Filter contexts are only set per server, so you need a folder property to tell whether a particular filter property should run or not.

I would really like to get this into TB 24 ASAP because of the string.

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 764843 [details] [diff] [review]
Review fixes

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

Overall, looks great! Just a few nits. 
One thing that struck me testing this was that i (of course) needed to go and enable periodic filters for the folder i wanted it applied to. Ok, i know it now, but it wasn't obvious and perhaps there should be some kind of notice popping up somehwere telling you that you need to do that.

BTW, you should get ui-r as well

::: mailnews/base/src/periodicFilterManager.js
@@ +8,5 @@
> + *   The only external call required for this is onLoad(). This should be
> + *   called before the mail-startup-done notification.
> + */
> +
> +const EXPORTED_SYMBOLS = ['PeriodicFilterManager'];

tiny nit: prefer " for consistency

@@ +40,5 @@
> +      this._initialized = true;
> +      Services.obs.addObserver(this, "mail-startup-done", false);
> +    }
> +  },
> +    

nit: trailing spacees

@@ +50,5 @@
> +    let servers = MailServices.accounts.allServers;
> +    for each (let server in fixIterator(servers, Ci.nsIMsgIncomingServer))
> +    {
> +      let nowTime = Date.now();
> +      // Make sure that the last filter time of all servers was in the past

nit: period at end

@@ +53,5 @@
> +      let nowTime = Date.now();
> +      // Make sure that the last filter time of all servers was in the past
> +      let lastFilterTime = server.getIntValue("lastFilterTime");
> +      let serverRateSeconds = server.getIntValue("periodicFilterRateSeconds");
> +      let realRateSeconds = serverRateSeconds < 30 ? this._defaultFilterRateSeconds : serverRateSeconds;

[personally i prefer parenthesis for ternary expressions]

Comment 25

[Kent James (:rkent)]

"perhaps there should be some kind of notice popping up" Let me suggest, in the interest of not creating a new string issue, that we enable by default the periodic filter for the inbox, in a followup bug. That can be done post string freeze. It has the advantage that it also matches the behavior of incomingFilters, which also are enabled by default only on the inbox. I would guess that a majority of use would be on the inbox anyway.

Also posting images for the UI review.

Comment 26

[Kent James (:rkent)]

Attachment: Filter editor image

Comment 27

[Kent James (:rkent)]

Attachment: Folder properties image

Comment 28

[:aceman]

Nice. I see that in comment 18 you mention also UI for "apply incoming filters to this folder". I think if the periodic ones (an advanced feature) gets into core, the incoming ones should too (or they can depend on the same folder property). Yes there is a separate bug for it.

Comment 29

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

Comment on attachment 764843 [details] [diff] [review]
Review fixes

So, after a long conversation in IRC, I'm going to ui-r- this.

The current implementation is confusing (and this is _totally_ not rkent's fault!) because I expect the folders that a filter runs on to be part of the filter, not part of the folder, and while it sounds like it will be a lot of work to change that, I really think that allowing us to have, say, several periodic filters running on distinct sets of folders is worth the wait.

Finally, as I said in IRC, I would totally support us promoting FiltaQuilla a little more for people who want periodic filters (or even more advanced filters in general).

Thanks,
Blake.

Comment 30

[Ian Neal]

Comment on attachment 764843 [details] [diff] [review]
Review fixes

>+++ b/mail/base/content/msgMail3PaneWindow.js
>+  // Load the periodic filter timer
Nit: Comment should end with a full stop/period if starting with a capital letter.

>+++ b/mailnews/base/public/nsMsgFolderFlags.idl
>-[scriptable,uuid(fbe7cba8-3141-4c44-9660-99af6b53f27e)]
>+[scriptable,uuid(fbe7cba8-3141-4c44-9660-99af6b53f27f)]
I'm sure uuids usually change by more than single character (possibly to emphasis they have changed, not sure).

>+++ b/mailnews/base/src/Makefile.in
>@@ -88,16 +88,17 @@ EXTRA_COMPONENTS = \
> 		nsMailNewsCommandLineHandler.js \
> 		msgAsyncPrompter.js \
> 		newMailNotificationService.js \
> 		msgBase.manifest \
> 		$(NULL)
> 
> EXTRA_JS_MODULES = \
> 		virtualFolderWrapper.js \
>+		periodicFilterManager.js \
This has been bitrotted and should now be in the equivalent moz.build

>+++ b/suite/mailnews/msgMail3PaneWindow.js

>+  // Load the periodic filter timer
Nit: as per first review comment.

The Filter Rules dialog (with the Archiving patch applied too) is looking a bit top heavy and it would be nice to expose the periodicFilterRateSeconds preferences but that could be a follow up bug so r=me

Comment 31

[Ben Bucksch (:BenB)]

Comment on attachment 764843 [details] [diff] [review]
Review fixes

bwinton, please reconsider.

Bug 11039 comment 99 is disheartening. We have very few contributors left, so we need to appreciate the work of those that are left.

If the patch is a strict improvement over the state before, I think it should be taken, even if it's not the perfect solution. What we witness here is a classical contributor burnout. We cannot affort that anymore, we must not lose contributors like that anymore.

Looking at the screenshot from attachment 765059 [details], the new UI is a lot better than it is now. The current dropdown is confusing, the checkboxen make a lot more sense. (I would arrange them horizontally, though, because they take too much space on the screenshot.) I think you should take the patch just for that.

What's more, filters on outgoing messages (bug 11039) are painfully missing and part of the useless time I spent every day and the computer chore that increases my stress level and distracts my thoughts from important things. In short: bad UI. It really needs a fix, and this bug here is at least a workaround. So, it's important form that angle, too.

The patch has 2 reviews, which are hard enough to come by. I don't know why the UI review was denied, comment 29 doesn't say anything substantive, but the UI in the screenshots are a clear *improvement* over current state and thus should be allowed in.

Please reconsider, both for the best of the product and for the best of the community.

Comment 32

[Ben Bucksch (:BenB)]

> We have very few contributors left, so we need to appreciate the work of those that are left.

Actually: We always need to appreciate contributors, no matter how many we have. But now, it's a matter of pure survival.

I've shortly looked at the patch in attachment 764843 [details] [diff] [review], and it's surprisingly small for a feature like that, and nicely componentized, so good code design, too. I personally don't like much the idea of a periodic action for anything, I'd much rather trigger on Sent mail, but I assume rkent tried that first and failed in the depths of nsMsgCompose.cpp.

Comment 33

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

Comment on attachment 764843 [details] [diff] [review]
Review fixes

Yeah, a lot of the details for the rejection were in the long irc conversation, which I think might have been a mistake.

The main problem with the patch is that there's no way to select folders or accounts to run it on.  (Or, rather, you can only have one set of folders to run periodic filters on.)  So in your case, if you've got more than one account, and wanted to have two different periodic filters on the sent messages, you'ld be out of luck.  Or if you wanted to have a periodic outgoing filter, and a different periodic incoming filter, again you'ld be out of luck.  So, to get a ui-r+, we would need to add some sort of way to choose which folders the filter applies to.

Finally, while I believe rkent knows how much I appreciate the work he does, I think he would also agree that that doesn't mean that we should accept any patch that anyone wants to write, for that way lies an entirely different sort of madness.

Comment 34

[Ben Bucksch (:BenB)]

> if you've got more than one account, and wanted to have two different periodic filters on the sent
> messages, you'ld be out of luck.  Or if you wanted to have a periodic outgoing filter, and a
> different periodic incoming filter, again you'ld be out of luck.  So, to get a ui-r+, we would
> need to add some sort of way to choose which folders the filter applies to.

Ah, thanks for the explanation. That I can understand.

Comment 35

[Ian Neal]

Just a note, this patch has bitrotted further.

Comment 36

[Murat]

Is this resolved by now and which TB version will it be integrated in ?
I'm on TB 24.1.0 on a MAC OS X 10.8.5 and seeing the same annoying behaviour.

Comment 37

[Kent James (:rkent)]

This bug did not get accepted without requiring a fairly extensive rework of the patch, to add some capability to set individual filters per-folder (see bug 294632). So that patch did not land, and there are no current plans to work on it.

I still think this concept is worth doing, and I still think that the proposed solution to the per-folder enabling problem (to turn on or off periodic filters on specific folders) would have been an improvement over doing nothing.

Comment 38

[opera wang]

It might be better to add finish notification for MailServices.filters.applyFiltersToFolders, so you can know the current run was done and can safely kicks off next one.

Comment 39

[:aceman]

(In reply to Kent James (:rkent) from comment #37)
> I still think this concept is worth doing, and I still think that the
> proposed solution to the per-folder enabling problem (to turn on or off
> periodic filters on specific folders) would have been an improvement over
> doing nothing.

I agree here. The feature is disabled by default and user has to choose which folders the filters will apply to and whether he is OK with the "problem" that ALL the periodic filters will apply onto that folder.

While this does not cover all wished use cases, it covers some. So there should be no harm in enabling it.

Comment 40

[:aceman]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #33)
> The main problem with the patch is that there's no way to select folders or
> accounts to run it on.  (Or, rather, you can only have one set of folders to
> run periodic filters on.)  So in your case, if you've got more than one
> account, and wanted to have two different periodic filters on the sent
> messages, you'ld be out of luck.

(In reply to Ben Bucksch (:BenB) from comment #34)
> > if you've got more than one account, and wanted to have two different periodic filters on the sent
> > messages, you'ld be out of luck.  Or if you wanted to have a periodic outgoing filter, and a
> > different periodic incoming filter, again you'ld be out of luck.  So, to get a ui-r+, we would
> > need to add some sort of way to choose which folders the filter applies to.
> 
> Ah, thanks for the explanation. That I can understand.

Wait, filters are normally defined per server/account.
Does the patch change this current expectation? I need to check.
But even if, it should be doable to only run filters confined to their account, inside PeriodicFilterManager.notify().

Comment 42

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #32)

> I've shortly looked at the patch in attachment 764843 [details] [diff] [review]
> [review], and it's surprisingly small for a feature like that, and nicely
> componentized, so good code design, too.

I agree, the patch is nice and simple, we should really revive it.

> I'd much rather trigger on Sent
> mail, but I assume rkent tried that first and failed in the depths of
> nsMsgCompose.cpp.

Outgoing filters are already implemented too.

Comment 43

[:aceman]

Guys, can we think about this again? It may not be perfect and does not cover all use cases (like Blake describes), but it should cover some users, e.g. running periodically on Inbox and potentially some other chosen folders is doable with this (which should be the main use of this). Each account has its own set of periodic filters. Sent folder will be excluded by default.

Will there be any harm in landing this feature?

I'm willing to revive the patch, unbitrot it, address review comments and polish it some more.

Comment 44

[Wayne Mery (:wsmwk)]

I don't see a problem with proceeding

Comment 45

[Ben Bucksch (:BenB)]

I'm also in favor of this incremental improvement.

Comment 46

[Ben Bucksch (:BenB)]

See comments 31-34.

Comment 47

[:aceman]

Comment on attachment 764843 [details] [diff] [review]
Review fixes

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

::: mailnews/base/src/periodicFilterManager.js
@@ +83,5 @@
> +      server.setIntValue("lastFilterTime", nowTime);
> +      let foldersToFilter = server.rootFolder.getFoldersWithFlags(Ci.nsMsgFolderFlags.ApplyPeriodicFilters);
> +      if (!foldersToFilter.length)
> +        continue;
> +      MailServices.filters.applyFiltersToFolders(server.getFilterList(null), foldersToFilter, null);

Actually this line seems to run ALL filters of that server (not only those of Periodic type) and even disabled ones. applyFiltersToFolders() just runs all the passed in filters in the list. At least today. Kent, can you please confirm? I will fix this.

Comment 48

[:aceman]

Attachment: 864187.patch v2

This is a refreshed patch (almost all hunks didn't apply). Addressed the review comments from Magnus and Ian. Updated to current syntax and the new .jsm file even passes eslint.
I fixed the logic problem with running all filters, now it runs only enabled filters of the Periodic type. Another problem was that nsIMsgIncomingServer.setIntValue (getIntValue) only handle int32 values, thus storing Date.now() overflowed the value, causing the nextFilterTime always be in the past, thus filters were run at each check (1-minute timer), instead of the 10 minutes set in mail.server.default.periodicFilterRateSeconds . So the '/ 1000' in the new patch is to store seconds instead of milliseconds, which do fit into int32. It will overflow in year 2038. If that is a problem we can make it / 10000, thus counting in 10-second granularity which would still be enough for this use case (we do not want the filters run too often and with seconds precision). We could even store minutes (/ 60000) because the check still happens in one minute intervals so setting e.g. 90 seconds will not be honored as precisely. We could then switch the pref to also contain minutes.

Another change I made is I stripped the folder flag to set which folders the periodic filters apply to, as this is subject to further discussion and possibly different implementation. I hard-coded that the periodic filters only run on Inbox and it is clearly displayed to the user. In this way it does not yet cover all user use-cases, but one clearly communicated one, where the user can choose if he wants this or not without surprises.

I suggest we could land this confined to Inbox to also see the perf impact for users actually enabling the periodic filters.
I can look at per-filter selector of folders to apply to in a followup bug.

Comments welcome :)

Comment 49

[Richard Marti (:Paenglab)]

Comment on attachment 9058154 [details] [diff] [review]
864187.patch v2

The patch doesn't apply on nsMsgFilterService.cpp. Is a other patch needed?

Comment 50

[:aceman]

Yes, that hunk applies onto bug 697522, but you can also ignore it (not apply that part), it is not important for seeing the periodic filters feature.

Comment 51

[Richard Marti (:Paenglab)]

Comment on attachment 9058154 [details] [diff] [review]
864187.patch v2

Looks good for the actual state. Maybe the time of the interval can be added at Periodically (on Inbox)" to show the user what the filter frequency is.

Because the frequency is globally set, later could be a setting added on the main filter dialog to set the filter period.

Comment 52

[:aceman]

Attachment: 864187.patch v2.1

Better? :)
I also reworked the sanitization of the run period so that user can't set a period below 60 seconds (via prefs for now) and bring TB to its knees.

Comment 53

[Richard Marti (:Paenglab)]

Comment on attachment 9059872 [details] [diff] [review]
864187.patch v2.1

You are planning that the interval is set for each filter individually? If no, then the textbox should be hidden and only shown as text. With the textbox shown the user thinks he can set the interval there.

Comment 54

[:aceman]

Attachment: 864187.patch v2.2

OK, fixed the interval display.
Please still note comment 48 as it needs deciding.

Comment 55

[:aceman]

This patch contains strings, can we get it finished in time for TB68?

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 9061183 [details] [diff] [review]
864187.patch v2.2

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

Many of the hunks won't apply

::: mail/base/content/msgMail3PaneWindow.js
@@ +525,5 @@
>  
>    window.addEventListener("AppCommand", HandleAppCommandEvent, true);
> +
> +  // Load the periodic filter timer.
> +  PeriodicFilterManager.onLoad();

why is this (all) inside the PopupNOoifications lazy getter? Are we sure the notification it's going to listen to didn't already happen?

::: mail/locales/en-US/chrome/messenger/FilterEditor.dtd
@@ +26,5 @@
>  <!ENTITY contextOutgoing.label "After Sending">
>  <!ENTITY contextOutgoing.accesskey "S">
>  <!ENTITY contextArchive.label "Archiving">
>  <!ENTITY contextArchive.accesskey "A">
> +<!ENTITY contextPeriodic.label "Periodically (on Inbox)">

we can't have it for other folders?

::: mail/locales/en-US/chrome/messenger/filter.properties
@@ +28,5 @@
>  # %S=the name of the filter that is being copied
>  copyToNewFilterName=Copy of %S
> +# LOCALIZATION NOTE(periodLength)
> +# %S=the number of seconds
> +periodLength=Interval: %S seconds

I would think this should be minutes in the UI

::: mailnews/base/search/src/PeriodicFilterManager.jsm
@@ +18,5 @@
> +
> +var log = Log4Moz.getConfiguredLogger("mail.periodicFilterManager",
> +                                       Log4Moz.Level.Warn,
> +                                       Log4Moz.Level.Warn,
> +                                       Log4Moz.Level.Warn);

all these should use const

@@ +59,5 @@
> +    Services.obs.addObserver(this, "quit-application");
> +  },
> +
> +  // periodic callback
> +  notify(aTimer) {

timer

@@ +95,5 @@
> +          newFilterIndex++;
> +        }
> +      }
> +      log.debug("PeriodicFilterManager apply periodic filters to server " + server.prettyName);
> +      if (newFilterIndex > 0)

what's this check for?

@@ +120,5 @@
> +
> +    return serverRateSeconds;
> +  },
> +
> +  observe(aSubject, aTopic, aData) {

and please also for these no a-notation

Comment 57

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #56)

Comment on attachment 9061183 [details] [diff] [review]
why is this (all) inside the PopupNOoifications lazy getter? Are we sure the
notification it's going to listen to didn't already happen?

May that be a false impression from the patch? This is called from onLoadMessenger() which does not seem related to PopupNotifications. See https://searchfox.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#524 .

::: mail/locales/en-US/chrome/messenger/FilterEditor.dtd

+<!ENTITY contextPeriodic.label "Periodically (on Inbox)">

we can't have it for other folders?

You would need to read the other 55 comments ;)
In short: yes we want this for other folders too, but nobody could agree on a way how to choose those other folders. To just run them on all folders by default wasn't deemed safe as there may be different filters needed for e.g. Inbox vs. Sent. Kent's version had a method to choose the folders (but globally for all filters) via "folder properties" (right click a folder), which is a bit obscure and maybe does not scale too (does not automatically apply to subfolders when they are created).

So that's where the patch stalled and of course Kent can't play with it any longer.

So my proposal to solve this dead-lock is to only run the filters on Inbox as first stage.
Then in a followup bug we can flesh out a method and UI about how the user can specify the folders for each filter.

I still need you to decide comment 48 (the year 2038 problem).

@@ +95,5 @@

•      newFilterIndex++;
  

•    }
  

•  }
  

•  log.debug("PeriodicFilterManager apply periodic filters to server " + server.prettyName);
  

•  if (newFilterIndex > 0)
  

what's this check for?

A micro-optimization to not even call MailServices.filters.applyFiltersToFolders if we have no filters to run. Of course MailServices.filters.applyFiltersToFolders can bail out if there are no filters, so I can drop this redundant check if you want.

Comment 58

[Magnus Melin [:mkmelin]]

I think the label should't still say anything about inbox...
For comment 48, let's just store minutes which will anyway be the usable unit there.
For the micro-optimization, yeah maybe not do that - we don't elsewhere in the similar situation either.

Comment 59

[Ben Bucksch (:BenB)]

So my proposal to solve this dead-lock is to only run the filters on Inbox as first stage.

I think that's reasonable.

It's on Inbox where we need it, because new mail arrives there by definition.
Mail arrives in other folders only
a) with server-side filters, but then we combine server-side filtering with TB client-side filtering? That's really an edge case.
b) after manually moving it, after which filters should probably not apply.

So, while you can certainly construct cases where one might want it on other folders, they are really edge cases.
And, like aceman said, it makes the UI really complicated.

And I think this is a fair first step.

Please just remove the brackets in the label: "Periodically on Inbox"

Comment 60

[Ben Bucksch (:BenB)]

(Or remove " on Inbox" entirely, because I think it's implied - normal filters also apply on Inbox, actually.)

Comment 61

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #59)

b) after manually moving it, after which filters should probably not apply.

That's questionable. We have manually run filters so user may want to cleanup his folders once in a while. Running them automatically would be a help for him. Also I have heard cases where user would move a message into a folder (e.g. Inbox) and then expect the message is filtered (even to another folder).

(In reply to Ben Bucksch (:BenB) from comment #60)

(Or remove " on Inbox" entirely, because I think it's implied - normal filters also apply on Inbox, actually.)
Some apply on Sent when you check the right box.

Comment 62

[Ben Bucksch (:BenB)]

Some apply on Sent when you check the right box.

Right, but if you don't, they apply only on Inbox. So, "Inbox only" is actually implied for most existing filters, without explicitly saying so in the UI.
I'm just saying that it's nothing new or outstanding.

Comment 63

[:aceman]

Attachment: 864187.patch v2.3

Refreshed the patch and addressed the comments.

It seems now after the filter editor lost the <grid> elements, the second column of items does not align as good vertically to the items in the first column.

Comment 64

[Magnus Melin [:mkmelin]]

Comment on attachment 9065243 [details] [diff] [review]
864187.patch v2.3

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

Other than my suggestions below, LGTM. r=mkmelin

::: mail/base/content/msgMail3PaneWindow.js
@@ +523,5 @@
>  
>    window.addEventListener("AppCommand", HandleAppCommandEvent, true);
> +
> +  // Load the periodic filter timer.
> +  PeriodicFilterManager.onLoad();

we could make this something less generic. Perhaps .setupFiltering()

::: mail/locales/en-US/chrome/messenger/filter.properties
@@ +30,5 @@
>  # %S=the name of the filter that is being copied
>  copyToNewFilterName=Copy of %S
> +# LOCALIZATION NOTE(periodLength)
> +# %S=the number of minutes
> +periodLength=Interval: #1 minute;Interval: #1 minutes

I think instead of having this label separately it should just be put into the Periodically main label, as


[ x ] Periodically, every 10 minutes

Comment 65

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #64)

+periodLength=Interval: #1 minute;Interval: #1 minutes

I think instead of having this label separately it should just be put into
the Periodically main label, as

[ x ] Periodically, every 10 minutes

Note the checkboxes are still in a tabular grid (when though without a real <grid>) thus this would shift the second column, with the "before/after classification" menulist, which would then be more separated from the "getting new mail" label with which it kinda composes a sentence.
We could remove the grid layout, it does not seem very needed here.

Comment 66

[Richard Marti (:Paenglab)]

Comment on attachment 9065243 [details] [diff] [review]
864187.patch v2.3

Looks good. I add a patch for the XUL rearrange and your patch updated to apply after my patch.

Comment 67

[Richard Marti (:Paenglab)]

Attachment: 864187-XUL-fix.patch

This patch simplifys the header part. The two columns aren't needed (also the grid was never needed here).

Comment 68

[Richard Marti (:Paenglab)]

Attachment: 864187-b.patch v2.3.1

Aceman's patch updated to apply on top of my patch. No string changes.

Comment 69

[:aceman]

Attachment: 864187.patch v2.4

Thanks for the update. The layout works nicely.

Comment 70

[Magnus Melin [:mkmelin]]

Comment on attachment 9065529 [details] [diff] [review]
864187.patch v2.4

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

::: mail/locales/en-US/chrome/messenger/filter.properties
@@ +30,5 @@
>  # %S=the name of the filter that is being copied
>  copyToNewFilterName=Copy of %S
> +# LOCALIZATION NOTE(contextPeriodic.label): Semi-colon list of plural forms.
> +# #1=the number of minutes
> +contextPeriodic.label=Periodically, every #1 minute;Periodically, every #1 minutes

the first for should be 
Periodically, every minute;

Comment 71

[:aceman]

Attachment: 864187.patch v2.4.1

Nice, thanks.

Comment 72

[:aceman]

Attachment: 864187.patch v2.4.2

Fixed commit message.

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0c6818af6df7
Remove the unneeded second vbox in the first groupbox of the filter editor. r=mkmelin
https://hg.mozilla.org/comm-central/rev/686c2fa1e7cb
Add possibility to run filters periodically (most of code by rkent, tweaked by aceman). r=mkmelin,ui-r=Paenglab DONTBUILD

Comment 74

[Jorg K (CEST = GMT+2)]

That somehow missed TB 24, see comment #23 ;-)

Comment 75

[Axel Grude [:realRaven]]

aceman - thanks for implementing this - I will add a UI to my Add-on quickFilters, if that helps! :)

Comment 76

[:aceman]

I have tested the performance of this on a 4GB folder with 1 million of messages. I tried a filter that had a rule that matched various numbers of messages (tried 800000 and also 50000) with having a single action of "Mark as unread" (while all messages were read before).

The filter has to check every message if it matches at every run and then applies the action to all of the matching ones.
The new filter logging has visualized that the filter engine was applying the action to all those 50000 messages at each run (whether the action had any real effect or not). The log grew to hundreds of MBs :)

The results are that the time it takes to process a folder (run a filter on all the messages in it) is strictly dependent on the number of messages that are changed by the actions, NOT by the number of messages neding to be checked.

So if those messages were Read and it really had to change the flag to Unread and store that to the mbox file and the msf database, that took like 20 minutes (which is even longer than the 10 minute interval between the periodic filters, but TB was stuck unresponsive in that time and it does not seem that a new filter run was started while the first one was running). But TB did NOT take any noticeable amounts of additional RAM while executing the filters.

If those matched messages were already Unread so that the filter action didn't really do any change to them, the execution was very fast, within seconds and mostly unnoticeable in process monitor. A user actively using TB in that time could maybe spot the hang for a moment, but such a big folder causes small hangs all over the TB UI so it is not making things worse.

So it is great the c++ filter engine is so efficient and the actions are actually checked if they result in any change to msg headers (at least the Read/Unread transition, and we could optimize others if it is found some action does not do this) and the case of not hitting the database is very quick. As that is the expected common case for these periodic filters. They will run on a folder often, but rarely will they change any message, even if they match more of them. They are to catch some one odd message that was added to the folder between runs (maybe manually) and hadn't had the actions applied to it yet. Or to move messages older than some time away from the folder. And similar use cases.

Only problem was, the test was done on a machine with heaps of RAM and Ryzen CPU so the whole 4GB folder and it's msf (300MB) and the filter log file (300MB) could be kept in memory and only written to disk later by the OS when TB was already back to responding to user. On the other hand, it was a debug build, which is much slower than a release one.
Results on a slower machine may vary. Hopefully, users with ~4GB of RAM will have smaller folders and less messages in them as they will already face problems elsewhere.

Comment 77

[:aceman]

(In reply to :aceman from comment #57)

•  log.debug("PeriodicFilterManager apply periodic filters to server " + server.prettyName);
  

•  if (newFilterIndex > 0)
  

what's this check for?
A micro-optimization to not even call MailServices.filters.applyFiltersToFolders if we have no filters to run. Of course >MailServices.filters.applyFiltersToFolders can bail out if there are no filters, so I can drop this redundant check if you want.

Sadly, this optimization wasn't aggressive enough.
Before we enumerate the periodic filters to run (if any) we first lookup the Inbox folder of each server with the line:
let foldersToFilter = server.rootFolder.getFoldersWithFlags(Ci.nsMsgFolderFlags.Inbox);
This adds those Inbox folders to the folder cache, which is quite small (30 folders by default). If you have many accounts (like I have 20 in test profile), even some "dead" ones, for which no mail is fetched or no filters defined, this may push out useful folders out of the cache.
I have filed bug 1554424 for this.

Comment 78

[Axel Grude [:realRaven]]

(In reply to :aceman from comment #77)

(In reply to :aceman from comment #57)

•  log.debug("PeriodicFilterManager apply periodic filters to server " + server.prettyName);
  

•  if (newFilterIndex > 0)
  

what's this check for?
A micro-optimization to not even call MailServices.filters.applyFiltersToFolders if we have no filters to run. Of course >MailServices.filters.applyFiltersToFolders can bail out if there are no filters, so I can drop this redundant check if you want.

Sadly, this optimization wasn't aggressive enough.
Before we enumerate the periodic filters to run (if any) we first lookup the Inbox folder of each server with the line:
let foldersToFilter = server.rootFolder.getFoldersWithFlags(Ci.nsMsgFolderFlags.Inbox);
This adds those Inbox folders to the folder cache, which is quite small (30 folders by default). If you have many accounts (like I have 20 in test profile), even some "dead" ones, for which no mail is fetched or no filters defined, this may push out useful folders out of the cache.
I have filed bug 1554424 for this.

We have to be careful if we are planning to exclude inactive accounts. It is possible to use inactive accounts (as I do) as "Local Folders" - meaning I have a number of old accounts with "dead" email addresses that are configured to not get mail from the servers - but we can set up filters to redirect mail from "active" Inboxes to these inactive ones, and then have cascading filters within the "dead" account.

So my question would be - how does the algorithm distinguish between accounts to determine they are "dead"? On the other hand, I don't know whether periodic running is necessary on a dead account when we use "cascaded" filters that react to (potentially read) emails that were moved over from "live" folders?

Comment 79

[:aceman]

(In reply to Axel Grude [:realRaven] from comment #78)

We have to be careful if we are planning to exclude inactive accounts. It is possible to use inactive accounts (as I do) as "Local Folders" - meaning I have a number of old accounts with "dead" email addresses that are configured to not get mail from the servers - but we can set up filters to redirect mail from "active" Inboxes to these inactive ones, and then have cascading filters within the "dead" account.

No worries, we have no concept of 'inactive'/'dead' accounts. I only meant practically dead (which do not download from server and user only occasionally visits them), but we do not detect those in any way. Loading the Inbox of those every few minutes IF there are no periodic filters is useless and may affect other useful folders. Further comments in bug 1554424 please :)

Comment 80

[Bruce M. Axtens]

NI: I have a filter that I use on certain local folders but not on others. It deletes items that are more than 2 days old. If I say that that filter is periodic every 10 minutes, how do I limit it to a folder? What would be good is that the folder name be made available to the filter definition then I can create folder-specific periodic filters.

Comment 81

[:aceman]

Currently the periodic filters only run on Inbox folder so it is not possible to run them on other folder.
Exactly because there is no way to define the folders to run on.
What you propose is one method and there are also others so one has to be chosen and actually implemented in a new bug.