1653690 - TB 78 UX regression: applying filter(s) on a filter should not ask to continue even if a filter fails. (TB asks this only for filter that moves the message to the original folder.)

Status: RESOLVED FIXED

(Thunderbird :: Filters, defect)

Description

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

I have switched 78.0 and found a few annoying things.

One is this.

I subscribe to a large number of mailing lists and also handles some paper submission to an annual conference/symposium in cooperation with a support of a society of IEEE.
To classify all these and work/personal e-mails, I have a large number of filters.

Sometimes filters don't get executed for whatever the reason.: bugs, or I am locking a folder by doing something, etc.
In those cases when I realized something went wrong, I simply let the filters run on a folder, say, Inbox for a while to sort everything into the intended folders.

Well, it worked just fine before 78.
Unfortunately, with 78, it seems that when no message matches a filter in the list of filters, TB asks the user "No match was found. Should I continue to apply the filters?" by a modal dialog. (Sorry I am using a Japanese localized version on this Windows 10 PC and I have no idea what the English version shows there.)
That is, the application of the filter stops right there WITHOUT the user interaction.

This is BAD.

I don't know the rationale for the introduction of the checking of the failure. Maybe a badly-constructed filter not matching any message as intended.

But as you can imagine, this interferes with my intention of cleaning up misfiltered messages from a large folder unattended.
I used to be able to simply run this procedure on a large folder, say, have that 30,000 messages and leave for lunch and come back. By the time I came back, hopefully the filtering was finished. I probably have close to 100 filters :-(
But with the MODAL dialog requesting to know if I want to continue, this scenario does not work any more.

Help.

Either we get rid of this check or at least,
we should ask at the time of invoking applying filters on a folder,
"- Should we stop and ask each time a filter fails to match any message (YES or NO)?"
(No here means we apply filters till the end no matter what.)
and let someone like me to run filters without useless (to me) check till the last filter unbothered if the user answers NO.

TIA

PS: This is VERY STRANGE THOUGH.
Just a few hours ago, I did something similar on a smallish folder and let the filters run on it. It worked without any such complaints if I am not mistaken. But the list of filters includes ones that would never fire on the messages in the small filter.
Whereas, now I am running filters on my large Inbox and this check appears now and then.

Worse, the filtering seems to take much longer in 78 than in the previous version. Something is wrong.

Comment 1

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

Aha, now I have come to realize one aspect of this bug.

I am now running the filters on Inbox.

It turns out I have a few filters that sets some property like "this is not a spam" then move it AGAIN into Inbox.
(I bet the move would have been NOOP essentially.)

Now, TB complains that the filter failed ONLY on these rules which would have moved the messages , if matched, to the same Inbox folder.

A few hours ago, when I ran the filters on a smallish folder, it is a copy of mail archive copied from outside, and so it is given a new name and no filter would have tried to move a message into this new folder again. Thus the message to move a message into Inbox did not produce any check/complaint.

But now I am running a set of filters ON the INBOX to move messages onto various filter, somehow TB seems to complain when no message matches a filter that would have moved the message to the same INBOX.

Mysterious part solved.

Speed part I am not sure. I somehow assumed the interval between modal dialog's appearances was proportional to the time success filter rule being applied. But now it becomes clear that only a subset of filters that would move the message into Inbox produces the modal dialog, maybe the speed is not that bad.

Comment 2

[Wayne Mery (:wsmwk)]

duplicate?

Comment 3

[Magnus Melin [:mkmelin]]

I kind of doubt this is a regression. The message you see is likely https://searchfox.org/comm-central/rev/1129249f43acb470071711b9c25b8f3743ab8f97/mail/locales/en-US/chrome/messenger/filter.properties#20 which has been in the source "forever".

Maybe something else changed, in your configuration or in the code.

Comment 4

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

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

I kind of doubt this is a regression. The message you see is likely https://searchfox.org/comm-central/rev/1129249f43acb470071711b9c25b8f3743ab8f97/mail/locales/en-US/chrome/messenger/filter.properties#20 which has been in the source "forever".

That is my point in comment 1.
Maybe I was not clear.

It seems that in TB 78, the move of a message to the same filter seems to be handled as a failure (not sure if it is always or sometimes ).
That is why I start to see this message, I think.

Maybe something else changed, in your configuration or in the code.

I think the error code setting of message move when the originating and the target folders are the same has changed in the code.
It is set to an error value (at least sometimes).

Comment 5

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

I think the behavior is due to
https://searchfox.org/comm-central/source/mailnews/base/search/src/nsMsgFilterService.cpp#749

        case nsMsgFilterAction::CopyToFolder: {
          nsCString uri;
          curFolder->GetURI(uri);
          BREAK_ACTION_IF_FALSE(!uri.Equals(actionTargetFolderUri),   <=== This
                                "Target folder is the same as source folder");

Instead of making it an error (BREAK_ACTION_IF_FALSE), it would be neat if we can simply CONTINUE (because this is a NOOP) after possibly logging this.
I am not sure if there is an appropriate macro near the beginning of the file. If I am not mistaken, "COTINUE_..." macro seems to pop up a dialog(?).

Introduced in Bug 1547508 (?)

To reiterate, I have a large number of filters which include filters that set attribute of a matching message such as "this is not a spam" and move it into "Inbox".
Before TB78, I could apply the whole set of filters on "Inbox" without an issue. I could start the filtering and went for a cup of coffee and done with it. (My Inbox is always large and the # of filters is large.)
Now, TB78 complains due to the same identity of source/destination folder and the processing stops with modal dialog for each such filter, and until I click the dialog, the processing does not continue. Bad.

Please note that the whole set of filters can be applied to a different folder, i.e., not "Inbox", (quite likely imported from elsewhere), and in the case of newly imported folder with a newly created name, the whole filter runs uninterrupted without being stopped by unwanted dialog.
Thus, applying the same set of filters (as a whole) is a great way to import and filter new messages from elsewhere. (One reason why setting an attribute of a message and moving it "Inbox" in some filters.)

I wonder if the source line mentioned above can be modified to continue with possibly a suitable logging BUT WITHOUT the modal dialog.
If a popup is just for notification and does not block the processing to continue, I can live with it (hmm, maybe).

Comment 6

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

I feel it should be "CONTINUE..." when one applies "filters on a folder".
If we apply a single filter on a message or something, it can err to warn just in case.

I need an unattended operation for applying "filters on a (potentially big) folder".

Comment 7

[:aceman]

Right, while doing bug 1547508 I cleaned up and unified some error checking and reporting. It may be I changed the behaviour in his case.
Looks like there was a check
if (!actionTargetFolderUri.IsEmpty() && !uri.Equals(actionTargetFolderUri)) which skipped the whole action, but now it reports an error.
A no-op in this case looks correct and may have been the case before. I'll check it out.

Thanks for noticing.

Comment 8

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

(In reply to :aceman from comment #7)

Right, while doing bug 1547508 I cleaned up and unified some error checking and reporting. It may be I changed the behaviour in his case.
Looks like there was a check
if (!actionTargetFolderUri.IsEmpty() && !uri.Equals(actionTargetFolderUri)) which skipped the whole action, but now it reports an error.
A no-op in this case looks correct and may have been the case before. I'll check it out.

Thanks for noticing.

Thank you for your attention, aceman.

Comment 9

[:aceman]

Attachment: 1653690.patch

Can you please try this?

Comment 10

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

Attachment: patch was tested with this filter.

(In reply to :aceman from comment #9)

Created attachment 9167741 [details] [diff] [review]
1653690.patch

Can you please try this?

Dear Aceman,

I tried the patch with the C-C TB (fetched about a week ago.)

The patch works. It no longer complains about failed filter action.
(I am setting a non-junk status of messages with "apt-listchanges" in the subject header and move it to Inbox.

The filter moves the messages to Inbox when it is applied to a different folder and there are messages with "apt-listchanges" in the header line.

The filter executes silently (no longer shows the modal dialog about failed filter action and asks me to continue ?) when it is executed on Inbox and there are messages with "apt-listchanges" in the subject header.

Thank you (!)

Oops, I noticed a bit of anomaly in the log. I may have introduced a bug several years ago when I worked on the filter logging.
More on this in the next comment.

Comment 11

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

Attachment: Messages were marked as no spam and is left in Inbox without move. However, log says TB "moved" it. A bit confusing.

Comment 12

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

Comment on attachment 9169763 [details]
Messages were marked as no spam and is left in Inbox without move. However, log says TB "moved" it. A bit confusing.

OK, in the previous filter setting, when a message is Inbox and has "apt-listchanges" in the header, it is marked as non junk, but
since it is already in Inbox, no move takes place.

However, Filter log shows such messages are moved into Inbox.
(Which, in a warped mathematicall sense, may not be incorrect, but is quite counter-intuitive.)

So this message could be improved IMHO, but strictly speaking, this probably has been around for quite a while though I am not sure.

TIA

Comment 13

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

Comment on attachment 9167741 [details] [diff] [review]
1653690.patch

This works. Please see comment 10, but please see also comment 11, too.

Thank you.

Comment 14

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

Hmm, has bugzilla changed in terms of how one sets feedback (or how one sets that one answers a feedback request from others)?
I think I figured it out.

Comment 15

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #12)

OK, in the previous filter setting, when a message is Inbox and has "apt-listchanges" in the header, it is marked as non junk, but
since it is already in Inbox, no move takes place.

However, Filter log shows such messages are moved into Inbox.
(Which, in a warped mathematicall sense, may not be incorrect, but is quite counter-intuitive.)

If you see https://searchfox.org/comm-central/source/mailnews/search/src/nsMsgFilterService.cpp#709 we actually log the filter action to the filter log before any action takes place. It may still fail and then we log failure, otherwise we log nothing. So this would need to file a new bug to revamp the logging system. Also, we have the detailed NSPR filter logging, for interested users, which now will show that the message wasn't actually moved as the target folder equals the source one.

Comment 16

[:aceman]

Attachment: 1653690.patch v1.1

Updated patch due to changed source file paths.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9170202 [details] [diff] [review]
1653690.patch v1.1

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

LGTM, thx! r=mkmelin

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/26a89ae4447a
do not report failure if a filter tries to move a message to the same folder. r=mkmelin

Comment 19

[:aceman]

Comment on attachment 9170202 [details] [diff] [review]
1653690.patch v1.1

This needs to go to ESR after some time.

[Approval Request Comment]
Regression caused by (bug #): bug 1547508
User impact if declined: some valid filters reported as having errors
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9170202 [details] [diff] [review]
1653690.patch v1.1

[Triage Comment]
Approved for esr78

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/35643778c5e7

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0fb742aa4eb2
followup - clang-format. rs=clang-format DONTBUILD