Closed Bug 253234 Opened 20 years ago Closed 18 years ago

[patch]No Option to set Junk Mail as Read

Categories

(Thunderbird :: Mail Window Front End, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ray, Assigned: jens.b)

References

Details

Attachments

(1 file, 6 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040724
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040724

In Mozilla plain, there is an option to mark all mail that is junk read.  This
option is missing in Thunderbird.  

The option in Mozilla allows the user to have all mail marked as junk marked read.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
That was Seamonkey bug 193625, which apparently was never ported to Thunderbird.
I was wondering what happened to that when I moved to Thunderbird. Of course all
the reasons why this was important in bug 193625 still apply.
OS: Windows XP → All
Hardware: PC → All
I think you can work around this bug for the time being by adding this to prefs.js:

user_pref("mail.server.server1.markAsReadOnSpam", true);

I'm waiting for a spam to come in so I can see if it works. However front end
access to this option is critical (or else we should set this pref by default,
but I doubt that will be a popular solution).
That workaround didn't work, either. Even with that set my spam still shows as
read, so there's some background work that also needs to be done.
Unread*
this worked fine for me, on the thunderbird trunk, at least - not sure about the
1.0 branch
Still haven't managed to build Thunderbird, but this patch is basically the
same changes the .js, .xul and .dtd files that were applied to seamonkey.
Somebody with build capabilities tell me whether this is working (no reason it
shouldn't, right? ...) - or wait for me to get my build process working.
Assignee: mscott → eyalroz
Status: NEW → ASSIGNED
Comment on attachment 160621 [details] [diff] [review]
same changes to chrome as in the seamonkey patch

Feature seems to be working (finally managed to build tbird), so it's review
time.
Attachment #160621 - Flags: superreview?(bienvenu)
Attachment #160621 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 160621 [details] [diff] [review]
same changes to chrome as in the seamonkey patch

If this is a UI port then you just need one of mscott or bienvenu to review.
This port is mostly a UI port, but there's some non-UI js code.
*** Bug 258842 has been marked as a duplicate of this bug. ***
Please fix this missing crucial feature
*** Bug 258572 has been marked as a duplicate of this bug. ***
Attachment #160621 - Flags: superreview?(bienvenu) → superreview+
is this patch still valid and if so any chance it could get checked in?
AFAIK it's still valid, and if it isn't it's just a matter of minor tweaking -
remember, it's just chrome changes, the backend is there since I fixed bug
193625. Henrik, we just need to get someone to review the patch.
*** Bug 278164 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1?
Summary: No Option to set Junk Mail as Read → [patch]No Option to set Junk Mail as Read
(In reply to comment #15)
> AFAIK it's still valid, and if it isn't it's just a matter of minor tweaking -
> remember, it's just chrome changes, the backend is there since I fixed bug
> 193625. Henrik, we just need to get someone to review the patch.

As far as I understand sr=bienvenu is sufficient for this patch, because it only
changes files in mail/. Is this correct, David?
switching to Thunderbird from Outlook...this is a big annoyance for me...any
chance this could be put into the 3.1 bugs?...please
I do wish someone would take the time to review this bug. CC list members are
more than welcome to find reviewers other than Neil for it. I'm not sure I have
time to make any _corrections_, but I _am_ 95% certain the patch works well as is.

This is another one of the reasons I'm sticking with seamonkey... :-(
Comment on attachment 160621 [details] [diff] [review]
same changes to chrome as in the seamonkey patch

Brian, do you have time to review this? According to eyalroz, this is just a
port of a Seamonkey feature missing in Thunderbird.
Attachment #160621 - Flags: review?(neil.parkwaycc.co.uk) → review?(bryner)
would certainly consider a patch but this is not a blocker.
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Attachment #160621 - Flags: review?(bryner) → review?(dmose)
Attached patch unbitrotted version of #160621 (obsolete) — Splinter Review
Unbitrotting the patch; carrying over sr=bienvenu.
Attachment #160621 - Attachment is obsolete: true
Attachment #190235 - Flags: superreview+
Attachment #190235 - Flags: review?(dmose)
Attachment #160621 - Flags: review?(dmose)
Blocks: 220933
Comment on attachment 190235 [details] [diff] [review]
unbitrotted version of #160621

> 
>-function saveJunkMsgForAction(aServer, aMsgURI, aClassification)
>+function determineActionsForJunkMsgs(aView, aIndices, aActionParams)

Can you add some comments here about what this function is supposed to do as
well as what the params are and how they are used?  doxygen format preferred. 
Additionally, why is aActionParams an inout param at all rather than just a
return value? -- it makes the code significantly less obvious on first reading.

>   var spamFolderURI = spamSettings.spamFolderURI;
>   if (!spamFolderURI)
>-    return;
>-  
>-  var spamFolder = GetMsgFolderFromUri(spamFolderURI);
>-
>-  if (spamFolder) 
>   {
>-    gJunkKeys[gJunkKeys.length] = msgHdr.messageKey;
>-    gJunkTargetFolder = spamFolder;
>+    dump('no spam folder!');
>+    return;
>   }

This doesn't seem to be much better than just returning.  Given that this state
is an action that the user could inadvertantly cause (by mistakenly deleting
the spam folder), it seems to me that we should get the nsIPromptService and
pop up a dialog here.  Even if there's no time for that now, at least put an
XXX comment here suggesting it for the future.


>-function performActionOnJunkMsgs()
>+function performActionsOnJunkMsgs(aIndices)
> {
>-  if (!gJunkKeys.length)
>-  {
>-    gJunkTargetFolder = [];
>-    return;
>-  }
>+  var treeView = gDBView.QueryInterface(Components.interfaces.nsITreeView);
>+  var actionParams = { markRead: false, junkTargetFolder: null };
>+  determineActionsForJunkMsgs(treeView,aIndices,actionParams);
> 
>-  var indices = new Array(gJunkKeys.length);
>-  for (var i=0;i<gJunkKeys.length;i++)
>-    indices[i] = gDBView.findIndexFromKey(gJunkKeys[i], true /* expand */);
>+  if (!aIndices.length)
>+    return;

Why does this check & return happen after the call to
determineActionsForJunkMsgs?

>
>+            if (aLastMessage) 
>+            {
>+              gJunkmailComponent.endBatch();
>+              performActionsOnJunkMsgs(aJunkMsgIndices);
>+            }

I don't think endBatch is on the junkmail interface any more.  Unless I'm wrong
about that, I would have expected this to have turned up in testing.  Can you
re-test with JS strict warnings on and make sure this doesn't do anything
unexpected?

>     // if we are whitelisting, check if the email address is in the whitelist addressbook.
>-    var spamSettings = aMsgHdr.folder.server.spamSettings;
>-    if (spamSettings.useWhiteList && spamSettings.whiteListAbURI)
>+    if (aWhiteListDirectory)

The above comment is no longer quite correct; we're really checking to see if
we were given a whitelist addrbook at all. 

> 
>-function analyzeFolderForJunk()
>+function filterFolderForJunk()

This function could use some comments interspersed with the code for easier
reading. startBatch is also gone from the junk mail component, I believe.
Attachment #190235 - Flags: review?(dmose) → review-
(In reply to comment #23)

Unfortunately, I can't work on mozilla for the time being, and specifically not
on this patch. I also no longer remember that JS code, which I haven't really
touched in well over a year. So I'm asking someone else to step up and take this
upon himself/herself. Like I said last year, the code here is basically the same
as in seamonkey, except that after this patch was posted the start batch / end
batch were removed so they should indeed no longer be used.
Taking. Thanks for your work, Eyal!
Assignee: eyalroz → jens.b
Status: ASSIGNED → NEW
Attached patch patch v2 (obsolete) — Splinter Review
Addressing review comments. Additionally, I added a comment explaining how
analyzeMessageForJunk() works and how it's called, and changed some broken
indentation.

This patch was tested both with incoming junk and the "Tools->Run Junk Controls
on Folder" command - while testing the previous patch, I forgot the latter and
hence did not discover the not working calls to startBatch/endBatch.

Carrying over sr=bienvenu from original attachment #160621 [details] [diff] [review].
Attachment #190235 - Attachment is obsolete: true
Attachment #190879 - Flags: superreview+
Attachment #190879 - Flags: review?(dmose)
Comment on attachment 190879 [details] [diff] [review]
patch v2

OK, this is looking good.  A few more comments/questions:

>-function analyze(aMsgHdr, aNextFunction)
>+/**
>+ * Starts the message classification process for a message. This method creates
>+ * a listener object for each message that saves the junk score, and calls
>+ * performActionsOnJunkMsgs() once all messages are classified.
>+ *
>+ * @param aMsgHdr
>+ *        The header of the message to classify.
>+ * @param aMsgIndex
>+ *        The message's index inside the folder.
>+ * @param aJunkMsgIndices
>+ *        A global array for collecting the indices of messages that are
>+ *        classified as junk. The array is passed to performActionsOnJunkMsgs()
>+ *        once all messages are classified.

I'm not sure what "global" is intended to mean here.  More to the point,
though, I don't see why aJunkMsgIndices is a parameter
on analyzeMessageForJunk at all, since it doesn't appear to be used outside of
the function.  Why not just
make the array local to analyzeMessageForJunk?

>+function filterFolderForJunk()
> {
>
> [...]
>
>   for (var i = 0; i < count; i++) {
>+    var msgURI;
>     try
>     {
>-      messages[i] = view.getURIForViewIndex(i);
>+      msgURI = view.getURIForViewIndex(i);
>     }

Just use |var msgURI = ....| inside the try block.

>+      // blow off errors here - dummy headers will fail
>+      msgURI = null;
>+    }
>+    if (msgURI)
>+    {
>+      var msgIndex = i;
>+      var msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI);
>+      analyzeMessageForJunk(msgHdr, msgIndex, junkMsgsArray, (i == count-1),
>+                            whiteListDirectory);
>+    }
>   }

What's the point of having a separate msgIndex variable?

> function JunkSelectedMessages(setAsJunk)
> {
>   MsgJunkMailInfo(true);
>   gDBView.doCommand(setAsJunk ? nsMsgViewCommandType.junk : nsMsgViewCommandType.unjunk);
>+  // XXX TODO
>+  // consider whether these messages should be
>+  // marked as read (if markAsReadOnSpam is set)
> }

It seems to me that the obvious thing to do would be to mark them as read. 
What rationale is there for doing otherwise?
Attachment #190879 - Flags: review?(dmose) → review-
(In reply to comment #27)
> >+ * @param aJunkMsgIndices
> >+ *        A global array for collecting the indices of messages that are
> >+ *        classified as junk. The array is passed to performActionsOnJunkMsgs()
> >+ *        once all messages are classified.
> 
> I'm not sure what "global" is intended to mean here.  More to the point,
> though, I don't see why aJunkMsgIndices is a parameter
> on analyzeMessageForJunk at all, since it doesn't appear to be used outside of
> the function.  Why not just
> make the array local to analyzeMessageForJunk?

The function is called for each individual message, and once the classification
(which happens asynchronously) is done, a listener gets activated and puts the
message index into aJunkMsgIndices. When the last listener gets activated, it
calls performActionsOnJunkMsgs(), passing the aJunkMsgIndices array. So
aJunkMsgIndices cannot be local, as it would lose its values between invocations
of analyzeMessageForJunk. It has got to be the same instance for all listener
objects, so either it has to be a parameter to analyzeMessageForJunk(), or it
has to be global (not declared in a function).

This mechanism is indeed a bit weird, and it took me a while to understand why
the original author did it this way, but I couldn't come up with a
simplification yet. Any ideas? Perhaps using only one listener instance that
keeps track of the messages which have to be classified in a member variable
(without passing things around constantly)?

> > function JunkSelectedMessages(setAsJunk)
> > {
> >   MsgJunkMailInfo(true);
> >   gDBView.doCommand(setAsJunk ? nsMsgViewCommandType.junk :
nsMsgViewCommandType.unjunk);
> >+  // XXX TODO
> >+  // consider whether these messages should be
> >+  // marked as read (if markAsReadOnSpam is set)
> > }
> 
> It seems to me that the obvious thing to do would be to mark them as read. 
> What rationale is there for doing otherwise?

This function is used when marking messages as junk, which is the scope of bug
220933. At bug 220933 comment 7, the reporter states that he didn't want the
messages marked as read when they are auto-classified, but only when he manually
marks them as junk. Besides, it seems this function only applies to
toolbar/menu, not to clicking the 'Junk' column.

I want to work out that stuff at the other bug, and get this one in first (last
but not least because unlike this bug, bug 220933 most likely hasn't any l10n
impact and can slip in later, or could be done in an extension if it doesn't get in)
(In reply to comment #28)
> 
> This mechanism is indeed a bit weird, and it took me a while to understand why
> the original author did it this way, but I couldn't come up with a
> simplification yet. 

Ah, I see now.  I was misunderstanding as well.  That's one strike against this form,
as it's not obvious from reading what's going on.

> Any ideas? Perhaps using only one listener instance that
> keeps track of the messages which have to be classified in a member variable
> (without passing things around constantly)?

That would work.  Another option would be declaring analyzeMessageForJunk inside of 
filterFolderForJunk, which would allow the array to live in the outer function but be accessible to the 
inner, due to JS's lexical closure.  Though your original point of just making this a global sounds fine to 
me as well (and is probably the least amount of work).  Feel free to do whichever of these three options 
you prefer.

> > > function JunkSelectedMessages(setAsJunk)
> > > {
> > >   MsgJunkMailInfo(true);
> > >   gDBView.doCommand(setAsJunk ? nsMsgViewCommandType.junk :
> nsMsgViewCommandType.unjunk);
> > >+  // XXX TODO
> > >+  // consider whether these messages should be
> > >+  // marked as read (if markAsReadOnSpam is set)
> > > }
> > 
> > It seems to me that the obvious thing to do would be to mark them as read. 
> > What rationale is there for doing otherwise?
>
> [...]
> 
> I want to work out that stuff at the other bug, and get this one in first (last
> but not least because unlike this bug, bug 220933 most likely hasn't any l10n
> impact and can slip in later, or could be done in an extension if it doesn't get in)

Sounds reasonable to me.
Attachment #190879 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
I replaced analyzeMessageForJunk() with an object that keeps track of pending
messages and implements the listener. That didn't shorten the code, but its
workings should be far more obvious now.
Attachment #192062 - Flags: superreview+
Attachment #192062 - Flags: review?(dmose)
Attachment #192062 - Attachment is obsolete: true
Attachment #192062 - Flags: review?(dmose)
Attached patch patch v3.1 (obsolete) — Splinter Review
... now without dump() statements.
Attachment #192063 - Flags: superreview+
Attachment #192063 - Flags: review?(dmose)
Comment on attachment 192063 [details] [diff] [review]
patch v3.1

>     var messageURI = aMsgHdr.folder.generateMessageURI(aMsgHdr.messageKey)
>         + "?fetchCompleteMessage=true";
>-    gJunkmailComponent.classifyMessage(messageURI, msgWindow, listener);
>+    gJunkmailComponent.classifyMessage(messageURI, msgWindow, this);
>+
>+    this.messages[messageURI] = {hdr: aMsgHdr, index: aMsgIndex};

Because of the way the code currently works, it's probably not possible to have
the callback happen before you assign this.messages[messageURI].  However, I
don't know if that's guaranteed to be the case forever.  As a hedge, I'd
suggest that you switch the order of these two statements.  Otherwise, this
code looks great -- much more readable.  r=dmose with that one change.
Attachment #192063 - Flags: review?(dmose) → review+
Addressing review comment, fixing two typos and a JS strict warning. Splitting
into backend and frontend part, as the latter is up to mscott.

Carrying over r=dmose.
Attachment #192063 - Attachment is obsolete: true
Attachment #192118 - Flags: review+
Attachment #192118 - Flags: superreview?(bienvenu)
Attachment #192118 - Flags: superreview?(bienvenu) → superreview+
Attached patch frontend patch (obsolete) — Splinter Review
Carrying over r=dmose. Scott, can you have a look at this? It adds a radio
button labelled "Mark messages determined to be Junk as read" in the junk mail
controls, below the "Move incoming messages determined to be junk mail to"
options.
Attachment #192119 - Flags: superreview?(mscott)
Attachment #192119 - Flags: review+
Comment on attachment 192118 [details] [diff] [review]
backend patch (checked in on trunk)

I'd like to have this in the beta, possibly even without UI. Risk should be
low, as this is basically a port of SeaMonkey code running since last year.
Attachment #192118 - Flags: approval1.8b4?
No longer blocks: 220933
Checked in "backend patch" on the trunk:

Checking in mail3PaneWindowCommands.js;
/cvsroot/mozilla/mail/base/content/mail3PaneWindowCommands.js,v  <--  mail3PaneW
indowCommands.js
new revision: 1.24; previous revision: 1.23
done
Checking in mailCommands.js;
/cvsroot/mozilla/mail/base/content/mailCommands.js,v  <--  mailCommands.js
new revision: 1.21; previous revision: 1.20
done
Attachment #192118 - Attachment description: backend patch → backend patch (checked in on trunk)
Comment on attachment 192118 [details] [diff] [review]
backend patch (checked in on trunk)

let's wait on the rest of this and mscott's review. please rerequest approval
when that's happened.
Attachment #192118 - Flags: approval1.8b4?
*** Bug 311972 has been marked as a duplicate of this bug. ***
I think it'd be great for Thunderbird to have the option in its Junk Mail Controls to automatically mark as read messages that are deemed to be junk, provided that if the sender requests a return receipt, no return receipt is sent.

Also, it would be great (another enhancement on this enhancement) that (I know this depends on other bugs/enhancement requests) when marking a messages marked as junk in the junk mail folder as NOT JUNK, the message gets moved back to its original folder (INBOX) and then the read status is set to NOT READ (to make it easy to find the message that we just moved back there by marking it not junk).

Thanks!
So will this be implemented one day?
This bug is now over 1 year old, so IMHO it should be done (thunderbird is under active development, isn´t it *ggg)
The suggestion from the previous poster with moving messages back is also very great, I did submit a bug report for that, too.
In TB 1.5 on XP Pro SP2, even if I use 'user_pref("mail.server.server1.markAsReadOnSpam", true);' in prefs.js, I get the following:

New spam arrives. Junk controls fail to identify it.
I mark it manually as spam, which moves it to a separate IMAP account.
In the new account, the (now marked as read) email is still marked as read but not as spam.

A systray "envelope" icon appears to notify me that I have new mail in my junk folder -- even though the folder contains no unread mail.

The reworking of the junk settings at bug 257990 has included a checkbox for this option; see bug 257990 comment 36.  This is available now in the 3a1 (trunk) builds; I'm not sure if it's on the branch yet.

Note that with these changes, there is a single preference controlling this behavior, rather than several account-specific ones.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Is there any chance that this patch will end up in Thunderbird 2?
(In reply to comment #43)
> Is there any chance that this patch will end up in Thunderbird 2?

As far as I know, it is already in Thunderbird *1.5*. While this bug is marked as FIXED, the front-end patch has not been checked into the trunk, let alone the 1.8 branch (Thunderbird2).

We're still waiting for mscott to approve it, correct?
> As far as I know, it is already in Thunderbird *1.5*. While this bug is marked
> as FIXED, the front-end patch has not been checked into the trunk, let alone
> the 1.8 branch (Thunderbird2).
> 
> We're still waiting for mscott to approve it, correct?

Ok, I really meant the feature, not the patch.

Comment on attachment 192119 [details] [diff] [review]
frontend patch

The front-end part is fixed both on branch and trunk (prefs -> privcacy -> junk), so the front-end patch should be obsolete.
Attachment #192119 - Attachment is obsolete: true
Attachment #192119 - Flags: superreview?(mscott)
(In reply to comment #46)
> (From update of attachment 192119 [details] [diff] [review] [edit])
> The front-end part is fixed both on branch and trunk (prefs -> privcacy ->
> junk), so the front-end patch should be obsolete.

All I want is a one simple checkbox "mark junk mail as read". Will this feature be in Thunderbird 2.0?

It is not in alpha 1, which makes me very disappointed. I have no idea about your development process, so could you tell me what I can do to help the feature to be in the final Thunderbird 2? If it's not there, it will not be for years or ever, and I fear it's Outlook time then for me. :/
It's in 2.0a1, like I said: under preferences/options -> Privacy -> Junk -> [] Mark messages determined to be junk as read.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: