16902 - Filter/Search mail based on any header

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[Phil Peterson]

In 4.5 we added the ability to filter mail messages based on "arbitrary
headers". So you could filter on X-Mailer or Resent-From, etc.

We wanted this to work for news too, but I don't think it did. We should fix
that up in mozilla. We can file a separate bug on the news part if that seems
better.

I can't quite find a feature bug on customizable headers (for filtering,
compose, display) but I'll file one and make this bug dependent on it.

Comment 1

[laurel]

Earlier in the year, it was decided not to have arbitrary header filtering or
searching for news in 5.0. Are you changing that decision?

Comment 2

[Phil Peterson]

Uh oh. I don't remember that decision. Scott, Sol or David?

Comment 3

[Scott MacGregor]

I remember arbitrary headers for news being one of the first cut items we had
back in january when we had those meetings =). I'm pretty sure Phil was there
too.

Comment 4

[Phil Peterson]

Ok, I guess I blocked it out. So let's narrow this bug to just mail. I filed
16913 for news.

Comment 5

[Phil Peterson]

Fix dependencies

Comment 6

[John_David_Galt]

I can't tell whether this works in 4.7 or not, because the window for entering
new headers on which to filter (Edit|Message Filters|New|Advanced) is not
explained anywhere including Windows Help.  Please do not mark this "Fixed"
without documenting how to use the feature.

Comment 7

[Phil Peterson]

I'm all for improving the documentation (adding headers is mentioned only
briefly), but this bug is for writing the code, not improving the doc. Doc bugs
on this go to [e-mail]

Comment 8

[Phil Peterson]

M16

Comment 9

[selmer (gone)]

Would not hold beta 2 for this.  Marking M17.

Comment 10

[selmer (gone)]

[FEATURE] bugs past M16 are OUT for this release.  Marking M20.  If you disagree 
with this action, please help me explain it to the PDT.

Comment 11

[Phil Peterson]

Adding 4xp keyword since 4.5+ supported this

Comment 12

[Matthew Tuck [:CodeMachine]]

Can we at least get the really common extra headers?  I have about 100 filters
and probably half use Resent-From.

If this is not going to happen, what will happen on upgrading?  I assume N6 will
be able to use or upgrade your previous N4 filters, so will the filters be
nuked, stop working, or what?

Comment 13

[laurel]

adding jglick and alecf to cc list.
Latest filter ui spec (and in the april issues meeting) custom headers for mail
filters is listed as a P2 item.

Is M20 your final answer? (what about custom headers for search -- I haven't
looked for a search bug yet...)

Comment 14

[Scott MacGregor]

I think this belongs to alecf now (at least the UI part)...I'll let him make the
final call.

Comment 15

[Alec Flett]

yeah, I'm going to make m18 right now, bug hopefully I'll get in for beta2

Comment 16

[Phil Peterson]

I don't think this work is on the exception list that selmer and kmurray gave to
PDT. Is it really a feature?

Comment 17

[Alec Flett]

nope, it's just an extension to some work I've already done.

Comment 18

[scottputterman]

mass moving to M18 and adding nsbeta3 keyword

Comment 19

[laurel]

Are we using this bug for the search messages ui having custom header
capability, too, or do you want search logged separately?

Comment 20

[Alec Flett]

nope, all the code is shared.

Comment 21

[David :Bienvenu]

reassigning to myself.

Comment 22

[David :Bienvenu]

accepting

Comment 23

[laurel]

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

Comment 24

[laurel]

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

Comment 25

[laurel]

Added mail3 keyword for 6.5 consideration since quite a few reports/comments
seem to be cropping up about custom headers

Comment 26

[scottputterman]

adding nsbeta1- keyword.  We want to work on some of the more commonly used
features of search and filters before doing this.

Comment 27

[laurel]

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

Comment 28

[Garth Wallace]

Think bug 16913 may be a dup

Comment 29

[Garth Wallace]

Gah, sorry. Disregard my last comment.

Comment 30

[laurel]

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

Comment 31

[Jesse Ruderman]

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

Comment 32

[laurel]

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

Comment 33

[laurel]

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

Comment 34

[laurel]

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

Comment 35

[Gervase Markham [:gerv]]

bienvenu: what's the status here? Is this on any radars? Should it be?

Gerv

Comment 36

[David :Bienvenu]

I'm not currently working on this and don't have any near term plans to do so. 
I would like to do this at some point, however.

Comment 37

[David Hallowell]

Adding helpwanted based on bienvenu's comments.

Comment 38

[Thomas Dodd]

What would it take to support the filter part?
So imported filters or hand editing the rules
file works? The UI can wait, as long as there
is a way to support the filters in the file.

Most mailing lists use X-BeenThere or X-Loop,
and many use the List-* headers.

Filtering is almost useless without some way
to filter on any header. Most users will have
the filters from NS4 to migrate, of the skill
to edit the rules file, so only the filter engine
is "required", the UI could wait for a UI coder.

Comment 39

[David :Bienvenu]

The backend isn't ported from 4.x, so there's both frontend and backend work to
do. The backend work involves both the filters/search code, and the imap/pop3
code. I agree that doing the backend work first makes sense.

Comment 40

[Alfonso Martinez]

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

Comment 41

[Navin Gupta]

I am willing to work on this one if no one else has yet started on it. 

Comment 42

[laurel]

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

Comment 43

[Gervase Markham [:gerv]]

naving: no-one is working on it to my knowledge, and many people would love you
to work on it :-) There are a number of bugs in our filters UI; you may want to
review those before starting work, because it may well be easy to fix a lot of
them on the way through.

Gerv

Comment 44

[Navin Gupta]

taking.

Comment 45

[Navin Gupta]

We already have the back-end code in place. It was not working. I have 
a one liner fix for the problem. I will work on the UI part next.

Comment 46

[Navin Gupta]

Attachment: proposed fix for back-end part.

Comment 47

[Navin Gupta]

The fix is to compare the correct part of each header line with the arbitrary
header. I have made it caseInsensitive, can change it if someone has a good 
reason not to do so.

 

Comment 48

[Navin Gupta]

david, please review. 

Comment 49

[Navin Gupta]

I think offset is going to matter when comparing, will attach a new patch. 

Comment 50

[Navin Gupta]

Attachment: proposed fix for backend, v2

Comment 51

[Navin Gupta]

This is how we parse other headers like To, CC etc, should work fine. 

Comment 52

[Alec Flett]

I think that part of the issue is that the IMAP envelope doesn't give you
anything beyond the standard header, so you need to specifically request it, no?

Comment 53

[Navin Gupta]

I did notice that, but didn't know that you will have to specifically
request it for imap. For pop3, it comes by default. 

Comment 54

[Navin Gupta]

cc jglick, can you point to me the specs for this feature. I have started 
with 4x as the base. 

Comment 55

[jglick]

Navin, this what you are looking for?
http://www.mozilla.org/mailnews/specs/filters/#Headers

Would apply to Search as well.

Comment 56

[Navin Gupta]

4x is little bit different, it has an advanced button that takes you to the 
dialog for adding customized headers. I believe, that is much easier and less 
confusing to do because once you have added those headers, they will appear in
the dropDown. They will appear in the same place where we right now see  
"Customize headers..." (from specs).

Comment 57

[jglick]

The new design works similarly, except instead of an "Advanced" button, which 
gives the user no indication it is associated with adding headers, they select 
"Customize" from the Header selection menu. This opens a dialog which allows 
them to create additional Headers. Once created, the new headers will appear in 
the Header list (same as 4.x).

Comment 58

[Gervase Markham [:gerv]]

The new UI design is excellent, apart from the fact that it seems to omit a
section for saying exactly what should happen to the message if it's caught by
the filter. On this point, I would add a request:

People often complain about 4.x filters that it's not possible to do (A OR B)
AND (C or D) or similar. The current UI permits only ALL OF/ANY OF (A, B, C). I
think the best way of making simple UI for the generic mechanism is merely to
say, when you are deciding what to do with the passed message, have an option to
pass to another filter, i.e.:

File in [Folder from folder list]
Delete
Mark Read
...
Route to [Filter from filter list]

i.e. permit cascading of filters. This would be a minimal UI increase (one
additional option) but a great increase in functionality.

Gerv 

Comment 59

[Alec Flett]

gerv: complex boolean logic in filters is a completely seperate feature, and
requires a lot of back end support... theres a bug around somewhere, I suggest
tracking it down and adding yourself to the CC.

Comment 60

[Gervase Markham [:gerv]]

I admit I'm not familiar with the internals of the code (perhaps because naving
hasn't written it yet) but I don't understand how this can be so hard.

function checkIfCaughtByFilter(filter) {
...
return (boolean) caught;
}

-> 

function checkIfCaughtByFilter(filter) {
...
if caught && (userhaschosentopasstoanotherfilter)
  return checkIfPassedFilter(filter2)
else
  return caught;
}

Gerv

Comment 61

[David :Bienvenu]

Alec is suggesting, I believe, that implementing complex boolean filters is the
right way to go, though hard. He was not saying that your approach would be hard. 

Comment 62

[Gervase Markham [:gerv]]

Complex boolean filters would be both hard, and hard to do UI for. My idea is
both simple in concept and (probably) execution, and has almost the same level
of power as complex boolean filters. 

If no-one is stepping up to implement complex boolean filters, let's get the 95%
case by asking naving to add this while he's working in the code.

Gerv

Comment 63

[David :Bienvenu]

I don't think Navin should do this - I don't believe we've agreed that this is a
good thing for the UI, among other things. Did I miss something? I personally
think this UI would be confusing and non-intuitive for users. 

Comment 64

[Alec Flett]

enough! gerv, take this up in a bug that's actually relevant - this bug is about
supporting any header, not complex boolean filters.

Comment 65

[Navin Gupta]

ya, this bug is not about complex boolean algebra. Please take it to another 
bug.

Comment 66

[Navin Gupta]

ok, I have got imap and pop3 working for both search and filters. working 
on news...

Comment 67

[Navin Gupta]

Attachment: proposed fix

Comment 68

[Navin Gupta]

The fix includes fix for both backend/UI and for pop3 imap (online/offline) and
news offline. 

The main thing here was to design the UI and hook-up and make changes to the 
backend. The UI is as described in the specs and I will attach the screenshot.
The main part of the fix is to read the pref customheaders and then initialize
the table that contains all the attributes and their corresponding operators
with the custom headers. The XBL widget uses this table and prefs to build up 
the searchTerms widget. It maintains a one to one relationship between value ids 
and the label. This is needed for loading correct labels when passing 
information either from search widget to back-end or vice-versa. Add a special 
item "Customize.." to the table, when selected launches the custom headers 
add/remove dialog. The "Replace" functionality seems redundant and also does not 
work in 4x so I have left it for now. 

The backend involves downloading headers in case of imap and we need to 
download headers that are used in filters. I have made it so that we can cache
the headers until the user changes the filters and commits the changes. Moved
all the code from imapMiscellaneousSink to imapServerSink. Made changes so 
that it work for news offline. Also removed code no longer used. 

david and seth, please review. 

Comment 69

[Navin Gupta]

Attachment: screen-shot of ui

Comment 70

[Navin Gupta]

Attachment: screen-shot of headers in search-attributes.

Comment 71

[Gervase Markham [:gerv]]

These screenshots are of the Search UI - I take it this also applies to Filters,
right?

While you are in the Message Search box, could you take five seconds to clean up
some of its more serious problems? For example, the fact that, by default, the
Results section is only 2 and one half messages high, that it doesn't seem to
remember if you resize the dialog or reposition the splitter, and that the box
with the search criteria in should expand and contract as you click "More" and
"Fewer" rather than starting far too large, and scrolling when you click "More"
enough times? :-)

Gerv

Comment 72

[David :Bienvenu]

+// return -1 if no more local lines, length of next line otherwise.*/

shouldn't put that '*/' at the end of this comment

+    if (m_passedHeaders)
+
		m_numLocalLines--; // the line count is only for body lines

looks like you left a tab in here

This routine nsMsgFilterList::GetArbitraryHeaders assumes that the caller called 
GetShouldDownloadArbitraryHeaders first. That seems like a dangerous assumption
to me. It would be better to combine those routines into one, if you can't
figure out a way of making it so that you have to call GetShould first (or
figure out a way of throwing an assert if the client doesn't do what you've
implicitly decided they will). Or make GetShouldDownloadArbitraryHeaders call
GetArbitraryHeaders itself, so that the result is pre-calculated.

Other than that, the C++ code looks ok. I can't speak for the js.

Comment 73

[Alec Flett]

out of morbid curiosity, what do the AOL servers do when you ask for individual
header values?

Comment 74

[Navin Gupta]

for aol imap servers "search" command is not supported. But it does allow to 
download the custom headers which is what we need for filtering. I have patch to 
make this work. 

Comment 75

[Navin Gupta]

Attachment: fix for aol imap servers to download arbitrary headers

Comment 76

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

gerv:

>While you are in the Message Search box, could you take five seconds to cleanup
>some of its more serious problems? For example, the fact that, by default, the
>Results section is only 2 and one half messages high, that it doesn't seem to
>remember if you resize the dialog or reposition the splitter

covered in another bug, and beyond the scope of this bug.

>and that the box
>with the search criteria in should expand and contract as you click "More" and
>"Fewer" rather than starting far too large, and scrolling when you click "More"
>enough times? :-)

gerv, is there a bug for this?  if not, can you log it?

Comment 77

[Nobody; OK to take it and work on it]

> covered in another bug, and beyond the scope of this bug.

I hope he finds five seconds to change the relative flex of the two halves of
the dialog. This alone would be a great improvement, and it's a two character
change.

> gerv, is there a bug for this?  if not, can you log it?

Logged as bug 103742 in my alter ego as [e-mail].

Gerv

Comment 78

[Navin Gupta]

Attachment: proposed fix, v2

Comment 79

[Navin Gupta]

the patch may have"no newline at end" in one or more places, i will fix that
before checking in. 

Comment 80

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

r=sspitzer  

naving gave me a demo, and this will be a big improvement.

and, since we used the same pref as 4.x, migration will work.

remember to remove the unneeded "xmlns:nc="http://home.netscape.com/NC-rdf#"

when you land this, make sure to log a bug with all the known UI issues so we
can get to them later, when we have more time.

Comment 81

[David :Bienvenu]

if (headers && m_arbitraryHeaders)

should just be if (headers)

nsMsgSearchAttrib::CustomizeHeaders

should either be
nsMsgSearchAttrib::CustomizedHeaders

or nsMsgSearchAttrib::CustomHeaders


customize is a verb
string getArbitraryHeaders();
boolean getShouldDownloadArbitraryHeaders();

these should be readonly attribute  string arbitraryHeaders
and readonly attribute shouldDownloadArbitraryHeaders

Why do we have nsMsgSearchAttrib::OtherHeader

and nsMsgSearchAttrib::CustomizeHeader? Aren't they the same?

Comment 82

[Navin Gupta]

Attachment: proposed fix, v3

Comment 83

[Navin Gupta]

May have some other unrelated files in there, will not checkin those.

Comment 84

[David :Bienvenu]

Comment on attachment 52852 [details] [diff] [review]
proposed fix, v3

sr=bienvenu (for the changes associated with this bug)

Comment 85

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

navin, can you log a bug with all the known issues?

Comment 86

[Navin Gupta]

fix checked in, I will log those issues now, before I forget. 

Comment 87

[laurel]

Feature basically in and functional.
Any specific issues with the custom header feature will be logged separately.
OK using oct29 commercial trunk build: win98, linux rh6.2, mac OS X