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

[meta] finish junk mail feature

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.4beta

People

(Reporter: sspitzer, Unassigned)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 40 obsolete files)

133.92 KB, image/gif
Details
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

Created:
Updated:
Size: