To allow extensions to create their own filter lists (for example: sieve), filter lists should be pluggable. I propose to use a pref that defines the type of filterlist for each server. This pref value can be used in a contract-id to create the right module.

Patch implements my proposal. The current local filters are a special case, because they have a special interface to create them. This might change later.

Thanks for the patch, I probably won't have time to poke at this until after 3.0b1 ships. Perhaps rkent would be up for a first review pass?

If I simply apply the patch, it breaks any existing filters. I think that is because mail.server.default.filter.type does not get applied to existing servers. Also, it did not apply against current trunk - though that was easily fixed. My bigger concern though is that a lot of existing functionality assumes the default filter configuration, and breaks without it. For example, if I do Tools/MessageFilters, it opens the Filter list dialog, and if I ask to edit will open the filter editor - all of which AFAIK assume many details of the current filterlist. Does your larger patch deal with these issues? One little nit: + if (!filterType.EqualsLiteral("local")) { prevailing mailnews style is for brackets to be on the next line.

My tests for local filters still worked. But i'll check again. Afaik, all code you mentioned go through nsMsgIIncomingServer to get the list of filters, and thus still works for remote filters. There are a few other cases, like the search folders. iirc, that uses filters too. But I think it's correct for that to go not be an other filter type. It's a special re-use of filter code, but doesn't work with the same set of rules. My bigger patch will implement a filterlist. As long as you implement the nsIMsgFilterList interface, the UI doesn't even have to know. The UI still works when editing a sieve filter list. That was the entire point of my approach :) I will need to make a few changes though, but that's only to make it work with filter lists that have an async nature (like stored on a network)

Could you provide an updated patch on bug 79525 that works with the patch for this current bug? I think it would be easier for me to evaluate the effectiveness of this current patch, if I can test it with your current full implementation.

I loaded the patch for bug 79525 (attachment 348619 [details] [diff] [review]), the patch v1 above, and set mail.server.default.filter.type equal to sieve. When I run the application, I don't get anywhere, and I get errors in the error console: Error: [Exception... "'JavaScript component does not have a method named: "getFilterNamed"' when calling method: [nsIMsgFilterList::getFilterNamed]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: loadStartFolder :: line 817" data: no] Source File: chrome://messenger/content/msgMail3PaneWindow.js Line: 817 Comments?

That error basically means that the sieve filter list is being used. I didn't implement the getFilterNamed function yet (because sieve filters have no name.) But looking closer, it reveals a problem. I was planning on just implementing the function some way or another, even if it doesn't really work. But the problem I'm seeing now is that the function is called from the spam stuff, and seems to be really used. I need to figure out what to do with that.

At this point, I'd like to wait for rkent's review before I sr this patch, and my impression from talking to him is that there are still issues preventing him from doing that. Based on that, I'm removing the sr? to get it out of my review queue for now; feel free to renominate once it's reviewed or if my understanding of the situation is somehow incorrect.

The problem is that the filterlist [1] is used for two things: the user defined filters and thunderbirds internal filters. The internal filters is things like looking at the servers spam headers to train the bayes filter. Now, the user defined filters should be pluggable, so they can be stored on a sieve server. The internal filters must stay internal. It is clear that those two usages should be split. Multiple options come to mind: - Add a nsIMsgIncomingServer::internalFilterlist attribute. This filterlist contains all the internal filters. Code needs to be added to actually execute the filters in there. - More flexible then above: add a list of filterlists. Just in case something wants yet another filter list. Does feel like designing just because it is theoretically better, not because it is easier or nicer in practice. - Add some kind of composite filterlist. This can then put all the call to the actual implementations. Advantage is that no callers need to be changed. Downside is that it add an extra layer of complexity. - Let the filterlists deal with it. If sieve does not want to store those filters, let it have an internal local list. Downside: filterlists can't really know where to put a certain filter. Leaks too much knowledge into the filter lists. With this in mind, I think the first option is the best. Any opinions? [1] http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgIncomingServer.idl#203

(In reply to comment #9) > > Multiple options come to mind: > - Add a nsIMsgIncomingServer::internalFilterlist attribute. This filterlist > contains all the internal filters. Code needs to be added to actually execute > the filters in there. > ... > > With this in mind, I think the first option is the best. Any opinions? > Why would you change the name of the existing parameter? It seems to me like you could add a new nsIMsgIncomingServer::externalFilterList parameter, and leave the existing parameter as it is. Would that work for you? It would still be helpful if I could see your existing SIEVE work in action, even if there are issues with it. I've never been able to get your patch to work. If you could come up with a working patch, that would help me a lot.

Creating a new internalFilterList was indeed a lot of trouble. There are a bit too many callers of that. Instead, I added a editableFilterList attribute. The UI calls that. It's usually the same as the old filterlist. But it's a different list if you use sieve. This way, all the internal filter still work, and you can edit your external filters.

I installed patches from bug 79525 and this bug, mucked around to get the fake sieve server running on port 2000, but I still had a blank message filter screen. I was however able to get it to start showing me something by changing the following line in nsMsgIncomingServer::GetEditableFilterList: + rv = mFilterList->SetFolder(msgFolder); to: + rv = mEditableFilterList->SetFolder(msgFolder); With that, I see 4 filters named "None" in the filter editor, with different definitions. But that then generates the mystery, why did I need to do that? That is, is your patch wrong, or am I setting something up incorrectly still?

Yes, you are right. It looks a whole lot more logical to have the change you suggested. It leaves me at a mystery why it worked for me though. For now, just assume your change is there. I'll update my patch for the next version. Thanks for testing!

As a general comment, I think there is likely to be a lot more refactoring of the filter interfaces before you can really do a good job of supporting external filters such as Sieve. The existing interfaces just make too many assumptions about the local nature of the filters. But that would be a difficult task, so I can understand why this simpler approach might be preferable in the short run. I see no harm in adding the customization hooks that you are requesting here. At http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/resources/content/AccountManager.js#334 there is a getFilterList. Shouldn't that also use the editable list? That is a hint to edit the lists. >+ nsIMsgFilterList getEditableFilterList(in nsIMsgWindow aMsgWindow); >+ void setEditableFilterList(in nsIMsgFilterList aFilterList); Current practice is to add Doxygen-style information for new or modified methods in .idl files. See for example the documentation of compactAll in nsIMsgFolder.idl Also, the whole editableFilterList concept needs better documentation in the code, probably in nsIMsgIncomingServer.idl. That should make it clear the conditions when these calls apply (which is only when certain preferences are set.) >+ // XXX Should the editable filterlist also be updated to match the >+ // new folder name? Yes. >+ >+ if (!filterType.EqualsLiteral("local")) { I wonder if "local" is the right name here. It makes sense for your use case - but not for the more general one where someone might want to have some local, custom filter list. Perhaps "standard"? >+ mEditableFilterList = do_CreateInstance(contractID.get(), &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIMsgFolder> msgFolder; >+ // use GetRootFolder so for deferred pop3 accounts, we'll get the filters >+ // file from the deferred account, not the deferred to account, >+ // so that filters will still be per-server. >+ rv = GetRootFolder(getter_AddRefs(msgFolder)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = mFilterList->SetFolder(msgFolder); As I mentioned earlier, this should be mEditableFilterList > >+// Message filter settings >+pref("mail.server.default.filter.type", "local"); >+pref("mail.server.default.filter.editable.separate", false); >+pref("mail.server.default.filter.editable.type", "local"); You should document what these mean. Also, would it be possible to eliminate the "separate" preference? Then if editable.type is missing or local (or my suggestion "standard") then you would return the default GetFilterList.

Thanks Kent, for the review. Sorry for the late reply... I updated the patch to your comments. > >+// Message filter settings > >+pref("mail.server.default.filter.type", "local"); > >+pref("mail.server.default.filter.editable.separate", false); > >+pref("mail.server.default.filter.editable.type", "local"); > > You should document what these mean. Also, would it be possible to eliminate > the "separate" preference? Then if editable.type is missing or local (or my > suggestion "standard") then you would return the default GetFilterList. I thought about that, but I think it won't work. There might be cases where both lists are of the same type, but not really the same. It would also cause problems with the setFilterList methods. I don't think it's so bad to have the extra pref.

I tried this tonight. I loaded patch v3 above, and then attachment 363486 [details] [diff] [review] (patch v4)from bug 79525. But when I try to run it and connect with the server, I am getting: Created SIEVE filter list Fetching filters getScriptList started Rebuild filter list list: [xpconnect wrapped nsIMsgFilterList @ 0xa39fcd8 (native @ 0x6a43da8)] v: 1 count: 0 ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "Component returned failure code: 0x804b000d (NS_ERROR_CONNECTION_ REFUSED) [nsIScriptableInputStream.available]" nsresult: "0x804b000d (NS_ERROR_ CONNECTION_REFUSED)" location: "JS frame :: file:///S:/tb/test/tb-debug/mozilla /dist/bin/components/nsManageSieve.js :: anonymous :: line 410" data: no] ************************************************************ --WEBSHELL 06E0A010 == 10 Do you know what's wrong? Also, the patch was not uploaded properly, so the normal web-based diff view is not available. Could you please fix that?

I fixed the attachment, it's a patch now. For the error, I'm not sure what is going on. But any way, it comes from the sieve patch, and not the patch in this bug. Do you think you can review the patch here as it is now, or do you need a working usecase for it? It would be easier for me if both patches could develop independently.

I would always prefer to run a patch to see it in action, but I did that earlier so perhaps it is not needed now. So I'll review the code without solving the issues with that other patch. As a general comment, like I said in comment 14, I think in the long run we will want a different approach to this. SIEVE is to me just another context for filtering. There has been some minimal beginnings of support for contexts in the "Checking mail or manual" stuff. I'm doing more in bug 198100. I think eventually SIEVE should just be another context that can be defined for a filter list, so that you could then run SIEVE as well as other filters contexts on messages from the server. Otherwise, I mostly have nits. + * + * @see nsIMsgIncomingServer.idl for more information on the different + * filterlists */ nsIMsgFilterList getFilterList(in nsIMsgWindow msgWindow); void setFilterList(in nsIMsgFilterList filterList); + nsIMsgFilterList getEditableFilterList(in nsIMsgWindow aMsgWindow); + void setEditableFilterList(in nsIMsgFilterList aFilterList); You still need DOxygen documentation on these calls + + /** + * Get user editable filter list. This does not have to be the same as + * the filterlist above, typically depending on the users preferences. + * The filters in this list are not processed, but only to be edited by + * the user. + * @see getFilterList + */ + nsIMsgFilterList getEditableFilterList(in nsIMsgWindow aMsgWindow); + + /** + * Set user editable filter list. + * This does not persist the filterlist, @see setFilterList + * @see getEditableFilterList + */ + void setEditableFilterList(in nsIMsgFilterList aFilterList); You need the @param documentation for the calls above. + // XXX Should the editable filterlist also be updated to match the + // new folder name? In my first review, I commented "Yes" to this line - which meant that I expected you to add the code that would update the folder name on the editable filter list. Is there some reason that you don't want to do that? + if (!filterType.EqualsLiteral("default")) { + nsCAutoString contractID("@mozilla.org/filterlist;1?type="); The preferred style in mailnews C++ is to have the bracket on a new line. + nsresult rv = GetBoolValue("filter.editable.separate", &editSeparate); + if (NS_FAILED(rv) || !editSeparate) { + return GetFilterList(aMsgWindow, aResult); + } No brackets here around the return statement, because it is a single line. + rv = GetRootFolder(getter_AddRefs(msgFolder)); + NS_ENSURE_SUCCESS(rv, rv); + + rv = mEditableFilterList->SetFolder(msgFolder); + NS_ENSURE_SUCCESS(rv, rv); + + NS_ADDREF(*aResult = mEditableFilterList); + return NS_OK; + } The blank lines here have spaces in them, please remove. You might check all of your patch for this. +// Is there a seperate list with for editable filters? sic, should be "separate"

Kent, thanks for doing the review. I attached an updated patch, to fix the review comments.

A few remaining nits: + /** + * Get user editable filter list. This does not have to be the same as + * the filterlist above, typically depending on the users preferences. + * The filters in this list are not processed, but only to be edited by + * the user. + * @see getFilterList + * + * @param aMsgWindow @ref msgwindow "The standard message window" + */ + nsIMsgFilterList getEditableFilterList(in nsIMsgWindow aMsgWindow); + You need to document the @return. Yes I know it is rather trivial, but we should consistently mention all parameters and returns in the idl DOxygen. + + /** + * Get user editable filter list. This does not have to be the same as + * the filterlist above, typically depending on the users preferences. + * The filters in this list are not processed, but only to be edited by + * the user. + * @see getFilterList + * + * @param aMsgWindow @ref msgwindow "The standard message window" + */ + nsIMsgFilterList getEditableFilterList(in nsIMsgWindow aMsgWindow); + You need to document the @return here as well. + nsCString filterType; + rv = GetCharValue("filter.type", filterType); + NS_ENSURE_SUCCESS(rv, rv); + + if (!filterType.EqualsLiteral("default")) + { + nsCAutoString contractID("@mozilla.org/filterlist;1?type="); + contractID += filterType; + ToLowerCase(contractID); + mFilterList = do_CreateInstance(contractID.get(), &rv); + NS_ENSURE_SUCCESS(rv, rv); + You have extra spaces in the blank line above, please remove. + rv = mFilterList->SetFolder(msgFolder); + NS_ENSURE_SUCCESS(rv, rv); + ditto + +NS_IMETHODIMP +nsMsgIncomingServer::GetEditableFilterList(nsIMsgWindow *aMsgWindow, nsIMsgFilterList **aResult) +{ + if (!mEditableFilterList) + { + PRBool editSeparate; + nsresult rv = GetBoolValue("filter.editable.separate", &editSeparate); + if (NS_FAILED(rv) || !editSeparate) + return GetFilterList(aMsgWindow, aResult); + more extra spaces r=me with these nits resolved.

I neglected to mention that the patch v4 is bit rotted, and needs updating with current versions of the code. I fixed the bitrot before I did my review.

Comment on attachment 374790 [details] [diff] [review] patch v4 Consider rkent's comments to be fixed.

Comment on attachment 374790 [details] [diff] [review] patch v4 sorry for the delay - one nit - I think you need to add an NS_ENSURE_ARG_POINTER(aResult) somewhere, either in the server or folder impls, or both: nsMsgIncomingServer::GetEditableFilterList(nsIMsgWindow *aMsgWindow, nsIMsgFilterList **aResult) + sr=bienvenu with that fixed.

Patch for check-in. Carrying forward r and sr.

Patch checked in