As a companion to bug 419356 "Allow extensions to add custom filter actions", I would like to provide hooks for extensions to provide custom search options with the existing nsIMsgDBHdr-based search backend. This is fairly easy to do in contexts where the message header is available, and difficult to do in online contexts. I will probably then not do this, at least initially, for online cases. But filtering is typically done using the offline technology anyway, so this is not much of a restriction for filters. This is primarily targeted toward message filters, as search enhancements seem to be heading toward gloda.

This patch requires https://bugzilla.mozilla.org/attachment.cgi?id=380509 from bug 198100. It works with no known problems - but there are lots of things that I have not checked yet.

This is a demo extension, quickly hacked together from remnants of FiltaQuilla, that implements a search term that does a regular expression match to the subject.

This demonstration extension provides two custom search terms. One uses a text input box, the second a menulist, to demonstrate setting up alternate value UI methods using CSS (lie we do in custom filter actions). This will work with a patch to follow. One search is a regular expression match on the subject. The second is a simulation of a search for three values of the Spam Assassin header (looking for the number of consecutive "*" characters), but I do the search on the subject instead of the header to make it easier to test the demo. The extension (and the patch to follow) work on both Thunderbird and Seamonkey.

By far the most difficult part of this patch is the UI involving mailWidget.xml, filterEditor,js, and searchTermOverlay.js. That code is enormously confusing. Terms like "value" and "attribute" are used in multiple ways, plus the same values are stored in multiple ways that have to be applied in just the right order to make it all work in all circumstances. I implemented as part of this patch support for invalid search attributes, which was originally prepared for bug 198100, but applies more generally. So if for example you select Manual filter context, select a junk search, then switch to Checking Mail, the junk search attribute is greyed out, and it will not let you save the filter. Previously it ignored this, and let you save the invalid filter. The filter widget part of mailWidgets is identical between SM and TB. Eventually I would really like to unfork this, as it is a real pain to support both files. Clearly this all needs some decent developer documentation. The sample extension helps a lot. But I need to prepare an article that describes all of the new filter extension features if they are going to be useful to extension writers.

Comment on attachment 385046 [details] [diff] [review] Use CSS selectors for value portion of search while Neil does the heavy lifting of the review, a couple comments: ? operator here: + if (this.valueIds.indexOf(typedValue) >= 0) // is value valid? + menulistLabel.disabled = false; + else + menulistLabel.disabled = true; indentation looks off here: + boolean getEnabled(in nsMsgSearchScopeValue scope, + in nsMsgSearchOpValue op); you need an NS_ENSURE_ARG_POINTER(aResult) here because NS_NewArrayEnumeror doesn't seem to check: + return NS_NewArrayEnumerator(aResult, mCustomTerms);

(In reply to comment #5) > (From update of attachment 385046 [details] [diff] [review]) > while Neil does the heavy lifting of the review I was hoping to avoid that as parts of it look oddly familiar... > ? operator here: > > + if (this.valueIds.indexOf(typedValue) >= 0) // is value valid? > + menulistLabel.disabled = false; > + else > + menulistLabel.disabled = true; Well, initially I'd have gone for the < operator ;-) menulistLabel.disabled = this.valueIds.indexOf(typedValue) < 0; But I'm still trying to work out what this code is doing... > + if (scope == Ci.nsMsgSearchScope.offlineMail && > + (!op || (op == Ci.nsMsgSearchOp::Is))) You can't use !op here, it's an integer, so you're comparing against Contains.

Comment on attachment 385046 [details] [diff] [review] Use CSS selectors for value portion of search >+ if (this.valueIds.indexOf(typedValue) >= 0) // is value valid? >+ menulistLabel.disabled = false; >+ else >+ menulistLabel.disabled = true; OK, I give up. What's the point of this? >+ let menuitem = menulist.getElementsByAttribute("value", this.value)[0]; Nit: should use .item(0) if you don't know that the item exists. >+ { // need to add a hidden menuitem Nit: use menuitem = menulist.appendItem(...); >+ menuitem.setAttribute("hidden", "true"); Nit: use .hidden >+ dump("\nCouldn't find custom term in ids[i]"); If you mean to keep this here, then it should end, not begin with, \n. >+ if (customTerm) >+ return customTerm.getAvailableOperators(this.scope, length); >+ else Nit: unnecessary else after return. >+ return [Components.interfaces.nsMsgSearchOp.Contains]; Not that I see any callers, but why not return null or []? >diff --git a/mail/locales/en-US/chrome/messenger/search-attributes.properties b/mail/locales/en-US/chrome/messenger/search-attributes.properties >--- a/mail/locales/en-US/chrome/messenger/search-attributes.properties >+++ b/mail/locales/en-US/chrome/messenger/search-attributes.properties >@@ -38,3 +38,4 @@ JunkStatus=Junk Status > JunkStatus=Junk Status > Label=Label > Customize=Customize⦠>+MissingCustomTerm=Missing Custom Term >\ No newline at end of file Don't want this. >+ if (scope == Ci.nsMsgSearchScope.offlineMail && >+ (!op || (op == Ci.nsMsgSearchOp::Is))) >+ return true; >+ return false; In fact you could write this as return scope == Ci.nsMsgSearchScope.offlineMail && op == Ci.nsMsgSearchOp::Is; (note that any filter worth its salt should provide all the negations of all the operators that it provides!)

(In reply to comment #7) > (From update of attachment 385046 [details] [diff] [review]) > >+ if (this.valueIds.indexOf(typedValue) >= 0) // is value valid? > >+ menulistLabel.disabled = false; > >+ else > >+ menulistLabel.disabled = true; > OK, I give up. What's the point of this? I'm not sure at what level to answer this question, so I'll give a long answer. It is possible to end up with an attribute that is not currently available in the filter. The main way this can happen is that you create a filter in one context, add a search attribute, then switch to a context where this attribute is unavailable. In the patch, you can do this by creating a Manual filter, add the junk status attribute, then switch to Incoming. Custom filters are also able to create attributes that are valid in some contexts and not in others. How should the code deal with this? What I decided to do was to show the menulist item as disabled, and then refuse to save the filter. The particular lines of code that you mention detect this. this.valueIds is a list of the available attributes, with standard attribute indices as integers and custom as strings. If the existing filter has an attribute that is invalid, then we disable the label on the menulist to give the menulist item a grayed-out look. Part of the fun of this code is that english terms are used in different ways. "value" can be the search term "value", or the current main result held in an element. Here it means the latter.

(In reply to comment #8) > It is possible to end up with an attribute that is not currently available in > the filter. The main way this can happen is that you create a filter in one > context, add a search attribute, then switch to a context where this attribute > is unavailable. In the patch, you can do this by creating a Manual filter, add > the junk status attribute, then switch to Incoming. Custom filters are also > able to create attributes that are valid in some contexts and not in others. You can also demonstrate this in the advanced search dialog - set a search condition using (say) attachment status, then switch from local to imap/news. (So I hope your behaviour change doesn't break advanced search!) > How should the code deal with this? What I decided to do was to show the > menulist item as disabled, and then refuse to save the filter. I think that would look unusual. In fact, I think it would look particularly strange in the Windows Classic theme where standalone disabled labels have a shadow. I would suggest disabling the other parts of the condition instead.

(In reply to comment #9) > I think that would look unusual. In fact, I think it would look particularly > strange in the Windows Classic theme where standalone disabled labels have a > shadow. I would suggest disabling the other parts of the condition instead. Isn't Windows Classic what runs on XP? That's what I use, and it works just fine. I'm not sure what you mean by "disabling the other parts of the condition instead".

Here is what the disabled menuitem looks like on my system.

(In reply to comment #10) > Isn't Windows Classic what runs on XP? It's what runs on W2K. It's available on XP and later, but not by default.

Your attachment 385756 [details] looks fine to me - am I missing something?

Comment on attachment 385046 [details] [diff] [review] Use CSS selectors for value portion of search >+ let menulistLabel = document.getAnonymousNodes(menulist)[0].childNodes[1]; Additional nit: This isn't guaranteed to work in the present, let alone the future; it's already possible for custom themes to rebind different XBL content to support a different appearance. (In reply to comment #15) >Your attachment 385756 [details] looks fine to me - am I missing something? Well, it's not obvious, but this one is titled "disabled condition elements" instead of "disabled label". This means that it's a mockup of the alternative effect that I requested :-)

OK Neil, I think I see why you are recommending the handling of disabled. I understand you as saying that what you proposed works (disabling the condition elements) without having to dive into the anonymous content. Let me see if I can implement that.

Just for the record, I went on to suggest another alternative over IRC, which was to set a custom attribute on the menulist and use CSS to override the text colour in this case (the label's colour inherits from the menulist).

(In reply to comment #18) > Just for the record, I went on to suggest another alternative over IRC, which > was to set a custom attribute on the menulist and use CSS to override the text > colour in this case (the label's colour inherits from the menulist). That would be easier to implement. It is a royal pain to transfer properties from the searchAttribute elements to the others.

I've fixed the nits, plus changed the disabled term to use a CSS selector now on a menulist attribute. I had not tested on advanced search before, there were a few small issues there I have fixed, plus I will need to post an updated demo extension.

Comment on attachment 386205 [details] [diff] [review] Use CSS on menulist for disabled, fixed nits, works for search >diff --git a/suite/themes/modern/messenger/filterDialog.css b/suite/themes/modern/messenger/filterDialog.css >--- a/suite/themes/modern/messenger/filterDialog.css >+++ b/suite/themes/modern/messenger/filterDialog.css >@@ -57,6 +57,10 @@ treechildren::-moz-tree-image(activeColu > width: 12em; > } > >+.search-menulist[unavailable="true"] { >+ color: GrayText; >+} >+ > .small-button { > min-width: 3em; > padding: 0px; Two nits: 1. In modern, we don't use GrayText, we use #8C99AB 2. searchDialog.css also needs this (fixed) style rule

Comment on attachment 386205 [details] [diff] [review] Use CSS on menulist for disabled, fixed nits, works for search is the news directory missing from this patch? It looks like nsNewsDownloader calls nsIMsgSearchSession::addSearchTerm.

I have not really changed to call pattern for addSearchTerm, only extended the definition of one of the existing parameters. As far as I understand it, the calls to addSearchTerm in nsNewsDownloader all specify attributes that are not affected by the new custom search capability. Note I did not change the UUID for the idl either. That was an intentional decision so that all of the code with addSearchTerm would not have to be updated.

Carrying over Neil's r+

Comment on attachment 387100 [details] [diff] [review] Fixed Neil's issues can you remove this dump? + if (customTerm) + strings[i] = customTerm.name; + else + dump("Couldn't find custom term in ids[i]\n"); + }

Comment on attachment 387100 [details] [diff] [review] Fixed Neil's issues approval for landing suite strings post-slushy-string-freeze for SeaMonkey 2.0b1

Carrying over r/sr.

This bug has late-i10n approval from dmose (mail) and kairo (suite), and is ready for checkin.

Checked in: http://hg.mozilla.org/comm-central/rev/db4d4379d404