Filter editor should not allow "Delete" and "Move" of same message

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Filters
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: rkent, Assigned: aceman)

Tracking

(Blocks: 1 bug, {polish, ux-error-prevention})

Trunk
Thunderbird 14.0
polish, ux-error-prevention
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gs], URL)

Attachments

(1 attachment, 1 obsolete attachment)

2.28 KB, patch
rkent
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
In the referenced GS thread, I noted that the filter editor allows you to request that a message be both moved and deleted, but that is asking that the original message be deleted twice. This should be disabled somehow.
Blocks: 741423
No longer blocks: 741423
(Assignee)

Comment 1

5 years ago
Created attachment 613795 [details] [diff] [review]
patch

I think it does what you wished. And also adds the same exclusion for Mark as read/Mark as unread (added in TB11, after this bug was filed).
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #613795 - Flags: review?(kent)
Attachment #613795 - Flags: feedback?(dbienvenu)
(Assignee)

Comment 2

5 years ago
Wayne, I am not sure how you mark the getsatisfaction references so I CC you here.
Keywords: polish
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [gs]
(In reply to :aceman from comment #2)
> Wayne, I am not sure how you mark the getsatisfaction references so I CC you
> here.

no problem. details https://wiki.mozilla.org/Thunderbird/Support/Workflow#Bugzilla

http://getsatisfaction.com/mozilla_messaging/topics/filters_not_working-13gkn = https://getsatisfaction.com/mozilla_messaging/tags/bug_541416
(Assignee)

Comment 4

5 years ago
Just note that this bug probably won't fix the problems the user reported. It just prevents creating new filters with illogical actions.
(Assignee)

Comment 5

5 years ago
There is a question in Bug 742155 whether multiple copy action could be allowed. Would the backend handle it?

Comment 6

5 years ago
(In reply to :aceman from comment #5)
> There is a question in Bug 742155 whether multiple copy action could be
> allowed. Would the backend handle it?

yes, the backend should allow multiple copies.
(Assignee)

Comment 7

5 years ago
Do we want to allow it in the UI? Currently the UI only allows one occurrence of each action within one filter, with the exception of 'Add Tag' and 'Forward' actions.
It is in the same spot as the patch is touching, I could add it.
I have tested it now in TB14 and it seems to work (multiple copies).
Keywords: ux-error-prevention

Comment 8

5 years ago
I'd say yes, if it works.

Comment 9

5 years ago
(In reply to Magnus Melin from comment #8)
> I'd say yes, if it works.

Yes, I agree.
(Assignee)

Comment 10

5 years ago
Created attachment 614126 [details] [diff] [review]
patch v2

Thanks, so here is the update.

The current state is:
Allow multiple actions of this kind:
- Add tag, Forward, Copy to
If either member from the following pairs exists, disallow the other one:
- Move to/Delete, Mark as read/Mark as Unread

Any other proposals?

(Also there are some actions that are completely hidden, depending on server type, but that is not the topic for this bug.)
Attachment #613795 - Attachment is obsolete: true
Attachment #614126 - Flags: review?(kent)
Attachment #613795 - Flags: review?(kent)
Attachment #613795 - Flags: feedback?(dbienvenu)
(Assignee)

Updated

5 years ago
Blocks: 742155
(Reporter)

Comment 11

5 years ago
I've looked at this briefly, and don't see any issues with it. But I really need to compile and test it. Since my normal development is done on aurora, it will take me a day or so to get a trunk compile working.
(Reporter)

Comment 12

5 years ago
(In reply to :aceman from comment #10)
> Created attachment 614126 [details] [diff] [review]
> patch v2
> 
> Thanks, so here is the update.
> 
> The current state is:
> Allow multiple actions of this kind:
> - Add tag, Forward, Copy to
> If either member from the following pairs exists, disallow the other one:
> - Move to/Delete, Mark as read/Mark as Unread
> 
> Any other proposals?
>

I've now compiled and briefly tested with this patch (v2), and it seems to work as advertised. But I immediately started asking, are there other standard filter actions that have the same issue?

Looking briefly at the core code, and testing briefly, I guess that the other standard async actions (forward, reply) seem to have core workarounds to allow the combined actions.

It's unfortunate though that these are not generalized in some fashion. I do know that many of the more interesting filter actions that people request require async operations of one sort or another, and although these can be implemented in a custom filter action, they usually interact badly with other async operations such as move and delete. It would be so nice if the core code supported that better.

But I guess that is not this bug.
(Reporter)

Updated

5 years ago
Attachment #614126 - Flags: review?(kent) → review+
(Assignee)

Comment 13

5 years ago
Thanks. I think this already expanded beyond the original request, but only taking the easy options.

So if you can find out any new actions that should be blocked or allowed to occur multiple times, please make a new bug because it seems it will need more discussion and maybe core changes.
Keywords: checkin-needed
(In reply to :aceman from comment #10)
> If either member from the following pairs exists, disallow the other one:
> - Move to/Delete

I think "Stop Filter Execution" is better prohibited when "Move to" or "Delete" is requested, because Move to/Delete implies "Stop Filter Execution" on the hit mail.
What do you think?

By the way, "Sorting of actions in msgFilterRules.dat based on priority of action" may be needed to avoid user's confusions, but it's different enhancement.
(Assignee)

Comment 15

5 years ago
That is true but it is not sure users would understand why "Stop Filter Execution" is disabled. They may think the option is not useful (when coupled with move/delete) and that filters will actually continue execution.
I see.
"Delete + Stop Filter Execution" and "Move to + Stop Filter Execution" won't produce any confliction nor any problem, except bug around "Stop Filter Execution" if exists.
Kent, you aren't listed as a MailNews peer on the Modules wiki. I'm assuming you are OK to do reviews, but you should probably update the page.
https://wiki.mozilla.org/Modules/All

http://hg.mozilla.org/comm-central/rev/ad44a8e284a7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0

Comment 18

5 years ago
I told aceman that rkent was good for this review and for reviewing search and filter code in general, even if he's reluctant to be an official peer.
(Assignee)

Updated

4 years ago
Blocks: 862739
You need to log in before you can comment on or make changes to this bug.