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

RESOLVED FIXED in Thunderbird 3.0b4

Status

--
enhancement
RESOLVED FIXED
17 years ago
9 years ago

People

(Reporter: jpt, Assigned: rkent)

Tracking

Trunk
Thunderbird 3.0b4
x86
Windows XP
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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

17 years ago
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?
(Reporter)

Comment 2

17 years ago
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

17 years ago
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.
(Reporter)

Comment 4

17 years ago
"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

17 years ago
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.  
(Reporter)

Comment 6

17 years ago
"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?

Comment 8

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

Comment 9

16 years ago
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
Status: UNCONFIRMED → NEW
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

Updated

14 years ago
Blocks: 66425
Product: MailNews → Core

Comment 11

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

Comment 12

14 years ago
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

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

Comment 14

12 years ago
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
(Assignee)

Comment 17

10 years ago
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
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 3.0rc1
(Assignee)

Comment 18

9 years ago
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.
(Assignee)

Comment 19

9 years ago
Created attachment 390577 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b4
(Assignee)

Comment 20

9 years ago
Created attachment 391173 [details] [diff] [review]
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.
Attachment #390577 - Attachment is obsolete: true
(Assignee)

Comment 21

9 years ago
Created attachment 391965 [details] [diff] [review]
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.
Attachment #391173 - Attachment is obsolete: true
Attachment #391965 - Flags: superreview?(bienvenu)
Attachment #391965 - Flags: review?(bienvenu)
(Assignee)

Updated

9 years ago
Whiteboard: [needs r/sr bienvenu]
(Assignee)

Comment 22

9 years ago
Is there any hope of getting this reviewed soon?

Comment 23

9 years ago
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

9 years ago
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

9 years ago
+  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...
(Assignee)

Comment 26

9 years ago
(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.

Updated

9 years ago
Attachment #391965 - Flags: superreview?(bienvenu)
Attachment #391965 - Flags: superreview+
Attachment #391965 - Flags: review?(bienvenu)
Attachment #391965 - Flags: review+

Comment 27

9 years ago
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...
(Assignee)

Comment 28

9 years ago
Created attachment 398744 [details] [diff] [review]
db-only patch, checkin first

This patch just adds the new db functions as suggested.
Attachment #391965 - Attachment is obsolete: true
(Assignee)

Comment 29

9 years ago
Created attachment 398745 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [needs r/sr bienvenu] → [needs checkin]
Both patches checked in:

http://hg.mozilla.org/comm-central/rev/35517958abb8
http://hg.mozilla.org/comm-central/rev/1ebec2bae790
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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.