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

RESOLVED FIXED in Thunderbird 14.0

Status

defect
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: rkent, Assigned: aceman)

Tracking

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

Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gs], )

Attachments

(1 attachment, 1 obsolete attachment)

2.28 KB, patch
rkent
: review+
Details | Diff | Splinter Review
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
Posted patch patch (obsolete) — Splinter Review
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)
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]
Just note that this bug probably won't fix the problems the user reported. It just prevents creating new filters with illogical actions.
There is a question in Bug 742155 whether multiple copy action could be allowed. Would the backend handle it?
(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.
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).
I'd say yes, if it works.
(In reply to Magnus Melin from comment #8)
> I'd say yes, if it works.

Yes, I agree.
Posted patch patch v2Splinter Review
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)
Blocks: 742155
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.
(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.
Attachment #614126 - Flags: review?(kent) → review+
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.
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
Closed: 7 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
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.
Blocks: 862739
You need to log in before you can comment on or make changes to this bug.