Closed Bug 169638 (spamfe) Opened 22 years ago Closed 19 years ago

[meta] finish junk mail feature

Categories

(SeaMonkey :: MailNews: Message Display, defect)

Product:
SeaMonkey
Component:
MailNews: Message Display
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.4beta

People

(Reporter: sspitzer, Unassigned)

References

(
URL
)

Details

Attachments

(2 files, 40 obsolete files)

screen shot
133.92 KB, image/gif
Details
revised second screen shot, per jglick
4.39 KB, image/gif
Details
front end work for spam / junk mail the patch I'm about to attach has the fixes for: 1) the control center 2) the log viewer 3) thread pane / stand alone msg window / 3 pane (I stole bienvenu work from the MOZ_FILTER_BRANCH Here's what's left for the front end: 1) need new icon from "junk" toolbar button (classic and modern, right now using "mark" button) 2) need to decide how to represent score in "Junk" column. (an image? as text?) 3) integrate with mscott's thread pane view work For the back end 0) have the spam BE code set the score on the header 1) add the hooks so when the user sets the junk level, we poke the spam backend code. 2) implement moving junk to folder 3) implement purging 4) implement white listing 5) have the spam BE code write to the log file. (see nsMsgFilter::LogRuleHit (), the same tricks apply for spam. in our case, get the server, then the spamSettings, then the logStream, and write to it. [remember to html escape]) 6) on hitting the junk button, have the FE poke the BE that the user has manually changed the score. I think that for spamassign, this is a noop. for bayesian, this means doing work. for the backend guys, see nsISpamSettings.idl the BE code should not be going through prefs to determine spam level, ab for whitelist, junk mail folder uri, purge prefs, etc. you just have to do server.spamSettings I'll attach my patch, attach some screen shots, and then seek reviews tomorrow.
oops, forgot the other FE todo items: 1) get junkMail.js to work for multiple servers (should be easy) 2) integrate with help (we have a help button, does nothing)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2beta
Attached patch patch (obsolete) — — Splinter Review
other things: 1) test stand alone msg window 2) mac build changes 3) ask david about the db changes there's some duplicated code behind nsIMsgFilterList and nsISpamSettings (and viewLog.js/xul and junkLog.js/xul). I can refactor this, but I'd rather land first and refactor while the backend people work on this, then refactor first and block the BE guys.
more FE work: 1) add other UI (context menu?) for setting message a junk / not junk 2) ask jglick if she wants a link on account central to launch the spam control dialog new patch coming, I got it to work for multiple servers. and some screen shots. the one bad bug, I'm having problems when I open the dialog twice in a session. I'll work on that tomorrow.
Attached patch updated patch (obsolete) — — Splinter Review
more FE items: 1) enable / disable UI based on check box state 2) get enable log to "stick" 3) get "Junk" button in stand alone msg window to work
Attachment #99814 - Attachment is obsolete: true
Attached image screen shot of the 3 pane (obsolete) —
Attached patch patch (obsolete) — — Splinter Review
other issue: 1) follow what I did for nsIMsgFilterList / nsIMsgIncomingServer::Shutdown()
Attachment #99817 - Attachment is obsolete: true
Attached patch diff, -uw (obsolete) — — Splinter Review
Attachment #99820 - Attachment is obsolete: true
function JunkSelectedMessages(setAsJunk) +{ + if (setAsJunk) { + gDBView.doCommand(nsMsgViewCommandType.junk); + } + else { + gDBView.doCommand(nsMsgViewCommandType.unjunk); + } +} + should just be: gDBView.doCommand(setAsJunk ? nsMsgViewCommandType.junk : nsMsgViewCommandType.unjunk); function SelectedMessagesAreJunk() + this just checks if the first selected message is junk, right?, so it should either be renamed or fixed. Did you fix my bug where the score column was showing up as the left-most column? The score stuff needs to be fixed a little, I believe (maybe it is fixed in Dan's code, but I doubt it) to normalize for the real number scores that the spam assassin backend sets (e.g., 3.4534) and the view code displays vs. the int that the front end code sorting code treats it as (the atoi call in nsMsgDBView). It seems to me that the easiest thing to do would be normalize it, perhaps in the sorting code, in essence, by multiplying by 1000 before sorting. you could use floats, or just move the decimal point 3 places to the right and zero fill. More comments as I go.
Comment on attachment 99821 [details] [diff] [review] diff, -uw I don't think the prefs should be on a per server basis. Rather it should be on per identity basis. This is how we do sent folder etc. I have written other code that treats them as identity.
Comment on attachment 99821 [details] [diff] [review] diff, -uw Furthermore looks like nsISpamSettings would do exactly what nsIMsgIdentity already does. Infact it already has JunkMailFolder attribute.
we may have different spam capabilities on different servers, and the user may want to have different spam settings on different servers, but use the same identity. Consider the differences between imap and pop - even though, AFAIK, we are adding the capability to do spam analysis on imap msg bodies, it comes with a fairly serious performance impact, whereas there's no performance impact for pop, and a user may chose not to pay that price. Consider adding spam capabilities to newsgroups - most of the spam settings don't apply well to news, but it could be very useful to have news messages marked as spam.
bienvenu wrote: > The score stuff needs to be fixed a little, I believe (maybe it is fixed in > Dan's code, but I doubt it No, I was focusing on other stuff and never got a chance to poke at this. Everything I did was on MOZ_FILTER_BRANCH. Looks like this patch is missing new files; I'm guessing "diff -uwN" is really what's needed.
the new files are already checked in.
> I don't think the prefs should be on a per server basis. > Rather it should be on > per identity basis. This is how we do sent folder etc. I have written other > code that treats them as identity. I disagree. the spam settings are w.r.t the incoming mail, which has to do with nsMsgIncomingServer. identities are all about sending mail. > Furthermore looks like nsISpamSettings would do exactly what nsIMsgIdentity > already does. Infact it already has JunkMailFolder attribute. then it will be removed. I'm going to work and david's comments. my goal is to land, unblock the BE guys, and then continue working on the FE while the BE guys work in parallel.
about the label column stuff, that's something I need to fix in tree.xml what happens is new columns don't have an ordinal, so by default, they appear on the left instead of on the right. I'll work on a fix for that.
I've landed my patch, but the new UI is not visible yet. I'll attach a patch that turns on the UI. an update to the spec is coming, too. instead of a simple folder picker for the junk mail folder, we need to do what account settings does. we need to have a choice between: ( ) Junk Mail on <server> (*) <folder> so that we create the junk mail folder lazily. I'll report back as I make changes for this, and as I fix things.
Attached patch patch to enable spam UI. (obsolete) — — Splinter Review
when I checked in, I addressed david's comments and I removed the unused spam folder attribute on the identity. it's per server.
Attachment #99821 - Attachment is obsolete: true
not sure if you did that already, but could you make a context menu entry for Junk as well? I often see already from the subject that a mail is spam; so I'd like to mark it as spam without clicking on it.
Christian: Good call, especially with bug 22994 / bug 160315 still lingering ;)
Attached patch Hide junk button in news (obsolete) — — Splinter Review
Correct me if I'm wrong, but the "junk" button doesn't appear to apply to news. So I suggest you hide it, like you do for the "delete" button.
back from vacation, going to start the open issues, including the one that neil pointed out.
talking with beard about spam, I don't think we're going to hide this when in news. while we won't be moving spam messages to a junk folder, we will be able to not show spam news messages when the view changes land. beard agrees that this will affect the bayesian histogram, but it should be affective in hiding spam from the user and making news usuable again. I'll continue to work on the UI and the issues I've listed, and the changes to the spec, from jglick.
Comment on attachment 100651 [details] [diff] [review] add the lazy folder UI and the changes to support it. sr=bienvenu - couple nits through AIM that Seth is going to address.
Attachment #100651 - Flags: superreview+
Attached patch include the pretty name stuff. (obsolete) — — Splinter Review
ignore the bits about
Attachment #100651 - Attachment is obsolete: true
QA Contact: olgam → laurel
"ignore the bits about" meaning ignore the changes to enable the spam UI.
Comment on attachment 100662 [details] [diff] [review] include the pretty name stuff. sr=bienvenu, with one comment over aim that Seth has fixed in his tree.
Attachment #100662 - Flags: superreview+
yes, I fixed the junkFolderNAme typo. checked in. still working on the UI, so this bug is not fixed yet.
Attachment #99819 - Attachment is obsolete: true
Attachment #100662 - Attachment is obsolete: true
I'm going to copy my (Bug 170555 Comment #20) jglick: that dialog barely fits at 800x600 (into a web browser), and it has way too many controls and way too much text for me to handle, and I spend lots of time reading large amounts of both. I'd suggest: 1. ditch most of the documentation sentences, if people need them they can click help. 2. change the off..highest into a select. 3. rename them. "lowest" and "highest" are useless, "tolerant" and "aggressive" are slightly better. 4. drop "When messages are ..." (I'm not sure this comment is necessary for seths version of the picture) 7. We could combine the two move items, it'd buy space Move to: <junk mail on Foo |v> |junk mail on bar | |... | |junk mail on local folders | |----------------------------| |other >| +----------------------------+ I know we haven't in other places, but i think that's a bug (it costs us a lot of space) 8. move always accept to the very top. (there are a few reasons, one is that you're trapping grayspace by putting it where you did, another is that of all the options it's one of the most important, it's also not remotely destructive whereas move and delete are) [x] Always accept mail from <personal address book|v> <Aggressively|v> filter inbound mail for junkmail ... 9. The title should say Junk Mail Controls for <accountprettyname> 10. Shouldn't it be < _View_ junk mail log >? 11. Why does this have the following buttons in combination: minimize, maximize, ok, and cancel. (If it's a dialog, which ok and cancel imply, then it probably shouldn't be sizable, especially given the content you have here, although it should be *much narrower*) -- As for deleting mail, I think it might be best if we wrote something like: [ ] Automatically delete old junk mail after [ 9] days of continuous use. continuous use would have to be defined, but basically if I only use mozilla three days out of twenty then the threshold wouldn't be met. Leaving mail open would not meet the definition either. Reading a few messages or opening multiple folders would. Last usage consideration. Suppose I get 1 junk message a day every day for a month. And suppose I use mozilla for days 20..28 of that month. On day 28 junkmail identified between 1 and 19 would be deleted but junk mail received on day 20 would not be automatically deleted until the next day I use mozilla.
The design just allows for selecting one addressbook to mark non-junk messages. This is overly restrictive. Ideally you should be able to select multiple addressbooks for the exclude list. I'm not sure how the UI would look.
Attached patch updated db patch, after talking to bienvenu. (obsolete) — — Splinter Review
Attachment #100695 - Attachment is obsolete: true
I don't think these checks in the watch and ignored code are useful - the flags are pretty much guaranteed to have changed, and if not, it doesn't matter anyway - it's just code bloat. Everything else looks fine. + + if (oldThreadFlags == threadFlags) + return NS_OK; + NotifyKeyChangeAll(threadKey, oldThreadFlags, threadFlags, instigator); return NS_OK; } @@ -1858,6 +1854,10 @@ } else threadFlags &= ~MSG_FLAG_WATCHED; + + if (oldThreadFlags == threadFlags) + return NS_OK; +
Comment on attachment 100750 [details] [diff] [review] updated db patch, after talking to bienvenu. sr=bienvenu, modulo my last comment.
Attachment #100750 - Flags: superreview+
I've removed the watched / ignore flag checkes, and when the tree opens, I'll check it in. thanks david.
about news spam, according to peterv / dmose, the bayesian filter works fine on news, which is what we expected. so, I'm to make our UI work for news (so a link for it in account central, the junk button doesn't disappear for news), news accounts appear in the list of accounts. I'll work on that while the tree is closed for wtc's landing.
Attached patch patch to search by score (obsolete) — — Splinter Review
Comment on attachment 100857 [details] [diff] [review] patch to search by score I don't think we should expose the score to the user at all like we do in this patch for searching. The user isn't going to know what kind of a value to enter for a score. We should instead make it look like searching by label...something like: "Message" "is" "Junk Mail/Good Mail/Unknown". I'm sure Jennifer or Robin can come up with better wording for the criteria but my point is the score should be completely hidden from the user. Also, instead of using the word 'score' everywhere (i.e. as an enum in nsMsgSearchAttrib) we should be more specific. JunkMailIndicator or JunkMailScore.
or "Junk Mail Status" "is/isn't" then a combo box for "Highest", "High", "Low", "Lowest" just like message priority.
Attached patch new patch for trunk to enable the spam feature (obsolete) — — Splinter Review
beard, update and apply this. let me know if you have problems with any of the things I added to the tools menu. I'll also report back with the problems I'm seeing.
Attachment #99898 - Attachment is obsolete: true
Attached patch updated patch to enable this feature in the UI (obsolete) — — Splinter Review
Attachment #101181 - Attachment is obsolete: true
Attached patch updated patch to enable this feature in the UI (obsolete) — — Splinter Review
Attachment #101445 - Attachment is obsolete: true
If we are going to do junk folder creation from JunkMail UI we might want to change the picker design and keep it similar to filter editor dialog. Also have a new folder button.
talking it over with jglick, she agrees with mscott about "Junk Mail Status" "is" "Junk/Not Junk/Unknown", or something to that effect. so I'll work on those changes. jglick has a bunch of other updates coming to the spec, which I'll be addressing. navin raises a valid point about a new folder button, but there are other places in the UI (copies and folders) where we don't allow for new folder creation. (the junk mail UI doesn't actually create the folder, that happens in the back end.) For now, I'll follow the spec. But jglick reads the bug report and she can comment more.
more work coming, obsoleting old patches.
Attachment #100216 - Attachment is obsolete: true
Attachment #100750 - Attachment is obsolete: true
Attachment #100857 - Attachment is obsolete: true
Attachment #101446 - Attachment is obsolete: true
Attached patch patch, for search and a bunch more (obsolete) — — Splinter Review
Attachment #101704 - Attachment is obsolete: true
Comment on attachment 102079 [details] [diff] [review] patch, for search and a bunch more } nsMsgSearchValue; @@ -208,7 +210,8 @@ (!(_a == nsMsgSearchAttrib::Priority || _a == nsMsgSearchAttrib::Date || \ _a == nsMsgSearchAttrib::MsgStatus || _a == nsMsgSearchAttrib::MessageKey || \ _a == nsMsgSearchAttrib::Size || _a == nsMsgSearchAttrib::AgeInDays || \ - _a == nsMsgSearchAttrib::FolderInfo || _a == nsMsgSearchAttrib::Location || _a == nsMsgSearchAttrib::Label) ) + _a == nsMsgSearchAttrib::FolderInfo || _a == nsMsgSearchAttrib::Location || \ + _a == nsMsgSearchAttrib::Label)) %} Don't you want to be adding junk score here since junks score is internally an int not a string? This code turns on the baysian-spam-filter directory. Is that intentional? Also, should it be controlled by a flag like smime? i.e. ifdef MOZ_BAYESIAN then DIRS += bayesian-spam-filter? sr=mscott assuming you disable the filter from the UI in nsMsgImapSearch.cpp before checking in.
Attached patch latest patch to enable this feature (obsolete) — — Splinter Review
Attachment #102079 - Attachment is obsolete: true
Attached patch latest patch to enable (obsolete) — — Splinter Review
Attachment #102130 - Attachment is obsolete: true
Attachment #102191 - Attachment is obsolete: true
Comment on attachment 102368 [details] [diff] [review] set score through the db so that we invalidate the row r/sr=bienvenu
Attachment #102368 - Flags: superreview+
Attached patch UI enabling patch (obsolete) — — Splinter Review
Merged to the CVS tip.
Attachment #102230 - Attachment is obsolete: true
Attached patch patch to enable the feature (obsolete) — — Splinter Review
Attachment #99818 - Attachment is obsolete: true
Attachment #100696 - Attachment is obsolete: true
Attachment #102368 - Attachment is obsolete: true
Attachment #102413 - Attachment is obsolete: true
Attachment #102417 - Attachment is patch: false
Attachment #102417 - Attachment mime type: text/plain → image/gif
Attached image second screen shot (obsolete) —
Typo in "Junk Mail Controls" (seen in "screenshot")?: [ ] Do not mark messages as junk mail if the sender is _in_ my addressbook. ["in"]
The most recent screenshots suggest that only 1 address book may serve as a white list. I think that is suboptimal. Many of us have personal, collected, and corporate address books, all of which should ideally be whitelisted.
Is this done in a way that would make it easy/feasable to plug in other learning algorithms for dealing with spam? I think that is a really wonderful feature but since there are so many ways how to deal with it it should provide a clear interface and allow alternative algorithms.
Moshe, I disagree. I dont believe that it would be a good thing to use an LDAP as a whitelist source, as the LDAP query would take time ... causing lag in the processing ... It should be using LOCAL address books ONLY ... but, I do think that the user should be able to select multiple local address books (via checkboxes) ...
Hopefully, in the near future, online (LDAP) address books will become quite common. Therefore, a solution to whitelist these should be sought (replication?).
Replicated LDAPs would be ok ...
yes, the filter plugin is just that - a plugin. So it should be fairly easy to add new plugins (though we don't support that yet, since we hard-code the contract-id for the plugin we're using now - it should be easy to make that a pref, or something like that)
In screenshot http://bugzilla.mozilla.org/attachment.cgi?id=102417&action=view there's a typo in the Junk Mail Log window. 2nd sentence at top is missing "to": "Use the check box below >>to<< enable logging." In Junk Mail Controls dialog, please add the word "mail" to this checkbox: "Move incoming messages determined to be junk >>mail<< to:" Great work!
And don't forget the typo mentioned in comment 60.
Attached patch fix dialog to always show up (obsolete) — — Splinter Review
Currently, the Junk Mail dialog only displays content the first time it's opened; subsequent opens display a blank dialog except for the OK/Cancel/Help buttons. For whatever reason, using <dialog> isn't working right. Changing to <window> fixes the problem.
Comment on attachment 102493 [details] [diff] [review] fix dialog to always show up marking as obsolete, since already checked in. thanks dmose!
Attachment #102493 - Attachment is obsolete: true
spelling mistakes fixed, too.
Attachment #102419 - Attachment is obsolete: true
um, what do the icons in the new column mean?
> um, what do the icons in the new column mean? I think it's: Dot - Not junk Trash can - Junk Trash can & ? - Might be junk
Needs something more intuitive in the way of icons. Perhaps bigger dots, Red dot - Junk, Green dot - ok, Yellow - maybe ? (traffic light) Or just the letter "S" for Spam (or "J" for Junk) and "?" for maybe. There isn't much room for detailed icons. Personally I'd like the score numbers, but that's already been discussed.
Icons are not yet finalized. Valid mail - no icon. This column is a junk mail indicator, don't call attention to valid msgs. Dot that is used to indicate column is a cycler is visible. User can click on cycler to tell the system the msg Is Junk (if appropriate). Is Junk - Junk Mail icon, match toolbar Junk Mail icon as close as possible. Meant to call out msgs that have been identified as junk. Unknown/Could be Junk - same icon as above with ?. User uses the cycler or Junk Mail toolbar icon to tell system the msg Is or Isn't Junk.
>navin raises a valid point about a new folder button, but there are other >places in the UI (copies and folders) where we don't allow for new folder >creation. Agree. I'd like to see us eventually go back to something similar to the 4.x design for selecting folders (would apply to this dialog and copies & folders), which took up less space on the actual dialog and also allowed for New folder creation.
In response to comment 76: Am I right in thinking that most messages will start in the unknown state? If that is the case then the use of the dot in this case could be inconsistent with its use in the Flag column. In the Flag column, messages start out as nothing (dot) and can then be changed to be flagged (flag icon). In the Junk column, most messages will start out as unknown (trash and ?) and can then be changed to be junk (trash) or not junk (dot). So a dot in the Flag column means not set, whereas in the Junk column it means set to not junk. Maybe it's just me but that seems a little strange. Perhaps it would be better to have: Dot - Unknown Trash can - Junk Check mark - Not junk (message has been 'approved' by the user as not junk) > don't call attention to valid msgs With the amount of spam some people get, making valid messages stand out wouldn't be a bad idea! ;-)
>Am I right in thinking that most messages will start in the unknown state? No. Most msgs will be either Valid or Junk (hopefully valid :-). Unknown should be less common, when the JM system just isn't certain about a particular msg. The more the user trains the system, the more infrequent "Unknown" should become.
i'm a bit confused. i've only started looking at cyclers (i'm trying to find out where onclick for a cycler goes, i'd like ctrl-clicking thread cyclers to behave sanely, they don't, but that's another bug). i'd like to be able to train mail by manually scoring junkmail. so ... suppose i get 100 messages and none of them are listed as junk. i'd like to be able to (see comment 3) click the junkmail cycler (currently empty) to mark a message as junk, and at the end of reading my messages it'd be nice if mail would try to build rules based on my decissions (this includes all messages in that folder which i read but did not mark as junk). if we did that, then there should be a little dot, just like there is for unread messages. ok, i read the comments, jglick isn't saying no dot, just that whatever it is for nonjunk shouldn't be prominent. clearly i agree. -- 'Enable' sounds silly, can't we just '[x] Log junkmail activity' and '[x] Control junkmail' ?
>>Am I right in thinking that most messages will start in the unknown state? > No. Most msgs will be either Valid or Junk (hopefully valid :-). Unknown > should be less common, when the JM system just isn't certain about a > particular msg. OK. So it's more about the system alerting you when there's a message that probably is or may be junk with valid being the 'normal' state. Ignore the rest of my comments, then.
notes for dmose, in order: 1) fix the 5 (not 3) state problem. (start with this patch: http://bugzilla.mozilla.org/attachment.cgi?id=102656&action=view) 2) fix the regression in tree.xml (varga might be able to help). new columns (with no ordinal, like junkStatus will be for users once we land) should be on the far right, not the far left. (you said it's on the left for Mail.app, can you screen shot?) we still want the regression fixed. 3) don't allow the user to use move for news accounts (in junkMail.js, use get the root folder from the incoming server, and check rootFolder.canDeleteMessages) 4) add code to classify incoming news messages if we decide #3 and #4 should wait, we should just stop showing news accounts in junkMail.xul. (it's a one liner to hide them)
note to dmose / beard / bienvenu: if you apply that patch, remove your .msf files. the string property is still "junkscore" but the values are no longer "", "0" and "100"
Comment on attachment 102656 [details] [diff] [review] patch for dmose, to fix the 3 state (need 5) problem beard pointed out. dmose has suggested (and I agree) that we want to add a new attribute to keep track of the origin of the score setting, instead of using the junkscore attribute. one reason is not all plugins will be yes/no like bayesian. he's working on a different approach.
Attachment #102656 - Attachment is obsolete: true
> For whatever reason, using <dialog> isn't working right. The change to a <window> is not permanent, I hope? You should be working on converting <window>s to <dialog>s, not the other way around! > fix the regression in tree.xml Does (in xul.css) treecols { -moz-binding: url("chrome://global/content/bindings/tree.xml#treecols"); + -moz-box-ordinal-group: 2147483637; } work? Or, add ordinal="25" (?) on the new column. P.S. You don't have enough splitters; if you show all the columns you can't resize the last one.
There's a problem in the spam code that dmose is trying to fix. When you toggle the spam status of a message (using the cycler in the thread pane) we don't run the message through the filter plugin (and tokenize it and add it to the spam db). To fix this problem, dmose is working to move the code from mailCommands.js to the db view. This way, the junk toolbar button, the spam related menu items, and now the thread pane cycler will go through the view. We'll do the right thing on the selected messages (or for the cycler, the clicked message). Perhaps for junk status, instead of moving all the code from mailCommands.js to nsMsgDBView.cpp, we should move some of it to the msg folder base class (since imap, local and news will need it). Or should we leave this in the view? Something needs to implement the listener interface. We need to do this: get the uri for the message, determine the current state of the message (junk / notjunk / undecided), determine the origin of the junk state (user set / auto set from the plugin when the message arrived), call mJunkMailService->SetMessageClassification(messageURI, oldClassification, newClassification, origin, listener); If we make the folder the listener, it can get the junk mail service from the incoming server. the listener would then set the new junk status on the message (by going through the database, so that the view will get updated), and mark that this message junk status originated from the user. For reference, here's how the flag code works, from nsMsgDBView.cpp if (flags != kNoImapMsgFlag || commandIsLabelSet) // can't get here without thisIsImapThreadPane == TRUE imapFolder->StoreImapFlags(flags, addFlags, imapUids.GetArray(), imapUids.GetSize()); Note, dmose will also be fixing it so that when we set the junk status on incoming mail, we'll set the junk status origin to be "auto". Comments?
the folder already implements the junkmail listener interface. Is that the listener you were talking about? However, it does a move if the message is spam and the user has their prefs configured that way...I'm more inclined to put this in the view, I guess.
sspitzer wrote: > Perhaps for junk status, instead of moving all the code from mailCommands.js > to nsMsgDBView.cpp, we should move some of it to the msg folder base > class(since imap, local and news will need it). > > Or should we leave this in the view? Something needs to implement the > listener interface. The one benefit to putting part of it in the folder that I see is that only the folder has to implement the listener, instead of both the folder and view. I'm not sure that this is a real big deal, however. Is this why you're proposing it, or are there other reasons as well? > If we make the folder the listener, it can get the junk mail service from the > incoming server. I assume the reasoning for this is so that once we support more than just bayesian plugin, we'll have some central place to decide what plugin(s) to use and won't be able to just call do_GetService() everywhere. Is that correct?
the listener on the folder will do a move if the user has their prefs configured to move spam. I don't know how you're going to distinguish a spam score from auto-classification, which would require the move, from a spam score from the user bonking the junk button or the thread pane cycler (remember mail can come in from biff, asynchronously). That's why I'm arguing for putting it in the view - there won't be any confusion as to what to do with the classified mail.
neil wrote: > > For whatever reason, using <dialog> isn't working right. > > The change to a <window> is not permanent, I hope? You should be working on > converting <window>s to <dialog>s, not the other way around! We could certainly file another bug to get the <dialog> issue fixed, and then switch back to dialog. What is the advantage to using <dialog> instead of <window>? >> fix the regression in tree.xml > > Does (in xul.css) > > treecols { > -moz-binding: url("chrome://global/content/bindings/tree.xml#treecols"); > + -moz-box-ordinal-group: 2147483637; > } > > work? Or, add ordinal="25" (?) on the new column. > > P.S. You don't have enough splitters; if you show all the columns you can't > resize the last one. Thanks for the suggestions; I ran into the splitter issue today in fact. I'll poke at these once I get done fixing the multi-state problem.
I've updated and move the private todo list to mozilla.org http://www.mozilla.org/mailnews/spamtodo.html (not there yet)
This is probably (hopefully) already addressed, but to be sure: Please make sure SpamFilter's delete or move operations respect the IMAP pref: *Mark as Deleted*.
moving out to 1.3 beta
Target Milestone: mozilla1.2beta → mozilla1.3beta
Attached patch 3/5 state fix, v1 (obsolete) — — Splinter Review
First cut at fixing the 3/5 state problem. Still needs a bit of cleanup, and the stuff in mailCommands.js still needs to be converted to using it.
Alias: spamfe
Comment on attachment 103269 [details] [diff] [review] 3/5 state fix, v1 Let's NOT add the new nsMsgJunkStatusOrigin type to nsIMsgFilterPlugin.idl, as it's simpler to just pass UNKNOWN as the classification when it is automatically classified. Makes the implementation of observeMessage asymmetric, yada, yada...
Attachment #103269 - Flags: needs-work+
So it sounds like you want to change the semantics of oldClassification. Would renaming it to oldUserClassification, and renaming the UNKNOWN state to NONE work for you?
beard, I'm confused by your comments. I thought we needed to the the plugin (somehow) about the state transformation: unknown -> [ junk auto | junk user | not junk auto | not junk user] junk auto -> not junk user junk user -> not junk user not junk user -> junk user not junk auto -> junk user I don't see how we can pass that information through to the plugin without modifying the interface? (for others on the cc list, the bayesian plugin needs to know the message origin, so it knows if has to update the good [or bad] list)
Here's what I'm saying, hopefully in a less cryptic fashion: As far as the bayesian junk mail filter is concerned, a message can only be in one of 3 states: UNKNOWN, JUNK, or GOOD. Here's what is implied by these states: UNKNOWN => nothing has been asserted to be true about the message, therefore it isn't part of the training set, and is eligible for classification by the filter. JUNK => the message is asserted to be spam, so its tokens should be part of the spam corpus. GOOD => the message is asserted to not be spam, so its tokens should be part of the good corpus. The fact that classification may be have been performed on a message already is of no concern to the filter. It is the business of the mail infrastructure to know about whether a message was classified automatically, or manually classified by the user. Here are the state transitions that should be presented to the filter: UNKNOWN -> JUNK : this should happen when the user manually classifies the message as spam. Whether or not the message was already classified as GOOD or JUNK by the filter, this is the state transition to use. UNKNOWN -> GOOD : this should happen when the user manually classifies the message as good. Whether or not the message was already classified as GOOD or JUNK by the filter, this is the state transition to use. GOOD -> JUNK : this should be presented if the user has made a mistake in classification, and wants to move the message from the good corpus to the spam corpus. JUNK -> GOOD : this should be presented if the user has made a mistake in classification, and wants to move the message from the spam corpus to the good corpus. GOOD -> UNKNOWN : this should happen if for some reason the user wants to reset the state of the message and it should be taken out of the good corpus. JUNK -> UNKNOWN : this should happen if for some reason the user wants to reset the state of the message and it should be taken out of the spam corpus. This should be sufficient to drive the bayesian filter's state machine. Now, the mail system will need to keep track of the fact that a message has been auto-classified by the filter as GOOD or JUNK. In fact, it would be sufficient to keep track of this by NOT adding an extra message attribute in this case, and only add an attribute if the user manually classifies the message. This is essentially 1 bit of information. At one point, I suggested treating the score as a bit field rather than a value from 0 to 100. Then, you could simply represent this as a binary string: "01" => automatic, junk "00" => automatic, good "11" => manual, junk "10" => manual, good Now, we can interpret the high order bit to mean that the message is in the UNKNOWN state as far as the filter's training set is concerned. You can also simply interpret this bit as a boolean that represents whether or not the message is in the training set.
talked it over with dmose and beard. here's the plan: the UI will keep track of junkscore and origin, and will do the morphing from those two attributes to the right states. (if original state is junk auto, or not junk auto, pass in unknown). this will allow the bayesian stuff to remain as is, and we'll just heavily comment the interface and the mailnews code, so that the assumptions are clear. something that beard, dmose and I agree on is that these interfaces are not frozen. the only thing we 100% agree on is that if we add support multiple plugins, or another implement an alternative plugin (not bayesian), the interfaces and UI will have to evolve. but we'd all rather get something reasonable working (and I think this approach is reasonable), then to delay this feature while we try to come up with the perfect interface. dmose is working on a new patch.
Attached patch dbview fix, v2 (obsolete) — — Splinter Review
This patch does several things: * cleans up unnecessary (and partially working) UI items * moves some interaction with the plugin from the UI javascript to nsMsgDBView.cpp * fixes the 3/5 state problem by adding a "junkscoreorigin" property * fixes several gcc warnings Still to do: * need to add back a few debugging dump()s * fix a race condition related to multiple uses of the Junk button or menu items
Attachment #103269 - Attachment is obsolete: true
Attached patch dbview patch, v3 (obsolete) — — Splinter Review
Batching race condition mostly cleaned up. Still need to revisit this because there is a minor issues where a few unneccessary writes happen since the IMAP URLs don't usually finish exactly in order; I'll add that to the todo list. Otherwise, I think we're ready for reviews...
Attachment #103420 - Attachment is obsolete: true
looks good. r=sspitzer, with just a few nits / questions: 1) should we #define "junkscore" and "junkscoreorigin"? (for code maintainability) 2) oldOriginStr.get()[0] will that be '\0' when the string is empty? 3) atoi(junkScoreStr) == 100 should this be > 50, to be consistent the other places? 4) why prefix? (the prevailing style is postfix) ++mOutstandingJunkBatches; for ( ; mOutstandingJunkBatches > 0 ; --mOutstandingJunkBatches ) { + --mBatchLevel; 5) - (void)filterPlugin->SetBatchUpdate(PR_TRUE); + filterPlugin->StartBatch(); since StartBatch() and EndBatch() have a return type, we should either cast to void or check it, right?
1) should we #define "junkscore" and "junkscoreorigin"? (for code maintainability) over aim, dmose and I decided to leave it as is. if (when) we add support for other filter plugins, or for multiple plugins, we'll probably need to fix all this, so just leave it be (and not rathole) on this right now.
> 1) should we #define "junkscore" and "junkscoreorigin"? (for code > maintainability) Discussed with sspitzer on AIM; decided to leave as is for the moment. > 2) oldOriginStr.get()[0] > will that be '\0' when the string is empty? Yes. > 3) atoi(junkScoreStr) == 100 > > should this be > 50, to be consistent the other places? Fixed. > 4) why prefix? (the prevailing style is postfix) > > ++mOutstandingJunkBatches; > > for ( ; mOutstandingJunkBatches > 0 ; --mOutstandingJunkBatches ) { > > + --mBatchLevel; Channelling scc/bbaetz here, prefix is preferred to postfix in part because because it more precisely expresses what is being done: ++a means this: a = a + 1; return a; a++ means this: SomeType b = a; a = a + 1; return b; This doesn't actually matter in the integer cases being used here, but in more complex types and expressions, it can cause an extra constructor call, which is why it's a good habit to get into. > 5) > > - (void)filterPlugin->SetBatchUpdate(PR_TRUE); > + filterPlugin->StartBatch(); > > since StartBatch() and EndBatch() have a return type, we should either cast to > void or check it, right? > OK, in debug builds, these will now spit out warnings if there is a problem. In addition to addressing the above comments, I made the following tiny changes: a) applied a tiny patch from bienvenu to nsImapMailFolder.cpp b) converted the analyze() code in nsMailCommands.js to use the new StartBatch/EndBatch methods (I had forgotten this in the last pass) c) fixed nsMsgIncomingServer::GetSpamFilterPlugin to not NS_ERROR just because the plugin wasn't found, especially important since the plugin is currently not built on the trunk by default. New patch forthcoming.
More correctly, I should have typed this: ++a means this: ++a; return a; a++ means this: SomeType b = a; ++a; return b;
Attached patch dbview patch, v4 (obsolete) — — Splinter Review
With modifications as noted in previous comment.
Attachment #103788 - Attachment is obsolete: true
Comment on attachment 103925 [details] [diff] [review] dbview patch, v4 Carrying forward r=sspitzer.
Attachment #103925 - Flags: review+
Attached patch feature enabling patch, v5 (obsolete) — — Splinter Review
Updated to correctly match the latest dbview patch. Even though they were in the last rev of this patch, I'm not convinced that the removal of the vbox ids is really supposed to be here, however.
Attachment #102416 - Attachment is obsolete: true
some ? nits: + if ( atoi(aJunkScore) > 50 ) { + junkStatus = nsIJunkMailPlugin::JUNK; + } else { + junkStatus = nsIJunkMailPlugin::GOOD; + } js has the ? operator, so: junkStatus = (atoi(aJunkStore) > 50) ? nsIJunkMailPlugin::Junk : nsIJunkMailPlugin::Good; + } else if (atoi(junkScoreStr) > 50) { + oldUserClassification = nsIJunkMailPlugin::JUNK; + } else { + oldUserClassification = nsIJunkMailPlugin::GOOD; + } same thing, ? operator. re this question, if we can score from the search window, then you can NOT assume that m_folder is set. Otherwise, it should be OK. + // XXX are we allowed to assume m_folder exists here? + // + nsCOMPtr<nsIMsgIncomingServer> server; live a little! :-) You could make it a PRUint32 (and move it out of between the PRBackedBools) - there's only one of these objects ever and it will make the generated code more efficient and I believe, compact. Up to you. + PRUint8 mBatchLevel; // allow for nested batches to happen
Comment on attachment 103927 [details] [diff] [review] feature enabling patch, v5 sr=bienvenu
Attachment #103927 - Flags: superreview+
> some ? nits: > > + if ( atoi(aJunkScore) > 50 ) { > + junkStatus = nsIJunkMailPlugin::JUNK; > + } else { > + junkStatus = nsIJunkMailPlugin::GOOD; > + } > > js has the ? operator, so: > junkStatus = (atoi(aJunkStore) > 50) ? nsIJunkMailPlugin::Junk : > nsIJunkMailPlugin::Good; > > + } else if (atoi(junkScoreStr) > 50) { > + oldUserClassification = nsIJunkMailPlugin::JUNK; > + } else { > + oldUserClassification = nsIJunkMailPlugin::GOOD; > + } > same thing, ? operator. Both are actually in C++ code; the reason I'm not using ? : there is because gcc (correctly) issues a warning that the operators on either side of the : are from different enums. This is true because XPIDL generates each constant as part of its own anonymous enum. Why XPIDL does this, I'm not sure. > re this question, if we can score from the search window, then you can NOT > assume that m_folder is set. Otherwise, it should be OK. > > + // XXX are we allowed to assume m_folder exists here? > + // > + nsCOMPtr<nsIMsgIncomingServer> server; OK, added this code: // if numIndices == 0, return quietly, just in case // if (numIndices == 0) { return NS_OK; } NS_ASSERTION(numIndices >= 0, "nsMsgDBView::ApplyCommandToIndices(): " "numIndices is negative!"); and switched to using: // get the folder from the first item (if it's the search view, // only one item can be touched at a time; if a regular folder view, // all items will have the same folder). // nsCOMPtr<nsIMsgFolder> folder; rv = GetFolderForViewIndex(GetAt(indices[0], getter_AddRefs(folder)); NS_ENSURE_SUCCESS(rv, rv); nsCOMPtr<nsIMsgIncomingServer> server; rv = folder->GetServer(getter_AddRefs(server)); NS_ENSURE_SUCCESS(rv, rv); > live a little! :-) You could make it a PRUint32 (and move it out of between > the PRBackedBools) - there's only one of these objects ever and it will make > the generated code more efficient and I believe, compact. Up to you. > + PRUint8 mBatchLevel; // allow for nested batches to happen Fixed.
Comment on attachment 103925 [details] [diff] [review] dbview patch, v4 sr=bienvenu, thx.
Attachment #103925 - Flags: superreview+
Attached patch dbview patch, v5 (obsolete) — — Splinter Review
Addresses bienvenu's sr comments.
Attachment #103925 - Attachment is obsolete: true
Comment on attachment 103932 [details] [diff] [review] dbview patch, v5 r=sspitzer,sr=bienvenu
Attachment #103932 - Flags: superreview+
Attachment #103932 - Flags: review+
Comment on attachment 103932 [details] [diff] [review] dbview patch, v5 dbview patch has been checked into the trunk.
Attachment #103932 - Attachment is obsolete: true
Taking; seth is focussing on other stuff at the moment.
Assignee: sspitzer → dmose
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Hardware: PC → All
Blocks: 168553
No longer depends on: 168553
Attached patch analyze-messages patch, v1 (obsolete) — — Splinter Review
Updated the "analyze messages" verbiage as discussed with the UI folks. I chose "u" as the accesskey, since "R" was already taken.
Comment on attachment 104591 [details] [diff] [review] analyze-messages patch, v1 r/sr=sspitzer
Attachment #104591 - Flags: superreview+
When will it be ready for testing? I'm willing to stress-test that feature.
Get the source, apply the patch and try it out! I'm testing this on my custom build and boy does it rock! Its learning quite fast and marking a lot of spam quite correctly. No major problems so far. But some stuff is still not implemented e.g. selecting a folder other than Junk doesn't work and there are no icons for Classic theme.
Hmm, I'm building it now but it takes ages every time... :( Any chance for a branch of test builds on ftp.mozilla.org ?
Comment on attachment 104591 [details] [diff] [review] analyze-messages patch, v1 Checked in.
Attachment #104591 - Attachment is obsolete: true
This stuff will be landing in 1.3 alpha, which isn't far now, but I don't anticipate test-builds before that.
Target Milestone: mozilla1.3beta → mozilla1.3alpha
OK, to make the life of testers easier (and to verify if I'm doing this correctly :) ) here's what I do to test this feature on Linux: 1. Test whether the tinderbox sidebar is all green so that I can pull the sources that would compile 2. Pull the sources: mkdir ~/mozilla_CVS cd ~/mozilla_CVS export CVSROOT=:pserver:anonymous@cvs-mirror.mozilla.org:/cvsroot cvs login # input "anonymous" as password cvs co mozilla/client.mk cd mozilla gmake -f client.mk checkout 3. Apply version 5 of feature-enabling patch: wget http://bugzilla.mozilla.org/attachment.cgi?id=103927&action=view mv attachment.cgi\?id\=103927 ../mozilla_spam_feature_v5.patch patch -p0 < ../mozilla_spam_feature_v5.patch 4. Build the default, debug version of Mozilla: gmake -f client.mk build 5. Wait about 2.5 hours on a PIII 666 MHz with 512 MB RAM 6. run mozilla: cd ~/mozilla_CVS/mozilla/dist/bin ./mozilla Are those steps correct?
Attached patch Placeholder icons patch, v1 (obsolete) — — Splinter Review
Placeholder icons for classic theme (copies of the modern ones); to be replaced when the real ones are ready.
Attachment #105305 - Attachment is obsolete: true
Attachment #105305 - Flags: superreview+
Comment on attachment 105305 [details] [diff] [review] Placeholder icons patch, v1 Checked in.
Attached patch feature enabling patch, v6 (obsolete) — — Splinter Review
New feature-enabling patch. Changes: * merged to tip * got rid of unnecessary ifdefs * temporarily disabled moving UI until we get more issues sorted out
Attachment #103927 - Attachment is obsolete: true
Comment on attachment 105308 [details] [diff] [review] feature enabling patch, v6 sr=sspitzer
Attachment #105308 - Flags: superreview+
Regarding attachment 105305 [details] [diff] [review] +treechildren:-moz-tree-image(junkStatusCol, notjunk) { + list-style-image: url("chrome://messenger/skin/icons/readcol-read.gif"); + padding-left: 0px; + padding-right: 4px; +} Classic does not have readcol-read.gif icon; the name of its icon is readmail.gif.
The whitelist addressbook(s) is probably the most important setting to avoid false positives. Since *all* my addressees are spread out in 5+ addressbooks (family, friends, clients, employees, subcontractors,...), being able to whitelist *multiple* AB's is esential. € 0.02 PS. This shouldn't affect the necessary absence of LDAP AB's since they are now also hidden.
Comment on attachment 105308 [details] [diff] [review] feature enabling patch, v6 r=bienvenu
Attachment #105308 - Flags: review+
> The whitelist addressbook(s) is probably the most important setting to avoid > false positives. Since *all* my addressees are spread out in 5+ addressbooks > (family, friends, clients, employees, subcontractors,...), being able to > whitelist *multiple* AB's is esential. € 0.02 > >PS. This shouldn't affect the necessary absence of LDAP AB's since they are now >also hidden. whitelisting is an interesting topic that we should discuss more in the newsgroups. one thing to consider are viruses like klez (?) which make it appear that you are getting mail from someone you know. I've found bayesian works great, with whitelisting disabled, after it's well trained. let's take this to the newsgroup, but feel free to also log an RFE bug about whitelisting against multiple addressbooks. (no guarantees it will make it in, of course)
Comment on attachment 105308 [details] [diff] [review] feature enabling patch, v6 Patch checked in; still need to check in mac build food; that will happen later today (the builds shouldn't break without it).
Attachment #105308 - Attachment is obsolete: true
Pratik: good catch on the classic skin icon. Fix for that has just been checked in.
Dan, If the icons are checked in, shouldnt someone make note of that in Bug #178566 ?
Andrew: no, the icons that are currently checked in are just placeholders until the ones mentioned in that bug are finished.
Comment on attachment 105528 [details] [diff] [review] xpinstall packager file changes Packager fix checked in, modulo CVS conflict merge changes.
Attachment #105528 - Attachment is obsolete: true
Lots of junk mail work left to do. Tommorrow, I'll spin up new bugs on issues still to be looked at, point to them from here, make a tracking bug, and then close this one.
Classification works great, but is it intended that the settings for moving messages are greyed out (e.g. because this feature needs work)? Or am I doing something wrong?
Aleksander: The checkin comment for this says: "Turn on basic junk-mail detection functionality in mail. Parts (such as moving to other folders) are still disabled as there is work to do there yet." I think that should be a good answer for you :)
urg... in my cvs build on linux the junk mail window controls window is _extremely_ wide, that is at least 1200 pixels or so. it doesn't fit on my screen completely. should I file a new bug for that?
Same here, running trunk build 2002110808. Fortunately it can be resized, although a minimum size might be a good idea..
This feature hates me :( In trunk build 2002110808 on WinXP (installed using full installer) I just get exceptions in JavaScript console. For example pressing the Junk button causes Error: [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIMsgDBView.doCommand]" nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)" location: "JS frame :: chrome://messenger/content/mailCommands.js :: JunkSelectedMessages :: line 503" data: no] Source File: chrome://messenger/content/mailCommands.js Line: 503 Did I miss something? Tried also deleting compreg.dat to no avail.
Looks like at least bayesflt.dll is missing from the installer..
the js error has been reported as bug 179150
Was it already mentioned that there is a spelling error in the description text in Junk Mail Controls dialog (unsolicted)? The sentence "An unknown icon is displayed if the message might be junk mail." could be interpreted quite hilariously :) How about removing "by" from "..can be fine-tuned by using.."?
My preliminary work with the new junk mail controls shows that it does not APPLY any controls if the mail was alread filtered by one of my OWN filters (example: I created a filter to move any email that does not contain MY email address in the To or CC filed to a "SPAM" folder ... any email that goes into that folder has been showing up as JUNK MAIL Status: UNKNOWN .... Ive seen this same event on other PCs with different filtering setups.) One other note: Although the Junk Mail controls for Newsgroups were supposed to be disabled, they are only partially disabled, currently a user can set them up (select checkboxes, whatnot), but none of the junkmail controls actually work. They can also mark messages as Junk/Not Junk in newsgroups.
Might I suggest a UI change: Instead of a drop-down selection for AddressBook (virtual Whitelist), why not allow the users to select multiple addressbooks (via dropdown checkboxes)... (I would like to use both Personal Addressbook & Collected Addresses as a minimum)
Collected addresses? Wouldn't the spammer's address be collected?
Ere, No, Collected Addresses only collects email addresses on SENT mail -- So, unless you are responding to a spammer (which is not a good idea), using the COLLECTED ADDRESSES should be just fine.
I have enabled the junk mail controls on all my four emails accounts, but nothing happens, neither on pre-filtered mails nor on accounts with no filters at all. The icon stay on unknown, even when i mark the messages as junk. The logs stays empty.
Vincent, see bug 179150
Now that this is turned on in the trunk, please file new bugs for issues related to junk mail problems. I'll be closing this bug soon.
Junk mail probably needs its own Component, no?
Dan, When is --document.getElementById("moveOnSpam").disabled = false;-- from junkmail.js going to be implemented in trunk builds ??? Is this open on another bug already (cant find it)? Can you tell me what Bug# it is with the problem(s) preventing the feature from being turned on ... None of the current dependencies seem to relate to this feature. I imagine you guys are having problems with moving the files to a "JUNK" directory if that directory does not exist yet ???
Attached patch Fix junk mail <dialog> (obsolete) — — Splinter Review
The problem with the junk mail dialog is the js, not the xul: var gMessengerBundle = document.getElementById("bundle_messenger"); doesn't work at the top level, you need to move it to the load handler. Is it worth hiding the junk mail column/button for those accounts where junk mail is disabled?
Attachment #105964 - Flags: superreview?(sspitzer)
Attachment #105964 - Flags: review?(dmose)
I noticed that the junk mail column/button was also listed in the Sent folder. I thought that was kind of ironic. ;) Shouldn't this filter only be applied on the incoming mail/newsgroups?
Ryan: I frequently test my Junk Mail training data by running Junk Controls on my "Sent" folders (because I've got lots of legitimate messages there, and can spot any false positives immediately).
> Is it worth hiding the junk mail column/button for those accounts where junk > mail is disabled? that's being discussed in another bug. see http://bugzilla.mozilla.org/show_bug.cgi?id=179012
dmose plans on marking this fixed and spinning off any open issues to new bugs. so let's take any new issues or comments to new bugs, but first check http://bugzilla.mozilla.org/showdependencytree.cgi?id=11035 for existing issues.
Comment on attachment 105964 [details] [diff] [review] Fix junk mail <dialog> this has been moved to bug 180215
Attachment #105964 - Attachment is obsolete: true
Comment on attachment 105964 [details] [diff] [review] Fix junk mail <dialog> moved to another bug, getting of the request radar
Attachment #105964 - Flags: superreview?(sspitzer)
Attachment #105964 - Flags: superreview-
Attachment #105964 - Flags: review?(dmose)
Attachment #105964 - Flags: review-
*** Bug 182443 has been marked as a duplicate of this bug. ***
Just wnat to make the observation that the icon in the toolbar is still set to the same as 'mark', which makes it awkward for those of us who like to have the toolbar as 'icon only - no text'. I know you have your hands full though, so this has to be low priority... Max.
I've been using this feature for about a week now, and I'm finding that I have one huge irritation with the way it's currently implemented, which is that every time I find a new bit of junk mail that the filters missed, I have to select the message, mark it as junk, and then (of course) delete it. I think the default behavior for the "junk button" should be to delete it (but not for toggling the icon in the message list or selecting "mark as junk" in the tools menu :-). Somehow, that's just what my brain "expects" to happen when I press a big button labelled "junk". It sound like a much more "active" verb than "Mark". Also, it would save the user a huge amount of effort during training. If need be, this could be a junk controls option for those rare people who want to collect their trash and preserve it in a shrine. Perhaps a way to do this is to have an "action when clicking Junk" preference that has 3 choices: do nothing, apply above filter action, and delete (with delete the default). I don't like arbitrary preferences in general, but since I'm asking for a feature with slightly inconsistent behavior, it seems only polite. I'm also confused about what the point is of having a separate "junk mail controls" dialog. Shouldn't that just be a tree item in the normal preferences? If evangelizing the feature (a good idea, IMHO) is the goal, a menu item that jumps directly to the tree item would be the way to go.
Yes, Yahoo free webmail has gone through this already. Initially you had (when logging into your mailbox) to select all non-already-identified as junk messsages, click on "This is Spam..." ... then go again and delete everything. Now it's a one-step and much easier... Selecting "This is Spam" does both "Mark as Junk", report it, and delete the emails. PS: I believe that Paul Graham initial document also specified this behavior, that User-Agents should have two delete buttons, "Delete" and "Delete as Junk", the last one doing being equivalent to "Mark as Junk" + "Delete" of course.
taking, I'll be trying to finish up the feature for 1.3 beta. about some of the comments people have made, jglick has an update spec that should address some of the common complaints, like automatically deleting messages when they are marked as junk. once it's public, I'll link to it. for now, I'm starting with finishing the code so that we can enable the "move to junk folder" UI. (see bug #181397)
Assignee: dmose → sspitzer
Status: ASSIGNED → NEW
Depends on: 181397
Target Milestone: mozilla1.3alpha → mozilla1.3beta
This looks great.. here are a few additional comments to make this as simple as possible for new users of this feature. Going back to what I've experienced with the Yahoo.com webmail "This is spam" feature, basically when you log into your mail every day, you get to see a few Junk mail messages that the built-in filter has not properly moved into the "Spam" folder by itself. So the first action is to select every message that has not been properly classified as Junk, and click on "This is spam", which should both flag it as spam and make it disappear from your life, either moving it into the "Spam" folder, or deleting it. So it should be a one-step, no-brainer, and as the accuracy of the scoring system improves with training, the user should have to do the manual step above less and less frequently. For these reasons, I suggest that: the checkbox "Automatically delete messages when I manually mark them as junk" should be checked by default for new users, to accomplish everything in a single step. Additionally, I have a nitpicking question about the Mark as Junk/Not Junk button, whose status depends on whether the messages you have selected is or is not junk. If we select multiple messages (inc ones that are labeled Junk and others Not Junk) can we explicitely specify the status of this button in the spec? I would suggest that the user is doing a bulk-select of these messages to either a) mark them as junk and delete them, or b) mark them as not junk b) doesn't make sense, because if you want to "unmark" as junk, you do not need to select messages that are not marked as junk so only case a) makes sense to me... -> in case of multiple selection of messages with junk/not junk ones, the button should read "Mark as Junk" and delete/move the messages if the corresponding pref is set. PS: one has to ensure that "marking as junk" a message within the selection that is already "marked as junk" is a no-operation and does not cause weird side effects. Hope this helps -- Nicolas
I think Nicolas' comment 171 implies this, but it should be possible to configure things so that manually marking a message as junk moves it to the junk folder. The current UI spec. seems to only support deleting/moving to the Trash. Why do I want this? Maybe I am strange, but I keep a folder named SPAM that holds thousands of junk messages I have received recently so that I can use it to re-train filtering such as that now built into Mozilla. Very useful.
Yes, sorry for not being very clear... The bottom line is that Spam is awfully annoying, and all it should take to make it disappear is multiple select of mis-classified messages and *one click* (button, whatever) to do everything that needs to be done. That's almost the Yahoo.com UI (it's actually 2-click, you select all messages, "This is Spam" in the drop-down, click on OK, and it takes you to an intermediate webpage where you can choose between [X] reporting spam ("more effective") [ ] block senders
Thinking about the above again... If you want to keep a "Junk archive" folder with all the confirmed/verified Spam you have ever received, doesn't that mean that we need 3 folders in fact? - Inbox, getting new messages scored as "probably not junk" - Junk, getting new messages scored as "probably junk" - Junk Archive (or Trash, depending on whether you want to keep or delete confirmed Junk) that keeps mails you have confirmed as Junk The reason for the above is that there are ALWAYS false positives and false negatives, even if the system aims at reducing this possibility as much as possible. Even a 99% correct scoring system will have 1 mail every 100 go into the wrong folder. So you will need to monitor your Inbox to manually "junkify" messages that were incorrectly scored, but you also need to monitor the "Junk" folder to "de-junkify" messages that were invalid scored as junk! If you keep all your junk archive (like 2000 messages) into the Junk folder, it is not going to be easy to go and check every day in there if any new message was deposited there due to an invalid scoring. Here's what Yahoo.com webmail currently uses: Incoming messages end up in either - Inbox - Junk next to the "Junk" and "Trash" folder is a special button, [EMPTY] that empties/deletes irremediably everything into the Junk folder or the Trash. So the work cycle of a user, when checking mail, is to: - open the Inbox, mass select stuff that wasn't scored as Junk correctly, click on "This is Spam" and have it disappear - open the Junk folder, verify nothing was incorrectly placed here, click on the "EMPTY" button that's it. If you have 2000 messages in the Junk folder, it is not going to be easy to spot individual messages that were not classified here correctly. I believe the Inbox / JUNK / Trash paradigm will correspond to the majority of users who do not want to archive JUNK mails. If you want to archive JUNK mails, then my suggestion is a 3-folder model: incoming email is scored and dispatched into Inbox or Junk, and once manually verified as JUNK, these emails are archived into a special folder called "JUNK Archive" or "Verified Junk" or whatever. This will keep the "Junk" inbox manageable and verify-able on a daily basis... What do you think Mark ?
Why not give the user access to the raw scores and to the thresholds defined for junk / nojunk (and unknown, see below)? As with each binary classifier, the crucial issue is false positives vs false negatives. Different users might have very different preferences which rates they find acceptable. Different thresholds for the junk class will give very different behavior of these rates, and the optimal threshold will largely depend on the importance a user assigns to the balance between them. Also, I do not see why classification uncertainty (i.e. scores which are not very close to 1.0 or 0.0) is not communicated to the user. These are the emails where manual inspections will probably be most important. Access to the raw score will also make it easier to define filter actions that are more adapted to personal preferences. This does not need to be enabled by default, but the score could be an additional column in the mail list and the thresholds could be set in a tab of the junk mail control dialog.
I am sure that having 2 junk related folders is overkill and too complex for most people. We should optimize the interface for what 80% of the users out there want. My guess is that the scenario describe in comment 174 probably meets most people's needs, but I don't have any real data. Just thinking about myself, I am very paranoid about false positives so I do not intend to enable automatic refiling of potential junk. My own work pattern looks like this: 1) Let the automatic junk classification do its thing on incoming messages. Now my Inbox contains some messages that are marked as junk. 2) Switch the Inbox view to "Junk" (I added this as a custom view). Select all and move the messages to my Junk folder. For this step it would be very convenient if I could just click on the Junk icon in the toolbar and have the refiling occur. 3) Switch the Inbox view back to "All" and manually mark and refile the remaining (undetected) junk messages as I read the non-junk messages. For this step it would again be very convenient if I could just click on the Junk icon in the toolbar and have the refiling occur. So I only need Mozilla to know about one Junk folder, but I do want separate options to control whether refiling occurs during automatic junk detection (I would leave that off) and whether refiling occurs when a message is manually marked as junk (I would turn that on).
Granted, people who want a 3 folder model can manually create a normal folder called "Junk Archive", and when they verify daily the "Junk Inbox" and spot nothing wrong, can mass select (Ctrl-A) all messages and move them manually into the "Junk Archive" folder. so this is doable manually and doesn't need custom code -- the Junk folder can be made small so that it's manageable on a daily basis. People who want to delete the Junk contents can also mass-select (Ctrl-A) and Shift-Delete to purge these emails once a day, although an "Empty Junk" menu next to "Empty Trash" could also be a one-step more convenient feature.
I'm a little confused on how far the implementation had gone. I'm using 2003010904 build and my Junk Mail Controls screen has the bottom section (the main part) greyed out, as if it was an incomplete feature. What's the real status of this? Is it really implemented in the beta, or is this a bug I should report? I'd like to beta test this feature if I could.
Brendan: see bug 181394 for the move functionality.
accepting. turning this into the meta bug for this feature. jglick is working on updating the spec, and I'm working on some of the backend and fe changes. the goal is still 1.3 beta
Status: NEW → ASSIGNED
Summary: front end work for spam / junk mail → [meta] finish junk mail feature
I'm not sure if this has been mentioned yet or not, but what happens if I don't want to keep a "junk archive"? For example, if I CTRL-A Select All a whole bunch of spam messages, and then subsequently delete them, will they still be remembered by the system? Or does it always need some sample spam to go by. The same goes for non-spam messages. I don't keep many "non-spam" messages around in my mail folders. Just the bare minimum, and the rest I just delete. Will MozMail remember these as being non-spam? Or once things are deleted, does MozMail act as if it never received those messages in the first place? Sorry if this question has already been addressed.
By the way, those who are interested in this bug/feature may also want to take a peek at bug 187044 which requests support for a challenge/response system. In challenge/response, you accept email only from users in your whitelist or if the sender sends you a password... otherwise, you reply with a message explaining how to figure out the password (and send it in the subject line). Sending a message to that address, or receiving the password from that address, adds that address to the whitelist, so the password is only needed to start conversations. Bug 169638 and bug 187044 are complementary - both are techniques for getting rid of junk aka spam, and hopefully they're different enough that the combination would be very effective.
QA Contact: laurel → esther
Bug 188232 may be a candidate to be part of this [meta] bug.
this won't be complete until 1.4, so moving out.
Target Milestone: mozilla1.3beta → mozilla1.4beta
nominating bug 194762 and bug 194779 for dependency
Nominating bug 193625
Adding two bugs which cause messages to be classified as SPAM even though the addresses are listed in the Personal Address Book
Depends on: 187239, 196777
Loosely related to junk mail front-end: Switching to Daylight Savings Time loses all junk mail status on messages apparently, according to bug 136049 comment 4, due to .msf files being invalidated and rebuilt by the DST switch.
When you mark a message as "Junk", the junk mail controls don't get executed. i.e. the message doesn't get moved into the Junk folder automatically. 5/15 nightly build.
What about delayed moving to junk folder If i set manualy junk status then is moved to junk this 3-5sec will be nice if i made mistake
How about using Edit | Undo Move/Junkify if you realize you made a mistake ? :-)
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Status: ASSIGNED → NEW
Flags: blocking-seamonkey1.0b?
err. junk mail is "finished". any remaining issues should live in other bugs.
Status: NEW → RESOLVED
Closed: 19 years ago
Flags: blocking-seamonkey1.0b?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: