Closed
Bug 193625
Opened 22 years ago
Closed 21 years ago
Option to mark incoming junk mail as read
Categories
(MailNews Core :: Filters, enhancement)
MailNews Core
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: trevor, Assigned: sspitzer)
References
Details
Attachments
(1 file, 15 obsolete files)
5.51 KB,
patch
|
neil
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030211
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030211
I tried out the junk mail control feature for the first time ever in Mozilla
1.3b, and MAN, this thing kicks a**! No spam at all for one whole week and
counting! I've got it trained on about 800 spams and a couple thousand hams, and
every time some spam comes into my inbox, Mozilla magically marks it as junk and
moves it to the junk mail folder.
There's only one problem. When a spam does come in, it's marked as "unread".
That makes my junk mail folder switch to bold text, drawing my attention to it
(as is the case with any Mozilla Mail folder containing unread messages).
Trouble is, I don't *want* to pay attention to spam. I want to mark the spam as
"read" and be done with it. There doesn't seem to be any option to do this in
the Junk Mail Controls dialog box. As a workaround, I tried turning off the
"Move incoming messages determined to be junk" option in Junk Mail Controls and
instead set up a filter with the following rules:
* Match all of the following: Junk Status - is - Junk
* Perform this action: Move to folder - Junk
* Perform this action: Mark the message as read
This filter has no effect when enabled. Mozilla properly flags incoming spam as
junk, but it doesn't move it and it doesn't mark it as read.
I'm sure there are people out there who would agree with me that a "Mark
incoming junk messages as read" option would be an excellent feature to have.
(I've searched Bugzilla for previous RFEs on this, but could only find generic
RFE bugs relating to the junk mail controls. I think this one's specific enough
to warrant a separate bug number.)
Reproducible: Always
Steps to Reproduce:
Comment 1•22 years ago
|
||
I agree, junk messages being marked unread is distracting. Whenever I receive
junk mail, the first thing I do is mark it as read.
Marking NEW, CC'ing jglick.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•22 years ago
|
||
*** Bug 195436 has been marked as a duplicate of this bug. ***
Comment 3•22 years ago
|
||
I personally like the existing behavior, since it allows me to quickly scan the
newly filed mail in the Junk folder for false positives. Not many of those seem
to pass through, but regardless, that is what I have found myself doing.
Comment 4•22 years ago
|
||
That's why the request is for an option.
Comment 6•22 years ago
|
||
It would be interesting also to have a new condition under message filters:
"Status - is - Junk". This would be a great improvement for whoever wants to
perform complex actions on incoming messages, because it allows to label just
the messages that are NOT spam too; not everybody moves spam in the junk folder,
someone still keeps it in the inbox.
Btw, is there anyone working on this? If not, is the "helpwanted" keyword
appropriate?
Comment 7•22 years ago
|
||
*** Bug 208481 has been marked as a duplicate of this bug. ***
I would love this too (The Bat! already has this).
If this is turned on by the user, there shouldn't be a [sound|popup]
notification either.
It's rather annoying to get a "you got mail" sound, only to find that it was all
spam.
Comment 9•22 years ago
|
||
This is a particular problem for IMAP mail users. The junk message is moved by
Mozilla to the Junk folder, but not purged from the INBOX nor marked as "read"
there. As a result, a mail monitor will still report "new messages" until the
INBOX is purged.
Perhaps this is a server-specific issue to a degree (reports deleted messages
flagged new when asked for new message count), but it is definitely not ideal
behaviour.
Perhaps the best solution would be to purge deleted junk mail after it has been
moved to the junk mailbox, but the option to mark it as read and 'to delete' is
almost as good.
Comment 10•22 years ago
|
||
I agree with comment 3; leaving the messages marked Unread makes it easy to know
that you should check your junk folder against false positives, before the mail
gets automatically deleted. Preventing notification on junk (bug 189289, patch
in progress) would remove most of the annoyance of new junk mail; I think
if/when that is implemented, this bug's feature will be much less important.
What I would like to see, however: mail that is *manually* marked as junk to be
marked as read at the same time. There's a much lower chance of that mail being
a false positive, and even if it were marked accidentally, the chance of losing
the mail permanently is even less.
Regarding Craig Ringer's comment 9: there are numerous bugs about IMAP and junk
(e.g. bug 193199, bug 193229, bug 208950, bug 212176) and about IMAP and
deletion; perhaps the complaint about messages being left in the inbox is better
addressed by one of those. See also bug 210607.
Comment 11•21 years ago
|
||
I also would like to see this, as an option. I don't agree that bug 189289
makes this option less desirable. In fact, the bug 189289 feature together
with the option to "mark junk mail as read" would really rock.
I look through my junk folder once or twice a week, I don't need to be
distracted everytime there's a new message there. See what I mean?
Note, this doesn't have to be the default behaviour, but it would make my life
happier if the possibility was there.
Comment 12•21 years ago
|
||
Currently, I have to open my junk mail folder every time I recieve junk, to mark
it as read, and for some ungodly reason, either as part of Courier IMAP, or
GkrellM's checking of it for new emails, any junk email gets my mail alarm stuck
on 'new mail'.
Admitidly it would be semi-useful to know that new junk mail had arrived, and
might need checking at some point, but leaving it marked as unread leaves many
other applications alerted and trying to poke the user to answer there email.
It would be very very very nice to have the option to mark it as read, if only
so I don't have to do it manually.
Comment 13•21 years ago
|
||
I agree. It would be nice to have this option, even if only in the prefs
(without UI) for the beginning.
However, I think that junk should be marked as read only when it is being moved
to the Junk folder or deleted. Incoming junk which is left in Inbox should not
be marked as read automatically. So in order to satisfy everyone we probably end
up with two options:
- mark incoming junk as read
- mark junk as read when moved or deleted
Comment 14•21 years ago
|
||
*** Bug 232274 has been marked as a duplicate of this bug. ***
Comment 15•21 years ago
|
||
This is the first patch I've ever written for Mozilla, and the first time I
have actually dabbled in the source code, so caveat emptor.
This adds a preference called 'markAsReadOnSpam' as part of the spam settings
(similiar to 'moveOnSpam', for example), and the UI for setting it to the junk
mail controls dialog. The setting is used on 2 occassions: when a message is
classified by the filter to be spam, if 'markAsReadOnSpam' is set, the message
is marked as read; when a message is manually marked as spam, if
'markReadAsSpam' is set, the message is marked as read. I have modified the
code for the first occassion in both C++ and Javascript; as for the second
occassion, I have not found a function which is run for manual marking of spam
(maybe there isn't such a function), but have added an advisory comment to the
function 'JunkSelectedMessages' in
mozilla/mailnews/base/content/mailCommands.js
I have tested the UI and the preference setting/unsetting, and the proper
functioning of C++ code (i.e. got some junk mail and the automatic
classification marked it as read when 'markAsReadOnSpam' was set, then took an
unread non-spam message, manually flagged it as spam without reading it, and it
was indeed marked read), but not the Javascript code since although I'm
generally familiar with the Javascript syntax I have near-to-zero experience
with writing JS scripts, for mozilla or otherwise.
As per what exactly this patches against, I think it patches against the trunk
(well, I used cvs diff -u etc etc blah blah , and what I had checked out seemed
to be the trunk, at least in the sense that I didn't specify and branch),
although I have also successfully applied it against the 1.6 source tarball
(with the exception of mozilla/mailnews/mailnews.js which for some reason seems
to be missing from my 1.6 source tarball - but this doesn't matter since this
pertains only to the default pref value generation).
Comment 16•21 years ago
|
||
You know, come to think of it, the marking-as-read, which my patch solves, is
only half the problem. I still feel unsatisfied... and I'll tell you why: on my
system, there's a filter which moves messages from the e-mail account inbox to
the local folders inbox. When spam arrives, the spam filter is not triggered;
rather, all messages not filtered into other folders are moved into the local
folders' inbox, including many messages that are spam, and only when I click my
local folders' inbox is the spam filter triggered; it then marks the spam as
spam, and now with my patch marks it also as read. But I still face an inbox
with an invalid 'unread messages' number until I click it, as well as the
'messages received' taskbar notification. Perhaps I should file another bug
asking that spam filtering be done on messages on arrival rather than on the
first click on the folder.
Comment 17•21 years ago
|
||
> Perhaps I should file another bug asking that spam filtering be done on
> messages on arrival rather than on the first click on the folder.
There already is one: bug 200788. Fixing it is going to require a fairly
extensive rewriting of the current junk controls; see also bug 223591,
bug 202605.
Also note that there is an RFE (bug 198961) that runs somewhat in the opposite
direction, requesting that once a message is filtered, it be exempted from junk
filtering, and another (bug 196964) to individually exempt folders from junk
filtering.
Comment 18•21 years ago
|
||
Well, if anyone decides to move the junk filtering code to some other point in
the life of an incoming message, if my patch is applied then they'll simply move
the code with my own bit of code which also marks it as read (if necessary), and
it'll work that way too.
Comment 19•21 years ago
|
||
Maybe I'm spamming this bug a little, but does anyone have any idea how I can
get someone to review my patch? Nobody seems to be responding...
Comment 20•21 years ago
|
||
Eyal, to ask for review/superreview, edit the patch and set the flags. You can
find more info here: http://www.mozilla.org/hacking/reviewers.html
And if that doesn't help, the best place to ask would be #mozilla -
irc://moznet/%23mozilla
Prog.
Comment 21•21 years ago
|
||
Comment on attachment 140414 [details] [diff] [review]
proposed patch adding this functionality
personally i'd much rather be able to do it as in comment 6.
is there anything preventing you from implementing it that way?
Attachment #140414 -
Flags: review?(bienvenu)
Comment 22•21 years ago
|
||
Comment 6's basic idea is:
> It would be interesting also to have a new condition under message filters:
> "Status - is - Junk".
That's bug 196036. It is not currently feasible because junk status is not
assigned to a message until JMC executes -- which happens after filter
execution: bug 223591.
Comment 23•21 years ago
|
||
(In reply to comment #21)
> personally i'd much rather be able to do it as in comment 6.
> is there anything preventing you from implementing it that way?
Yes, the fact that implementing this is a rather large-scale change of the
mailnews backend, which, being a novice mozilla fixer-upper, I am not yet able
to make. I of course agree that it would be a better solution, which would also
solve the additional problem I described, as well as bug 196036 and others. Note
that it would also render the 'moveOnSpam' option meaningless, since that too
could be implemented as just another filter. But until this happens, my patch is
more moderate fix.
Comment 24•21 years ago
|
||
Comment on attachment 140414 [details] [diff] [review]
proposed patch adding this functionality
this looks pretty good, except that it doesn't handle the imap case for new
mail marked as junk.
also, we don't use this as a method argument:
nsCOMPtr <nsISpamSettings> spamSettings
you just want to pass in an nsISpamSettings * here
Attachment #140414 -
Flags: review?(bienvenu) → review-
Comment 25•21 years ago
|
||
I haven't figured out whether I can actually overwrite a patch or not. So
here's a second version of the patch. This removes a redundant marking-as-read
I had and adds support for IMAP folders. I tested it with an IMAP folder, but
only the mark-spam-as-read-when-manual-marking-as-spam functionality, not the
mark-spam-as-read-when-bayesian-filters-marks-as-spam functionality. I have
also not tested the Javascript code at all, and I have no idea whether it works
(especially now that I'm doing more well-meaning-but-maybe-misguided trickery
in there). Finally, I am not very well versed in the memory management
instrumentation, so that may need to be looked over, and there's the issue of
Biff, which I don't think my patch should address but it's not impossible that
there's some linkage which you are welcome to make me aware of (actually I'd
rather you didn't since that way I'll keep the belief that my work is done).
Updated•21 years ago
|
Attachment #140414 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #140825 -
Flags: review?
Updated•21 years ago
|
Attachment #140825 -
Flags: review?
Updated•21 years ago
|
Attachment #140826 -
Flags: review?
Comment 27•21 years ago
|
||
Comment on attachment 140826 [details] [diff] [review]
proposed patch v2
Setting specific reviewer.
Prog.
Attachment #140826 -
Flags: review? → review?(bienvenu)
Comment 28•21 years ago
|
||
I have successfully tested my v2 patch with IMAP folders, for both types of
functionality (i.e. now also
mark-spam-as-read-when-the-bayesian-filter-marks-as-spam). The problem (not my
problem) is that for servers with high delay it takes quite a long time before
such changes become apparent.
Comment 29•21 years ago
|
||
Can you expand (in the bug) on this comment? What happens? It worries me a little.
+ // (for some reason the folder doesn't like the msgHdr
+ // we got from the URI)
You can remove this comment:
//this next line may not work for IMAP:
//aMsgHdr.markRead(true);
// so, instead, let's try this:
In these two places, I'd remove the NS_ENSURE_SUCCESS(rv, rv), because that
aborts the whole process, needlessly, by returning out of the function.
+ messageArray->AppendElement(messageISupports);
+ rv = srcFolder->MarkMessagesRead(messageArray, true);
+ NS_ENSURE_SUCCESS(rv,rv);
+ rv = mDatabase->MarkRead(msgKey, true, this);
+ NS_ENSURE_SUCCESS(rv, rv);
+ }
Re biff, since we ignore junk mail for the purposes of biff, I don't think your
patch should cause problems.
Comment 30•21 years ago
|
||
That's funny... I was sure the msgHdr had been causing the MarkMessagesRead to
fail; I guess it must have been something else I had been doing wrong. Anyway,
it seems that the way the code is now, the marking as read works just fine with
the msgHdr obtained from the URI, in both local and IMAP folders. So I removed
the extra header-retrieving code. Also replaced 2 NS_ENSURE_SUCCESS 's with if
!NS_SUCCEEDED NS_WARNING("etc") 's, removed a useless comment, and fixed a bug
where if you had manual marking disabled, the markAsReadOnSpam code would not
be reached.
Updated•21 years ago
|
Attachment #140826 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #140926 -
Flags: review?(bienvenu)
Updated•21 years ago
|
Attachment #140826 -
Flags: review?(bienvenu)
Comment 31•21 years ago
|
||
Comment on attachment 140926 [details] [diff] [review]
proposed patch v3
> + if (!NS_SUCCEEDED(rv))
don't do !NS_SUCCEEDED
use NS_FAILED. similarly, don't do !NS_FAILED, use NS_SUCCEEDED.
Attachment #140926 -
Flags: review?(bienvenu) → review-
Comment 32•21 years ago
|
||
timeless: done, but someone should say this in some FAQ, instead of having
novice patchmakers re-submit due to such trivialities.
I'll mark as ? without requestee.
Attachment #140926 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #140969 -
Flags: review?
Comment 33•21 years ago
|
||
Comment on attachment 140969 [details] [diff] [review]
proposed patch v4
oh, there's nothing wrong w/ asking bienvenu for the r+. I can only give r- :)
Attachment #140969 -
Flags: review? → review?(bienvenu)
Comment 34•21 years ago
|
||
I agree with comment 3 that junk should be automaticly mark as read incase the
wrong messages get marked as junk.
However what about marking all junk messages as read on exit?
Because of the date not all messages get displayed in the right order therefor
making it hard to distinguish which mails newly arrived and got marked as junk.
If all junk messages get marked as read when you exit Mozilla mail you know
which new messages got marked as junk the next time your receive new email.
Comment 35•21 years ago
|
||
comment 34 is a request for the feature of doing something at exit with messages
received in this session. That's much more general in scope than this bug.
Comment 36•21 years ago
|
||
Comment on attachment 140969 [details] [diff] [review]
proposed patch v4
If you want this to perform well on imap, you'll need to save off the messages
to mark as read, and marking them all read at once when done, instead of
marking them read one by one. So both mailCommands.js's saveJunkMsgForAction
and nsMsgDBView::SaveJunkMsgForAction should be extended to build up an array
of message(s) to be marked read, like it does for messages to be marked junk,
and do it at the end.
Comment 37•21 years ago
|
||
(In reply to comment #34)
Well, that seems like another feature to me. The reason I want the "mark junk
mail as read" option is because I don't _want_ to know when new messages get
marked as junk. Mozilla now gets it right over 99% of the time and I want the
spam to silently enter the junk folder. About once a week I browse through the
junk folder (no false positives so far) and empty it.
Comment 38•21 years ago
|
||
Messeges manually marked as junk are now also marked read en-masse rather than
one-by-one.
Updated•21 years ago
|
Attachment #140969 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #140969 -
Flags: review?(bienvenu)
Updated•21 years ago
|
Attachment #141410 -
Flags: review?(bienvenu)
Comment 39•21 years ago
|
||
Comment on attachment 141410 [details] [diff] [review]
proposed patch v5
looks good - thx for doing this. And thx for changing the name of that method
to Actions :-)
Attachment #141410 -
Flags: review?(bienvenu) → review+
Updated•21 years ago
|
Attachment #141410 -
Flags: superreview?
Comment 40•21 years ago
|
||
Although I've asked for superreview, I still would like someone with Javascript
experience to help me test the JS changes I've made, before this patch is
considered for checkin. Any volunteers - please e-mail me.
Comment 41•21 years ago
|
||
Comment on attachment 141410 [details] [diff] [review]
proposed patch v5
I'll switch my review to a sr and ask Neil for a review.
Attachment #141410 -
Flags: superreview?
Attachment #141410 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #141410 -
Flags: review+
Comment 42•21 years ago
|
||
I don't understand why
a) the JS version marks each message read individually
b) the C++ version keeps track of two arrays, one for messages that are to be
moved, and one for messages that are to be marked as read... surely you only
need one array, and then decided afterwards whether to mark them as read and/or
move them?
Comment 43•21 years ago
|
||
In particular, I don't understand why (except one set is empty) the sets of
messages could be different.
Comment 44•21 years ago
|
||
if you're not moving junk mail to the junk folder, one will be empty - so I
guess the code at the end needs to check what the action is as well...
Comment 45•21 years ago
|
||
Neil: It's true that currently the sets are either the same or one of them is
empty. But when I wrote the patch I was assuming (falsely, for now) that we
might be handling messages not all from the same folder. Otherwise the entire
SaveJunkMsgForAction/s is useless, because either you save all of the messages
or none of them. The code sort of lends itself to my assumption... do you want
OnMessageClassified rewritten so as to be exactly suited for only handling
bunches of messages all from the same folder?
Comment 46•21 years ago
|
||
(In reply to comment #45)
>when I wrote the patch I was assuming (falsely, for now) that we might be
>handling messages not all from the same folder.
It doesn't matter. Just collect up all the messages, then at the end, decide
whether to ApplyCommandToIndices( nsMsgViewCommandType::markMessagesRead,
indices, numIndices ); and/or the existing move/delete code as appropriate.
Comment 47•21 years ago
|
||
Just in case anyone was wondering, the fix for bug 181631 seems to have no
impact on this bug and on the proposed fix. The fix is also invalid, as I have
taken Neil's criticism into account and have sent him a new proposed patch for
the nsMsgDBView part of the fix, and am awaiting his reply.
Updated•21 years ago
|
Attachment #141410 -
Attachment is obsolete: true
Attachment #141410 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 48•21 years ago
|
||
So as not to keep 'the public' out of the loop, here is a new patch with...
shall we say... better quality changes in nsMsgDBView.* and mailCommands.js
than the previous ones, and which is basically what I expect to be approved,
and is also tested to a reasonable extent, but may not be what is eventually
approved, so you might see a v7.
Comment 49•21 years ago
|
||
*** Bug 235523 has been marked as a duplicate of this bug. ***
Comment 50•21 years ago
|
||
Attachment #145213 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #145458 -
Flags: superreview?(bienvenu)
Attachment #145458 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 51•21 years ago
|
||
Comment on attachment 145458 [details] [diff] [review]
(hopefully) final patch
This one has the same filename as the one I was sent that passed testing ;-)
Attachment #145458 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Comment 52•21 years ago
|
||
can you do a -uw diff, to remove the changes that are just whitespace. It will
make it easier for me to review. When it comes time to checkin, we can apply the
full diff with the whitespace changes.
please remove the dump statements like:
+ dump('markRead = ' + actionParams.markRead + ' ; junkTargetFolder = ' +
actionParams + '\n');
and the commented out dump statements...
and this commented out code. We have cvs history to recover stuff like this.
+// this will need re-writing if it is to be used
+//function analyzeSelectedMessagesForJunk()
+//{
+// var messages = GetSelectedMessages();
+// analyzeMessagesForJunk(messages);
+//}
thx, - David
Comment 53•21 years ago
|
||
Removed dump()'s and commented-out function, as per David's request.
Comment 54•21 years ago
|
||
Attachment #145458 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #145509 -
Flags: superreview?(bienvenu)
Attachment #145509 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 55•21 years ago
|
||
Comment on attachment 145509 [details] [diff] [review]
proposed patch v9
[You don't need rereview for comment/dump removal]
Attachment #145509 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Updated•21 years ago
|
Attachment #145509 -
Flags: superreview?(bienvenu) → superreview+
Updated•21 years ago
|
Attachment #145458 -
Flags: superreview?(bienvenu)
Comment 56•21 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 57•21 years ago
|
||
this checkin had some bad line endings.
see my follow up checkin to nsMsgDBView.cpp (1.176)
It's not a big deal in .h / .cpp files, but I think it can cause problems in
.xul files.
Comment 58•21 years ago
|
||
mscott: you have that backwards, it's the .cpp/.h files where line endings matter.
.xml files don't care about whitespace in general, and as long as there's a line
ending in a js file, js is happy.
Comment 59•21 years ago
|
||
Sorry about that, I didn't check the patch line endings before checking in, and
when I did check in, and promptly conflicted with my other tree, I lost my build
machine for 12 hours and forgot what the conflict was when it came back up.
Comment 60•21 years ago
|
||
I think this patch caused a pretty nasty bug with junk mail controls where stop
writing out training changes because it introduces an unmatched call to
junkPlugin->Batch without a matching EndBatch call. See Bug #243680
Comment 61•21 years ago
|
||
this is not a patch, just a diff between two files until I get my codebase
up-to-date, rebuild, and am able to actually cvs diff something meaningful. The
change should fix the batch start/end mismatch which is implicated in bug
243680.
Comment 62•21 years ago
|
||
I manually entered your changes and made a diff against current trunk. After
verifying my changes are correct, you can probably mark your one as obsolete,
I'll not do it just yet.
Rebuilding.
Comment 63•21 years ago
|
||
Andreas, I appreciate the effort, but:
- I haven't listed the requisite change of nsMsgDBView.h
- your diff against the CVS has changes to nsMsgIncomingServer.cpp which are
irrelevant...
Comment 64•21 years ago
|
||
oops, the last one included a patch for a different bug...
Attachment #148775 -
Attachment is obsolete: true
Comment 65•21 years ago
|
||
Ok, sorry for the confusion.
I just rebuilt your proposal and it seems to work fine, write access to
training.dat does happen.
However, now that I'm already spamming all of you with this comment I figured I
could also add the current patch including the nsMsgDBView.h changes without
causing additional annoyance...
(Not sure if you want to add more comments or kill some of the commented-out
code, though)
Updated•21 years ago
|
Attachment #148776 -
Attachment is obsolete: true
Comment 66•21 years ago
|
||
Proposed patch with removal of redundancy in msMsgDBView.h, another short
comment in .h, and removed useless comment in .cpp .
Updated•21 years ago
|
Attachment #145509 -
Attachment is obsolete: true
Attachment #145510 -
Attachment is obsolete: true
Attachment #148774 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #148779 -
Flags: superreview?(bienvenu)
Attachment #148779 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 67•21 years ago
|
||
If someone decides to check my fix in, please replace some tabs at the line:
if (mNumMessagesInBatch == 0)
and the following line.
Thanks Andreas.
Updated•21 years ago
|
Attachment #148778 -
Attachment is obsolete: true
Comment 68•21 years ago
|
||
Comment on attachment 148779 [details] [diff] [review]
proposed fix for batch start/end bug
I don't like the way you reset mCurrentIndexInBatch at a different time to
mNumMessagesInBatch. In fact I think using a single variable
mNumMessagesRemainingInBatch would have made the logic clearer. Also I'm not
sure you're allowed to put an NS_ASSERTION as the only statement of an if.
Updated•21 years ago
|
Attachment #148779 -
Flags: superreview?(bienvenu)
Attachment #148779 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 69•21 years ago
|
||
Merged mNumMessagesInBatch and mCurrentIndexInBatch into a single
mNumMessagesLeftInBatch integer. Removed tabs. Fixed typo.
Attachment #148779 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #148956 -
Flags: superreview?(bienvenu)
Attachment #148956 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•21 years ago
|
Attachment #148956 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Comment 70•21 years ago
|
||
Comment on attachment 148956 [details] [diff] [review]
fix for batch start/end bug v2
I think Seth had a reason for using the uri but I don't remember what the
scenario was...requesting sr from him
Attachment #148956 -
Flags: superreview?(bienvenu) → superreview?(sspitzer)
Assignee | ||
Comment 71•21 years ago
|
||
> I think Seth had a reason for using the uri but I don't remember what the
> scenario was...requesting sr from him
sorry for the delay. let me look over the code and see if I can remember why I
did that.
Assignee | ||
Comment 72•21 years ago
|
||
actually, it was dmose who originally wrote that code:
see
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsMsgDBView.cpp&branch=&root=/cvsroot&subdir=mozilla/mailnews/base/src&command=DIFF_FRAMESET&rev1=1.121&rev2=1.122
from what I can tell from his original checking on 10/24/02, it was to handle
coalesing of junk batches.
if that still works, then I think we are safe.
Comment 73•21 years ago
|
||
> from what I can tell from his original checking on 10/24/02, it was to handle
> coalesing of junk batches.
>
> if that still works, then I think we are safe.
Yes, it does work (I have replaced the use of the URI with the simpler use of
index counting which achieves the same result). BTW, As I mentioned in one of
the patch comments, I think junk batches should not be coalesced! The plugin
interface should be such that EndBatch is called after the last message is
dispatched for classification, not after it is classified; and the plugin should
have its own policy of when to write its data file to the HDD.
Comment 74•21 years ago
|
||
Comment on attachment 148956 [details] [diff] [review]
fix for batch start/end bug v2
I wonder if the uri was used in case the user got two or more junk
classifications going at the same time, e.g., select 10 messages, mark as junk,
and while that's going on, select 10 more and repeat. What happens with your
patch in that scenario?
Comment 75•21 years ago
|
||
Each time the user selects some messages and marks them as junk,
ApplyCommandToIndices() is called with the new batch. Its pre-patch behavior was
to replace the previous 'URI of last message to be classified' (in case the last
batch had not yet been completed) with the last URI of the current batch - thus
the batches were 'coalesced'. What we do now is achieve the same effect by
adding the number of messages in the new batch to the 'remaining number of
indices'. Each time a message is classified this number decreases by 1, and when
it hits zero (rather than when the message with the 'URI of last message to be
classified' is classified), we perform the necessary actions on the messages,
and call the plugin's EndBatch().
Comment 76•21 years ago
|
||
Comment on attachment 148956 [details] [diff] [review]
fix for batch start/end bug v2
ok, great, thx.
Attachment #148956 -
Flags: superreview?(sspitzer) → superreview+
Comment 77•21 years ago
|
||
I'll check it in later today, if Neil doesn't beat me to it.
Comment 78•21 years ago
|
||
checked in, thx, Eyal.
Comment 79•20 years ago
|
||
Bug 253234: This feature does not exist in Thunderbird.
Comment 80•20 years ago
|
||
What, are you kidding me? I thought Thunderbird and Seamonkey MailNews shared
all the backend code! I need to port this now? Damn, I don't even know how to
build Thunderbird... :-(
Comment 81•20 years ago
|
||
no, it's just the front end for setting the option that needs to be ported...
Updated•20 years ago
|
Product: MailNews → Core
Comment 82•19 years ago
|
||
> Well, that seems like another feature to me. The reason I want the "mark junk
> mail as read" option is because I don't _want_ to know when new messages get
> marked as junk. Mozilla now gets it right over 99% of the time and I want the
> spam to silently enter the junk folder. About once a week I browse through the
> junk folder (no false positives so far) and empty it.
That's exactly what I'd like to do. Come on, just make it work before final TB2.
Updated•17 years ago
|
Product: Core → MailNews Core
Comment 83•16 years ago
|
||
It seems in recent trunk builds junk gets moved to the junk folder without being marked read. The pref is also gone from the dialogs for the different accounts. Intentional? Accidental?
You need to log in
before you can comment on or make changes to this bug.
Description
•