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

VERIFIED FIXED in Thunderbird 11.0

Status

MailNews Core
Filters
--
enhancement
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Matthew Flaschen, Assigned: aceman)

Tracking

Trunk
Thunderbird 11.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Updated

10 years ago
Version: unspecified → 1.5
(Reporter)

Comment 1

10 years ago
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.
Attachment #260661 - Flags: review+

Comment 2

10 years ago
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+

Updated

10 years ago
Assignee: mscott → nobody
Component: General → MailNews: Filters
OS: Linux → All
Product: Thunderbird → Core
QA Contact: general → filters
Hardware: PC → All
Version: 1.5 → unspecified

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 3

10 years ago
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

Comment 5

9 years ago
so is there a fix??

Comment 6

9 years ago
(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

9 years ago
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

9 years ago
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

9 years ago
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

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

Comment 11

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

any fix still?

Comment 12

9 years ago
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

7 years ago
I want this feature.

And it's not in 3.0.4 .

Comment 14

7 years ago
Add me to the requestor list!
(Assignee)

Comment 15

6 years ago
rkent, would you accept this patch if I unbitrotted it?
(Assignee)

Comment 16

6 years ago
Created attachment 569154 [details] [diff] [review]
unbittrotted patch
Assignee: nobody → acelists
Attachment #260661 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #569154 - Flags: review?(dbienvenu)
(Assignee)

Comment 17

6 years ago
Created attachment 569188 [details] [diff] [review]
2nd try, missed some files
Attachment #569154 - Attachment is obsolete: true
Attachment #569188 - Flags: review?(dbienvenu)
Attachment #569154 - Flags: review?(dbienvenu)

Comment 18

6 years ago
I think this patch only works for filtering after the fact, i.e., on already downloaded mail. Is that intentional?
(Assignee)

Comment 19

6 years ago
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

6 years ago
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

6 years ago
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-
(Assignee)

Comment 22

6 years ago
(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.
(Assignee)

Comment 23

6 years ago
(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.
(Assignee)

Comment 24

6 years ago
I understand now, I used source of TB8beta3 :(
(Assignee)

Comment 25

6 years ago
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.
Attachment #569188 - Attachment is obsolete: true
Attachment #572307 - Flags: review?(dbienvenu)

Comment 26

6 years ago
(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...
(Assignee)

Comment 27

6 years ago
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

6 years ago
(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.
(Assignee)

Comment 29

6 years ago
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.
(Assignee)

Comment 30

6 years ago
So will you please review it?
Version: unspecified → Trunk

Comment 31

6 years ago
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)
(Assignee)

Comment 32

6 years ago
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

6 years ago
parsenewmail state means the message was newly received. RunFiltersAfterTheFact is called for run after download.
(Assignee)

Comment 34

6 years ago
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?
Attachment #572307 - Attachment is obsolete: true
Attachment #574051 - Flags: review?(dbienvenu)

Comment 35

6 years ago
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-
(Assignee)

Comment 36

6 years ago
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

6 years ago
(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!
(Assignee)

Comment 38

6 years ago
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.
Attachment #574051 - Attachment is obsolete: true
Attachment #576604 - Flags: review?(dbienvenu)

Comment 39

6 years ago
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.
(Assignee)

Comment 40

6 years ago
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?
Attachment #576604 - Attachment is obsolete: true
Attachment #576620 - Flags: review?(dbienvenu)
Attachment #576604 - Flags: review?(dbienvenu)
(Assignee)

Comment 41

6 years ago
Maybe I have to reset the state somehow before "Doing test 4 MarkReadBody" ... So it seems I have caused the failure.
(Assignee)

Comment 42

6 years ago
Created attachment 576625 [details] [diff] [review]
7th version, fixed test

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)
(Assignee)

Comment 43

6 years ago
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 :)
Attachment #576625 - Attachment is obsolete: true
Attachment #576639 - Flags: review?(dbienvenu)
Attachment #576625 - Flags: review?(dbienvenu)

Comment 44

6 years ago
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+
(Assignee)

Comment 45

6 years ago
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

6 years ago
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+

Updated

6 years ago
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
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
(Assignee)

Updated

6 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.