Last Comment Bug 541416 - Filter editor should not allow "Delete" and "Move" of same message
: Filter editor should not allow "Delete" and "Move" of same message
Status: RESOLVED FIXED
[gs]
: polish, ux-error-prevention
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
:
Mentors:
https://getsatisfaction.com/mozilla_m...
Depends on:
Blocks: 862739 742155
  Show dependency treegraph
 
Reported: 2010-01-22 08:51 PST by Kent James (:rkent)
Modified: 2013-04-17 03:36 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.17 KB, patch)
2012-04-10 15:21 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v2 (2.28 KB, patch)
2012-04-11 12:48 PDT, :aceman
rkent: review+
Details | Diff | Splinter Review

Description Kent James (:rkent) 2010-01-22 08:51:46 PST
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.
Comment 1 :aceman 2012-04-10 15:21:55 PDT
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).
Comment 2 :aceman 2012-04-10 15:26:20 PDT
Wayne, I am not sure how you mark the getsatisfaction references so I CC you here.
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2012-04-10 16:14:18 PDT
(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
Comment 4 :aceman 2012-04-11 00:47:51 PDT
Just note that this bug probably won't fix the problems the user reported. It just prevents creating new filters with illogical actions.
Comment 5 :aceman 2012-04-11 11:22:32 PDT
There is a question in Bug 742155 whether multiple copy action could be allowed. Would the backend handle it?
Comment 6 David :Bienvenu 2012-04-11 11:43:54 PDT
(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.
Comment 7 :aceman 2012-04-11 12:21:28 PDT
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).
Comment 8 Magnus Melin 2012-04-11 12:29:40 PDT
I'd say yes, if it works.
Comment 9 David :Bienvenu 2012-04-11 12:31:26 PDT
(In reply to Magnus Melin from comment #8)
> I'd say yes, if it works.

Yes, I agree.
Comment 10 :aceman 2012-04-11 12:48:26 PDT
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.)
Comment 11 Kent James (:rkent) 2012-04-13 13:12:56 PDT
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.
Comment 12 Kent James (:rkent) 2012-04-17 16:02:10 PDT
(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.
Comment 13 :aceman 2012-04-18 02:51:11 PDT
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.
Comment 14 WADA 2012-04-18 03:02:25 PDT
(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.
Comment 15 :aceman 2012-04-18 03:27:58 PDT
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.
Comment 16 WADA 2012-04-18 03:42:04 PDT
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.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-04-18 16:02:38 PDT
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
Comment 18 David :Bienvenu 2012-04-18 16:16:22 PDT
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.

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