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

Status: VERIFIED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Matthew Flaschen]

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]

Attachment: 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]

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]

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 5

[andrew.pleasant]

so is there a fix??

Comment 6

[Kent James (:rkent)]

(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]

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]

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)]

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)]

Mark as Unread is now available as a custom filter action in the FiltaQuilla extension.

Comment 11

[andrew.pleasant]

but FiltaQuilla won't work on Thunderbird's current version for a Mac.

any fix still?

Comment 12

[Kent James (:rkent)]

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]

I want this feature.

And it's not in 3.0.4 .

Comment 14

[Ben2K]

Add me to the requestor list!

Comment 15

[:aceman]

rkent, would you accept this patch if I unbitrotted it?

Comment 16

[:aceman]

Attachment: unbittrotted patch

Comment 17

[:aceman]

Attachment: 2nd try, missed some files

Comment 18

[David :Bienvenu]

I think this patch only works for filtering after the fact, i.e., on already downloaded mail. Is that intentional?

Comment 19

[:aceman]

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]

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]

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]

(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]

(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]

I understand now, I used source of TB8beta3 :(

Comment 25

[:aceman]

Attachment: 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]

(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]

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]

(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]

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]

So will you please review it?

Comment 31

[David :Bienvenu]

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]

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]

parsenewmail state means the message was newly received. RunFiltersAfterTheFact is called for run after download.

Comment 34

[:aceman]

Attachment: 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]

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]

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]

(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]

Attachment: 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]

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]

Attachment: 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]

Maybe I have to reset the state somehow before "Doing test 4 MarkReadBody" ... So it seems I have caused the failure.

Comment 42

[:aceman]

Attachment: 7th version, fixed test

Now that test passes. I hope it is correct.

Comment 43

[:aceman]

Attachment: 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]

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]

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]

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)]

Comment on attachment 576639 [details] [diff] [review]
8th version, another test fix [Checkin: comment 47]

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