Last Comment Bug 376546 - Wishlist: Message Filters should have "Mark as unread" action
: Wishlist: Message Filters should have "Mark as unread" action
Status: VERIFIED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: Thunderbird 11.0
Assigned To: :aceman
:
Mentors:
: 450466 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-04 15:31 PDT by Matthew Flaschen
Modified: 2011-12-04 06:46 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Partial patch to add "Mark as unread" option (5.42 KB, patch)
2007-04-04 18:27 PDT, Matthew Flaschen
no flags Details | Diff | Splinter Review
unbittrotted patch (4.73 KB, patch)
2011-10-24 13:02 PDT, :aceman
no flags Details | Diff | Splinter Review
2nd try, missed some files (8.57 KB, patch)
2011-10-24 14:31 PDT, :aceman
mozilla: review-
Details | Diff | Splinter Review
3rd version (14.02 KB, patch)
2011-11-06 09:23 PST, :aceman
no flags Details | Diff | Splinter Review
4th version, adding the message into NewList (14.90 KB, patch)
2011-11-12 08:10 PST, :aceman
mozilla: review-
Details | Diff | Splinter Review
5th version, fixed style, PR_bool, added tests (19.94 KB, patch)
2011-11-23 13:20 PST, :aceman
no flags Details | Diff | Splinter Review
6th version, fixed tests (22.69 KB, patch)
2011-11-23 14:09 PST, :aceman
no flags Details | Diff | Splinter Review
7th version, fixed test (24.16 KB, patch)
2011-11-23 14:32 PST, :aceman
no flags Details | Diff | Splinter Review
8th version, another test fix [Checkin: comment 47] (25.47 KB, patch)
2011-11-23 15:44 PST, :aceman
mozilla: review+
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Matthew Flaschen 2007-04-04 15:31:44 PDT
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
Comment 1 Matthew Flaschen 2007-04-04 18:27:40 PDT
Created attachment 260661 [details] [diff] [review]
Partial patch to add "Mark as unread" option

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.
Comment 2 Karsten Düsterloh 2007-04-04 22:56:53 PDT
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...
Comment 3 Matthew Flaschen 2007-04-04 23:37:05 PDT
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.
Comment 4 Phil Ringnalda (:philor) 2008-08-13 13:13:11 PDT
*** Bug 450466 has been marked as a duplicate of this bug. ***
Comment 5 andrew.pleasant 2008-08-13 13:16:55 PDT
so is there a fix??
Comment 6 Kent James (:rkent) 2008-08-13 14:02:47 PDT
(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.
Comment 7 andrew.pleasant 2008-08-13 14:47:39 PDT
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.
Comment 8 Sven Giermann 2008-12-08 00:37:36 PST
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...
Comment 9 Kent James (:rkent) 2008-12-08 09:19:08 PST
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.
Comment 10 Kent James (:rkent) 2009-01-10 01:00:52 PST
Mark as Unread is now available as a custom filter action in the FiltaQuilla extension.
Comment 11 andrew.pleasant 2009-01-10 20:01:38 PST
but FiltaQuilla won't work on Thunderbird's current version for a Mac.

any fix still?
Comment 12 Kent James (:rkent) 2009-01-10 22:13:15 PST
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.
Comment 13 plop 2010-05-28 02:20:11 PDT
I want this feature.

And it's not in 3.0.4 .
Comment 14 Ben2K 2010-11-26 14:28:30 PST
Add me to the requestor list!
Comment 15 :aceman 2011-10-23 11:46:37 PDT
rkent, would you accept this patch if I unbitrotted it?
Comment 16 :aceman 2011-10-24 13:02:02 PDT
Created attachment 569154 [details] [diff] [review]
unbittrotted patch
Comment 17 :aceman 2011-10-24 14:31:23 PDT
Created attachment 569188 [details] [diff] [review]
2nd try, missed some files
Comment 18 David :Bienvenu 2011-10-25 06:29:02 PDT
I think this patch only works for filtering after the fact, i.e., on already downloaded mail. Is that intentional?
Comment 19 :aceman 2011-10-25 06:52:08 PDT
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.
Comment 20 David :Bienvenu 2011-10-25 07:47:34 PDT
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 21 David :Bienvenu 2011-10-25 07:49:29 PDT
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
Comment 22 :aceman 2011-10-25 08:33:21 PDT
(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.
Comment 23 :aceman 2011-10-25 08:36:53 PDT
(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.
Comment 24 :aceman 2011-10-25 10:37:21 PDT
I understand now, I used source of TB8beta3 :(
Comment 25 :aceman 2011-11-06 09:23:33 PST
Created attachment 572307 [details] [diff] [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.
Comment 26 David :Bienvenu 2011-11-07 07:21:23 PST
(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...
Comment 27 :aceman 2011-11-07 07:30:41 PST
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"?
Comment 28 David :Bienvenu 2011-11-07 07:35:23 PST
(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.
Comment 29 :aceman 2011-11-07 07:52:51 PST
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.
Comment 30 :aceman 2011-11-10 13:13:44 PST
So will you please review it?
Comment 31 David :Bienvenu 2011-11-11 08:37:56 PST
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
Comment 32 :aceman 2011-11-11 09:15:30 PST
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?
Comment 33 David :Bienvenu 2011-11-11 09:18:32 PST
parsenewmail state means the message was newly received. RunFiltersAfterTheFact is called for run after download.
Comment 34 :aceman 2011-11-12 08:10:30 PST
Created attachment 574051 [details] [diff] [review]
4th version, adding the message into NewList

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?
Comment 35 David :Bienvenu 2011-11-22 14:11:24 PST
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.
Comment 36 :aceman 2011-11-22 14:38:25 PST
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.
Comment 37 David :Bienvenu 2011-11-22 14:47:58 PST
(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!
Comment 38 :aceman 2011-11-23 13:20:54 PST
Created attachment 576604 [details] [diff] [review]
5th version, fixed style, PR_bool, added tests

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.
Comment 39 David :Bienvenu 2011-11-23 13:27:53 PST
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.
Comment 40 :aceman 2011-11-23 14:09:29 PST
Created attachment 576620 [details] [diff] [review]
6th version, fixed tests

Thanks for the hints! Fixed everything, tests pass. There seem to be some test failures (in test_imapFilterActions.js) unrelated to my changes?
Comment 41 :aceman 2011-11-23 14:12:35 PST
Maybe I have to reset the state somehow before "Doing test 4 MarkReadBody" ... So it seems I have caused the failure.
Comment 42 :aceman 2011-11-23 14:32:02 PST
Created attachment 576625 [details] [diff] [review]
7th version, fixed test

Now that test passes. I hope it is correct.
Comment 43 :aceman 2011-11-23 15:44:53 PST
Created attachment 576639 [details] [diff] [review]
8th version, another test fix [Checkin: comment 47]

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 :)
Comment 44 David :Bienvenu 2011-12-01 13:52:44 PST
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.
Comment 45 :aceman 2011-12-01 14:05:04 PST
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 46 Ian Neal 2011-12-01 14:57:53 PST
Comment on attachment 576639 [details] [diff] [review]
8th version, another test fix [Checkin: comment 47]

r=me for the Suite string additions
Comment 47 Jens Hatlak (:InvisibleSmiley) 2011-12-03 10:15:06 PST
Comment on attachment 576639 [details] [diff] [review]
8th version, another test fix [Checkin: comment 47]

http://hg.mozilla.org/comm-central/rev/0fe60a45c85f

Note You need to log in before you can comment on or make changes to this bug.