Closed Bug 208197 Opened 21 years ago Closed 15 years ago

If user marks a message as Not Junk, move message to inbox

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 3.0b2

People

(Reporter: ts, Assigned: Bienvenu)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030603 Mozilla Firebird/0.6
Build Identifier: 

Once a message has automatically been marked as junk, and the user then clicks
"Not junk". The message should be then moved back to it's orginally intended
folder (be passed through filters)

Reproducible: Always

Steps to Reproduce:
Maybe a blocker for bug 11035 ?
Blocks: 11035

*** This bug has been marked as a duplicate of 195006 ***
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago
Resolution: --- → DUPLICATE
Reopening to separate the two parts of bug 195006.
Status: RESOLVED → UNCONFIRMED
OS: Windows 2000 → All
Hardware: PC → All
Resolution: DUPLICATE → ---
The original folder is probably not always known. How about moving it back to
the Inbox? This is how SpamBayes works I believe.
*** Bug 220760 has been marked as a duplicate of this bug. ***
I see four possibilities:
1) when marking message as junk (automatically or manually), Mozilla should
remember where the message was at that moment; then, when the user un-junks the
message, move it back where it was
2) don't remember anything, but when the user un-junks a message, run the
filters on it to see if it gets moved somewhere; if not, just move it to inbox
3) always move to inbox when user un-junks a message
4) do nothing, WONTFIX this bug

There's a problem with options 2 and 3: a junk folder can be common for many
accounts, and without somehow marking the message, I suppose Mozilla doesn't
know which account the message came from, and so it doesn't know which inbox -
or which filters - are applicable.

So that actually leaves us with options 1 and 4, unless I fail to see something
else. Confirming the bug now.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Vaclav: 2) it isn't solution also for e-mails moved between folders by user,
futhermore filters could be also modified etc.

So, I agree about 1) and 4). 
Item 3 is a viable solution. Granted, some users could share a junk folder from
multiple incoming mail areas, is that the general case that needs to be
addressed? While 3 is less complex and doesn't necessarily meet 100% situations,
it should be considered worthwhile and valid. Personally, that would meet my
interest in this enhancement and is similar to how other spam products work (for
better or worse). Lastly, moving back to one of multiple folders (as opposed to
just to the Inbox) could cause user confusion in the multiple locations a
Ununked message could have moved.
If the message is moved, should it then be marked as unread? Currently, I select
all in the Junk folder and uncheck the false alarms. I then delete these,
resulting in the one or two non-spams to be marked as read and displayed. I
suppose the mark-as-read-only-after-set-time feature recently added to TB (and
prolly Mail?) can be used here, but then I really wouldn't want that feature to
work on anything but the Junk folder.

This seems a lot of hassle, though I wouldn't mind if it's implemented properly.
Blocks: 227327
*** Bug 232012 has been marked as a duplicate of this bug. ***
*** Bug 233537 has been marked as a duplicate of this bug. ***
*** Bug 233663 has been marked as a duplicate of this bug. ***
Vaclav: 1) can be easily implemented if new "X-Mozilla-FCC:" header like current
"FCC:" header for Drafts is written on mail move.
> FCC: mailbox://username@hostname/Sent

I think there is no need to track folder renaming in this function(Messaege
filter has).
If folder is renamed or deleted, or no "X-Mozilla-FCC:" header, message of
"Moving back folder not found" is sufficient.
*** Bug 250994 has been marked as a duplicate of this bug. ***
Sorry to make a useless comment, but what's the status of this bug? Is it fixed
for Thunderbird 0.8 in CVS, or is this only for Mozilla Mail (In which case,
should a seperate bug be opened for Thunderbird?)
Adam, this bug i not marked as fixed, nor has anybody mentioned a checkin or
even attached a patch. So what makes you think this is fixed anywhere?

This bug, as it is filed, is for the Mozilla Suite (otherwise it would be listed
unter the "Thunderbird" product). I could imagine some of the fix would help
both, but there's surely a certain amount of work to be done separately for each
product (but having the fix for one would make easier porting it to the other).
Having a separate Thunderbird (with refernce to this one) would not hurt, but
I'm not sure if it is necessary...
How about utilizing new "X-Account-Key" header if POP3 account?
(I could see "X-Account-Key" header in mails on 2004/5/22 and today but could
not see it in a mail on 2004/5/17.)  
*** Bug 265575 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
I filed this bug under Thunderbird. I don't know how it got changed to the suite.
(In reply to comment #19)
> I filed this bug under Thunderbird. I don't know how it got changed to the suite.
See Bug 272179 Comment #1 for Product/Component change.
*** Bug 227327 has been marked as a duplicate of this bug. ***
It seems to me there's an order issue here: I go to a folder to read messages
that have been automatically filed, and the junk mail filter runs *then* and
moves some of them to the junk folder.  99.99999% of these are not junk, mostly
mailing list postings.  Or in the case where spamassassin tags spam and I have
it directed to a Spam folder, when I go to check for false positives, it all
gets moved to the junk folder and I have to move it all back (I keep the
spamassassin tagged spam separate from what leaks through and tagged by mozilla
just for curiosity and comparison).  I guess what I would like to see is a check
box on the filter rules that says whether or not the filter rule should override
junk mail controls, and have them all only work on INBOX.  Once a message is
filed, it stays put unless you specifically run or apply the rules to another
folder.
Assignee: sspitzer → mail
 A couple of Core bugs have be duped against this one: Bug 232012 and Bug 233663, and blocks Thunderbird Bug 271712 for reasons set out in Bug 271712 comment #1 and Bug 271712 comment #2.

 Should the Product of this bug now be changed to Core?
*** Bug 255521 has been marked as a duplicate of this bug. ***
Assignee: mail → nobody
Component: MailNews: Main Mail Window → MailNews: Backend
Product: Mozilla Application Suite → Core
QA Contact: esther → backend
*** Bug 271712 has been marked as a duplicate of this bug. ***
No longer blocks: 271712
*** Bug 355908 has been marked as a duplicate of this bug. ***
This would be very useful.
An option to move the message to the corresponding inbox would probably be sufficient for most users.  From there they can be re-filtered.
For advanced users, a filter on the junk folder could check the junk status and move the message.  I think this would need an addition to filters.
Are there any rules how to build a new x-header?

Scott and David, what is your suggestion to get this RFE implemented?
That sounds good - I would vote for the idea from comment 8 + 9.

Option 3, move to inbox - and mark them as unread.

That wouldnt be the perfect option, but a good consense between "comfortable"
and "easy to fix".

I and my colleagues would be happy if it would be just this easy solution.

Perhaps some of this things could be made as option fields in the junk options
(like "mark the sent-back to inbox false positives as unread" in better english
 of course *g*) 



(In reply to comment #9)
> If the message is moved, should it then be marked as unread? Currently, I
> select
> all in the Junk folder and uncheck the false alarms. I then delete these,
> resulting in the one or two non-spams to be marked as read and displayed. I
> suppose the mark-as-read-only-after-set-time feature recently added to TB (and
> prolly Mail?) can be used here, but then I really wouldn't want that feature to
> work on anything but the Junk folder.
> 
> This seems a lot of hassle, though I wouldn't mind if it's implemented
> properly.
Henrik, you can't add a new x-header to a message you've already downloaded (well, not easily). You could easily set a string property on the msg hdr that gets stored in the .msf file that could store the uri of the folder you want the message to get moved to on "un junk".
If there are multiple places to return a "not junk" item, then simply ask!
I don't really see the problem here.  

But currently, if the incoming item is NOT junk and TB determines it to be NOT junk, it knows which folder to send it.  So it should be simple to send the item back to the incoming stream, and determine which folder it should go to knowing it is now declared "not junk".  ie: don't junk filter it, just move it.
(In reply to comment #13)
> If folder is renamed or deleted, or no "X-Mozilla-FCC:" header, message of
> "Moving back folder not found" is sufficient.

Please don't make dead end error messages like this.  You could at least ask where the user wants it to go. 

There should be a Junk Master mode which can be activated in the configuration:
When multiple users share an imap account and this is activated, a mail which has been marked as Junk or un-junked by another client should also be moved to the appropriate folder and the junk filter should learn from it.

For details, see https://bugzilla.mozilla.org/show_bug.cgi?id=413320

Thus there would be only one Junk Master and for the other clients Junk mail control should be turned off, they (=their user) will only manually mark and unmark mails as Junk and let the moving be done by the Junk Master.
The goal is that there is only one client which is responsible for sorting out Junk and only one training data file which needs to be trained.

When someone marks a mail as junk by accident, this could be undone very easy by unjunking the mail in the Junk folder and only one client (the Junk Master) needs to learn from it and move it back.
(In reply to comment #6)
> I see four possibilities:
> 1) when marking message as junk (automatically or manually), Mozilla should
> remember where the message was at that moment; then, when the user un-junks the
> message, move it back where it was
> 2) don't remember anything, but when the user un-junks a message, run the
> filters on it to see if it gets moved somewhere; if not, just move it to inbox
> 3) always move to inbox when user un-junks a message
> 4) do nothing, WONTFIX this bug
> 
> There's a problem with options 2 and 3: a junk folder can be common for many
> accounts, and without somehow marking the message, I suppose Mozilla doesn't
> know which account the message came from, and so it doesn't know which inbox -
> or which filters - are applicable.
> 
> So that actually leaves us with options 1 and 4, unless I fail to see something
> else. Confirming the bug now.

I cast my vote for option 3 
3 as well
3 from me too, but perhaps with a enhancement:

Give the "recognized as junk but later un-junked" mails a tag with this thunderbird tag feature - I do not know how it is called "Schlagwörter" in the german localisation.
Product: Core → MailNews Core
Flags: wanted-thunderbird3?
Attached patch Proposed fix v1 (obsolete) — Splinter Review
Always unjunks messages to the Inbox folder.

Notes:
- I've used GetInboxFolder(defaultServer) temporary because I had problems getting inbox folder of the current account, can you please advice ?
- message is being unjunked to the inbox even if it is in the inbox, is it worth to the if-then check in this case ?
- enhancements / unjunking intelligence can be fixed in follow-up bugs
Assignee: nobody → firefox
Status: NEW → ASSIGNED
Attachment #345537 - Flags: review?(bugzilla)
(In reply to comment #42)
> Created an attachment (id=345537) [details]
> Proposed fix v1
> Always unjunks messages to the Inbox folder.

If it unjunks the message to the inbox folder, will it then be filtered into the correct folder?
(In reply to comment #43)
> If it unjunks the message to the inbox folder, will it then be filtered into
> the correct folder?
Currently this is a pure move to the inbox without extras but as said it can be improved later on
(In reply to comment #42)
> - I've used GetInboxFolder(defaultServer) temporary because I had problems
> getting inbox folder of the current account, can you please advice ?

Find the msgHdr, and then GetInboxFolder(msgHdr.folder.server) perhaps?

> - message is being unjunked to the inbox even if it is in the inbox, is it
> worth to the if-then check in this case ?

Since you'd be getting the inbox folder anyway, might as well check that you're not alreday there...
(In reply to comment #45)
> Find the msgHdr, and then GetInboxFolder(msgHdr.folder.server) perhaps?
thanks! , patch coming (if you want feel free to do the review but junkCommands.js is under mailnews so second review of mailnews peer would be needed anyway)
Attached patch Proposed fix v2 (obsolete) — Splinter Review
Changes:
- moves to the proper inbox
- [FIXED] message is being unjunked to the inbox even if it is in the inbox
Attachment #345537 - Attachment is obsolete: true
Attachment #345803 - Flags: review?(bugzilla)
Attachment #345537 - Flags: review?(bugzilla)
(In reply to comment #43)
> If it unjunks the message to the inbox folder, will it then be filtered into
> the correct folder?
Is your use case as follows ? :
New message is catched by the message filter AND by the junk filter, junk filter wins in such cases and the message is moved to the junk folder and not the folder you did specify in the message filters dialog.
Does it happen often ?
Comment on attachment 345803 [details] [diff] [review]
Proposed fix v2

I'm going to pass this over to David, as I think he knows this area of code better than I do, and I've got a lot of big patches in my queue at the moment.

Also, adding Bryan our user experience guy onto the review list to check the behaviour is acceptable (which I think it probably is, just checking).
Attachment #345803 - Flags: ui-review?(clarkbw)
Attachment #345803 - Flags: review?(bugzilla)
Attachment #345803 - Flags: review?(bienvenu)
Comment on attachment 345803 [details] [diff] [review]
Proposed fix v2

thx for working on this. I've always wanted this feature. There are a few problems, though.

With your patch, we won't reload messages anymore when the junk status changes, even if we're not moving the message back. And I don't think your logic is quite right - we would only want to move the message back if (!isJunk && moveJunkMail) - what you've done, I think, is if (!isJunk || !moveJunkMail)

Also,I don't think you're clearing the junk flag anymore, in the case of marking a message as non-junk; you're just moving it back to the inbox.

I'm not clear on why you're doing what you're doing in the HandleJunkStatusChanged part either - the command handler should do the move as well as clearing the junk status, not the code that's called when the command has finished.

Finally, there are cases where this isn't quite the right thing to do. The message might be in a folder other than the inbox or the junk folder, e.g., moved there by a mail filter or the user. Or the user might be using a single junk folder for multiple accounts, in which case you don't know which inbox to move the message to. At the very least, you should be checking that the message is in a junk folder, not that it's not in the inbox...
Attachment #345803 - Flags: review?(bienvenu) → review-
> thx for working on this. I've always wanted this feature.
me too, good to hear that it is also wanted by the TB team
 
> With your patch, we won't reload messages anymore when the junk status changes,
> even if we're not moving the message back. And I don't think your logic is
> quite right - we would only want to move the message back if (!isJunk &&
> moveJunkMail) - what you've done, I think, is if (!isJunk || !moveJunkMail)
I need to add second "if" instead of only modifying the existing one...

> Also,I don't think you're clearing the junk flag anymore, in the case of
> marking a message as non-junk; you're just moving it back to the inbox.
+    gDBView.doCommand(nsMsgViewCommandType.unjunk);
seems to do fine

> I'm not clear on why you're doing what you're doing in the
> HandleJunkStatusChanged part either - the command handler should do the move as
> well as clearing the junk status, not the code that's called when the command
> has finished.
if you have on mind not using JunkSelectedMessages(false); then I would have to duplicate code added to the JunkSelectedMessages function or I'm missing sth ?
it was added so clicking on the junk status column would unjunk using new code - not only refresh
> // we used to only reload the message if we were toggling the message to NOT
JUNK from junk 

> Finally, there are cases where this isn't quite the right thing to do. The
> message might be in a folder other than the inbox or the junk folder, e.g.,
> moved there by a mail filter or the user. Or the user might be using a single
> junk folder for multiple accounts, in which case you don't know which inbox to
> move the message to. At the very least, you should be checking that the message
> is in a junk folder, not that it's not in the inbox...
yes, I've thought about checking for junk folder after attaching the patch
It's hard to cover all cases but I think we can assume (at least in this bug) that if the junk message is not in the junk folder then it has been either moved there manually or by message filter -> user knows it's the proper place and definitely not inbox

thank you for the insightful comments
This behavior sounds on track to me, as outlined in David's Comment #50.  I'm wondering if there exists (or exists a way to add) some meta-data about what folder a junk mail came from.  Otherwise I feel like our option becomes asking the user where to put the non-junk mail back to; not something I like.
Right, that's exactly what we would have to do, and we do have the technology to do it, though I think I would only handle the single machine case, not the general imap case...i.e., I would annotate locally.
It may sound biased but I think that not dealing with the IMAP case would quickly become a pain, especially since it is gaining wider adoption. If you think that it won't make in time for the tb3 timeframe but will be considered for the next minor/major release, then it's a pity but that's OK. OTH if you're going to scrap it completely then it looks to me some kind of lack of foresight (that is, considering the users' growing attitude to connect from a moltitude of different devices - be them mobile or not).
I think local only is going to be fine.  This would only effect the case where Thunderbird is being used from multiple machines.  I don't think mobile or other applications would be effected as they would need to understand the "came-from" meta-data applied to the message and offer the option to restore it.

Perhaps (and I love to keep insisting we'll have this) when we finalize weave integration this meta-data will be kept with weave such that multiple instances of Thunderbird can make use of it.
Attached patch Proposed fix v3 (obsolete) — Splinter Review
Changes:
- unjunkes to the inbox only from the junk folder
- restored changes made to the HandleJunkStatusChanged

Is there a need (and in this bug) to unjunk messages to the inbox by clicking on the junkcol icon ? (currently it only changes it junk status)
if yes then please give some hints because I had problems doing it correctly and the way it would also apply to the not currently selected messages.

Regarding moving back to the folder from which junk mail came from:
I think current patch covers most of the users (bug voters, cc users ?).
Using current available functions I can't get info from which folder junk mail came from, right ?
If yes, then I think it should be fixed in other / follow-up bug.
Attachment #345803 - Attachment is obsolete: true
Attachment #347103 - Flags: ui-review?(clarkbw)
Attachment #347103 - Flags: review?(bienvenu)
Attachment #345803 - Flags: ui-review?(clarkbw)
(In reply to comment #56)
> Is there a need (and in this bug) to unjunk messages to the inbox by clicking
> on the junkcol icon ? (currently it only changes it junk status)
> if yes then please give some hints because I had problems doing it correctly
> and the way it would also apply to the not currently selected messages.
probably in nsMsgDBView.cpp:
>  case 'j': // junkStatus column 
would have to be updated in a similar way JunkSelectedMessages(setAsJunk) was but unfortunately I don't have build environment ready ATM to test the changes and Bug 431375 is still not fixed :(
Thanks Przemyslawv for your work here. Just a small hint. Please keep the Mozilla coding style: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
Comment on attachment 347103 [details] [diff] [review]
Proposed fix v3

I'll let Bryan chime in about whether we should do this without knowing the real original folder.
How do I know where the message went? One annoying case (workaroundable, yes, but working around it implies you'd have already learned what's going to happen):
 1. scan through junk, land on a somewhat aged false positive
 2. decide not to read it now, just unjunk
 -> where'd it go? If the inbox is not of the empty kind, the message would tend to be somewhere not with the very recent messages, and marked read. OK, so we'll have better search, but...

IMO the unjunked message should appear unread (and probably new), iow despite not taking te "re-deliver" approach, follow it in the msg status respect, so that the message is findable and there's some (if a bit subtle) immediate feedback on where it appears.
(In reply to comment #60)
> IMO the unjunked message should appear unread (and probably new), iow despite
> not taking te "re-deliver" approach, follow it in the msg status respect, so
> that the message is findable and there's some (if a bit subtle) immediate
> feedback on where it appears.
if there is a need to mark message unread when unjunking then it should be a hidden pref (if you decide sth is not junk you already read it at least partially, right ?)

it is not yet decided if unjunking to inbox would be approved (if yes then probably it will be mentioned in the relnotes or sth), though I myself would like to split the enhancements into follow-up bugs especially because I'm not sure I would have currently the skills to take care of all of them

I can add that I've built successfully shredder from the sources so I can now verify / work on complete version of previous patch (nsMsgDBView.cpp changes).
Hmm, this seems more like a surface level issue to me.

I feel if we use a button like ( not junk ) then we cannot push a message back to the inbox.  However if, at least for now, we changed that button to be something like ( restore to inbox ) then I'd feel more comfortable with the change.  

The ( not junk ) button only indicates that it's going to change the status of the message, not move it.  So if we were to move a messages using 'not junk' we would really have to return it to it's original location.  While a button like ( restore to inbox ), and that text isn't perfect, at least lets a person know exactly what is going to happen with the message.

Also I particularly like the suggestion of the unread status change when moving a message back.  I believe the intention is to read the message after it has been restored from the junk folder.
This bug has been danced around for 4 1/2 years already.  

You guys are overthinking a simple problem.  

A year ago I said "if the incoming item is NOT junk and TB determines it to be NOT junk, it knows which folder to send it.  So it should be simple to send the
item back to the incoming stream, and determine which folder it should go to
knowing it is now declared "not junk".  ie: don't junk filter it, just move it."

The original #1 post on this subject said pretty much the same thing.

Let me clarify the solution further.
You get an email that TB declares is junk, be it heuristically or by junk filter criteria, and that is moved into the Junk Folder.  But TB must annotate the email of what function that declared it as junk.
The user reviews that email and clicks the "NOT JUNK" button.
TB removes the "junk" classification and places the email back into the incoming mail stream, with one exception, the function that originally declared it junk is BYPASSED this time only.
The email either is junked by some subsequent filter or, most likely, is sent to whatever folder the non-junk email would normally goto, usually the inbox.
TB can mark it as unread or whatever, it really doesn't matter that much.  The user has read it and is aware of it.  If the destination is some other user account sharing the same incoming mail stream, then, yeah, mark it as "unread".

End of story.

If the user is made aware of what filter originally junked it, then the user can modify the filter so as to not junk it in the future.

To the TB developers; Come on, you guys have solved far more complex problems than this one.  Four and a half years????  GOYA.

jgb
Bryan: what do you think about this:
- pressing "not junk" would mark message as not junk and would move it to the inbox folder (if unjunking from the junk folder) and it would mark it as unread (I will add a hidden pref to not mark as unread).

I'm seeing this bug as enhancement of marking message as "not junk" - it does the same but a little more (it's a new feature but mostly expected) - message was falsely classified so it is moved back to the inbox (we don't know from where it come from but if the mail was message filtered then it is small chance that it was marked as junk and user later unjunks it) - I would like to move it to it's original location but I don't know how to do it / if we have such knowledge.
As this is enhancement of unjunking function it should be listed in what's new for TB 3.0
IOW I don't think it is a good idea to change the name to "restore to inbox" - restore to inbox might not mean to the user marking as not junk ?
also a few buttons and menus would need to have their name changed.
So do you still want to change the name while fixing this bug ?

[edit] info regarding changes can be put also in the junkMailInfo.dtd (for first time users of junk filtering)
OK, let's not waste time here, please report Bryan if you would change your mind regarding my previous comment.
I prefer better / fully approved patches so in the meantime...
based on your comment:
> So if we were to move a messages using 'not junk' we
> would really have to return it to it's original location.
I will file bug for:
> I'm wondering if there exists (or exists a way to add) some meta-data about 
> what folder a junk mail came from.

> Right, that's exactly what we would have to do, and we do have the technology
> to do it

and add dependency, when it would be fixed I will go back to this bug.
Attachment #347103 - Flags: ui-review?(clarkbw)
Attachment #347103 - Flags: review?(bienvenu)
Depends on: 465794
I think with marking the message unread and moving it back to the Inbox we're making good progress, lets get that in and worry about better solutions later.
Changing summary since this bug was hijacked from its original purpose of moving the message to its intended folder.  So now we have to file another bug for what this bug, comments and dupes were originaly intended for.  Why not just do it right the first time?
Summary: If user marks a message as Not Junk, move message back to intended folder → If user marks a message as Not Junk, move message to inbox
Because bug 465794 is blocking it and it was decided that moving to inbox is better than waiting nobody knows how long for mentioned bug to be fixed.
Initial patch (v4) might be checked in and bug be left open for the second patch (for example) - hijacking was not planned.
Blocks: 466041
Filed bug 466041, for moving the message to its intended folder (not the inbox)
Attached patch Proposed fix v4 (obsolete) — Splinter Review
Changes since last patch:
- adds hidden pref mailnews.junk.MarkAsNotJunkMarksUnRead (defaults to true), if true message goes to inbox unread, if false read status is not changed
Attachment #347103 - Attachment is obsolete: true
Attachment #349399 - Flags: ui-review?(clarkbw)
Comment on attachment 349399 [details] [diff] [review]
Proposed fix v4

I'm not really a fan of hidden prefs in general but lets see how this goes.
Attachment #349399 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 349399 [details] [diff] [review]
Proposed fix v4

thx for working on this.

I think you should mark the message unread first, before moving the message.
Also, the gDBView.command(nsMsgViewCommandType.junk) should be on its own line.

also, might be a bit cleaner to do this:

let folder = gDBView.hdrForFirstSelectedMessage.folder;

then use folder and folder.server below...

and you should add mailnews.junk.MarkAsNotJunkMarksUnRead to mailnews.js, or this code will throw an exception
Attachment #349399 - Flags: review-
While you're at it, can we please make the pref name lower_case or camelCase like most other prefs. (And I don't know why the ".junk" part is there.)
(In reply to comment #71)
> I'm not really a fan of hidden prefs in general but lets see how this goes.
I know that TB limits prefs configurable by UI and my believe is to have at least a hidden pref than no pref at all (I think such is wanted here)
if you had on idea to add also UI for this than I can add it (here ? or in follow-up bug)

(In reply to comment #72)
> I think you should mark the message unread first, before moving the message.
I did it to not show message being marked as unread for a moment and disappearing but I can change that - users will knew that msg will me marked as unread though they would in the end anyway

> also, might be a bit cleaner to do this:
> let folder = gDBView.hdrForFirstSelectedMessage.folder;
might be, ok

> and you should add mailnews.junk.MarkAsNotJunkMarksUnRead to mailnews.js, or
> this code will throw an exception
I thought I did ;)

(In reply to comment #73)
> While you're at it, can we please make the pref name lower_case or camelCase
> like most other prefs. 
> (And I don't know why the ".junk" part is there.)
mailnews.markAsNotJunkMarksUnRead ?
> > and you should add mailnews.junk.MarkAsNotJunkMarksUnRead to mailnews.js, or
> > this code will throw an exception
> I thought I did ;)
Oh yes, so you did, sorry about that.

I just worry if you move a message before you mark it unread, it may not be unread in the target folder.
Attached patch Fix v5 (obsolete) — Splinter Review
updated to comments
Attachment #349399 - Attachment is obsolete: true
Attachment #349641 - Flags: review?(bienvenu)
(In reply to comment #74)
> > (And I don't know why the ".junk" part is there.)
> mailnews.markAsNotJunkMarksUnRead ?

The other spam related settings hare named mail.spam.xxx (sorry for being unclear!)
Comment on attachment 349641 [details] [diff] [review]
Fix v5

>+  if (setAsJunk)
>+    gDBView.doCommand(nsMsgViewCommandType.junk)

The style rule is: if one of the if/else use braces, all should.

>+  else
>+  {
>+    gDBView.doCommand(nsMsgViewCommandType.unjunk);
>+    let folder = gDBView.hdrForFirstSelectedMessage.folder;
>+    if (IsSpecialFolder(folder, MSG_FOLDER_FLAG_JUNK, true))
>+      {

Brace should be aligned with if
> The other spam related settings hare named mail.spam.xxx (sorry for being
> unclear!)
I don't want to enforce anything
mail.spam.markAsNotJunkMarksUnRead and placed after mail.spam.manualMarkMode ?

> The style rule is: if one of the if/else use braces, all should.
so even one-liners:
  if (setAsJunk)
  {
    gDBView.doCommand(nsMsgViewCommandType.junk);
  }
?
Yes.
Attached patch Fix v6 (obsolete) — Splinter Review
updated to comments
Attachment #349641 - Attachment is obsolete: true
Attachment #349646 - Flags: review?(bienvenu)
Attachment #349641 - Flags: review?(bienvenu)
I applied this patch and was playing around with it - I had some problems with assertions thrown getting prefs. Maybe it was a build problem - I'll try to investigate, but I think this will have to wait for b2.
WFM even on compiled build so it's probably a problem on your side, try to update the sources and do the fresh patched build.

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b3pre) Gecko/20081127 Shredder/3.0b1pre
David, Magnus - you are way more familiar with mozilla c++ code than I am, can I ask one of you to provide the patch for the "case 'j': // junkStatus column" in nsMsgDBView.cpp to update it with this bug changes ? (I assume this is wanted ?)
I can offer to compile the build and do the tests.
Attached patch proposed fix (obsolete) — Splinter Review
turns out that in order to do the junk column, we don't need the front end changes, and both the junk button and junk column can go through the c++ code.

I generalized the c++ methods so that they handle both the junk and unjunk cases.

firefox@poczta.neostrada.pl, it would be great if you want to try out the c++ changes.
Assignee: firefox → bienvenu
Attachment #349646 - Attachment is obsolete: true
Attachment #352627 - Flags: superreview?(neil)
Attachment #352627 - Flags: review?(bugzilla)
Attachment #349646 - Flags: review?(bienvenu)
Neil requested this.

In my tree, manually marking a message as junk with the imap delete model isn't imap deleting the message, but maybe the incoming junk filtering does that...
Attachment #352627 - Attachment is obsolete: true
Attachment #352650 - Flags: superreview?(neil)
Attachment #352650 - Flags: review?(bugzilla)
Attachment #352627 - Flags: superreview?(neil)
Attachment #352627 - Flags: review?(bugzilla)
(In reply to comment #86)
> In my tree, manually marking a message as junk with the imap delete model isn't
> imap deleting the message, but maybe the incoming junk filtering does that...
Well, it works in the old build that I use to read bugmail...
Attachment #352650 - Flags: superreview?(neil) → superreview+
Comment on attachment 352650 [details] [diff] [review]
undelete imap messages on non-junk with imap delete model

warning: 12 lines add whitespace errors.

>+                "the classification of a manually-marked junk message has\
>+                been classified as junk, yet there seem to be no such outstanding messages");
Nit: don't use \, use
"some text here "
"some more text here"

>+  nsresult rv = DetermineActionsForJunkChange(msgsAreJunk, moveMessages, 
>+                                            changeReadState, 
>+                                            getter_AddRefs(targetFolder));
Nit: indentation

>+nsMsgDBView::DetermineActionsForJunkChange(PRBool msgsAreJunk, 
>+                                         PRBool &moveMessages, 
>+                                         PRBool &changeReadState, 
>+                                         nsIMsgFolder** targetFolder)
Nit: indentation

>+  // handle the easy case of marking a junk message as good first...
>+  // move back to inbox, if any, and 
and what? ;-)

>+  if (!msgsAreJunk)
>+  {
>+    if (folderFlags & nsMsgFolderFlags::Junk)
>+    {
>+      prefBranch->GetBoolPref("mail.spam.markAsNotJunkMarksUnRead", 
>+                              &changeReadState);
I think the mark unread shouldn't depend on which folder the messages are in.

>+  nsresult DetermineActionsForJunkChange(PRBool msgsAreJunk,
>+                                       PRBool &moveMessages, 
>+                                       PRBool &changeReadState, 
>+                                       nsIMsgFolder** targetFolder);
Nit: indentation

>+pref("mail.spam.markAsNotJunkMarksUnRead", true);
I guess the prefs are somewhat inconsistent, as the mark as read pref is mailnews.ui.junk.manualMarkAsJunkMarksRead
Yup, but only that and firstuse use the mailnews.ui.junk scheme.
Thanks for taking, I can confirm that the patch works as expected.

patched Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b3pre) Gecko/20081215 Shredder/3.0b2pre
Comment on attachment 352650 [details] [diff] [review]
undelete imap messages on non-junk with imap delete model

So I'm getting into a weird loop here:

1) In IMAP inbox, mark message as junk.
-> For some reason this isn't moving it to junk straight away.

2) Move message to junk
-> Message is in junk, but has lost junk indication (bug 359339).

3) Mark as Junk then clear junk status
-> Message removed from junk folder, Inbox says it has one unread message

4) Select Inbox folder
-> Message not in inbox, seems to have been moved back to junk as that is now showing one new message.

5) Go to Junk folder, select message (which does now have junk status), select not junk.
-> Inbox shows 1 new message

6) Repeat 4 and 5

7) Select inbox. Previously junked message now shown in inbox with unread status.


This may be being made worse by bug 359339, but its still a strange loop to be getting into.
very odd - does #1 happen w/o this patch? I need to pick up this patch and address Neil's comments soon. What kind of IMAP server is this? Does it support user-defined keywords?
(In reply to comment #93)
> very odd - does #1 happen w/o this patch? I need to pick up this patch and
> address Neil's comments soon. What kind of IMAP server is this? Does it support
> user-defined keywords?

#1 happens on this profile without this patch (sorry I forgot to check earlier). My trunk nightlies (same server, different account) work fine. I played around with account settings and still couldn't get it to work.

The server is my standard Courier-IMAP one.
The first thing I notice is that we aren't creating the imap junk folder when I mark a message as junk.
I didn't have my imap account configured to move junk messages to the junk folder, so that explains why I wasn't getting a junk folder.

Standard8, can you attach a protocol log of a session showing the problem you're having? It's all working fine here.
(In reply to comment #96)
> I didn't have my imap account configured to move junk messages to the junk
> folder, so that explains why I wasn't getting a junk folder.

Argh, I had my account configured to move junk messages to the junk folder, but not the general preferences.

> Standard8, can you attach a protocol log of a session showing the problem
> you're having? It's all working fine here.

It is still not marking/keeping it as junk on move into the folder, so I'll attach a log in a few minutes.
Attached file Log of marking as junk
Hmm, not sure if you want this here or not, this is an extract of the log from just marking a message as junk (with move to junk folder switched on). It starts with the message already selected in the inbox, then clicking on the button, then gointo into the junk folder and seeing it is not marked as junk.
I can't tell from that log if that particular server supports user defined keywords - I'd need to see the response from the server when we issue a select on the inbox for that server.

What I can tell from that log is that we're not issuing protocol to set the junk flag on the message, though we're running a url to do so. In theory, if the server didn't support custom keywords, we'd remember that the next time we downloaded headers from the target folder that the moved msg should have the junk flag set.
(In reply to comment #99)
> I can't tell from that log if that particular server supports user defined
> keywords - I'd need to see the response from the server when we issue a select
> on the inbox for that server.

Ok, so my issue with not retaining the marking as junk is bug 359339. I'll attach a protocol log there in a while.

I've just tried this with our zimbra test server, and steps 3 - 7 are still happening in a loop. Could part of the problem be that the message is getting into the inbox as a new mail, rather than a read mail?
Ah, yes, Zimbra. Normally this wouldn't be an issue because even though the message looked new, we would have already classified it as junk/non junk, and wouldn't run it through the spam filter. But Zimbra strips off the junk flags when moving messages between folders, without telling us, and we really have no way of knowing that it's doing this because it claims to support user-defined keywords fully. 

The easiest way I can think of dealing with this is to set the pending attributes on the hdr moved back to the inbox even though the server claims to support user-defined keywords. I don't know if I can do that for just Zimbra servers or not...
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.1b3pre) Gecko/20081204 Thunderbird/3.0b1

Moving mail marked as junk by spamassassin from junk folder to Inbox makes the message to return to junk folder.

I believe this is related to this bug as it necessary to guarantee that the message stays where it is intended after unmarking it as junk.
Depends on: 359339
Comment on attachment 352650 [details] [diff] [review]
undelete imap messages on non-junk with imap delete model

This patch got included with the on on bug 359339 I believe.
Attachment #352650 - Flags: review?(bugzilla)
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago15 years ago
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
Verified fixed with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090208 Shredder/3.0b2pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090208 Shredder/3.0b2pre
Status: RESOLVED → VERIFIED
No longer depends on: 465794
Please don't remove bug 465794 from the dependency list. It's still valid. If you initially move junk mail to another account and undo this action, the message is not moved back to the origin account. Instead it lands in the inbox of same account.
Depends on: 465794
I can confirm the comment 106.

If TB is set up to move junk mail to a single Junk folder, located, say, in Local folders, marking mail as 'Not Junk' will move all the messages to the Local folders Inbox instead of the origin accounts Inboxes.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090207 Shredder/3.0b2pre
Re: 106, 107
While this is true please note that bug 466041 was filed to enhance the current behavior and so bug 465794 is not directly related to this closed bug but to the mentioned one.
Ok, thats true. Sorry for the confusion. Removing dependency.
No longer depends on: 465794
Depends on: 478840
You need to log in before you can comment on or make changes to this bug.