Closed Bug 127250 Opened 21 years ago Closed 13 years ago

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


(MailNews Core :: Filters, enhancement)

Windows XP
Not set


(Not tracked)

Thunderbird 3.0b4


(Reporter: jpt, Assigned: rkent)




(2 files, 3 obsolete files)

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.
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?
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.
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.
"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'.
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.  
"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.
Can we mark this a dup of bug 67421 and move discussion there?
If it is impossible to implement "body search" for IMAP, shouldn't this bug and
bug 67421 be closed out?
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
Severity: normal → enhancement
Ever confirmed: true
Summary: Creating "Body" filter condition doesn't stick → "Body" filter for IMAP messages downloaded for offline use.
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
Assignee: naving → sspitzer
QA Contact: laurel → nbaca
Blocks: 66425
Product: MailNews → Core
*** Bug 287610 has been marked as a duplicate of this bug. ***
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.

*** Bug 233902 has been marked as a duplicate of this bug. ***
Its so disapointing that this bug has existed for over 4 years with no progress.
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Filter on "Nobody_NScomTLD_20080620"
QA Contact: nbaca → filters
Product: Core → MailNews Core
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.
Assignee: nobody → kent
Target Milestone: --- → Thunderbird 3.0rc1
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.
Attached patch WIP but basically works (obsolete) — Splinter Review
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.
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b4
Attached patch review candidate (obsolete) — Splinter Review
This patch has no known issues, but I want to run with it a little more, and review it myself, before asking for review.
Attachment #390577 - Attachment is obsolete: true
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.
Attachment #391173 - Attachment is obsolete: true
Attachment #391965 - Flags: superreview?(bienvenu)
Attachment #391965 - Flags: review?(bienvenu)
Whiteboard: [needs r/sr bienvenu]
Is there any hope of getting this reviewed soon?
I've looked through it some, but I'm going to need to spend more time with it...probably not until next week, though.
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
+  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...
(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.
Attachment #391965 - Flags: superreview?(bienvenu)
Attachment #391965 - Flags: superreview+
Attachment #391965 - Flags: review?(bienvenu)
Attachment #391965 - Flags: review+
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.


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

can be (if I've got the logic right)
  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...
This patch just adds the new db functions as suggested.
Attachment #391965 - Attachment is obsolete: true
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.
Keywords: checkin-needed
Whiteboard: [needs r/sr bienvenu] → [needs checkin]
Both patches checked in:
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs checkin]
Depends on: 514801
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.