I selected 1500 msgs, then said "run junk mail controls on selected msgs" (apart from a handful of stalls/starts) I couldn't actually get it to stop categorizing when I realized it was mis categorizing more often that correct.

Looks like this is generally the same issue as reported in bug 179503 and bug 179504. The lack of UI to indicate progress is causing confusion about state, and inability to halt execution of categorization.

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

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

We need to do a better job of showing progress (at least for marking), and make any of these operations interruptible. Additionally, shutdown of the app while any of these things are going on needs to be dealt with.

We could use the status bar and progress meter to indicate what is happening. Cc'ing robin for wording suggestions. Would enabling the Stop button during this action make sense?

So, as I understand it, it should be possible to start a "mark" session running, and then continue doing other stuff with mail, including possibly switching folders. This would seem to have some UI implications. David/Seth can you confirm that this should work?

you can't read other messages in the same folder since we don't allow multiple simultaneous connections to the same folder. You could switch folders, however. Pressing stop should stop any running url, however. If the scoring code would chain the urls instead of firing them off all at once, and check if the stop button had been pressed before running each url, then we could make the stop button work. It might even stop all running urls if we attached the urls to the right msg window and load group, I'm not sure.

So, in particular, the ability to switch folders would seem to make the suggested UI a bit confusing, assuming that the throbber continues to cycle as URLs are being in the background. Enabling the stop button would be an excellent thing, however.

Perhaps a variation on the File Download box could be used? This would be handy for showing multiple operations running simultaneously (large folder copies from one IMAP server to another, Junk training, etc). This would also eliminate the need for the Mail window to remain open while these long-running operations proceed.

I am having trouble with this (or a similar) issue. I have some folders where I know all the messages are not junk, or are junk. I would like to be able to mark them as such, but the only controls I have are : 1) the 'junk' button, which toggles between junk and not junk - if you want to mark lots of messages not junk, you first have to mark them as junk, which takes twice as long as necessary. 2) clicking on the 'junk' icon for each message - again, you have to click twice to make it not junk and this method doesn't work for lots of messages. IMO, what we need is a little side menu on the 'junk' button, like with the 'Mark' button, which has the options, 'Mark Junk', 'Mark Not Junk', or something similar. Also, these options should be on the Message menu. As a side note, IMO the 'Mark' button should also have options for 'unread' - it has a way to mark messages are read, but not as unread, which can be a pain since it takes twice as long as necessary. Comments? Max.

Another point. I don't much like the Apple Mac 10.2.2 Mail application (commonly referred to as mail.app), but it does have a few nice features, one of which is applicable to this discussion. It has an 'activity viewer', which shows what activities are being performed and the progress with each - and also a 'stop' button for each. Just thought it would be great if mozilla had such a feature. It's worth taking a look. Max.

this patch changes the filter plugin api to pass in a msg window so that we can get progress and feedback when fetching messages to score them. This makes it much more obvious that we're fetching messages. I still need to make the urls chained instead of simultaneous.

Can we spin http://bugzilla.mozilla.org/show_bug.cgi?id=179162#c10 as a separate bug report or is this bug covering all the UI for junk controls? I just downloaded the trunk for the first time (had been using branch) and my first experience was trying to select all of my msgs (over 1000) in my Inbox (and other folders) and toggling them to show "not junk mail" in the thread pane. I also would like a UI to be able to do these two actions at once: Mark as Junk Mail and Delete. I find that I have to do two actions now to my junk mail which comes into my Inbox. There was considerable lapse time where the cursor was busy and then I had to toggle first to junk and then not junk mail. (The cursor busy is probably what David is fixing in his previous comment.)

Comment on attachment 107377 [details] [diff] [review] patch to add msgWindow to spam score urls r=dmose

Comment on attachment 107377 [details] [diff] [review] patch to add msgWindow to spam score urls sr=sspitzer

this is work in progress to chain message fetches in the bayesian spam filter plugin, and to call classifyMessages for imap code. I still need to implement streamMessage for mailbox and news, and to convert the mailbox code to call classifyMessages. I've made streamMessage a message service method so that the plugin code doesn't need to know how to set up the converter and set up the special header, etc... I had to reorganize the spam filter plugin a little to enable it to do multiple messages, though the actual chaining is only a few lines of code.

This fix chains message fetches when calling ClassifyMessages in the bayesian filter plugin. I added a StreamMessage method to the message service and implemented it in each of our three message services (this method can be used by other components besides the filter plugins). It optionally allows you to convert the data, and allows you to attach an arbitrary header. Using the message service also makes it so we will automatically download the spam messages for offline use if the folder is configured for that. I also cleaned up a leak in the mailbox protocol channel handling, and cleaned up some code. Dan and Seth, can I get some reviews? Thx.

Comment on attachment 107946 [details] [diff] [review] proposed fix Looks good. Pretty much all of my comments are minor things. r=dmose once they've been addressed. Some of the new lines you've added in your patch have raw tabs rather than spaces. + // this method streams a message to the passed in consumer. If aConvertData is true, it + // will create a stream converter from message rfc822 to star/star. It will also tack + // aAdditionalHeader onto the url (e.g., "header=filter"). Can you use doxygen style comments here (including @param or @arg for each parameter), so that stuff like http://unstable.elemental.com/mozilla/ will pick up this info automagically? + void streamMessage(in string aMessageURI, in nsISupports aConsumer, + in nsIMsgWindow aMsgWindow, + in nsIUrlListener aUrlListener, + in boolean aConvertData, + in string aAdditionalHeader, + out nsIURI aURL); How about using |ACString| instead of |string|? This allows us to conceivably avoid unnecessary allocations now, and, in the future, it should allow us to use nsSharableCString which will avoid copying entirely by having refcounted buffers. This comment also applies to the new |string|s in nsIImapService.idl. + for ( PRUint32 freeIndex=0 ; freeIndex < numMessagesToClassify ; ++freeIndex ) + PR_Free(messageURIs[freeIndex]); Since the strings in messageURIs came from an XPCOM getter, they should be freed using the XPCOM shared allocator (ie nsMemory::Free). + PR_Free(messageURIs); If you changed the array itself to be allocated with nsMemory::Alloc instead of PR_MALLOC, you could replace all three of the above lines with a single call to the NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY() macro from nsMemory.h. + for (PRInt32 i = 0; i < aNumMessagesToClassify; i++) + mMessageURIs[i] = strdup(aMessageURIs[i]); Sadly, we still need to use PL_strdup at the moment since Mac CFM builds don't supply strdup by default. Thankfully, CFM builds are moving to ports status soon. + imapUrl->SetFetchPartsOnDemand(PR_FALSE); This isn't going to cause non-text parts to be fetched, is it? + nsCOMPtr<nsIChannel> channel; + QueryInterface(NS_GET_IID(nsIChannel), getter_AddRefs(channel)); Using do_QueryInterface here is preferred these days because it's type-safe. This also applies to the NNTP changes. + aURIString.FindChar('?') == kNotFound ? aURIString += "?" : aURIString += "&"; It's probably a little faster to use character constants than string constants here. This also applies to the NNTP changes. + aURIString += "header="; + aURIString += aAdditionalHeader;; Condensing these into a single line (ie |aURIString += "header=" + aAdditionalHeader|) may avoid an unnecessary allocation. This also applies to the NNTP changes.

>+ nsCOMPtr<nsIChannel> channel; >+ QueryInterface(NS_GET_IID(nsIChannel), getter_AddRefs(channel)); >Using do_QueryInterface here is preferred these days because it's >type-safe. This also applies to the NNTP changes. It doesn't compile, unfortunately, since the cast to nsISupports is ambiguous due to multiple inheritance.. >+ imapUrl->SetFetchPartsOnDemand(PR_FALSE); >This isn't going to cause non-text parts to be fetched, is it? yes, it will, but we were fetching the whole message anyway - we weren't use mime parts on demand for the spam scoring - mime itself strips out the non-text parts. I'm not sure if the junk plugin will behave if we don't do that, but we can experiment later. mpod also affects text parts. I'll look at my editor settings - it might be putting tabs in idl files. I'll try to use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY() - I hate writing those little loops. as far as using strings is concerned, I think we should convert all of those at once.

Comment on attachment 107946 [details] [diff] [review] proposed fix sr=sspitzer, small nits: 1) + nsresult rv = NS_OK; + nsCOMPtr<nsIMsgFolder> folder; + nsXPIDLCString msgKey; + nsXPIDLCString mimePart; + nsCAutoString folderURI; + nsMsgKey key; + + rv = DecomposeImapURI(aMessageURI, getter_AddRefs(folder), getter_Copies(msgKey)); do nsresult rv = DecomposeImapURI(aMessageURI, getter_AddRefs(folder), getter_Copies(msgKey)); 2a) + aURIString += aAdditionalHeader;; extra semi-colon 2b) + aURIString += aAdditionalHeader;; extra semi-colon 3) as dmose pointed out, fix the strdup(), so that mac won't go red.

> as far as using strings is concerned, I think we should convert all of those at > once. You're right that someday we will need to do a big monster fix to retroactively change all the stuff that's currently |string|. But given that we still have performance worries (and since noone that I'm aware of has any immediate plans to do the big fix), continuing to use |string| when adding stuff to interfaces and creating new interfaces makes life in the interim unnecessarily slow.

there aren't any extra allocations or copies of strings because of this code - the calling code already has a const char * uri - wrapping it in an nsACString would add more code because we'd need to construct the nsACString and destroy it. I understand your point in general but I don't think it applies here.

Good point. OK, ignore that and the QueryInterface comment then.

taking, though I know I'm going to regret it :-) I'd also like to fix the progress so that it we get progress based on how many messages we've scored.

Small note: + nsCOMPtr<nsIChannel> channel; + QueryInterface(NS_GET_IID(nsIChannel), getter_AddRefs(channel)); The typesafe version of that would be: nsCOMPtr<nsIChannel> channel; CallQueryInterface(this, NS_STATIC_CAST(nsIChannel**, getter_AddRefs(channel)));

Using 2002120309 I'm unable to see evidence of "Use Junk Mail Controls on Selected Messages" actually doing anything with IMAP (this used to work). Is it possible that this patch has somehow broken the ability to select multiple messages and classify them?

yes, it does seem broken, but I don't think I broke it - there's a dump statement in there that's bogus. I'll attach a fix.

the dump statement was throwing a js exception afte URL was changed to URI.

Comment on attachment 108072 [details] [diff] [review] fix run junk mail controls this has r/sr=sspitzer

Since this was checked in checking for junk mail isn't run automatically on new messages in a folder when it's opened. Was that intentional? If it was using junk mail just became a PITA.

It works for me using IMAP - aren't you using IMAP too? It also does for POP. I don't know why it would not work for you unless it's somehow linux specific.

hmmm, after using mail for a while I've noticed that it is marking some messages, just not nearly as many as it used to. It's like it hits a certain message in the list and just stops. I'm not sure if my training database is screwed or if there's something else going on here.

There is definitely something still busted here. I can get new mail and watch the junk mail filters go and mark one or two of the messages as junk. Then it will stop. I can select all the new messages I just got, run the junk mail controls on the selected messages and it will mark all the junk as junk. I think that something is causing the junk filtering to stop too soon since it always stops marking junk a few messages into the process. You have to select and re-run it to get it to finish.

Since we're discussion UI in this bug, I'll throw my two cents in as well since I'm bothered by not being able to tell if a message has been put through the filter or not. Can we change the icon for junk to be three state? That is we have three settings: o Junk o Not junk o Unknown I used to see the little junk icon with a question mark for new messages but that doesn't seem to happen anymore. I don't know if that was a change or if it's because my training database is pretty large. Anyway, I was thinking of possible icons for the junk/not junk Maybe something like a little red 'X' for spam, a little green check mark or the green dot used to indicate new mail and a little question mark for unchecked mail? As the mail is filtered, you could watch the messages change from the question mark to the check mark or the 'X'. They are easy to see and are even color-blind friendly (green vs. red and check vs. 'X' vs. question mark.) As for a progress meeter we could do something like what getting new mail does. That is "Filtering message 3 of 25" since I'm pretty sure we know how many messages need to be filtered for spam status. You could also hook up the stop button to the process and if you stopped it you would still have some of the messages marked as junk/notjunk and some of the messages still with the little question mark indicating they are unfiltered. That way you could tell what messages are left to be filtered in case you want to do it later. What do people think? I think that gives a pretty clear picture to the user of what's happening, how long it's going to take to finish and how to stop it if it is going to take too long and even what to do to complete it if they stop it.

Chris, do you have your inbox configured to download for offline all new messages? I'm not seeing any stalling - I deleted my spam.msf and opened my spam folder and several hundred messages got classified. I just don't know how to recreate the problem you're seeing...all new messages in my inbox get scored w/o any stalling. Personally, I liked seeing the unknown state like we had before. Maybe I wouldn't care if I wasn't trying to debug it and everything worked perfectly.

> Chris, do you have your inbox configured to download for offline all new > messages? I'm not seeing any stalling - I deleted my spam.msf and opened my spam > folder and several hundred messages got classified. I just don't know how to > recreate the problem you're seeing...all new messages in my inbox get scored w/o > any stalling. No, I don't download for offline. I've also tried what you're suggesting on a folder with 598 messages in it and it only marked two of them as spam (obviously wrong.) I then selected the first 10 messages, most of which are spam and ran the junk controls on them. It marked about 8 of them as spam which is good as near as I can tell. I've also tried removing the .msf file as well and it doesn't seem to affect things. > > Personally, I liked seeing the unknown state like we had before. Maybe I > wouldn't care if I wasn't trying to debug it and everything worked perfectly. Me too, but I just wanted it so I could see what is classified and what isn't and so we can possibly add a way to stop filtering for huge folders. Who gets to make the UI decision about this, anyway? :)

the reason we got rid of the unknown state was that it was causing people to classify messages that they didn't need to classify - i.e., people assumed that "unclassified" meant "I need to classify this", whereas it means "this hasn't been classified". In other words, not all messages need to be classified. Maybe we could add a state - new/unread message that hasn't been classified, and then we'd get more feedback while classification was going on. I do intend to add a progress message - classifying 1 of XX messages, but it's going to be hard to stop it from getting overwritten by other status messages. Anyway, back to your problem. I don't know why the classification is stopping. Perhaps you could attach an imap protocol log of a reproducible case, e.g., have a folder with 10 unread junk messages, delete the .msf file and then select the folder. Then we can see how many message get fetched to get scored, and see if perhaps the last message we fetch isn't getting scored for some reason. http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap

See http://bugzilla.mozilla.org/show_bug.cgi?id=182386 for where the unknown state was turned off in the UI

> the reason we got rid of the unknown state was that it was causing people to > classify messages that they didn't need to classify - i.e., people assumed that > "unclassified" meant "I need to classify this", whereas it means "this hasn't > been classified". In other words, not all messages need to be classified. Maybe > we could add a state - new/unread message that hasn't been classified, and then I wasn't talking about unclassified, I meant that it had been run through the spam filter at all. After all, if you do stop classification (or there's some sort of error), how do you know what hasn't been looked at?

running a message through the spam filter classifies the message. Maybe what you want is messages that should have been run through the spam filter, but haven't yet. That's what I was trying to get at with "new/unread message that hasn't been classified" though of course, once you'd read them or gone to another folder and back, you'd lose that state.

This is the IMAP log from logging into my test folder after deleting the test.msf file. In this test, only one message is marked as junk, the one with the subject "A PLEA FOR YOUR ASSISTANCE". If you select all the messages and run the junk controls on them then 4 out of the 5 messages are marked as junk (yay, 100% correct!) The only legitimate message in the folder is from Roy.

from that log, we are downloading all 5 messages, which means we are running them through the junk mail classification (because of the way the chaining works, we won't try to fetch another message until the previous message is given to the classification code). And since the classification code is synchronous, we'll finish the classification before getting the next message. (I'm assuming you didn't manually select each message in the UI) So either the classification code is behaving differently in the folder load case, or we're reporting the wrong uri's when we classify messages. But I haven't seen either of those things happen to me, so I need to look a little deeper.

I don't know if this is helpful to you or not. This is the mbox file I'm using for testing and exhibits the problem.

Chris, could you turn on the spam filter logging (under tools | junk mail controls) and try this again and see what the logging says when you do this test?

It is in fact logging nothing, even though it's obviously marking files as junk. wtf? I'm 99% sure I didn't screw this up. I did use clear log before testing it, though.

oh, sorry, I suspect it only logs move actions, and you probably don't have it configured to move junk to a junk mail folder (since you have to hack your prefs.js by hand).

No, it logs finding junk mail as well. It just doesn't seem to do so reliably.

I'm now able to reproduce basically the opposite of what Chris is seeing - opening a folder with junk messages marks all but one of them as junk, but selecting the same messages and classifying them marks all but three of them as junk. I'll investigate further.

We need to re-init the tokenizer when re-using it across multiple message classifications - otherwise, the spam classification is really based on the sum of all the messages, which is very bad.

Comment on attachment 108381 [details] [diff] [review] proposed fix sr=sspitzer can you ping beard for a review?

Comment on attachment 108381 [details] [diff] [review] proposed fix Pinging Patrick for a review as well.

Comment on attachment 108381 [details] [diff] [review] proposed fix r=dmose, but definitely wait for beard's review too

Comment on attachment 108381 [details] [diff] [review] proposed fix This definitely needs to be in 1.3 - I'd like Patrick to look at this code, but I wouldn't hold it for that.

Comment on attachment 108381 [details] [diff] [review] proposed fix a=blizzard on behalf of drivers for 1.3a (yay!)

Comment on attachment 108381 [details] [diff] [review] proposed fix Rename ReInit() to Clear(), and you should call it AFTER analyzing each message, instead of BEFORE, otherwise you'll retain excess memory after the last message.

Comment on attachment 108400 [details] [diff] [review] patch addressing Patrick's comments r=beard

This is checked in now, right? I see a checkin last night with this bug named in it.

yes, that last patch was checked in. The bug is still open because there's still more work to do, but the problem that Blizzard was seeing should be fixed.

I think following case are missing : - 'N' key (go to next unread message) donesn't need to go into the Junk folder ! - context menu of the junk folder miss a 'empty this junk mail' entry. Except that, you made a very good job !

I was going to post this as a different bug, but is seems like it might go here. Regarding the UI for Spam filtering, I am getting very confused. It seems like 1 or 2 buttons are responsible for 3 or 4 actions. EX How you do train the Junk mail controls to identify spam as apposed to running the spam filter to mark existing messages as junk? Also, Since the move incoming junk messages to folder Junk has been inabled, where shouldn't their be another option in the drop down menu to 'move all marked junk mail to folder JUNK?' I would suggestion adding the following (in some way) to the tools menu in the mailnews display: 1 Train Junk Mail Controller Using Current Folder/Messages 2 Run Junk Mail Classifier on Selected Folder/Messages 3 Run Junk Mail Filter on Selected Folder/Messages 1 this trains the system 2 this marks existing messages as junk/not junk 3 this moves and messages marked junk in the selected folder to the Junk folder Hope this helps.

I agree with comment 61 in general. Specifically, though, having a single big "Junk" button on the tool bar that can either junk or unjunk a message is just wrong. At the very least, it should be disabled if both spam and ham messages are selected, because the chances of intentionally wanting to toggle both some spam and some ham at the same time are miniscule. Also, doing this by accident can lead to serious dataloss issues if mail is accidentally miscategorized. Also, I don't think the ability to reclassify "spam" into "ham" is likely to be used often enough to justify the existance of a toolbar button. If the common wisdom disagrees, and insists that it be the same as the button that marks mail as "spam", that button should at least not say "Junk" when "spam" is selected.

taking this dumping ground bug, it might be a dup of another bug I already own.

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

Bug 201174 basically request progress bar for ALL message filters, not just junk. Should/can these two issues be dealt with at once?

I agree with comment #34 here, in that more states are definitely needed for the Junk Status flag. I posted bug 209898, requesting five states, but I'd gladly settle for three. (The extra two states I requested were "Manually marked as Junk" and "Manually marked as Non-Junk". These would help ensure that the Mozilla junk classifying engine would not accidentally override the user's manually selected choices.) The third state, "Unknown", is definitely a necessity. Currently, Mozilla can't tell the difference between a new incoming message, and a message that has already been classified as Non-Junk! This can confuse Mozilla and break the spam-recognition in certain situations, especially when new unrecognized messages appear on the server without Mozilla having downloaded their bodies yet. In certain odd situations that are difficult to reproduce but happen often enough when using offline and/or IMAP mail, Mozilla assumes unrecognized messages to be Non-Junk instead of Unknown. Adding a third state will help to fix this.

Even if sender is in my address book, Mozilla still marks as junk. Need a button/icon to say that in the future all messages from that sender will be non-junk. This issue of not learning makes me think of switching to another e-mail message system.

(In reply to comment #67) > Even if sender is in my address book, Mozilla still marks as junk. Need a > button/icon to say that in the future all messages from that sender will be > non-junk. This issue of not learning makes me think of switching to another > e-mail message system. Did you tell Mozilla to use your address book as a whitelist? Tools | junk mail controls check the "do not mark messages as junk if the sender is in my address book" checkbox.

In SeaMonkey 1.1.1 there is no "Junk Mail Controls..." option in the Tools menu.

sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.

I've lobbied for 5 visible states of junk mail marking, in the UI: * Automatically marked as Junk * Automatically marked as Not Junk * Manually marked as Junk * Manually marked as Not Junk * Not marked yet Mail that the user has manually marked would have a "stronger" state than mail that has been automatically marked. It would be "sticky", and not overridden by automatic filtering. This would help keep the filter correctly trained when the user runs filters on folders that were previously manually marked before, as their manual marking decisions wouldn't be rudely reset. It would also make it obviously clear to the user that some feedback has indeed occurred, when they use the manual marking menu. The manual marking menu would then have 3 choices: * Mark as Junk * Mark as Not Junk * Remove Markings The third choice would remove all markings from the message, causing it to return to the initial state of "Not marked yet". Until junk mail filters get ran, either manually or automatically, every message would begin in this "Not marked yet" state. The current mass confusion between "Not Junk" and "Not marked yet" is a huge source of problems. There's no difference in the UI at all between messages considered to be "Not Junk" and messages that simply haven't been marked yet. I'd wager a guess that over a dozen bugs stem from this one, horribly bad, design decision.

Filter on "Nobody_NScomTLD_20080620"

What is the state of the patches here? Two of them have superreview. Have they been merged?

meta bug 531787 covers junk issues. so we can exclude all junk issues as candidates for flagging with [filter-mgmt]