440635 - Need a separate manual filter context

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Jeff Beckley]

Attachment: Manual filter context implementation

In Classic Eudora the user has the ability to specify whether a filter is matched against a message at different filtering contexts: incoming, outgoing, and manual.

Bug 359236 is a tracking bug for all of the different filter contexts that Classic Eudora supports, but this one is just for the manual context.

Included is the implementation for the manual filter context by adding checkboxes for Incoming and Manual in the Filter Rules dialog.

Since previous filters didn't distinguish between the two contexts, all filters worked at both times.  The patch includes code to makes sure that legacy filters get the manual context so that they continue to work how they have in the past.  Also, newly created filters get marked for both contexts so that behavior is consistent with previous versions.

The patch updates both Thunderbird and SeaMonkey.

Comment 1

[Jeff Beckley]

Attachment: Screen shot of new UI

Here's a screen shot of what the new UI looks like under Windows so that folks can see it without having to apply the patch.  The "Incoming" and "Manual" checkboxes at the top are the new additions.

Comment 2

[David :Bienvenu]

what's a "manual message"? Is this for the case that you can run filters on already downloaded messages?

Comment 3

[Jeff Beckley]

Yes, it's when manually applying filters to messages, either through the Tools->Run Filters on Folder, or the Run Now button in the Message Filters dialog.

Comment 4

[[e-mail]]

Attachment: Alternate UI possibility

Since we already have enable/disable in the filter list UI, maybe that should be extended to apply to incoming/manual filtering?

Comment 5

[David :Bienvenu]

I think I much prefer Neil's suggestion - Brian?

Comment 6

[Jeff Beckley]

I like Neil's idea as well.  It's good that you can see the contexts in which the filters operate while you manipulate their order.

One thing to consider is that I will also be adding two additional contexts as well: before sending an outgoing message and after.  So there will be 4 columns in addition to the name.  Will that be too much?

Comment 7

[Kent James (:rkent)]

The problem with Neil's suggestion is that it is applied too late to be used to determine the appropriate validity table terms to show in filter definitions.

One of the issues that we face, part of what I call the scope problem, is that different filters can be applied at different scopes - and sometimes we mix those scopes in definitions. For example, junk status cannot be applied to automatic filters, but can be applied to manual filters. So it would be great if the scope was set at the time of definition, as in Jeff's proposal, so that we could use the correct validation table during definition.

To work correctly with the current validity tables, the selections would either need to be mutually exclusive, or we would need to add additional logic (if even possible) to only allow the lowest common denominator of valid scopes that were selected.

Other related validity issues are offline versus online, and pre-junk versus post-junk. This may not be the bug to solve all of those issues, but we should at least consider where we are headed, rather than introducing lots of additional problems by mixing scopes here.

Comment 8

[Jeff Beckley]

Scope is different from context.  Scope is *what* you are operating on (e.g. an offline mail store like Local Folders, an online mail store like IMAP, a local address book, etc.).  Scope is important because it determines which matching criteria is available to use (the fields to match against and comparison operations), hence the use of validity tables.  Context is *when* you are performing the operation (e.g. when mail is being downloaded from the server or when the user manually asks to filter messages).

For filters the validity tables are loaded up when the properties of a specific filter are going to be edited (i.e. right before the Filter Rules dialog is shown), and which validity table is loaded up depends on the account type which the filter getting edited belongs to.  The tables also get loaded up when you choose a different field to filter against because the comparison operators change depending on what field you are searching (e.g. Subject vs Date).

Previous to this patch, the filter editing and storage code had no concept of when a filter would be used because all filters created would be matched both when messages were being downloaded and when messages were being manually filtered against.  This patch adds the ability for a filter to have flags for when the filter will be considered, but what the filter gets filtered against is still determined by the account the filter belongs to.

Comment 9

[Jeff Beckley]

I guess one thing that was left out of the discussion in the this bug is *why* someone would want different filters for incoming vs. manual filtering.

The bug that this is a blocker for, bug 359236, has a couple of users that express why they want manual filtering separate from incoming filtering.  Over the years I've seen some Classic Eudora users with their own particular organizational techniques.  A common usage model I've seen by a number of users is to have incoming filters move messages for mailing lists in to different mailboxes, except when messages addressed directly to the user left in the Inbox to be more visible.  Then they use manual filters once the message has been dealt with.  Especially important to these types of users is the ability to have manual filtering be done as a shortcut key because it is an action they want to perform frequently.  Some users go as far as having all messages go in to their Inbox, and the only type of filtering they do is manual.

Comment 10

[Jeff Beckley]

One thing I just thought of with Neil's UI is that disabling filters is slightly trickier.  To disable a filter you would have to uncheck all of the contexts (just Incoming and Manual for now, but soon will include Before Outgoing and After Outgoing when those contexts are implemented).  To re-enable a filter you would have to remember which contexts the filter was set at previously and then restore those.

We also need to choose the right words for the various contexts.  Classic Eudora used "Incoming", "Outgoing" (it only did after sending, not before), and "Manual".  Those were OK, but if you try to put them in a sentence like I did with this new UI, then it gets awkward.  What about "Checking", "Before Sending", "After Sending", and "Manual Filtering"?  Seems too wordy, though.  Maybe just not putting them in a sentence and living with the ungrammatical: "Incoming", "Pre-Outgoing", "Post-Outgoing", "Manual"? 

Comment 11

[Kent James (:rkent)]

(In reply to comment #8)
> Scope is important because it determines which matching
> criteria is available to use (the fields to match against and comparison
> operations), hence the use of validity tables.  Context is *when* you are
> performing the operation (e.g. when mail is being downloaded from the server or
> when the user manually asks to filter messages).

Junk variables and attachment status are not available in the automatic context, but should be available in the manual context. See these comments at http://mxr.mozilla.org/mozilla/source/mailnews/base/search/src/nsMsgImapSearch.cpp#688:
 
// junk status and attachment status not available for offline mail (POP) filters
// because we won't know those until after the message has been analyzed.
// see bug #185937

Comment 12

[[e-mail]]

Comment on attachment 325923 [details] [diff] [review]
Manual filter context implementation

>-  const long News=0xb;
Hmm, is this constant correct? 0xb = 1011 (binary)

>+    gFilterContextIncoming.checked = true;
>+    gFilterContextManual.checked = true;
You don't need this, simply specify checked="true" in the XUL.

>+    <hbox align="baseline">
I'd prefer align="center".

>+      <checkbox id="incomingContext" style="margin-left: 0px; margin-right: 0px"
>+                label="&incomingContext.label;" accesskey="&incomingContext.accesskey;"/>
>+      <checkbox id="manualContext" style="margin-left: 0px; margin-right: 0px"
>+                label="&manualContext.label;" accesskey="&manualContext.accesskey;"/>
I don't like these inline styles. If you really think adjustments are nececssary then you should put them into the theme's CSS files.

sr=me with the constant double-checked and the rest fixed.

Comment 13

[David :Bienvenu]

Re having four checkbox columns for scope, I'm not sure that Outgoing filters will show up in the same list as incoming/manual filters. Outgoing filters might be associated with an identity or an smtp server. Associating them with an incoming server isn't quite right. I'm not sure what the UI should look like, though. I wonder what Eudora/Outlook do.

Comment 14

[Karsten Düsterloh]

I'd put it the other way round: is our way of associating filters with (incoming) servers - albeit of technical logic - a concept "natural" to users?
I don't think so.
Filters should filter all incoming stuff alike, and I only should think about servers and such if I explicitly want/need to. 
That is, while not attached to specific servers (internally speaking: attached to all), there should be a filter criterion to narrow the scope upon just one (incoming - or even outgoing) server...

filters for everything
+- filters for incoming stuff
   +- filters for stuff incoming on a specific server
+- manual filters 
   +- manual filters for specific accounts
+- filters for outgoing stuff
   +- filters for stuff outgoing over a specific server

Comment 15

[Jeff Beckley]

(In reply to comment #11)
> Junk variables and attachment status are not available in the automatic
> context, but should be available in the manual context. See these comments at
> http://mxr.mozilla.org/mozilla/source/mailnews/base/search/src/nsMsgImapSearch.cpp#688:
> 
> // junk status and attachment status not available for offline mail (POP)
> filters
> // because we won't know those until after the message has been analyzed.
> // see bug #185937

Ah, yes, I see.  The current code just does static LCD functionality, only providing what can be done at message download.

We could implement some kind of dynamic LCD, depending on which contexts you have selected.  It would be tricky, though.  Say a user creates a filter, checks it to be the Manual context only, and creates a match against Junk Status.  Then what should happen if the user selects the Incoming context?  Should the Junk Status match condition just be removed?  What if that was the only matching condition?  Would it be good just to warn the user and leave it?  I could see some usage models wanting to continue doing the check for Manual filtering, but not Incoming filtering.  What if the user then deselects the Manual context?  Should it remove the match condition then?  What if that was an accidental click by the user, and the user reselects the Manual context, should the match condition then reappear?  You could solve some of these issues by not giving warnings on the changing of the context checkboxes, but waiting until the user OKs the dialog.

What I will do is continue this implementation assuming a static LCD approach like the current code already does.  I'll open up a new bug for rethinking this given the dynamic nature that selectable contexts opens up.

Thanks for pointing these out, Kent.

Comment 16

[Jeff Beckley]

(In reply to comment #12)
> (From update of attachment 325923 [details] [diff] [review])
> >-  const long News=0xb;
> Hmm, is this constant correct? 0xb = 1011 (binary)

Ah, that was a bug in the original code.  That should be a combination of NewsRule and NewsJavaScript, which should be 0x04 | 0x08 = 0x0c.  I'll fix that.  It should use the constants themselves, rather than the developer trying to do the math (which, as we see, can be wrong).


> >+    gFilterContextIncoming.checked = true;
> >+    gFilterContextManual.checked = true;
> You don't need this, simply specify checked="true" in the XUL.

Yes, that's simpler and more obvious in the XUL.


> >+    <hbox align="baseline">
> I'd prefer align="center".

I had to use baseline in order to get the text in the checkboxes to align with the text in the labels surrounding the checkboxes.


> >+      <checkbox id="incomingContext" style="margin-left: 0px; margin-right: 0px"
> >+                label="&incomingContext.label;" accesskey="&incomingContext.accesskey;"/>
> >+      <checkbox id="manualContext" style="margin-left: 0px; margin-right: 0px"
> >+                label="&manualContext.label;" accesskey="&manualContext.accesskey;"/>
> I don't like these inline styles. If you really think adjustments are
> nececssary then you should put them into the theme's CSS files.

They were done so that the checkboxes didn't have too much extra space around them.  Given some of the other comments about the UI, this all may be changing.  I'll put it in the CSS if it remains.

 
> sr=me with the constant double-checked and the rest fixed.

Thanks, Neil.

Comment 17

[Jeff Beckley]

(In reply to comment #13)
> Re having four checkbox columns for scope, I'm not sure that Outgoing filters
> will show up in the same list as incoming/manual filters. Outgoing filters
> might be associated with an identity or an smtp server. Associating them with
> an incoming server isn't quite right. I'm not sure what the UI should look
> like, though. I wonder what Eudora/Outlook do.

You're right, David.  Outgoing filters most likely won't co-mingle with incoming/manual filters, given the current structure of filters.  I'll address this in my response to Karsten's comments.

Comment 18

[David :Bienvenu]

Re Karsten's comments - none of my filters are the same in my different accounts. Put an other way, I would need to add a condition to all my filters in all my accounts to check what the incoming account is. It may be the norm that people want the same filters across all accounts, imap and pop3, work and home, etc, but I don't believe that's a foregone conclusion.

Comment 19

[Jeff Beckley]

(In reply to comment #14)
> I'd put it the other way round: is our way of associating filters with
> (incoming) servers - albeit of technical logic - a concept "natural" to users?
> I don't think so.
> Filters should filter all incoming stuff alike, and I only should think about
> servers and such if I explicitly want/need to. 
> That is, while not attached to specific servers (internally speaking: attached
> to all), there should be a filter criterion to narrow the scope upon just one
> (incoming - or even outgoing) server...
> 
> filters for everything
> +- filters for incoming stuff
>    +- filters for stuff incoming on a specific server
> +- manual filters 
>    +- manual filters for specific accounts
> +- filters for outgoing stuff
>    +- filters for stuff outgoing over a specific server

Classic Eudora takes an even simpler model: all filters are in one big list, and on a per-filter basis you can say which contexts the filter applies and as part of the matching conditions you can specify an account ("personality" in Eudora's parlance).  It's a very flexible model, but it does cause some advanced users to have a long list of filters, which makes it somewhat hard to organize.

The structure you give above causes overlap as well, for users who want to do similar filtering across different contexts.  It would be an immediate doubling of filters with TB today, as currently all filters apply both to the incoming and manual contexts.  They would have to be migrated in such a way.  It's not uncommon for Eudora users to have filters that apply to both the incoming and manual contexts.  Somewhat unusual is mixing outgoing with the other 2, but I have seen folks who create mailboxes based on on people, and they want all incoming and outgoing messages from/to a person to go in to a single mailbox

Maybe a solution would be to have all filters exist in one big list, but then have a way to view only the filters for a specific account?  It would be a problem to allow reordering in the account view, so maybe reordering would be disabled there.

How about an All Accounts list in addition to the per-account lists that are there now?  The main problem I see with that is Kent's scope issue: how to deal with differing sets of functionality across different accounts.  It's just news accounts that are drastically different in capabilities, though.  POP3, IMAP, and RSS are all fairly similar since filtering happens locally.  Maybe this new list gets called All Non-News Accounts, or has a comment that news accounts don't use these filters.

Comment 20

[Jeff Beckley]

(In reply to comment #15)
> I'll open up a new bug for rethinking this given the dynamic nature
> that selectable contexts opens up.

That is bug 442165.

Comment 21

[Jeff Beckley]

Attachment: UI Improvements and other fixes

Changed the wording and placement of the context checkboxes.

Corrected some of the problems that Neil suggested:
- Cleaned up the nsMsgFilterType constants so the no longer use hand computed values
- Moved the default checking of context checkboxes in to XUL instead of in JS
- Got rid of the inline styles in the XUL because they're no longer necessary

Continuing sr=Neil, waiting on r=mnyromyr and ui-review=clarkbw.

Comment 22

[Jeff Beckley]

Attachment: New filter context UI

Here's what the UI looks like in the new patch.

Comment 23

[Karsten Düsterloh]

Comment on attachment 327375 [details] [diff] [review]
UI Improvements and other fixes

>Index: mailnews/base/resources//content/mailWindowOverlay.js
>===================================================================
                               ^^
How did you manage that?! This even breaks Bugzilla's ability to show diffs! :-D
(patch doesn't care.)

>Index: mailnews/base/search/public/nsMsgFilterCore.idl
>===================================================================
> typedef long nsMsgFilterTypeType;
> 
>-[scriptable,uuid(e9b548ba-304e-11d3-8b33-00a0c900d445)]
>+[scriptable,uuid(7d368017-e7b4-446c-98ee-e3e256741947)]
> interface nsMsgFilterType {
>     /* these longs are all actually of type nsMsgFilterTypeType */

I wonder what this awkward distinction between nsMsgFilterTypeType and nsMsgFilterType is good for and if that reason does still apply today...


Now some nits: ;-)

>+  const long InboxRule =       0x01;
>+  const long InboxJavaScript = 0x02;
>+  const long Inbox =           InboxRule | InboxJavaScript;
>+  const long NewsRule =        0x04;
>+  const long NewsJavaScript =  0x08;
>+  const long News =            NewsRule | NewsJavaScript;
>+  const long Incoming =        Inbox | News;
>+  const long Manual =          0x10;
>+  const long All =             Incoming | Manual;

We should have  None = 0x00  as well, because you're using that later on as an initialization!
What about RSS? Will this fall under Inbox*?

>Index: mailnews/base/search/resources/content/FilterEditor.js
>===================================================================
>+  if (!gFilterContextIncoming.checked)
>+    gFilter.filterType = 0;

This should be Components.interfaces.nsMsgFilterType.None then.
Furthermore, if one the branches of an if construct needs brackets, the other should have as well, even if  it's just one line. (Yes, I know that the rest of the file is rather untidy. ;-))

>   else
>-    gFilter.filterType = Components.interfaces.nsMsgFilterType.InboxRule;
>+  {

...

>+  if (!gFilter.filterType)
>+    gFilter.enabled = false;

Using
  gFilter.filterType == Components.interfaces.nsMsgFilterType.None 
would be clearer here, too.
 
>Index: mailnews/base/search/resources/content/FilterEditor.xul
>===================================================================
>+    <hbox align="baseline">

"center" would be better.

>+      <label value="&conditionStartDesc.label;" control="booleanAndGroup"/>
>+      <checkbox id="incomingContext" checked="true"
>+                label="&incomingContext.label;" accesskey="&incomingContext.accesskey;"/>
>+      <checkbox id="manualContext" checked="true"
>+                label="&manualContext.label;" accesskey="&manualContext.accesskey;"/>

If an element needs more than one line, put each attribute on its own line, aligned vertically under the first one. Furthermore, the label and the two checkboxes form an accessibility entity and may need aria-labelledby attributes (and I'm not sure if the control attribute value is valid in this very special case). Best to ask one of the a11y guys here.

>Index: suite/locales/en-US/chrome/mailnews/FilterEditor.dtd
>===================================================================
>-<!ENTITY conditionDesc.label "For incoming messages which:">
>+<!ENTITY conditionStartDesc.label "For messages filtered at the following times:">
>+<!ENTITY incomingContext.label "Checking Mail">
>+<!ENTITY incomingContext.accesskey "C">
>+<!ENTITY manualContext.label "Manual">
>+<!ENTITY manualContext.accesskey "u">
>+<!ENTITY incomingContext.label "Incoming">
>+<!ENTITY incomingContext.accesskey "n">
>+<!ENTITY manualContext.label "Manual">
>+<!ENTITY manualContext.accesskey "u">
>+<!ENTITY conditionEndDesc.label "messages which:">

These are bit too many entities, and conditionEndDesc.label isn't even used.

And I, too, am rather uncomfortable with the wording here...

Comment 24

[Jeff Beckley]

(In reply to comment #23)
> (From update of attachment 327375 [details] [diff] [review])
> >Index: mailnews/base/resources//content/mailWindowOverlay.js
> >===================================================================
>                                ^^
> How did you manage that?! This even breaks Bugzilla's ability to show diffs!
> :-D
> (patch doesn't care.)

Not sure how that happened!  I thought maybe passing a trailing slash in the directory parameter to cvs diff might cause that, but I just tested that and I didn't get the double slash.


> >Index: mailnews/base/search/public/nsMsgFilterCore.idl
> >===================================================================
> > typedef long nsMsgFilterTypeType;
> > 
> >-[scriptable,uuid(e9b548ba-304e-11d3-8b33-00a0c900d445)]
> >+[scriptable,uuid(7d368017-e7b4-446c-98ee-e3e256741947)]
> > interface nsMsgFilterType {
> >     /* these longs are all actually of type nsMsgFilterTypeType */
> 
> I wonder what this awkward distinction between nsMsgFilterTypeType and
> nsMsgFilterType is good for and if that reason does still apply today...

Seems to be somewhat of a pattern in search code: <http://mxr.mozilla.org/mozilla/search?string=these+longs&find=\.idl>.


> >+  const long InboxRule =       0x01;
> >+  const long InboxJavaScript = 0x02;
> >+  const long Inbox =           InboxRule | InboxJavaScript;
> >+  const long NewsRule =        0x04;
> >+  const long NewsJavaScript =  0x08;
> >+  const long News =            NewsRule | NewsJavaScript;
> >+  const long Incoming =        Inbox | News;
> >+  const long Manual =          0x10;
> >+  const long All =             Incoming | Manual;
> 
> We should have  None = 0x00  as well, because you're using that later on as an
> initialization!

OK, I'll add the None.  


> What about RSS? Will this fall under Inbox*?

Yes, RSS filters have the Inbox filters run on them

 
> >Index: mailnews/base/search/resources/content/FilterEditor.js
> >===================================================================
> >+  if (!gFilterContextIncoming.checked)
> >+    gFilter.filterType = 0;
> 
> This should be Components.interfaces.nsMsgFilterType.None then.
> Furthermore, if one the branches of an if construct needs brackets, the other
> should have as well, even if  it's just one line. (Yes, I know that the rest of
> the file is rather untidy. ;-))

OK.


> >+  if (!gFilter.filterType)
> >+    gFilter.enabled = false;
> 
> Using
>   gFilter.filterType == Components.interfaces.nsMsgFilterType.None 
> would be clearer here, too.

Right, will do.


> >Index: mailnews/base/search/resources/content/FilterEditor.xul
> >===================================================================
> >+    <hbox align="baseline">
> 
> "center" would be better.

I addressed this in comment 16: baseline is needed to align the text properly.


> >+      <label value="&conditionStartDesc.label;" control="booleanAndGroup"/>
> >+      <checkbox id="incomingContext" checked="true"
> >+                label="&incomingContext.label;" accesskey="&incomingContext.accesskey;"/>
> >+      <checkbox id="manualContext" checked="true"
> >+                label="&manualContext.label;" accesskey="&manualContext.accesskey;"/>
> 
> If an element needs more than one line, put each attribute on its own line,
> aligned vertically under the first one.

OK.


> Furthermore, the label and the two
> checkboxes form an accessibility entity and may need aria-labelledby attributes
> (and I'm not sure if the control attribute value is valid in this very special
> case). Best to ask one of the a11y guys here.

In looking at <http://developer.mozilla.org/en/docs/index.php?title=XUL_accessibility_guidelines#Form_labels> and some Firefox examples, there should be a group around the checkboxes (not necessarily a visible one) and then the label should use a control attribute to refer to the group.  I'll make that change.


> >Index: suite/locales/en-US/chrome/mailnews/FilterEditor.dtd
> >===================================================================
> >-<!ENTITY conditionDesc.label "For incoming messages which:">
> >+<!ENTITY conditionStartDesc.label "For messages filtered at the following times:">
> >+<!ENTITY incomingContext.label "Checking Mail">
> >+<!ENTITY incomingContext.accesskey "C">
> >+<!ENTITY manualContext.label "Manual">
> >+<!ENTITY manualContext.accesskey "u">
> >+<!ENTITY incomingContext.label "Incoming">
> >+<!ENTITY incomingContext.accesskey "n">
> >+<!ENTITY manualContext.label "Manual">
> >+<!ENTITY manualContext.accesskey "u">
> >+<!ENTITY conditionEndDesc.label "messages which:">
> 
> These are bit too many entities, and conditionEndDesc.label isn't even used.

Ah, some old entities from my first approach got left in.  I'll fix that up.


> And I, too, am rather uncomfortable with the wording here...

Right.  I'm waiting to hear from clarkbw on this.


Thanks very much for the review, Karsten.

Comment 25

[Jeff Beckley]

Attachment: Changes addressing Karsten's comments

Fixes based on Karsten's review comments:
- Added "None" to nsMsgFilterType
- Compare using nsMsgFilterType.None instead of 0
- Extra braces around if branch to match else branch
- One XUL attribute per line, lined up with previous attribute
- Put the context checkboxes in a group and refer to that group in the preceding label

Also added some code in nsMsgFilter to default filters as being marked as applying in the Incoming and Manual contexts by default (which would only really matter when a filter was being created programmatically, as the UI already handles the correct context defaults), and removed some code that was no longer being used.

Comment 26

[Karsten Düsterloh]

Comment on attachment 329351 [details] [diff] [review]
Changes addressing Karsten's comments

>Index: mailnews/base/search/public/nsMsgFilterCore.idl
>===================================================================
>+  const long None =            0x00;
>+  const long InboxRule =       0x01;
>+  const long InboxJavaScript = 0x02;
>+  const long Inbox =           InboxRule | InboxJavaScript;
>+  const long NewsRule =        0x04;
>+  const long NewsJavaScript =  0x08;
>+  const long News =            NewsRule | NewsJavaScript;
>+  const long Incoming =        Inbox | News;
>+  const long Manual =          0x10;
>+  const long All =             Incoming | Manual;

Please align the =.

r=me, with that and given we find a more satisfying wording.


How about:
"When filtering messages by [x] checking mail [x] manual filters..."

And I come to think that forcing "twolineness" on the text is doomed to fail anway when we get more than two options here...

Comment 27

[Jeff Beckley]

After discussion in #maildev about the UI, what we finally came up with (many thanks to Bryan) is this combobox:

Apply filter when: [ Checking Mail                | v ]
                   | Manually Run                     |
                   | Checking Mail or Manually Run    |
                   +----------------------------------+

I'll make a new patch with this UI, and get Karsten's = alignment as well.

Comment 28

[Karsten Düsterloh]

(In reply to comment #6)
> I will also be adding two additional contexts as well:
> before sending an outgoing message and after.

(In reply to comment #27)
> Apply filter when: [ Checking Mail                | v ]
>                    | Manually Run                     |
>                    | Checking Mail or Manually Run    |

This will get rather messy once we'll have 4 contexts = 15 combinations...

Comment 29

[Bryan Clark (DevTools PM) [:clarkbw]]

Things aren't any better with checkboxes though.  From a widget point of view they seem simple.  But when you look at how we have to present them to the user and how you have to change them to get what you want it gets messy fast.  

Example: If we only enable 2 of the 4 contexts (incoming and manual) but a person wants one of the other two contexts they have to uncheck 2 contexts that are defaulted to and check the one they want.  At the same time anyone who wants to create a filter with our default context options (the assumed majority of people) still has to read all the options available and decide that the checked / unchecked selections are correct for every filter.

Comment 30

[Karsten Düsterloh]

> Things aren't any better with checkboxes though.

Sure. I just didn't know if you had taken Jeff's planned extensions into account.

>  From a widget point of view they seem simple.

Well, you could symbolize the situation here with a square, which has the four contexts as the corners. If you draw circles around the corners with the edge as the radius, you'll get the square devided into areas of influence, which will symbolize 9 of the 15 combinations (subsquares in the abstraction below). Including the corners, you'll get 13 combinations with one click and all combinations with just two clicks.

A                B
 #----+----+----#
 | AB | AB | AB |
 | C  |    |  D |
 +----+----+----+
 | A  | AB |  B |
 | C  | CD |  D |
 +----+----+----+
 | A  |    |  B |
 | CD | CD | CD |
 #----+----+----#
C                D

I somehow doubt that widget would qualify as "simple", though. ;-)

Comment 31

[Jeff Beckley]

Attachment: New UI with combobox

Here's the new UI as discussed in comment 27.  I also put in the = alignment Kartsen mentioned.

Bryan, does this look like what we talked about? (see attached screen shot)

Continuing r=mnyromyr, sr=neil.

Comment 32

[Jeff Beckley]

Attachment: New screen shot with combobox UI

Comment 33

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 332044 [details] [diff] [review]
New UI with combobox

Yep, that looks good.

Comment 34

[Mark Banner (:standard8)]

Checked in changeset id: 62:0b9574280e22

Comment 35

[Mark Banner (:standard8)]

Attachment: SpamAssassin.sfd diffs

I was playing around with searches and Filters on SeaMonkey/Thunderbird today, I noticed that hg was saying that SpamAssassin.sfd had changed although I hadn't touched it.

These are the changes it is showing up. Are these correct? We should probably make similar changes to the SpamCatcher.sfd and SpamPal.sfd files as well.

Comment 36

[Mark Banner (:standard8)]

Attachment: Fix spam files

This is a follow up to sync our Spam*.sfd versions with the current version in the app. This will stop the files being regenerated (and hence causing diffs on platforms that use symlinks) when going into the account manager junk settings pane.

Comment 37

[David :Bienvenu]

Comment on attachment 333907 [details] [diff] [review]
Fix spam files

why does it think the condition line changed? A line ending?

Comment 38

[David :Bienvenu]

oh, sorry, x-spam-flag vs x-Spam-Flag - is that intentional?

Comment 39

[Mark Banner (:standard8)]

(In reply to comment #38)
> oh, sorry, x-spam-flag vs x-Spam-Flag - is that intentional?
> 
Checked in without that change (not intentional), changeset id 105:d94f4dfc4ce0.

I believe this bug is now fixed.