Closed
Bug 253234
Opened 21 years ago
Closed 19 years ago
[patch]No Option to set Junk Mail as Read
Categories
(Thunderbird :: Mail Window Front End, enhancement)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ray, Assigned: jens.b)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
jens.b
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 1•21 years ago
|
||
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.
Comment 6•20 years ago
|
||
this worked fine for me, on the thunderbird trunk, at least - not sure about the
1.0 branch
Comment 7•20 years ago
|
||
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 8•20 years ago
|
||
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 9•20 years ago
|
||
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.
Comment 10•20 years ago
|
||
This port is mostly a UI port, but there's some non-UI js code.
Comment 11•20 years ago
|
||
*** Bug 258842 has been marked as a duplicate of this bug. ***
Comment 12•20 years ago
|
||
Please fix this missing crucial feature
Comment 13•20 years ago
|
||
*** Bug 258572 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Attachment #160621 -
Flags: superreview?(bienvenu) → superreview+
Comment 14•20 years ago
|
||
is this patch still valid and if so any chance it could get checked in?
Comment 15•20 years ago
|
||
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.
Comment 16•20 years ago
|
||
*** Bug 278164 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Flags: blocking-aviary1.1?
Summary: No Option to set Junk Mail as Read → [patch]No Option to set Junk Mail as Read
Comment 17•20 years ago
|
||
(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?
Comment 18•20 years ago
|
||
switching to Thunderbird from Outlook...this is a big annoyance for me...any
chance this could be put into the 3.1 bugs?...please
Comment 19•20 years ago
|
||
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... :-(
Assignee | ||
Comment 20•20 years ago
|
||
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)
Comment 21•20 years ago
|
||
would certainly consider a patch but this is not a blocker.
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Assignee | ||
Updated•20 years ago
|
Attachment #160621 -
Flags: review?(bryner) → review?(dmose)
Assignee | ||
Comment 22•20 years ago
|
||
Unbitrotting the patch; carrying over sr=bienvenu.
Attachment #160621 -
Attachment is obsolete: true
Attachment #190235 -
Flags: superreview+
Attachment #190235 -
Flags: review?(dmose)
Assignee | ||
Updated•20 years ago
|
Attachment #160621 -
Flags: review?(dmose)
Comment 23•20 years ago
|
||
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-
Comment 24•20 years ago
|
||
(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.
Assignee | ||
Comment 25•20 years ago
|
||
Taking. Thanks for your work, Eyal!
Assignee: eyalroz → jens.b
Status: ASSIGNED → NEW
Assignee | ||
Comment 26•20 years ago
|
||
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].
Assignee | ||
Updated•20 years ago
|
Attachment #190235 -
Attachment is obsolete: true
Attachment #190879 -
Flags: superreview+
Attachment #190879 -
Flags: review?(dmose)
Comment 27•19 years ago
|
||
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-
Assignee | ||
Comment 28•19 years ago
|
||
(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)
Comment 29•19 years ago
|
||
(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.
Assignee | ||
Updated•19 years ago
|
Attachment #190879 -
Attachment is obsolete: true
Assignee | ||
Comment 30•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Attachment #192062 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #192062 -
Flags: review?(dmose)
Assignee | ||
Comment 31•19 years ago
|
||
... now without dump() statements.
Attachment #192063 -
Flags: superreview+
Attachment #192063 -
Flags: review?(dmose)
Comment 32•19 years ago
|
||
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+
Assignee | ||
Comment 33•19 years ago
|
||
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+
Assignee | ||
Updated•19 years ago
|
Attachment #192118 -
Flags: superreview?(bienvenu)
Updated•19 years ago
|
Attachment #192118 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 34•19 years ago
|
||
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+
Assignee | ||
Comment 35•19 years ago
|
||
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?
Comment 36•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Attachment #192118 -
Attachment description: backend patch → backend patch (checked in on trunk)
Comment 37•19 years ago
|
||
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?
Comment 38•19 years ago
|
||
*** Bug 311972 has been marked as a duplicate of this bug. ***
Comment 39•19 years ago
|
||
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!
Comment 40•19 years ago
|
||
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.
Comment 41•19 years ago
|
||
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.
Comment 42•19 years ago
|
||
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: 19 years ago
Resolution: --- → FIXED
Comment 43•18 years ago
|
||
Is there any chance that this patch will end up in Thunderbird 2?
Comment 44•18 years ago
|
||
(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?
Comment 45•18 years ago
|
||
> 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 46•18 years ago
|
||
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)
Comment 47•18 years ago
|
||
(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. :/
Comment 48•18 years ago
|
||
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.
Description
•