Last Comment Bug 775665 - Change Filter Context UI to checkboxes
: Change Filter Context UI to checkboxes
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
https://bugzilla.mozilla.org/attachme...
Depends on: 561762
Blocks: 790876 11039 479823
  Show dependency treegraph
 
Reported: 2012-07-19 11:10 PDT by Kent James (:rkent)
Modified: 2012-10-19 17:19 PDT (History)
11 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
change filter ui to checkboxes (12.53 KB, patch)
2012-07-27 07:27 PDT, xzer
mconley: feedback-
Details | Diff | Review
patch for send filter (26.74 KB, patch)
2012-07-27 07:35 PDT, xzer
no flags Details | Diff | Review
WIP patch v2 (14.92 KB, patch)
2012-09-11 15:41 PDT, :aceman
no flags Details | Diff | Review
WIP patch v3 (16.96 KB, patch)
2012-09-12 14:12 PDT, :aceman
bwinton: ui‑review-
rkent: feedback+
Details | Diff | Review
one suggestion to organize the flow (23.14 KB, image/png)
2012-09-14 16:05 PDT, Axel Grude [:realRaven]
no flags Details
patch v4 (22.25 KB, patch)
2012-09-30 15:39 PDT, :aceman
bwinton: ui‑review+
rkent: feedback+
Details | Diff | Review
patch v5 (23.26 KB, patch)
2012-10-06 06:41 PDT, :aceman
neil: review+
Details | Diff | Review
patch v6 (22.99 KB, patch)
2012-10-07 12:55 PDT, :aceman
rkent: review+
Details | Diff | Review
patch v7 (22.99 KB, patch)
2012-10-18 09:27 PDT, :aceman
acelists: review+
Details | Diff | Review

Description Kent James (:rkent) 2012-07-19 11:10:12 PDT
(This is a followup from bug 479823  comment 0)

Comment on attachment 643864 [details]
An idea how the Filter Rules could look

This would be a useful feature, as would the similar filter on send.

The issue to me is the UI above. What you have done is the natural extension of this, and exactly what I did ("Checking mail (after classification) and "Checking Mail (after classification or Manually Run" are my additions).

But we are running into a combinatorial explosion here. The original mistake was the first change, with taking two features ("Incoming" and "Manual") and adding three options for that when what we should have had was checkboxes. The same filter should be able to applied in any valid combination of contexts without requiring 4,8,16, or 32 choices as we add features.

I suppose what would be useful is another bug to change this UI, and make the current bug and the "Filter on Send" bug dependent on it.

I have an aversion to opening enhancement requests that I am not going to undertake myself, but aceman if you would like to take a stab at the UI change, then we might be able to move forward with these other very important bugs.
Comment 1 :aceman 2012-07-19 11:21:33 PDT
So the dropdown list of 7 combinations events when to run the filter should be converted to an array of checkboxes, like this:

Apply filter when:
# Checking Mail
#   (after classification)
# Manually Run
# <some possible new event>

The events can be chosen in any combination.
Comment 2 Kent James (:rkent) 2012-07-19 11:38:37 PDT
Yes that's the general idea. There are complications though (like incoming can be either before or after classification, but not both).

I'm not sure what to do with "after classification". It allows you to filter after the junk mail processing occurs, so that you can use the junk-related filter searches, at the cost of delaying your message filters until after the whole batch is received. I doubt if many people understand that though. In retrospect that should have been detected automatically.

But the checkboxes seem the best way to go so that we can add "After Send" and "At Archive" in the future. Not sure if "Before Send" makes sense or not.
Comment 3 xzer 2012-07-19 18:40:28 PDT
It seems that we eventually start to address the issues in filter function.

aceman, you mentioned a simple list of checkboxes, but I rather like what I tried at bug 11039, attachment 423202 [details].

rkent, I think "before send" is more useful than "after send". Many people asked me whether I can apply filters before sending because they want to avoid some often-mistaken cases or they want to add some special headers to certain messages or other reasons. On the other hand, most people use "after send" to move or tag messages, which in fact can be alternated by using search folder for most cases.
Comment 4 Kent James (:rkent) 2012-07-19 21:14:52 PDT
I also like xzer's proposal. Do you understand what the issue was with it? As far as I can tell it was just "let's talk about it".
Comment 5 :aceman 2012-07-20 00:06:34 PDT
Yeah, why can't that patch be finished? xzer, can you finish it? It looks like what we want. The difference is that the bug here is only to rework existing options (UI) and not add any new backend so it should be easier to land.
Bug 11039 already adds the filter at send time functionality.
Comment 6 xzer 2012-07-20 00:23:01 PDT
I think I can try to pick up the patch again and commit a smaller fix for UI style change only at first. For the send time functionality, I am not sure whether I can have time to finish it, it is so long ago....

Any way, we can have a start here.
Comment 7 Kent James (:rkent) 2012-07-20 00:23:50 PDT
I recommended this bug because shorter bugs are easier to review and land. So if we could get this landed, then the front end issues would be largely resolved, and we could then only deal with the backend changes needed to the filters to add the additional contexts.
Comment 8 :aceman 2012-07-20 00:34:35 PDT
xzer, that would be great. Please try to revive and split the patch into the 2 bugs. If after that you do not find enough time to drive the patches until checkin, we can probably help you (I'd try the UI patch that you attach here, rkent may finish the backend in bug 11039).
Comment 9 Magnus Melin 2012-07-20 11:53:02 PDT
(In reply to Kent James (:rkent) from comment #2)
> I'm not sure what to do with "after classification". It allows you to filter
> after the junk mail processing occurs, so that you can use the junk-related
> filter searches, at the cost of delaying your message filters until after
> the whole batch is received. I doubt if many people understand that though.
> In retrospect that should have been detected automatically.

Indeed, we should probably fix that if it's going to be checkboxes, or the UI easily grows large enough to be very confusing.
Comment 10 :aceman 2012-07-26 12:58:51 PDT
xzer, you can also give us permission to use your patch and split/update it as needed.
Comment 11 xzer 2012-07-27 07:27:20 PDT
Created attachment 646573 [details] [diff] [review]
change filter ui to checkboxes
Comment 12 xzer 2012-07-27 07:33:15 PDT
Sorry for being late. I have split it to two patches, but there are some compile errors in the second patch for send filter functionality due to a disappeared class. I think I have no time to fix the second patch in the next week, so it would be nice if anybody could take it over if we want to finish it as soon as possible.

Btw, where should I submit the uncompleted send filter functionality patch? I don't know whether the original bug 11039 is an appropriate place since it depends on the UI patch.
Comment 13 xzer 2012-07-27 07:35:57 PDT
Created attachment 646577 [details] [diff] [review]
patch for send filter

I temporarily submit the send filter patch here, please be careful of that it is a uncompleted patch because of missing deprecated class.
Comment 14 Kent James (:rkent) 2012-07-27 08:04:19 PDT
I'm proposing that we do the UI change here separately from bug 11039 (and initially without the send feature) to simplify moving forward with 11039 and the similar "on archive" bug. So at least for now, this is the correct place for the UI patch.

But you have submitted the send filter patch together with the UI patch. Could we split off the UI portion, and just try to do that first (without the Send feature)? IIRC that was the sticking point anyway in making progress with 11039, and smaller patches are generally preferred and easier to review.
Comment 15 Ben Bucksch (:BenB) 2012-07-28 03:16:19 PDT
To not complicate the UI for filters further, making it even more scary, I'd suggest to move these "run when" checkboxen into a secondary dialog (or, if you wish, expandable, default hidden). Incoming, outgoing and manual would be checked by default - because I think most users will want this - and only disabled when you go to this UI.
Comment 16 xzer 2012-07-29 18:23:58 PDT
rkent

Because I have no time in this week and I want to have a place to share my current stage so that others can take it over if there are someone wanted.

I am sorry if here is a bad place for this purpose, anyway, I think the first patch for UI change can be reviewed and tested now.
Comment 17 :aceman 2012-09-10 03:30:21 PDT
xzer, have you found some time for the patch? Or can we start to work on it instead of you?
Comment 18 :aceman 2012-09-10 05:20:52 PDT
Comment on attachment 646577 [details] [diff] [review]
patch for send filter

This is moved to bug 11039.
Comment 19 :aceman 2012-09-10 05:26:10 PDT
(In reply to Ben Bucksch (:BenB) from comment #15)
> To not complicate the UI for filters further, making it even more scary, I'd
> suggest to move these "run when" checkboxen into a secondary dialog (or, if
> you wish, expandable, default hidden). Incoming, outgoing and manual would
> be checked by default - because I think most users will want this - and only
> disabled when you go to this UI.

I'd agree with the expandable version so I'll implement it if I get the chance. Otherwise here is the screenshot of how the feature should look like according to the patch attached: https://bugzilla.mozilla.org/attachment.cgi?id=423202 .
Comment 20 xzer 2012-09-10 06:05:00 PDT
:aceman 

Sorry, I think it is really hard to me to get time to complete the work. I give it up, please take it over.
Comment 21 :aceman 2012-09-10 06:08:10 PDT
Thanks for your work on the patch so far.
I think I understand how it works so I try to continue with it.
Comment 22 :aceman 2012-09-11 03:37:26 PDT
Comment on attachment 646573 [details] [diff] [review]
change filter ui to checkboxes

IanN, mconley, can you look at the patch and see variable names like 'rdBfCl'. Are those OK or should they be more descriptive? rdBfCl probably represents 'ReceivedBeforeClassification'
Comment 23 :aceman 2012-09-11 15:41:19 PDT
Created attachment 660240 [details] [diff] [review]
WIP patch v2

So I updated the patch with these refinements:
- the new code should now produce the same values of the filter type (nsMsgFilterType) as current code. Uses .InboxRule instead of .Incoming.
- if server is defered, only disable the "After classification" radiobox instead of hiding it. The value will still be preserved if untouched so the user should see it.
- if all checkboxes are unchecked, force at least one to be checked, choosing the "manual" one.
- added Seamonkey strings

TODO:
- add accesskeys to the new labels
- implement the collapsing of the whole widget (if this is really wanted). It is planned to have at least 3 rows in the future (now it has 2).
Comment 24 :aceman 2012-09-11 15:43:10 PDT
(For those not applying the patch but wanting to see how it looks like, see the screenshot in the URL field.)
Comment 25 :aceman 2012-09-12 11:52:51 PDT
Bwinton, should I add the collapsing of the whole box after clicking a button? Something line in the Tools->Clear recent history dialog.

Also another question is if I should force the Manual Run to be checked if nothing else is, or I can allow all checkboxes to get unchecked but error out at an attempt to save/close the editor dialog.
Comment 26 Mike Conley (:mconley) - (needinfo me!) 2012-09-12 12:48:40 PDT
Comment on attachment 646573 [details] [diff] [review]
change filter ui to checkboxes

Review of attachment 646573 [details] [diff] [review]:
-----------------------------------------------------------------

Hey xzer! Thanks for the patch!

Just put some general thoughts below. Let me know if you have any questions!

-Mike

::: mailnews/base/search/content/FilterEditor.js
@@ +53,5 @@
>        // deferred, (you must define them on the deferredTo server instead).
>        let server = gFilterList.folder.server;
> +      if(server.rootFolder != server.rootMsgFolder)
> +      {
> +        gFilterTypeDelegator.rdAfCl.style.display = "none";  

Instead of writing directly to the node's style, can you just set the collapsed attribute to true?

@@ +215,5 @@
> +}
> +
> +function initializeFilterTypeDelegator()
> +{
> +  gFilterTypeDelegator = {

If we remove the setTimeout's from init, is this still a delegator? If not, this should be renamed.

I think this object should be documented so we know exactly what it's doing and why.

@@ +216,5 @@
> +
> +function initializeFilterTypeDelegator()
> +{
> +  gFilterTypeDelegator = {
> +      ckMan : document.getElementById("ckManual"),

Yeah, these variable names are a little opaque. Could they be made more descriptive?

@@ +230,5 @@
> +      init : function()
> +      {
> +        //It seems that there is only this way can handle the event
> +        //of check status changing on both of mouse click and keyboard 
> +        //operation. Is there an another better way?

You should add an event listener for the "change" event, like this:

let checkbox = document.getElementById("some-checkbox");
checkbox.addEventListener("change", function(aEvent) {
  // Do whatever you'd like in here. You can read checkbox's checked status with
  // aEvent.target.checked. Or use the checkbox from the closure. Whichever.
});

@@ +299,5 @@
> +        
> +        this.click();
> +      },
> +      
> +      click : function(event)

I'd prefer if this was called "onClick". This is the function you should perhaps fire with your checkbox "changed" event listener.
Comment 27 :aceman 2012-09-12 13:34:00 PDT
Thanks mconley, but you have done overwork here :) I asked only for the variable names. If you see my WIP patch v2, most of the lines you object to no longer exist :)
Comment 28 :aceman 2012-09-12 14:12:13 PDT
Created attachment 660575 [details] [diff] [review]
WIP patch v3

Like this better? :)
Comment 29 :aceman 2012-09-12 14:15:21 PDT
Maybe some tooltips could be added to the "classification" radios to explain what they mean?
Comment 30 xzer 2012-09-12 16:30:24 PDT
I believe a tooltip is absolutely needed because I had been asked many times for what the meaning of classification is from my sendfilter addon users.
Comment 31 Magnus Melin 2012-09-12 22:49:03 PDT
Better yet, get rid of the user facing choice of classification, and just auto-detect if that's needed or not. This would also greatly simplify the UI. May be another bug though.
Comment 32 :aceman 2012-09-13 00:20:04 PDT
Yes, it was already proposed in comment 2. But I am not able to do that so we split it into new bug 790876. rkent may look at it later.

I'll add some explanation tooltips for now.
Comment 33 Kent James (:rkent) 2012-09-13 08:26:25 PDT
(In reply to Magnus Melin from comment #31)
> Better yet, get rid of the user facing choice of classification, and just
> auto-detect if that's needed or not. This would also greatly simplify the
> UI. May be another bug though.

Yes I agree this is the plan.
Comment 34 Blake Winton (:bwinton) (:☕️) 2012-09-13 12:38:06 PDT
Comment on attachment 660575 [details] [diff] [review]
WIP patch v3

> Bwinton, should I add the collapsing of the whole box after clicking a
> button? Something line in the Tools->Clear recent history dialog.

No, I think that would be too weird for just a couple of options.  (That dialog looks like that to match Firefox.)

> Also another question is if I should force the Manual Run to be checked
> if nothing else is, or I can allow all checkboxes to get unchecked but
> error out at an attempt to save/close the editor dialog.

After playing with it for a while, I don't think you should force the manual run to be checked, since it felt confusing, and if I accidentally unclick the "Checking Mail" then I need to go through way more steps to recover.

The other thing is that the layout feels a little odd to me.  I wonder if, instead of a pair of radio buttons, another control (like a checkbox) might be a better choice…  Or perhaps it should be
[] Manually Run
[] Checking Mail
  () Before Classification
  () After Classification

Oh, or:

[] Manually Run
[] Checking Mail [Before Classification|v]
                 [After Classification   ]

(with a drop-down.)  The tricky part with that would be to localize it, but hopefully we can make it not entirely ugly when not localized…

>+++ b/mail/locales/en-US/chrome/messenger/FilterEditor.dtd
>@@ -12,22 +12,25 @@
>+<!ENTITY contextBeforeCls.label "Before Classification">
>+<!ENTITY contextBeforeCls.accesskey "B">
>+<!ENTITY contextAfterCls.label "After Classification">
>+<!ENTITY contextAfterCls.accesskey "f">

How about "Before Junk Scanning", or "Before Junk Testing"?

Thanks,
Blake.
Comment 35 :aceman 2012-09-13 12:56:49 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #34)
> Comment on attachment 660575 [details] [diff] [review]
> WIP patch v3
> 
> > Bwinton, should I add the collapsing of the whole box after clicking a
> > button? Something line in the Tools->Clear recent history dialog.
> 
> No, I think that would be too weird for just a couple of options.  (That
> dialog looks like that to match Firefox.)
As said, there will be 3 rows of options in the future.

> > Also another question is if I should force the Manual Run to be checked
> > if nothing else is, or I can allow all checkboxes to get unchecked but
> > error out at an attempt to save/close the editor dialog.
> 
> After playing with it for a while, I don't think you should force the manual
> run to be checked, since it felt confusing, and if I accidentally unclick
> the "Checking Mail" then I need to go through way more steps to recover.
Ok, so I'll add the error message at save.

> The other thing is that the layout feels a little odd to me.  I wonder if,
> instead of a pair of radio buttons, another control (like a checkbox) might
> be a better choice…  Or perhaps it should be
> [] Manually Run
> [] Checking Mail
>   () Before Classification
>   () After Classification
That will be too high, even more considering the 3rd row that will be added in the future, that will have another suboptions too...

> Oh, or:
> [] Manually Run
> [] Checking Mail [Before Classification|v]
>                  [After Classification   ]
> 
> (with a drop-down.)  The tricky part with that would be to localize it, but
> hopefully we can make it not entirely ugly when not localized…
I don't see any localization difference in this and the original version with the radio's. What is the change here? Also, is it possible to disable an item in a dropdown? If yes, and the disabled item was previously selected, will it still be preserved if just opening the dropdown and not selecting anything?

> >+++ b/mail/locales/en-US/chrome/messenger/FilterEditor.dtd
> >@@ -12,22 +12,25 @@
> >+<!ENTITY contextBeforeCls.label "Before Classification">
> >+<!ENTITY contextBeforeCls.accesskey "B">
> >+<!ENTITY contextAfterCls.label "After Classification">
> >+<!ENTITY contextAfterCls.accesskey "f">
> 
> How about "Before Junk Scanning", or "Before Junk Testing"?
Depends on rkent if the classification really only contains junk scanning. It is called "afterPlugins" in the code, so maybe there are other operations hidden in there (like extension added actions).
Comment 36 Kent James (:rkent) 2012-09-14 10:29:01 PDT
Comment on attachment 660575 [details] [diff] [review]
WIP patch v3

Yes, I really like this! It will be very easy to add "Before Send" and "Before Archive" in the future.

The "classification" part is what is questioned by people. As I said, I think we should remove this completely before this feature ever hits a release, so I don't think we should put too much effort into that.

I did not review the code, just compiled it and looked at the results.
Comment 37 Blake Winton (:bwinton) (:☕️) 2012-09-14 12:57:12 PDT
(In reply to :aceman from comment #35)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #34)
> > Comment on attachment 660575 [details] [diff] [review]
> > > Bwinton, should I add the collapsing of the whole box after clicking a
> > > button? Something line in the Tools->Clear recent history dialog.
> > No, I think that would be too weird for just a couple of options.  (That
> > dialog looks like that to match Firefox.)
> As said, there will be 3 rows of options in the future.

Even still.  That's not a lot of options.

> > The other thing is that the layout feels a little odd to me.  I wonder if,
> > instead of a pair of radio buttons, another control (like a checkbox) might
> > be a better choice…  Or perhaps it should be
> > [] Manually Run
> > [] Checking Mail
> >   () Before Classification
> >   () After Classification
> That will be too high, even more considering the 3rd row that will be added
> in the future, that will have another suboptions too...

So, it seems a little like you're asking me to review a UI that we haven't coded yet.  If you've got plans for what it will look like in the future, maybe you can post an image and I can review that?

> > Oh, or:
> > [] Manually Run
> > [] Checking Mail [Before Classification|v]
> >                  [After Classification   ]
> > (with a drop-down.)
> I don't see any localization difference in this and the original version
> with the radio's. What is the change here?

"Checking Mail After Classification" reads like a sentence.

> Also, is it possible to disable an item in a dropdown?
> If yes, and the disabled item was previously
> selected, will it still be preserved if just opening the dropdown and not
> selecting anything?

I believe so.  Can you check and let me know.

> > >+++ b/mail/locales/en-US/chrome/messenger/FilterEditor.dtd
> > >@@ -12,22 +12,25 @@
> > >+<!ENTITY contextBeforeCls.label "Before Classification">
> > >+<!ENTITY contextBeforeCls.accesskey "B">
> > >+<!ENTITY contextAfterCls.label "After Classification">
> > >+<!ENTITY contextAfterCls.accesskey "f">
> > How about "Before Junk Scanning", or "Before Junk Testing"?
> Depends on rkent if the classification really only contains junk scanning.
> It is called "afterPlugins" in the code, so maybe there are other operations
> hidden in there (like extension added actions).

It seems like we might be removing the classification part entirely, and just base it on what selections the user made in their filter settings.  (i.e. if they're asking for a junk score, automatically make it afterPlugins…)

Thanks,
Blake.
Comment 38 :aceman 2012-09-14 13:07:48 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #37)
> (In reply to :aceman from comment #35)
> > (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #34)
> > > Comment on attachment 660575 [details] [diff] [review]
> > > > Bwinton, should I add the collapsing of the whole box after clicking a
> > > > button? Something line in the Tools->Clear recent history dialog.
> > > No, I think that would be too weird for just a couple of options.  (That
> > > dialog looks like that to match Firefox.)
> > As said, there will be 3 rows of options in the future.
> 
> Even still.  That's not a lot of options.

What about 4?

> > > The other thing is that the layout feels a little odd to me.  I wonder if,
> > > instead of a pair of radio buttons, another control (like a checkbox) might
> > > be a better choice…  Or perhaps it should be
> > > [] Manually Run
> > > [] Checking Mail
> > >   () Before Classification
> > >   () After Classification
> > That will be too high, even more considering the 3rd row that will be added
> > in the future, that will have another suboptions too...
> 
> So, it seems a little like you're asking me to review a UI that we haven't
> coded yet.  If you've got plans for what it will look like in the future,
> maybe you can post an image and I can review that?

I've mentioned the screenshot several times:
https://bugzilla.mozilla.org/attachment.cgi?id=423202

> > > >+++ b/mail/locales/en-US/chrome/messenger/FilterEditor.dtd
> > > >@@ -12,22 +12,25 @@
> > > >+<!ENTITY contextBeforeCls.label "Before Classification">
> > > >+<!ENTITY contextBeforeCls.accesskey "B">
> > > >+<!ENTITY contextAfterCls.label "After Classification">
> > > >+<!ENTITY contextAfterCls.accesskey "f">
> > > How about "Before Junk Scanning", or "Before Junk Testing"?
> > Depends on rkent if the classification really only contains junk scanning.
> > It is called "afterPlugins" in the code, so maybe there are other operations
> > hidden in there (like extension added actions).
> 
> It seems like we might be removing the classification part entirely, and
> just base it on what selections the user made in their filter settings. 
> (i.e. if they're asking for a junk score, automatically make it
> afterPlugins…)

Yes rkent plans to remove these "classification" options in a follow up bug before TB24, therefore it is not very productive to argue about the exact wording/position too much :)
Comment 39 Axel Grude [:realRaven] 2012-09-14 16:05:59 PDT
Created attachment 661395 [details]
one suggestion to organize the flow

Just a suggestion on how the flow could be visualized. the listboxes represent actions of what happens before (left) and after (right) the trigger; the checkboxes represent what triggers it. positions of the actions can be changed with the arrow buttons (or by double clicking an item)

I personally find this easier to see at a glance than lots of radio / checkboxes
Comment 40 :aceman 2012-09-23 14:55:27 PDT
I'll upload a new patch once bug 561762 is landed as there is a collision in code changes.
Comment 41 :aceman 2012-09-30 15:39:07 PDT
Created attachment 666388 [details] [diff] [review]
patch v4

Another try.
Comment 42 Blake Winton (:bwinton) (:☕️) 2012-10-01 11:36:30 PDT
Comment on attachment 666388 [details] [diff] [review]
patch v4

This is a giant improvement on what we have.  ui-r=me.

(I think we will want to be careful when we add more things in the future, but for now, this is awesome!)

Thanks,
Blake.
Comment 43 Kent James (:rkent) 2012-10-01 15:48:50 PDT
Comment on attachment 666388 [details] [diff] [review]
patch v4

This looks great to me also!

(I did not review the code, just compiled it and tested it).
Comment 44 :aceman 2012-10-02 00:16:06 PDT
rkent, great but who do we persuade to review it if not you? :)
Comment 45 Kent James (:rkent) 2012-10-02 06:54:33 PDT
(In reply to :aceman from comment #44)
> rkent, great but who do we persuade to review it if not you? :)

I didn't say that I would not review the code, only that I had not. I just wanted to make it clear that your request was for feedback, so that is what I gave, and not review.
Comment 46 :aceman 2012-10-02 07:14:11 PDT
OK, understood:)
Comment 47 neil@parkwaycc.co.uk 2012-10-04 17:19:59 PDT
Comment on attachment 666388 [details] [diff] [review]
patch v4

>+      if (server.rootFolder != server.rootMsgFolder)
>+        gFilterTypeSelector.disableAfterPlugins();
So, this is to allow someone to fix their filter to run before plugins if they decide to defer their account?

>+    let nsMsgFilterType = Components.interfaces.nsMsgFilterType;
You use this a lot. Make this a global var/const perhaps?

>+  gFilterTypeSelector = {
>+      chkBoxManual: document.getElementById("runManual"),
Nit: incorrect indentation

>+      chkBoxIncoming : document.getElementById("runIncoming"),
Nit: "menulist", so why shorten "checkbox"?

>+        if (type & nsMsgFilterType.Manual)
>+          this.chkBoxManual.checked = true;
>+        else
>+          this.chkBoxManual.checked = false;
Nit: this.checkboxManual.checked = type & nsMsgFilterType.Manual;

>+          if (type & nsMsgFilterType.PostPlugin)
>+            this.menulistIncoming.selectedItem = this.menuitemAfterPlugins;
>+          else // type & nsMsgFilterType.Incoming
>+            this.menulistIncoming.selectedItem = this.menuitemBeforePlugins;
Could write this as this.menulistIncoming.selectedItem = type & nsMsgFilterType.PostPlugin ? this.menuitemAfterPlugins : this.menuitemBeforePlugins; which would allow you to write this.checkboxIncoming.checked = type & (nsMsgFilterType.PostPlugin | nsMsgFilterType.Incoming);

>+      /**
>+       * Enable the "before/after classification" menulist depending on
>+       * whether "run when incoming mail" is selected.
>+       */
>+      classificationState: function()
Even doEnabling() would be a better function name :-P

>-  var actionList = filter.actionList;
>-  var numActions = actionList.Count();
>+  let actionList = filter.actionList;
>+  let numActions = actionList.Count();
Must you really do these drive-by style changes?

>+            <menulist id="pluginsRunOrder"
>+                      command="cmd_updateFilterType"
>+                      disabled="true">
Does that actually do anything?

> <!ENTITY contextDesc.label "Apply filter when:">
>+<!ENTITY contextIncomingMail.label "Getting New Mail:">
>+<!ENTITY contextIncomingMail.accesskey "G">
> <!ENTITY contextManual.label "Manually Run">
>+<!ENTITY contextManual.accesskey "R">
>+<!ENTITY contextBeforeCls.label "Filter before Junk Classification">
>+<!ENTITY contextBeforeCls.accesskey "B">
>+<!ENTITY contextAfterCls.label "Filter after Junk Classification">
>+<!ENTITY contextAfterCls.accesskey "J">
I'm not a big fan of this wording but I forgot to look at what the plan is before doing the review...
Comment 48 :aceman 2012-10-05 00:34:07 PDT
(In reply to neil@parkwaycc.co.uk from comment #47)
> Comment on attachment 666388 [details] [diff] [review]
> patch v4
> 
> >+      if (server.rootFolder != server.rootMsgFolder)
> >+        gFilterTypeSelector.disableAfterPlugins();
> So, this is to allow someone to fix their filter to run before plugins if
> they decide to defer their account?
This does not change the current state of affairs. Do you propose anything better? On defered account a filter that is AfterPlugins would probably never run but it is valid. So we leave it as is. Just disable the item so that new filters are not created with it. Do you propose something else?

> >+    let nsMsgFilterType = Components.interfaces.nsMsgFilterType;
> You use this a lot. Make this a global var/const perhaps?
Ok, I'll make it a member of the gFilterTypeSelector.

> >-  var actionList = filter.actionList;
> >-  var numActions = actionList.Count();
> >+  let actionList = filter.actionList;
> >+  let numActions = actionList.Count();
> Must you really do these drive-by style changes?
NO:)

> 
> >+            <menulist id="pluginsRunOrder"
> >+                      command="cmd_updateFilterType"
> >+                      disabled="true">
> Does that actually do anything?
Should set recompute the type of filter when the menu selection changes. What do you mean?

> > <!ENTITY contextDesc.label "Apply filter when:">
> >+<!ENTITY contextIncomingMail.label "Getting New Mail:">
> >+<!ENTITY contextIncomingMail.accesskey "G">
> > <!ENTITY contextManual.label "Manually Run">
> >+<!ENTITY contextManual.accesskey "R">
> >+<!ENTITY contextBeforeCls.label "Filter before Junk Classification">
> >+<!ENTITY contextBeforeCls.accesskey "B">
> >+<!ENTITY contextAfterCls.label "Filter after Junk Classification">
> >+<!ENTITY contextAfterCls.accesskey "J">
> I'm not a big fan of this wording but I forgot to look at what the plan is
> before doing the review...

The plan is to kill the one global menulist and make it checkboxes to allow for many more events. The wording should not be worse than the original state. After much discussion it seems this wording is the best we could come up so far. Any proposals?
Comment 49 neil@parkwaycc.co.uk 2012-10-05 16:48:36 PDT
(In reply to aceman from comment #48)
> (In reply to comment #47)
> > (From update of attachment 666388 [details] [diff] [review])
> > >+    let nsMsgFilterType = Components.interfaces.nsMsgFilterType;
> > You use this a lot. Make this a global var/const perhaps?
> Ok, I'll make it a member of the gFilterTypeSelector.
Actually that's worse than what you've got now, so please don't.

> > >+            <menulist id="pluginsRunOrder"
> > >+                      command="cmd_updateFilterType"
> > >+                      disabled="true">
> > Does that actually do anything?
> What do you mean?
I mean, what is the point of the disabled="true" attribute?
Comment 50 :aceman 2012-10-05 16:57:41 PDT
The menu is initially disabled until a call to .setType enables it as needed.
Comment 51 :aceman 2012-10-06 05:54:10 PDT
So it is basically a safety measure - disable it until we decide what state it should be in. Yes, the state is decided in *onLoad() so there is only a small timeframe. I'll remove it if you think it is superfluous.
Comment 52 :aceman 2012-10-06 06:41:17 PDT
Created attachment 668778 [details] [diff] [review]
patch v5
Comment 53 neil@parkwaycc.co.uk 2012-10-07 12:02:43 PDT
Comment on attachment 668778 [details] [diff] [review]
patch v5

>+function initializeFilterTypeSelector()
>+{
>+  /**
>+   * This object controls code interaction with the widget allowing specifying
>+   * the filter type (event when the filter is run).
>+   */
>+  gFilterTypeSelector = {
>+    checkBoxManual: document.getElementById("runManual"),
>+    checkBoxIncoming : document.getElementById("runIncoming"),
>+
>+    menulistIncoming: document.getElementById("pluginsRunOrder"),
>+
>+    menuitemBeforePlugins: document.getElementById("runBeforePlugins"),
>+    menuitemAfterPlugins: document.getElementById("runAfterPlugins"),
[I wish this could be a global but then you get the problem of when do you get the element references.]

>+      if (this.checkBoxManual.checked) {
>+          type |= nsMsgFilterType.Manual;
>+      }
>+
>+      if (this.checkBoxIncoming.checked) {
>+        if (this.menulistIncoming.selectedItem == this.menuitemAfterPlugins) {
>+            type |= nsMsgFilterType.PostPlugin;
[A couple of lines have slightly unusual indentation.]

>   <separator/>
[This separator seems somewhat useless now.]

>+            <menulist id="pluginsRunOrder"
>+                      command="cmd_updateFilterType">
>+              <menupopup>
>+                <menuitem id="runBeforePlugins"
>+                          label="&contextBeforeCls.label;"
>+                          accesskey="&contextBeforeCls.accesskey;"/>
>+                <menuitem id="runAfterPlugins"
>+                          label="&contextAfterCls.label;"
>+                          accesskey="&contextAfterCls.accesskey;"/>
[It's unusual to have accesskeys in menulist menuitems.]

> <!ENTITY contextDesc.label "Apply filter when:">
[This used to be a <label> but a <caption> generally doesn't end in a colon.]
Comment 54 :aceman 2012-10-07 12:55:50 PDT
Created attachment 668947 [details] [diff] [review]
patch v6

Thanks.
Comment 55 klonos 2012-10-12 10:37:36 PDT
Is this meant to allow triggering/running filters on tag assignment?
Comment 56 Kent James (:rkent) 2012-10-12 11:22:29 PDT
(In reply to klonos from comment #55)
> Is this meant to allow triggering/running filters on tag assignment?

This bug is a preparation for subsequent enhancements that could add more complex filter triggers without the combinatorial explosion of the existing UI. Tag assignment would be an example, though I am not aware of previous discussions of that. On send, on a time schedule, and on archive are the more obvious candidates.
Comment 57 Kent James (:rkent) 2012-10-17 11:50:30 PDT
Comment on attachment 668947 [details] [diff] [review]
patch v6

Review of attachment 668947 [details] [diff] [review]:
-----------------------------------------------------------------

OK this looks good to me with just a couple of minor nits - but if you think I am imposing my personal taste feel free to land it as is.

::: mailnews/base/search/content/FilterEditor.js
@@ +264,5 @@
> +    /**
> +     * Sets the checkboxes to represent the filter type passed in.
> +     *
> +     * @param aType  the filter type to set in terms
> +     *              of Components.interfaces.nsMsgFilterType values.

Nit: alignment off by one

@@ +275,5 @@
> +
> +      this.checkBoxManual.checked = aType & nsMsgFilterType.Manual;
> +
> +      this.checkBoxIncoming.checked = aType & (nsMsgFilterType.PostPlugin |
> +                                              nsMsgFilterType.Incoming);

Nit: align the two nsMsgFilterType lines
Comment 58 :aceman 2012-10-18 09:27:33 PDT
Created attachment 672824 [details] [diff] [review]
patch v7

Thanks.
Comment 59 Ryan VanderMeulen [:RyanVM] 2012-10-19 17:19:43 PDT
https://hg.mozilla.org/comm-central/rev/75f636a2c533

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