Run Junk Mail Controls (on selected messages) (bug 195315) does not work. STR: 1. select some messages 2. a) context menu: mark -> run junk mail controls, or b) main menu: message -> mark -> run junk mail controls expected results: analyze and junk filter selected messages actual results: An error occurred executing the cmd_recalculateJunkScore command [Exception... "'[JavaScript Error: "analyzeMessagesForJunk is not defined" {file: "chrome://messenger/content/mail3PaneWindowCommands.js" line: 598}]' when calling method: [nsIController::doCommand]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 93" data: yes] work-around: move selected messages to a temporary folder and run junk controls on that folder looks like the analyzeMessagesForJunk function was removed from mailnews/base/resources/content/mailCommands.js by 1.83 neil%parkwaycc.co.uk 2004-04-16 03:24 Bug 193625 Option to mark incoming junk mail as read p=eyalroz@technion.ac.il r=me sr=bienvenu and from mail/base/content/mailCommands.js by its tb port 1.21 gavin%gavinsharp.com 2005-08-14 05:59 Bug 253234: Add option to set junk mail as read, backend patch by Jens Bannmann <jens.b@web.de>, r=dmose, sr=bienvenu Presumably the bug is present in seamonkey as well, if it has the corresponding ui.

What probably happened is that originally, http://lxr.mozilla.org/mozilla/source/mailnews/base/resources/content/mailCommands.js had this: // not used yet, but soon function analyzeMessagesForJunk() { var messages = GetSelectedMessages(); analyzeMessages(messages); } and it was removed with my patch from 193625. If someone wanted it back, they should have rewritten it, using the function function analyzeMessageForJunk(aMsgHdr, aMsgIndex, aJunkMsgIndices, aLastMessage, aWhiteListDirectory) since analyzeMessages() and analyzeMessage() no longer exist. Unless, of course, this has been broken since 2004, in which case one wonders whether it isn't for the best to simply remove that item from the menus - since it seems it has not been used by anyone since.

> and it was removed with my patch from 193625. If someone wanted it back, they > should have rewritten it, using the function I'm not sure about "they", since it looks like that broke an existing feature added two months earlier, regardless of the apparently stale comment... > Unless, of course, this has been broken since 2004, in which case one wonders > whether it isn't for the best to simply remove that item from the menus - since > it seems it has not been used by anyone since. Right, I didn't find a bug reported on it, and the temporary-folder workaround makes it possible, if a bit convoluted, to get the same results without the specific ui -- otoh the analysis failing to run isn't necessarily distinguishable from the analysis resulting in the same classifications as before, so people using the feature might have just not noticed it being broken.

Comment on attachment 216746 [details] [diff] [review] Proposed patch this worries me - the junk mail plugin might be happier with us calling ClassifyMessages on all the messages at once instead of calling ClassifyMessage in a tight loop. The junk mail plugin will in turn serialize the requests. With this patch, if you run this command with 10 imap messages selected, we'll fire off 10 imap urls all at once. It will probably work, but it's much nicer to serialize them. Also, TB doesn't have analzyeMessageForJunk in its mailCommands.js The problem with using ClassifyMessages is that you'll need to be able to map back to a msg hdr from a uri in onMessageClassified - to do that from js, you can use nsIMessenger's msgHdrFromURI method.

Comment on attachment 216746 [details] [diff] [review] Proposed patch Yeah, what David said. Please rewrite analyzeMessagesForJunk().

The resulting classification component depends on whether classification has to be able to cope with virtual folders.

Note that I occasionally get the following problem running junk mail controls on an IMAP folder, which seems to mean that I don't get enough callbacks. ###!!! ASSERTION: syntax error in generic parser: '!error', file nsIMAPGenericParser.cpp, line 92 ###!!! ASSERTION: nsStreamListenerTee not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsStreamListenerTee.cpp, line 40 ###!!! ASSERTION: nsStreamListenerTee not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsStreamListenerTee.cpp, line 40 ###!!! ASSERTION: TokenStreamListener not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsBayesianFilter.cpp, line 735 ###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 110

You only have to read half of the patch, as the other half just keeps SeaMonkey in sync with Thunderbird (I lied, I wrote it the other way around; bug 253234 diverged the code slightly although this patch only syncs up the classifier). I moved some of the shared code in to the message classifier constructor, and instead of sending all messages for classification immediately the notification callback is used to trigger the next classification. When the last message has been classified I set all the junk scores and perform the spam actions.

(In reply to comment #8) 1. I would like to see nice long comments explaining the intended flow of execution, e.g. instead of: if (this.msgHdrArray.length == 1) this.analyzeNextMessage(); I would write: if (this.msgHdrArray.length == 1) { /* the junk mail plugin is currently inactive, and this is the first message to enter the queue, so we start it working (asynchronously); */ this.analyzeNextMessage(); } /* at this point, either there are no messages to classify or the plugin is already working on some message */ 2. I would rename the arrays to reflect the fact that they're queues. How about an array of URI and view index pairs? Or is that too performance-wasteful? And isn't there some queue class in js (or in mozilla specifically) other than an array? 3. Even if the classifier acts asynchronously, why do we need to have many function calls pushing single message URIs and view indices into the array? Can't they all be pushed together (in that case, the "== 1" will move to a "== 0" check earlier in the code to distinguish the case of beginning with an empty queue). 4. GetLoadedMsgFolder().setJunkScoreForMessages(this.junkMsgHdrs, "100"); shouldn't we avoid these magic numbers?

(In reply to comment #9) >I would rename the arrays to reflect the fact that they're queues. How about an >array of URI and view index pairs? Or is that too performance-wasteful? And >isn't there some queue class in js (or in mozilla specifically) other than an >array? The classified headers already have to go in separate arrays. But I was basing the code on the old Suite version, not the Thunderbird version which already uses the array of header (not URI) and view index pairs. >Even if the classifier acts asynchronously, why do we need to have many >function calls pushing single message URIs and view indices into the array? >Can't they all be pushed together (in that case, the "== 1" will move to a "== >0" check earlier in the code to distinguish the case of beginning with an empty >queue). Group By Sort means that a given view index might not actually correspond to a message, so I can't even use the array of selected indices. However if you prefer I can build up the arrays in the caller and pass them in all at once. >GetLoadedMsgFolder().setJunkScoreForMessages(this.junkMsgHdrs, "100"); I can make these (k)onstants.

Changes made: * Stored the incoming messages in a single array * Made the junk scores constants * Added some meagre comments (Dammit Jim, I'm a programmer, not a tech writer!)

(In reply to comment #11) Insufficient comments IMHO. Please explain the flow of execution in a decent-size comment before each function / method. Please make it clear that the index-URI-pair array is used as a queue. Also, what about replacing for (var i = 0; i < indices.length; i++) { var msgURI = view.getURIForViewIndex(indices[i]); var messageService = messenger.messageServiceFromURI(msgURI); var msgHdr = messageService.messageURIToMsgHdr(msgURI); messageClassifier.analyzeMessage(msgHdr, indices[i]); } with something like var msgURIs = view.getURIsForViewIndices(indices[i]); /* isn't it always the same service? */ var messageService = messenger.messageServiceFromURI(msgURIs); var msgHdrs = messageService.messageURIsToMsgHdrs(msgURIs); messageClassifier.analyzeMessages(msgHdrs, indices[i]); } As for the constants - surely they must be defined elsewhere, globally?

>/* isn't it always the same service? */ Yes, it's always the same service. But I can't do any other optimization because group by sort means that a particular index may not refer to a message. >As for the constants - surely they must be defined elsewhere, globally? nsMsgDBView.cpp just uses == nsIJunkMailPlugin::JUNK ? "100" : "0"

Comment on attachment 217753 [details] [diff] [review] Updated for review comments thx, Neil.

Also caching the message service. No, I can't do it before the loop, because I might not actually have any real messages selected.

(In reply to comment #15) > No, I can't do it before the loop, because I > might not actually have any real messages selected. But in that case, isn't count == 0 anyway? Also, what about the rest of my suggested modifications from comment 12?

(In reply to comment #16) >(In reply to comment #15) >>No, I can't do it before the loop, because I >>might not actually have any real messages selected. >But in that case, isn't count == 0 anyway? Also, what about the rest of my >suggested modifications from comment 12? Comment 13 was supposed to cover that, unless I misunderstood and you wanted me to duplicate the addressbook checking code so that I can build up a single array of messages to pass to the classifier.

> Comment 13 was supposed to cover that I don't see that it covered my first question in comment 16... > But I can't do any other optimization because group by sort > means that a particular index may not refer to a message. I don't understand what this means. Why can't you get the message service first, then do everything vectorized as I suggested in comment 12? >nsMsgDBView.cpp just uses == nsIJunkMailPlugin::JUNK ? "100" : "0" So that has to be fixed to, doesn't it? :-)

(In reply to comment #18) >I don't understand what this means. Why can't you get the message service >first, then do everything vectorized as I suggested in comment 12? Because I can't guarantee that the selection is on a real message.

(In reply to comment #18) >So that has to be fixed to, doesn't it? :-) You'd better file a backend bug then ;-)

> You'd better file a backend bug then ;-) Filed bug 336615.

This bug seems to have gone inactive, yet the patches are critical for future junkmail work. Is there any way that this could be moved forward? Also, TB is not affected on MOZILLA_1_8_BRANCH but the suite is. This bug occurs in the current 1.1 Seamonkey release candidate. Perhaps a separate bug should be filed for the Seamonkey branch version?

(In reply to comment #21) > > You'd better file a backend bug then ;-) > Filed bug 336615. This has landed, can the work go on for that bug?

Bug 336615 was cosmetic, and doesn't really help much with progress on this bug. I requested blocking though on TB3 since this is non-working exposed functionality. Neil, do you have any plans to try to move this forward?

(In reply to comment #25) > Neil, do you have any plans to try to move this forward? Not as such, no... Someone could unbitrot it/update it for bug 336615 and ask for review - Mnyromyr would probably be the best reviewer at this point.

Marking as blocking for the reason Kent suggests.

I'll do a post- bug 366491 version of this (since they will bit rot each other).

(In reply to comment #28) > I'll do a post- bug 366491 version of this (since they will bit rot each > other). If you want to do a SeaMonkey version of that patch too, you'll need to port at least bug 373270. In mailCommands.js the functions determineActionsForJunkMsgs, messageClassifier and filterFolderForJunk are different or missing from SeaMonkey's version. I did a WIP patch to make TB and SM on par until I discovered there are a few bugs open, which would change part of the code anyway (like that one and the one you mentioned). If you want it leave a message for me on irc.

If I understand you correctly, bug 373270 was fixed on TB only, and still needs to be fixed in SM (is there a bug filed for that?) but interactions with various other bugs are making this difficult. Correct me if I am wrong. My intention is to provide patches for TB and SM. I'll look into the issue of porting the bug 373270 changes to SB mailCommands.js that you mentioned when I move forward with this. Right now I'm hoping to keep things stable enough to prevent bitrot on bug 366491 before it lands. This forking of the js code really makes this stuff challenging. Do you know if there is any practical way or discussions to unfork some of these functions?

(In reply to comment #30) > If I understand you correctly, bug 373270 was fixed on TB only, and still needs > to be fixed in SM (is there a bug filed for that?) but interactions with > various other bugs are making this difficult. Correct me if I am wrong. That's correct and no, there's no bug for it (at least I don't know of one). > This forking of the js code really makes this stuff challenging. Do you know if > there is any practical way or discussions to unfork some of these functions? The easiest thing to do could be to fix the code in TB and copy the junk related functions 1:1 to SeaMonkey and get all missing changes in one step. On the long run, it's probably the best to think about an unfork of junk-related controls. But that's a thing MoMe-people should talk about with Mnyromyr and Neil on irc.

I've updated Neil's patch, and moved all of the junk-related fucntions to a new junkCommands.js which can be shared with SM. But this patch only covers TB. Things to note: 1) When manually running junk controls on a folder, I do not process messages that have previously been marked by the user. The theory here is that user marking is higher value than the bayes filter. Also, a common use case might be that someone trains a few messages, then reruns the controls. You don't want to undo the set junkscore of the user-trained messages in that case. 2) Previously messages that were whitelisted were simply ignored. Now, I set the correct junkstatusorigin and junkscore for them. 3) I did not add junkCommands.js to all of the existing places where mailCommands.js appears, only to the main window. I think that is correct, but int might be worth checking. 4) I added a statusbar message that shows % processed of the junk. In some cases, this alternates with a "Downloading messages" message from IMAP. This might not be desireable to all, but it would be my preference. 5) I removed some junk strings from messenger.properties that I could not find in the current tree 6) I noted but did not correct the existing error, that the code does not work correctly with XF saved searches.

Comment on attachment 323942 [details] [diff] [review] Same approach as Neil, updated and deforked >+ this.percentDonePrevious = percentDone; >+ var percentStr = percentDone + "%"; >+ window.MsgStatusFeedback.showStatusString(gMessengerBundle. >+ getFormattedString("junkAnalysisPercentComplete", [percentStr])); IMHO you should use MsgStatusFeedback.showProgress(percentDone);

(In reply to comment #34) > (From update of attachment 323942 [details] [diff] [review]) > IMHO you should use MsgStatusFeedback.showProgress(percentDone); > I tried that. The problem is that .showProgress is being continually sent by the download process, so the progressmeter flickers between the junk value, and the download value. I could not figure out any way to turn that off.

(In reply to comment #35) > I tried that. The problem is that .showProgress is being continually sent by > the download process, so the progressmeter flickers between the junk value, and > the download value. I could not figure out any way to turn that off. We obviously need an API that can download a bunch of messages in one hit for us to process them for junk - that API would also provide correct progress :-)

It may be that downloading and analyzing for junk is slow enough that this doesn't matter, but usually when we do code like this we also check that a certain amount of time has past since the last time we updated the percent done, so that we don't update the percent the last time it changed was less than a second or so... + if (percentDone != this.percentDonePrevious) + { + this.percentDonePrevious = percentDone; + var percentStr = percentDone + "%"; + window.MsgStatusFeedback.showStatusString(gMessengerBundle. + getFormattedString("junkAnalysisPercentComplete", [percentStr])); + }

(In reply to comment #37) > .. usually when we do code like this we also check that a > certain amount of time has past since the last time we updated the percent > done I'll be happy to fix this. Do you have any other comments though before I revise the patch?

Comment on attachment 323942 [details] [diff] [review] Same approach as Neil, updated and deforked I'm unconvinced that this updates the /Junk /NonJunk keywords on the imap server (my guess is that was always broken). Other than that, this patch looks OK, but we might want a new bug for that. Could you verify that's the case? It would seem to me that we should be keeping track of which messages changed junk state (from junk to non junk or vice versa) and update on the server...

I revised the code to only post the status message once per second per comments. It's ready to checkin. Concerning the issue of setting IMAP keywords, this patch does not do that. I will file a follow-up bug for that. Also, we need a follow-up bug to support XF search folders, which will not work correctly with the existing code.

I defined bug 438654 for the IMAP keywords issue, and bug 438656 to support XF searches correctly. Also setting the checkin-needed keyword. There' also no real reason that we can't do a unit test of this, so I've added the flag as a reminder of that.

Checking in mail/base/jar.mn; /cvsroot/mozilla/mail/base/jar.mn,v <-- jar.mn new revision: 1.111; previous revision: 1.110 done Checking in mail/base/content/mailCommands.js; /cvsroot/mozilla/mail/base/content/mailCommands.js,v <-- mailCommands.js new revision: 1.51; previous revision: 1.50 done Checking in mail/base/content/mailWindowOverlay.xul; /cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v <-- mailWindowOverlay.xul new revision: 1.245; previous revision: 1.244 done Checking in mail/locales/en-US/chrome/messenger/messenger.properties; /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messenger.properties,v <-- messenger.properties new revision: 1.55; previous revision: 1.54 done RCS file: /cvsroot/mozilla/mailnews/base/resources/content/junkCommands.js,v done Checking in mailnews/base/resources/content/junkCommands.js; /cvsroot/mozilla/mailnews/base/resources/content/junkCommands.js,v <-- junkCommands.js initial revision: 1.1 done

Comment on attachment 324655 [details] [diff] [review] patch to check in [Checkin: Comment 42] Just some thoughts in passing ;-) >+ var copyService = Components.classes["@mozilla.org/messenger/messagecopyservice;1"]. >+ getService(Components.interfaces.nsIMsgCopyService); With the full Components.classes my preference is to use .getService i.e. Components.classes["@mozilla.org/messenger/messagecopyservice;1"] .getService(Components.interfaces.nsIMsgCopyService); >+ // empty the processed array in case more messages are added >+ this.mJunkMsgHdrs.clear(); Is this still useful given that we create a new instance every time? >+function filterFolderForJunk() >+{ processFolderForJunk(true);} I think this is possibly as ugly as you could make it. If you want to save space, my preference would be for function filterFolderForJunk() { processFolderForJunk(true); } >+ var tmpMsgURI = gDBView.getURIForViewIndex(0); Hmm, what happens in group view when the first index is a dummy header? >+ var msgClassifier = new MessageClassifier(tmpMsgHdr.folder, totalMessages); Hmm, what if the total count is incorrect due to dummy headers? >+ try >+ { >+ var msgURI = gDBView.getURIForViewIndex(index); >+ } >+ catch (ex) >+ { >+ // blow off errors here - dummy headers will fail >+ var msgURI = null; >+ } >+ if (msgURI) >+ { >+ var msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI); >+ msgClassifier.analyzeMessage(msgHdr, whiteListDirectory); >+ } This looks very weird to me, I would have written something like this: try { var msgURI = gDBView.getURIForViewIndex(index); var msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI); msgClassifier.analyzeMessage(msgHdr, whiteListDirectory); } catch (ex) { // blow off errors here - dummy headers will fail }

Reopening to fix Neil's concerns. As he suspected, the checked in patch does not work in group view.

I addressed all of Neil's issues except this: >+ var msgClassifier = new MessageClassifier(tmpMsgHdr.folder, totalMessages); Hmm, what if the total count is incorrect due to dummy headers? The totalMessages are only used to calculate the percentage complete in the status display. I don't believe it is worth another pass through the indices to detect dummy headers to correct his.

Comment on attachment 326553 [details] [diff] [review] Fixed Neil's nits plus groupview problems >+ var copyService = Components.classes["@mozilla.org/messenger/messagecopyservice;1"] >+ .getService(Components.interfaces.nsIMsgCopyService); >+ this.mJunkMsgHdrs = Components.classes["@mozilla.org/array;1"] >+ .createInstance(Components.interfaces.nsIMutableArray); This is difficult to demonstrate in Bugzilla given its tendency to wrap but the idea is that the .getService/.createInstance should line up with the .classes >+ window.MsgStatusFeedback.showStatusString(gMessengerBundle >+ .getFormattedString("junkAnalysisPercentComplete", [percentStr])); I wasn't really concerned about these are they're really difficult ;-) window.MsgStatusFeedback.showStatusString( gMessengerBundle.getFormattedString("junkAnalysisPercentComplete", [percentStr])); is technically my preferred solution, but I'd leave it as it was before now. >+function filterFolderForJunk() { processFolderForJunk(true);} >+function analyzeMessagesForJunk() { processFolderForJunk(false);} Space between ; and } please (like I said!) >+ // dummy headers will fail, so look for another >+ continue; Although not strictly necessary I guess this makes it more readable.

I fixed the last few nits, so the patch should be ready to check in.

Comment on attachment 217284 [details] [diff] [review] Prevent running junk mail controls on search results [Checkin: Comment 50] {{ 2006-04-05 08:26 neil%parkwaycc.co.uk ... Don't run junk mail controls on virtual folders b=324953 r+sr=bienvenu }}

(In reply to comment #50) > (From update of attachment 217284 [details] [diff] [review]) > {{ > 2006-04-05 08:26 neil%parkwaycc.co.uk ... Don't run junk mail > controls on virtual folders b=324953 r+sr=bienvenu > }} > I don't understand this comment.

(In reply to comment #51) > (In reply to comment #50) > > (From update of attachment 217284 [details] [diff] [review] [details]) > > {{ > > 2006-04-05 08:26 neil%parkwaycc.co.uk ... Don't run junk mail > > controls on virtual folders b=324953 r+sr=bienvenu > > }} > > > I don't understand this comment. He's pointing out when I checked it in, since I didn't mention it myself.

Bug 440616 both bit rotted the last patch, as well as broke the functionality of junkCommands.js (as reported in bug 441987). This updated patch fixes the bit rot, and removes the extraneous call to getJunkmailComponent.

Comment on attachment 326870 [details] [diff] [review] [checked in] un bitrot, remove getJunkMailComponent call Checking in mailnews/base/resources/content/junkCommands.js; /cvsroot/mozilla/mailnews/base/resources/content/junkCommands.js,v <-- junkCommands.js new revision: 1.3; previous revision: 1.2 done

This FIXED bug is flagged with in‑testsuite? It would be great if assignee or someone else can clear the flag if a test is not appropriate. And if appropriate, create a test and plus the flag to finish off the bug.

added as https://litmus.mozilla.org/show_test.cgi?id=7721 in litmus - coomment correct if test is not proeprly covering the bug.

This is in-litmus, so cancelling the in-testsuite option for now, if we later move the test from litmus to mozmill (or something) then we can handle that in a separate bug/location.