127250 - "Body" filter for IMAP messages downloaded for offline use.

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Joel Thornton]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.8+) Gecko/20020221
BuildID:    2002022103

Creating a "Body" condition in any filter, saving the filter, and then restarting Mozilla will change your condition into a "Subject" condition with the 'verb' unset (contains/is/..). It seems the Body condition doesn't stick.

Reproducible: Always

Steps to Reproduce:
1. Create a new message filter. 
2. Set filter to "Body" "contains". Put some text in the text box.
3. Exit MailNews and any open Mozilla windows.
4. Restart MailNews.
5. Take a look at your new filter.

Re-setting the filter to its correct setting and then restarting yields the same results.

Comment 1

[laurel]

It works for me using feb21 commercial trunk. (POP) Body filter created and
remained through exit/restart, did not reset to subject contains. 

Anything else you can tell us about your scenario which might help in
reproducing? Do you have problems with any other filter criteria resetting?
Do you have problems with filters in general saving?
What kind of account is this?

Comment 2

[Joel Thornton]

I am set up with only one IMAP server on the mailserver side, and 3 NNTP servers on the news side.  I'll try doing a fresh install of Mozilla shortly and report results.

Comment 3

[laurel]

Body is a filter criteria only for POP accounts. I'm not sure how it is that
you're getting a body filter criteria.  Body is valid for IMAP searching (not
filtering) if the folder has been downloaded for offline use.

Comment 4

[Joel Thornton]

"Body" condition only for POP folders: I assume this is intentional, but is it
possible to get it permitted also for IMAP folders which are set to download offline automatically?  As long as the body of a message is available, it would make sense to allow filters to apply to it.

Since filters are applied only at the time of message download, I think it would only work if "Body" conditions act upon IMAP folders which are set to be used 'offline'.

Comment 5

[laurel]

No. No body filter criteria option is/should be provided even for IMAP folders
downloaded for offline use.

So, this is an invalid state to have your filters in for IMAP.  Did you edit
your filter file (rules.dat) to introduce the body filter for your IMAP account?
Otherwise, I can't figure out how you were able to get the body choice.  

Comment 6

[Joel Thornton]

"No body filter criteria option is/should be provided even for IMAP folders
downloaded for offline use.": why not?  Speaking just hypothetically for the
moment, could this be reasonably implemented if it was desired? It certainly
seems like a desirable feature for anyone using filters and IMAP.

I didn't edit my rules.dat.  It could be that I previously had POP servers
configured but no longer do, or it could be some detritus from an earlier build
of Mozilla (I haven't done a completely fresh install since .97).

I will try a fresh install and see if that eliminates the option.

Comment 7

[Eric Vaandering (no email)]

Can we mark this a dup of bug 67421 and move discussion there?

Comment 8

[Wayne Mery (:wsmwk)]

If it is impossible to implement "body search" for IMAP, shouldn't this bug and
bug 67421 be closed out?

Comment 9

[Sander]

Morphing this request to ask only for body filters for downloaded for offline
use IMAP messages and confirming as valid RFE distinct from bug 67421. Module
owner or whoever has knowledge of the technical issues at stake here may decide
to mark both bugs invalid as happened previously in bug 56781

Comment 10

[(not reading, please use [e-mail] instead)]

as with 4.x, we should be able to do body filters, if we are downloading the
entire imap message, for offline use.

this isn't the same as bug #67421, but related.

different, as with that bug, we won't save the message to disk, nor download the
non text parts.

with download for offline, we would

Comment 11

[R.K.Aa.]

*** Bug 287610 has been marked as a duplicate of this bug. ***

Comment 12

[Ted H.]

OK, I see my problem is that I use an IMAP folder for ALL my inbound mail; its
on a unix box that uses fetchmail to get all my mail into a single account. I'm
using QMail and Bincimap on the server.  

But I don't see why filtering the body of an IMAP message (downloaded or not) is
"impossible."  Afterall, my messages downloaded and other rules distribute
them to local mail boxes... but they are not filtered by the local rules when
delivered, as far as I can see.  I need to junk mail containing certain URLs in
the body.  Frustrating.

Ted

Comment 13

[Mike Cowperthwaite]

*** Bug 233902 has been marked as a duplicate of this bug. ***

Comment 14

[Lonni J Friedman]

Its so disapointing that this bug has existed for over 4 years with no progress.

Comment 15

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 16

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 17

[Kent James (:rkent)]

I had an IRC discussion with bienvenu today about how this might be done. As a quick summary, when an IMAP body filter exists, add an explicit call using the IMAP service to download the message bodies (bypassing autosync), and use the post-download notification there to apply the filter instead of the current application in NormalEndHeaderParseStream.

Comment 18

[Kent James (:rkent)]

The current patch for bug 198100 actually allows IMAP body search for offline messages, in the new postPlugin filter context. Assumming that makes TB3, hopefully there will be that method available in TB3 for IMAP body search. But this bug is still valid for the Incoming filter context, using the basic approach from comment 17.

Comment 19

[Kent James (:rkent)]

Attachment: WIP but basically works

Here's a working patch (including a large unit test for most IMAP filter actions) but I still have a lot of testing to do.

Comment 20

[Kent James (:rkent)]

Attachment: review candidate

This patch has no known issues, but I want to run with it a little more, and review it myself, before asking for review.

Comment 21

[Kent James (:rkent)]

Attachment: Support custom search, tweak handling of not new

OK, let's get this reviewed. I tried to remove anything I could that was weird, and tried not to take any new risks that I didn't have to. I would really like to get this landed weeks before a beta to reduce risk.

Not sure who else to add to review, so just giving it all to bienvenu for now.

Comment 22

[Kent James (:rkent)]

Is there any hope of getting this reviewed soon?

Comment 23

[David :Bienvenu]

I've looked through it some, but I'm going to need to spend more time with it...probably not until next week, though.

Comment 24

[David :Bienvenu]

I've looked through this - I don't hate it, and I do love the tests. It is a bit scary, though.  I'm running with it now.

There is some commented out code in the tests that should be removed, and the comments to me should probably be removed :-)

I'd get some tryserver builds up, but I'm not sure there's a lot of pent-up demand for this feature

Comment 25

[David :Bienvenu]

+  nsMsgKey key = -1;
+  msgHdr->GetMessageKey(&key);
+  ContainsKey(key, &notify);

should use nsMsgKey_None instead of -1. And it looks like this code could move after the check to see if the value has changed.

Are all the changes to nsImapMailFolder::ApplyFilterHit needed, in particular, to go through the db instead of just using the msg hdr, and the removal of the proto_flags stuff that jcranmer put in? I have a bad feeling that some of that code was that way for a reason...

Comment 26

[Kent James (:rkent)]

(In reply to comment #25)
 
> Are all the changes to nsImapMailFolder::ApplyFilterHit needed, in particular,
> to go through the db instead of just using the msg hdr, and the removal of the
> proto_flags stuff that jcranmer put in? I have a bad feeling that some of that
> code was that way for a reason...

The changes to ApplyFilterHit are really at the heart of this, because the key issue that I had to face was to make ApplyFilterHit work either before or after the hdr is added to the database. (Note we had already done this on some of the code, for example SetStringProperty). You can't just use the message header to make changes after the message is added to the database, because after it is added you need the notifications that the database provides. The other way I could have approached this was to duplicate the entire functionality of ApplyFilterHit in a separate version that worked after the header is added to the database, but I don't think that is the right approach. So, in short, I believe they are necessary.

I don't know what you mean by "the removal of the proto_flags stuff" It's still there AFAICT.

Comment 27

[David :Bienvenu]

Comment on attachment 391965 [details] [diff] [review]
Support custom search, tweak handling of not new

sorry for the delay; I've been wrestling with the risk/rewards issues of this patch.

this:

+         else if (askBeforePurge && !aWindow)
+           okToCompact = PR_FALSE;
          else
            okToCompact = PR_TRUE;

can be (if I've got the logic right)
else
  okToCompact = aWindow || !askBeforePurge;

+  if (offlineInboxMsgFolder)
+    *filterScope = nsMsgSearchScope::offlineMailFilter;
+  else
+    *filterScope = nsMsgSearchScope::onlineMailFilter;

can be *filterScope = offlineInboxMsgFolder ? ... (I think modern compilers are ok with enums in ? expressions)

 
r/sr=me, if you fix that, and my previous comments.

Re potential regressions, you might want to consider breaking this patch up into the utility stuff which isn't going to break anything, e.g. the new methods on nsIMsgDatabase, and then the stuff where the rubber hits the road, which might need to be backed out if there are bad regressions we can't fix before b4...

Comment 28

[Kent James (:rkent)]

Attachment: db-only patch, checkin first

This patch just adds the new db functions as suggested.

Comment 29

[Kent James (:rkent)]

Attachment: filter patch, checkin second

This adds the main purpose of this bug, which is to implement the filter functions. The previous patch can run without this one, but this patch requires the previous patch.

Comment 30

[Mark Banner (:standard8)]

Both patches checked in:

http://hg.mozilla.org/comm-central/rev/35517958abb8
http://hg.mozilla.org/comm-central/rev/1ebec2bae790