Closed Bug 376546 Opened 14 years ago Closed 9 years ago

Wishlist: Message Filters should have "Mark as unread" action

Categories

(MailNews Core :: Filters, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 11.0

People

(Reporter: matthew.flaschen, Assigned: aceman)

References

Details

Attachments

(1 file, 8 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.11) Gecko/20070328 Ubuntu/deltad-security Firefox/1.5.0.11
Build Identifier: 1.5.0.10

It would be useful if a "Mark as unread" action were available as a message filter action.  That way, I can have an early rule mark a large group of messages read, then have a later one unmark a subset of that group.  Right now, I'd like to mark all messages from myself read, but later unmark the messages that are only to me (the others will have been moved to folders before this final rule runs).

Reproducible: Always
Version: unspecified → 1.5
This is a patch that makes most of the necessary changes, as far as I can tell.  I'll try to finish it eventually, but it would be easier for someone familiar with the code.
Attachment #260661 - Flags: review+
Comment on attachment 260661 [details] [diff] [review]
Partial patch to add "Mark as unread" option

Don't set r+ if you're not a reviewer, especially not on partial patches. 
Furthermore, I think it would be better not to have yet another markAsSomething action, but have several action values for a markAs action...
Attachment #260661 - Flags: review+
Assignee: mscott → nobody
Component: General → MailNews: Filters
OS: Linux → All
Product: Thunderbird → Core
QA Contact: general → filters
Hardware: PC → All
Version: 1.5 → unspecified
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm sorry.  I meant to request review, not say I had reviewed it.  I won't set anything in the future.  I do agree with you about a single markAs action, but I have little idea how to implement that.
Product: Core → MailNews Core
Duplicate of this bug: 450466
so is there a fix??
(In reply to comment #5)
> so is there a fix??
> 
This is not currently implemented. I might want to add this as part of some work I am planning to do to control BIFF notification through filters, which would be a similar function of adding another action to set a message flag.
could you please. Sounds simple, but given the IMAP configuration I am not allowed to change .. this is the one thing that prevents me from using Thunderbird - and certainly I can't be the only one in this situation. thanks.
Unbelievable that nobody seems to care about this easy-to-fix and gives-much-effort bug...

Why are there so many default actions, that are not available as filter actions?
Just collected bug 384787, bug 398875 and this one, which are all requesting just tagging features for filters...
The recent fix that I did for bug 419356 allows this kind of functionality to be added in extensions. Although I did not include this particular action in the demo extension in that bug, I'll try to remember to include it in the extension I will release soon, that will work with TB b1 (or the next SM release, whatever that is.)

The extension will be released on addons.mozilla.org, probably under the name filtaquilla. I'll also make a note in my blog at http://mesquilla.com when it is released.
Mark as Unread is now available as a custom filter action in the FiltaQuilla extension.
but FiltaQuilla won't work on Thunderbird's current version for a Mac.

any fix still?
The only changes being done to the current version are stability and security related, so at this point a "fix" would be to include it in the upcoming 3.0 version, not the current version.
I want this feature.

And it's not in 3.0.4 .
Add me to the requestor list!
rkent, would you accept this patch if I unbitrotted it?
Attached patch unbittrotted patch (obsolete) — Splinter Review
Assignee: nobody → acelists
Attachment #260661 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #569154 - Flags: review?(dbienvenu)
Attached patch 2nd try, missed some files (obsolete) — Splinter Review
Attachment #569154 - Attachment is obsolete: true
Attachment #569188 - Flags: review?(dbienvenu)
Attachment #569154 - Flags: review?(dbienvenu)
I think this patch only works for filtering after the fact, i.e., on already downloaded mail. Is that intentional?
You mean on POP3/local folders? And not on IMAP directly on the server? This is what the original patch in comment 1 did, I updated it to current state of the trunk (there were other actions added since that time).
I think it would be good if it worked in all cases, so please point me to the file where I should add this too.
This patch doesn't apply correctly...

You want to be changing the dtd files in the tree, not the ones in dist, which are copied from the tree. In this particular case, it would be mail/locales/en-US/chrome/messenger/FilterEditor.dtd

not:
--- org/mozilla/dist/bin/chrome/en-US/locale/en-US/messenger/FilterEditor.dtd	2011-10-24 21:13:47.000000000 +0200
+++ new/mozilla/dist/bin/chrome/en-US/locale/en-US/messenger/FilterEditor.dtd	2011-10-24 21:13:35.000000000 +0200

Also, nsMsgFilter.cpp diffs didn't apply.
Comment on attachment 569188 [details] [diff] [review]
2nd try, missed some files

this shows you the various places you need to change to apply filter hits - http://mxr.mozilla.org/comm-central/search?string=ApplyFilterHit
Attachment #569188 - Flags: review?(dbienvenu) → review-
(In reply to David :Bienvenu from comment #20)
> This patch doesn't apply correctly...
> 
> You want to be changing the dtd files in the tree, not the ones in dist,
> which are copied from the tree. In this particular case, it would be
> mail/locales/en-US/chrome/messenger/FilterEditor.dtd
That file is straight on the top of the patch...

> 
> not:
> ---
> org/mozilla/dist/bin/chrome/en-US/locale/en-US/messenger/FilterEditor.dtd
> 2011-10-24 21:13:47.000000000 +0200
> +++
> new/mozilla/dist/bin/chrome/en-US/locale/en-US/messenger/FilterEditor.dtd
> 2011-10-24 21:13:35.000000000 +0200
Ok, will remove these.

> Also, nsMsgFilter.cpp diffs didn't apply.
I used the trunk from 2011/10/19, will check. Thanks.
(In reply to David :Bienvenu from comment #21)
> this shows you the various places you need to change to apply filter hits -
> http://mxr.mozilla.org/comm-central/search?string=ApplyFilterHit

I see, will modify those too.
I understand now, I used source of TB8beta3 :(
Attached patch 3rd version (obsolete) — Splinter Review
Patch against comm-central pulled via HG 6/11/2011. Comment 20 and comment 21 addressed. Still don't know about comment 18. I need more info from David.
Tested on POP3 and News account, can't check IMAP.
Attachment #569188 - Attachment is obsolete: true
Attachment #572307 - Flags: review?(dbienvenu)
(In reply to :aceman from comment #25)
> Created attachment 572307 [details] [diff] [review] [diff] [details] [review]
> 3rd version
> 
> Patch against comm-central pulled via HG 6/11/2011. Comment 20 and comment
> 21 addressed. Still don't know about comment 18. I need more info from David.
> Tested on POP3 and News account, can't check IMAP.

What more info do you need from me? It looks to me like you changed the pop3 and imap filtering code using the info in the link I gave you...
I am not sure what you meant in comment 18 and whether the patch addresses it. Yes, I changed the places in the link from you. Does it solve the problem of "only filtering after the fact"?
(In reply to :aceman from comment #27)
> I am not sure what you meant in comment 18 and whether the patch addresses
> it. Yes, I changed the places in the link from you. Does it solve the
> problem of "only filtering after the fact"?

It should, yes.
Ok, thanks. I have put comments (questions) into nsParseNewMailState::MarkFilteredMessageUnread function in the patch. I would need your answers to them and then those comments can be removed.
So will you please review it?
Version: unspecified → Trunk
Comment on attachment 572307 [details] [diff] [review]
3rd version

To restore the new flag, you'll need to try something like this:

http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2544

though obviously, it would be mDatabase, not destMailDB
Attachment #572307 - Flags: review?(dbienvenu)
Now I am not sure the new flag should be restored. I just negated "mark as read" which removes New, which is fine.
But restoring New makes no sense when the filter is manually run.
I'll think about that.

When I am in nsParseNewMailState::MarkFilteredMessageRead, do I know whether the message was just received (it is filter run after download)? Can I check this state somewhere?
parsenewmail state means the message was newly received. RunFiltersAfterTheFact is called for run after download.
I am not sure why is the applyFilter functionality not shared between those 2 classes (yes, it is coded quite differently). There probably is a reason.
But one must be attentive to apply changes to both places.

So I have updated the patch. Do I need to check if the message already is in the NewList? Or does the list handle duplicates fine?
Attachment #572307 - Attachment is obsolete: true
Attachment #574051 - Flags: review?(dbienvenu)
Comment on attachment 574051 [details] [diff] [review]
4th version, adding the message into NewList

Thx, this is looking a lot better; sorry for the delay in reviewing it.

since this always returns NS_OK, it should be void. Same for MarkFilteredMessageUnread

-int nsParseNewMailState::MarkFilteredMessageRead(nsIMsgDBHdr *msgHdr)
+nsresult nsParseNewMailState::MarkFilteredMessageRead(nsIMsgDBHdr *msgHdr)
 {

You should add a test to \mailnews\imap\test\unit\test_imapFilterActions.js for marking unread. And ideally \mailnews\news\test\unit\test_filter.js, though I'm not going to insist on that. I can't find tests for local filters, so you don't need to add tests for those.

m_curFolder should line up with break, so it should be indented two fewer spaces: (yes, I know the other code is indented incorrectly, but we should move in the direction of correct indentation)

+      case nsMsgFilterAction::MarkUnread:
+          m_curFolder->MarkMessagesRead(m_searchHitHdrs, PR_FALSE);
+        break;
       case nsMsgFilterAction::MarkFlagged:
-        m_curFolder->MarkMessagesFlagged(m_searchHitHdrs, PR_TRUE);
+          m_curFolder->MarkMessagesFlagged(m_searchHitHdrs, PR_TRUE);
         break;

It would be great if you could use true and false instead of PR_TRUE and PR_FALSE, though I'm not going to insist on that.
Attachment #574051 - Flags: review?(dbienvenu) → review-
Argh, guys, you are often punishing me for code that is not really my fault :) I always read the patches should adapt to formatting and style of surrounding code. Shouldn't those style fixes you suggest be done globally in some other bug and for all functions in the file? When I do your suggestions now, my code will be good but the rest will stay ugly. I bet somebody will come around and fix my code to be in line with the rest, not noticing the rest is wrong :)

So, I will do these fixes tomorrow. I hope those are the last nits :) I need to get this big patch out of my tree so I can progress on other bugs.
(In reply to :aceman from comment #36)
> Argh, guys, you are often punishing me for code that is not really my fault
> :) I always read the patches should adapt to formatting and style of
> surrounding code.

Only if the formatting and style of the surrounding code is one of our allowed formats/styles. In particular, you're supposed to use the prevailing braces style.

> Shouldn't those style fixes you suggest be done globally
> in some other bug and for all functions in the file?

You changed something that was formatted correctly to make it formatted uncorrectly, which is why I complained.

> When I do your
> suggestions now, my code will be good but the rest will stay ugly. I bet
> somebody will come around and fix my code to be in line with the rest, not
> noticing the rest is wrong :)

That's why we have code reviews :-)

> 
> So, I will do these fixes tomorrow. I hope those are the last nits :) I need
> to get this big patch out of my tree so I can progress on other bugs.

Yes, other than that, the patch looks and works fine. Thx for your work on this!
OK, I added the tests. But I don't know how to run them, so please try them out.
I added a new sample message for it. But 'hg diff' did not pick up the added file so I added it into the patch manually. I hope it works.
Attachment #574051 - Attachment is obsolete: true
Attachment #576604 - Flags: review?(dbienvenu)
the top of this page tells you how to run an xpcshell test:

https://wiki.mozilla.org/User:Bienvenu/Cheat_Sheet

If you hg add the sample message, hg diff should pick it up.
Attached patch 6th version, fixed tests (obsolete) — Splinter Review
Thanks for the hints! Fixed everything, tests pass. There seem to be some test failures (in test_imapFilterActions.js) unrelated to my changes?
Attachment #576604 - Attachment is obsolete: true
Attachment #576620 - Flags: review?(dbienvenu)
Attachment #576604 - Flags: review?(dbienvenu)
Maybe I have to reset the state somehow before "Doing test 4 MarkReadBody" ... So it seems I have caused the failure.
Attached patch 7th version, fixed test (obsolete) — Splinter Review
Now that test passes. I hope it is correct.
Attachment #576620 - Attachment is obsolete: true
Attachment #576625 - Flags: review?(dbienvenu)
Attachment #576620 - Flags: review?(dbienvenu)
The adding of a new message broke test in a file I was not modifying:(
I have now run all tests and can't see any problem related to my patch (I really hope the fail in test_offlineStoreLocking.js is not my fault).
This is really my last version for today. David, you can look at it :)
Attachment #576625 - Attachment is obsolete: true
Attachment #576639 - Flags: review?(dbienvenu)
Attachment #576625 - Flags: review?(dbienvenu)
Comment on attachment 576639 [details] [diff] [review]
8th version, another test fix [Checkin: comment 47]

thx, looks good. 

Technically, I don't think I can sign off on the suite changes, though I don't think anyone will complain since they're just strings.
Attachment #576639 - Flags: review?(dbienvenu) → review+
Those are just string additions so shouldn't harm. But I added them for safety, because the new filter action is in mailnews core so it can happen suite will pick it up too and would need the strings (the menuitems declarations are in mailnews core too).

Thanks for your help. Do I need any other review here?
Comment on attachment 576639 [details] [diff] [review]
8th version, another test fix [Checkin: comment 47]

r=me for the Suite string additions
Attachment #576639 - Flags: review+
Keywords: checkin-needed
Comment on attachment 576639 [details] [diff] [review]
8th version, another test fix [Checkin: comment 47]

http://hg.mozilla.org/comm-central/rev/0fe60a45c85f
Attachment #576639 - Attachment description: 8th version, another test fix → 8th version, another test fix [Checkin: comment 47]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.