294094 - Redesign / Simplify the Filter Rule Dialog

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Scott MacGregor]

Currently, the filter actions portion of the filter rule dialog is a bit
cumbersome. It currently consists of a listbox that contains 10 action items
(soon to be 12 when reply and forward are both added).

Instead of showing all of these items, I'd like to take play out of the Mail.app
handbook which has a much nicer way of presenting all of this information.

1) Start with just a single row in the listbox instead of all 10 action items.
This initial row consists of:

[ Combo Box Filled With Action Items]                      [+][-]

where the + / - buttons add and remove action rows from the list box just like
they now do for the search criteria.

2) The Combo Box lists the available actions for the filter and might be
organized like this:

Move Message
Copy Message
--------------------
Forward Message
Reply to Message
--------------------
Mark As Read
Mark As Flagged
Label Message As
Set Priority To
--------------------
Delete Message
Delete From POP Server (hidden for non POP servers)
Fetch Body From POP Server (hidden for non POP servers)

3) When an action item gets selected inside a combo box, it then shows up as
disabled in other combo boxes for other action rows the user may have added
using the + buttons. In other words, once an action is set up, we disable it in
the menu lists so future actions can't be configured to use the same action.

4) When an action item is chosen, we should dynamically load an XBL widget
appropriate for that action which would contain the 'extra' items that action
may need in that row. for instance, if I chose 'Copy Message', we should then
insert a folder picker combo box after the Action combo box and before the +/-
buttons so the row would look like

[ Copy Message ] to [ Folder Picker Combo Box ]     [+] [-]

Comment 1

[Scott MacGregor]

I'd love to get to this for 1.1. Maybe someone will be interested and jump in. 

Comment 2

[Scott MacGregor]

Attachment: work in progress

just saving off some of the work for this feature.

The widget is coming along nicely.

Comment 3

[Scott MacGregor]

Attachment: screen shot

I've now got the new filter widget properly creating the correct set of filter
actions for a given filter and it can now save them out too. Still needs lots
of polish work before its done but it is now functional.

Comment 4

[[e-mail]]

That's not the best use of XBL I've seen... may I suggest some tweaks:
XUL:
<listcols>
  <listcol/>
  <listcol flex="1"/>
  <listcol/>
  <listcol/>
</listcols>
<listitem class="filteractionrow" value="movemessage"/>
CSS:
.filteractionrow { -moz-binding: url(...#filteractionrow); }
listcell[type="movemessage"] { -moz-binding: url(...#movemessage); }
XBL:
<binding id="filteractionrow">
  <content allowevents="true">
    <xul:listcell class="filteractionmenu" xbl:inherits="value"/>
    <xul:listcell xbl:inherits="type=value"/>
    <xul:listcell>
      <xul:button class="small-button" label="+"
onclick="this.parentNode.parentNode.addRow();"/>
      <xul:button class="small-button" label="-"
onclick="this.parentNode.parentNode.removeRow();"/>
    </xul:listcell>
    <xul:listcell/>
  </content>
</binding>
The main ideas here are a) to avoid rebinding the row every time the selection
changes b) to specify nodes on which functions are defined rather than relying
on backward-compatible scope rules (which are subject to change... if and when
someone audits the codebase...) c) not to rebind elements which take focus.

Comment 5

[Scott MacGregor]

Attachment: updated snapshot, still a work in progress

I moved to the XBL model Neil suggested in an earlier comment (thanks Neil!).

I'm still having problems when opening an existing filter where we dynamically
create several action rows and then try to fill them in. The content
(methods/properties/etc.) for the action row doesn't exist after I create and
append the node to the DOM. Instead I have to fire a timer to let the stack
unroll and then I can start initializing the widget. I'm working around this
right now by firing a timer to set the value on the filteraction, and then
another timer after that to actually fill in the inner menu list item after it
has been bound to the document. Still need to fix that instead of hacking
around it. But you get the general idea....

Comment 6

[[e-mail]]

Comment on attachment 184727 [details] [diff] [review]
updated snapshot, still a work in progress

Only had time to glance at this so far...

>+*   content/messenger/searchWidgets.xml             (content/searchWidgets.xml)
Any chance this can live next to FilterEditor.xul?

>-    content/messenger/FilterEditor.xul              (/mailnews/base/search/resources/content/FilterEditor.xul)
>+*   content/messenger/FilterEditor.xul              (/mailnews/base/search/resources/content/FilterEditor.xul)
Typo?

Comment 7

[[e-mail]]

Comment on attachment 184727 [details] [diff] [review]
updated snapshot, still a work in progress

>+listcell[type="movemessage"] { 
>+  -moz-binding: url("chrome://messenger/content/searchWidgets.xml#filteraction-movefolder");
>+}
>+
>+listcell[type="copymessage"] { 
>+  -moz-binding: url("chrome://messenger/content/searchWidgets.xml#filteraction-movefolder");
>+}
>+
>+menulist[type="filteractiontarget"] {
>+  -moz-binding: url("chrome://messenger/content/searchWidgets.xml#filteractiontarget");
>+}
Actually my idea here was to rename #filteractiontarget to replace
#filteraction-movefolder in the XBL (or you could rename
#filteraction-movefolder to #filteractiontarget in the CSS i.e.
>listcell[type="movemessage"] ,
>listcell[type="copymessage"] {
>  -moz-binding: url("chrome://messenger/content/searchWidgets.xml#filteractiontarget");
>}
)

>+function lazyListBuild2(aActionIndex)
>+{
>+  var filterAction = gFilter.actionList.QueryElementAt(aActionIndex, Components.interfaces.nsIMsgRuleAction);
>+  var listItem = gFilterActionList.getItemAtIndex(aActionIndex);
>+  listItem.initWithAction(filterAction);
> }
The idea here would be to set some attribute on the listrow that identified the
action, then the constructor for the listitem would call initWithAction where
necessary.

Either way, there's a potential issue with listboxes containing large numbers
of items added by script: the XBL is only created when the item is made
visible.

>+function convertFilterActionToString(aFilterAction)
>+function convertStringToFilterAction(aFilterActionString)
Since filter actions are integers it might be simpler to have an array of
strings gFilterActionStrings = [, "movemessage", "setpriorityto" ... ]; and
index that (and use the new gFilterActionStrings.indexOf for the reverse).

Comment 8

[Scott MacGregor]

Attachment: updated patch (still a work in progress, not complete!)

saving off some more work before I go on vacation.

Comment 9

[Scott MacGregor]

Attachment: updated patch

Still not done yet but getting closer to finishing up the loose ends.

Added support for Reply With Template and Forward Message (based on the work
David recently landed)

Comment 10

[Scott MacGregor]

Attachment: 'feature complete' patch

Added the last set of remaining features that I wanted to do before moving this
to the review stage:

1) We now walk through the filter actions, validating them (and propmting if
necessary) before saving the filter.

2) We now disable the remove button when there is only one row left in the
filter action list

3) The actions list box is now fixed at 4 rows with over flow scrollbars if you
add more than 4 actions. 

4)I added some filler padding to the right of the +/- buttons to account for
the scrollbar when it shows up. I'm still messing with that.

Comment 11

[Scott MacGregor]

Attachment: updated screen shot

Comment 12

[Scott MacGregor]

"The idea here would be to set some attribute on the listrow that identified the
action, then the constructor for the listitem would call initWithAction where
necessary."

Neil, when I tried to mess around with something like that idea before, I found
that I couldn't attach a filter action to the list row because the list row
hadn't fully gotten bound to the listbox yet. If I set a field for a field
action on my list row after I dynamically created the row, it wasn't really
getting set on the listbox. When the ctor for the child listcell was invoked,
the parents defaultFilterAction field was initialized to null. The same code to
initialize the field when set on a timer worked fine (like the existing
lazybuild methods).

Comment 13

[[e-mail]]

Perhaps you can set an attribute on the listrow that's an index to an array of
actions, then the constructors can figure the rest out from that?

Comment 14

[Scott MacGregor]

Attachment: updated patch

1) I'm not setting a field on the list row (mInitialActionIndex) which a rule
action target widget can later use so it can properly initialize itself. This
eliminates all of the timeout / lazybuild methods in the old patch. This works
fine for all of the rule action target widgets with the exception of a folder
picker. I still need a timeout for that because the RDF content for the menu
list isn't getting built at the time that the xbl binding is getting
constructed. Hence I'm still using:

setTimeout(function(aPicker, aFilterAction) {
aPicker.pickFolder(aFilterAction); }, 0, 
			   actionTarget, aFilterAction.targetFolderUri); 
inside initWithAction

2) I changed the naming scheme I was using for these new widgets. We now have:

ruleaction --> binding to a list row that encapsulates the entire rule action
ruleactiontype --> the first cell in a rule action, the action type (forward,
reply, etc.)
ruleactiontarget-base --> shared base class widget for all rule action targets.

Each rule action target (label list, priority list, folder list, etc) has its
own specific ruleactiontarget-**** binding.

Current problems:
1) The aforementioned timeout to initialize the folder picker (I haven't found
a way around that yet).

2) When scrolling an element into view (that was not yet visible), the
constructor for the rule action target is not seeing that the list row has a
default action index associated with it. This is a big problem because it means
if you have more than 4 actions (the default # of visible rows) then we don't
show you the right filter actions for rows that aren't visible at construction
time. I still need to fix this.

3) Once the review process gets further alone, I'll add in the DTD and jar.mn
changes for the suite. I just don't want to be changing things in two places
until most of the initial review comments get flushed out. 

I'm going to ask Neil to start reviewing this patch to get the ball rolling
(he's already been giving me good pointers), even though I still have 3 issues
to address above because I'm sure we'll have some more changes to make based on
comments.

Comment 15

[Scott MacGregor]

"1) I'm not setting a field"

should be 
1) "I'm now setting a field.."

Comment 16

[Scott MacGregor]

using an attribute instead of a field for the initial action index fixed the
problem with action rules which get constructed by scrolling them into view. 

Comment 17

[Scott MacGregor]

(In reply to comment #16)
> using an attribute instead of a field for the initial action index fixed the
> problem with action rules which get constructed by scrolling them into view. 

Actually I was mistaken. Switching to a property instead of a field still has
the problem.

Comment 18

[Scott MacGregor]

(In reply to comment #14)
> 
> 2) When scrolling an element into view (that was not yet visible), the
> constructor for the rule action target is not seeing that the list row has a
> default action index associated with it. This is a big problem because it means
> if you have more than 4 actions (the default # of visible rows) then we don't
> show you the right filter actions for rows that aren't visible at construction
> time. I still need to fix this.

I think the problem is that when you scroll a new row into view, I'm seeing the
rule target get constructed twice. i.e. the ctor for ruleactiontarget-base gets
called twice on the same row. I'm not sure why that's happening. 

Comment 19

[Scott MacGregor]

Comment on attachment 186148 [details] [diff] [review]
updated patch

canceling the review request. I've got a better patch that fixes the remaining
issues I was having.

Comment 20

[Scott MacGregor]

Attachment: updated patch

This version of the patch fixes the problems I was having with scrolling an
existing filter rule row into view for the first time...(see comment 14, issue
#2)

I also made the add button smart enough to add the new filter action row after
the current row, and to scroll it into view.

I'm still using a timeout to set the target folder on the folder picker when
creating an existing filter rule (see comment 14 item #1)

Comment 21

[Scott MacGregor]

I meant to clarify that I fixed the problem with scrolling an existing action
into view for the first time by:

Making both the action type menu list and the action target element both report
back into the parent after they've been successfully constructed and initialized
for the first time. The list item row XBL then clears the initial action index
after both of these child widgets have finished constructing themselves. The
problem was happening because when the search row is initially visible, the rule
action type menu list gets built first, but when you are scrolling a new row
into view, the rule target action was getting built first. We need to make sure
we don't clear our initial action index until both have been created and bound
to the list item row.

Comment 22

[[e-mail]]

Some JS console output that this patch spews, may be hiding real errors:
Warning: assignment to undeclared variable elements
Source File: chrome://messenger/content/searchWidgets.xml
Line: 87
Error: document.getAnonymousNodes(actionTarget) has no properties
Source File: chrome://messenger/content/searchWidgets.xml
Line: 219
Warning: reference to undefined property menuEl.localName
Source File: chrome://messenger/content/searchWidgets.xml
Line: 404
Warning: redeclaration of var listItem
Source File: chrome://messenger/content/FilterEditor.js
Line: 337, Column: 8
Source Code:
    var listItem = gFilterActionList.getItemAtIndex(index);

Comment 23

[[e-mail]]

Comment on attachment 188613 [details] [diff] [review]
updated patch

Comments so far:

>+listcell[type="markasread"], listcell[type="deletemessage"], 
>+listcell[type="ignorethread"], listcell[type="watchthread"], listcell[type="markasflagged"],
>+listcell[type="deletefrompopserver"], listcell[type="leaveonpopserver"], listcell[type="fetchfrompopserver"] {
Use .ruleactiontarget here and .ruleactiontarget[type=...] below (snipped).

> <!ENTITY markFlagged.label "Flag the message">
> <!ENTITY label.label "Label the message:">
> <!ENTITY killThread.label "Ignore thread">
>-<!ENTITY watchThread.label "Watch thread">
> <!ENTITY deleteFromServer.label "Delete from POP server">
> <!ENTITY fetchBodyFromServer.label "Fetch body from POP server">
>-<!ENTITY setJunkScore.label "Set Junk Status to:">
>-<!ENTITY forwardTo.label "Forward to:">
>-<!ENTITY replyWithTemplate.label "Reply with template:">
> <!ENTITY filterName.label "Filter name:">
> <!ENTITY filterName.accesskey "i">
Nit: aren't some these other entities removable too?

>Index: mailnews/base/search/resources/content/searchWidgets.xml
Reviewing the new file first, it's easier ;-)

>+<?xml version="1.0"?>
>+
>+# ***** BEGIN LICENSE BLOCK *****
XML-friendly version please, either the original XML licence block or
bsmedberg's #ifdef version.

>+      <xul:menulist flex="1" onpopupshowing="buildActionPopupList();" onpopuphidden="changedMenuListValue()">
I think you need to use oncommand instead of onpopuphidden here. In fact, I
think you could probably use XBL handlers instead of methods. Also, the only
reason that buildActionPopupList() works in anonymous content was that jst
didn't want to trawl through the codebase fixing print preview and other
bindings which abused the system, so he special-cased XUL to do a slow search
of functions. I see you got it right further down though.

>+      <field name="menupopup">document.getAnonymousNodes(this)[0].menupopup</field>
I feel that an additional field for the menulist would improve readability. Or
in certain cases more local variables would be suitable, e.g. for mListBox
below.

>+            elements = this.menupopup.getElementsByAttribute("enablefornews","true");
>+            for (i=0; i < elements.length; i++) 
Some spacing nits: space before comma; space around equals; no trailing spaces.

>+              elements[i].collapsed = scope != Components.interfaces.nsMsgSearchScope.newsFilter;
I think you'll need to use .hidden instead of .collapsed here (and elsewhere).

>+            var menuItems = this.menupopup.getElementsByTagName('menuitem');
>+            for (var index in menuItems)
This would be nice if it worked, and even nicer if "for each (var menu in
menuItems)" worked, but it doesn't, because for some reason the length, item
and namedItem properties are also enumerated. So it's back to a 0 to length
loop :-(

>+              menu.disabled = menu.value in unavailableActions;
For historical reasons, menus don't have a disabled property, so we're stuck
with the attribute...

>+      <property name="numVisibleActions" readonly="true">
I'm tempted to suggest you make this a countNumVisibleActions() method. Same
goes for getUserActionsList() below.

>+            var numVisibleActions = 0;;
Nit: doubled ;

>+      <!-- returns an array containing all of the filter actions which are currently being used by other filteractionrows -->
Nit: it's not an array, it's a hash

>+            // now iterate over each list item in the list box
>+            var index = 0;
>+            while (index < listBox.getRowCount())
Nit: why not a for loop?

>+        <xul:button class="small-button" label="+" onclick="this.parentNode.parentNode.addRow();"/>
>+        <xul:button class="small-button" label="-" onclick="this.parentNode.parentNode.removeRow();" anonid="removeButton"/>
I've always wondered whether this should have a unicode minus sign, or even
custom images instead.

>+      <field name="mRuleActionTargetInitialized">false</field>
This never seems to get set anywhere. I'd like to think that's because it's
unnecessary, and all the dependent code can get removed ;-)

>+                // we can't seem to call pickFolder directly here because the xul template for that menu list
>+                // hasn't populated the list yet! XXX: temporary hack to get things going. 
I wonder if the xbl picker would resolve this issue.

>+            if (!isValid && errorString && gPromptService)
Nit: errorString is only set if !isValid, so you could do eliminate isValid.

>+              case "forwardmessage":
>+                filterAction.strValue = document.getAnonymousNodes(actionTarget)[0].value; 
>+                break;
>+              case "replytomessage":
>+                filterAction.strValue = document.getAnonymousNodes(actionTarget)[0].getAttribute("value");  
>+                break;
Does .value not work in this case? If so you could merge it with the previous
case. I should file a followup bug to merge targetFolderUri with strValue for
further simplification.

>+              listItem.setAttribute('class', 'ruleaction');
Nit: you can use .className here.

>+              var before;
>+              var index = this.mListBox.getIndexOfItem(this) + 1;
>+              if (index < this.mListBox.getRowCount())
>+                before = this.mListBox.getItemAtIndex(index);
>+              
>+              if (before)
>+                this.mListBox.insertBefore(listItem, before);
>+              else
>+                this.mListBox.appendChild(listItem, this);
this.mListBox.insertBefore(listItem, this.nextSibling); should work, as if the
nextSibling is null insertBefore is equivalent to appendChild (which btw has
only one parameter) although in fact we implement appendChild in terms of
insertBefore.

>+      <xul:menulist style="min-width: 15em;">
As all of the filter widgets (imho including the action list) need a min-width
of 15em perhaps a class that we can style would be more appropriate?

>+            var menuItems = document.getAnonymousNodes(this)[0].menupopup.getElementsByTagName('menuitem');
Nit: As we know the elements are all menuitems we can just use .childNodes
here.

>+            for (index in menuItems)
Again, this needs a length loop, sigh :-(

>+            // propogating a pre-existing hack to make the label get displayed correctly in the menulist
Spelling nit: propagating (I kept writing event.stopPropogation() and wondering
why it didn't work, so learned to spell it!)

>+            // now that we've changed the labels for each menu list. We need to use the current selectedIndex
>+            // (if its defined) to handle the case where we were initialized with a filter action already.
>+            var currentIndex = document.getAnonymousNodes(this)[0].selectedIndex || 0;
>+            document.getAnonymousNodes(this)[0].selectedIndex = currentIndex ? 0 : 1;
>+            document.getAnonymousNodes(this)[0].selectedIndex = currentIndex;
Setting selectedItem to null is a slightly neater hack. I was wondering what
the || 0 was for, though.

>+                var header = enumerator.getNext();
>+                if (header instanceof Components.interfaces.nsIMsgDBHdr)
>+                {
>+                  var msgHdr = header.QueryInterface(Components.interfaces.nsIMsgDBHdr);
The instanceof is supposed to make the QueryInterface unnecessary, although I
hear rumors that this is busted on trunk :-(

>+                                  oncommand="pickFolder(event.target.parentNode.parentNode.getAttribute('id'))"/>
Again, you should use an XBL command handler, although you would have to add
some logic to find the node with the id. Of course this is all irrelevant if
you manage to get the xbl picker working.

Comment 24

[[e-mail]]

Comment on attachment 188613 [details] [diff] [review]
updated patch

> var gPrefBranch;
This can go too, it was only used for labels, and that's now in the XBL.

>+    newActionRow.setAttribute('class', 'ruleaction');
Nit: you can use .className here, and again lower down.

>+    gFilterActionList.appendChild(newActionRow);
> 
>+    listItem = gFilterActionList.getItemAtIndex(actionIndex);
Doesn't this retrieve the new action row you just appended?

>     initializeSearchRows(scope, filter.searchTerms);
>+  updateRemoveRowButton();
Unnecessary as initializeSearchRows already calls it.

>+      gPromptService.alert(window,gFilterBundle.getString("cannotHaveDuplicateFilterTitle"),
Nit: space after comma.

What happened to the "New Folder..." buttons? Just hadn't had time to convert
them yet?

As the UK's Catchphrase catch phrase goes, "It's good, but it's not right." ;-)

Comment 25

[Scott MacGregor]

Attachment: take 2

I think I've addressed all of Neil's review comments.

This patch also includes work to convert to the new folder picker widget (which
allowed me to get rid of the last setTimeout hack in the earlier patch as
well).

Notes:

1) Keyboard navigation inside the menulist with the popup closed allows you to
select disabled items (Bug 302060)

2) Keyboard navigation inside the new folder picker menulist when the popup is
closed is also not working

3) I've included the corresponding seamonkey changes (dtd and CSS)

4) I'm still having problems using just .ruleactiontarget for the base class
declaration for all the generic action targets in messenger.css for some reason
things get very broken if I don't list each type separately. Still working on
it.

5) I created a new XBL binding in mailWidgets.xml (well I hijacked an unused
one called locationPopup that was not hooked up) to represent a menu list for
folder s that can have messages copied into them. The template builder syntax
is not quite right yet as it still lists news servers (just the server not the
actual folder) because of this rule:
<rule nc:CanFileMessages="false" nc:CanSearchMessages="true" iscontainer="true"
isempty="false">

For some reason the tree gets very unhappy if I have an empty rule at the
beginning for skipping these nodes. I'm still playing around with that. 

6) I was hoping to try this out for a little while without the buttons for new
folder as I thought it cluttered things up. I wanted to see how many people
notice first before adding it back.

Comment 26

[[e-mail]]

(In reply to comment #25)
> 3) I've included the corresponding seamonkey changes (dtd and CSS)
Not quite... the jar.mn and messenger.css changes were missing.
Speaking of jar.mn, it would be nice if you could make the list of dependencies
in searchWidgets.xml an XML comment so that a) it wouldn't annoy my XML editor
and b) it wouldn't need to be preprocessed for suite.

>4) I'm still having problems using just .ruleactiontarget for the base class
>declaration for all the generic action targets in messenger.css for some reason
>things get very broken if I don't list each type separately. Still working on
>it.
WFM - I just commented out the rest of the selectors as a test.

>5) I created a new XBL binding in mailWidgets.xml (well I hijacked an unused
>one called locationPopup that was not hooked up) to represent a menu list for
Please don't hijack it, that's for bug 21344 eventually. I probably need to
update its patch for bitrot though.

>folder s that can have messages copied into them. The template builder syntax
>is not quite right yet as it still lists news servers (just the server not the
>actual folder) because of this rule:
><rule nc:CanFileMessages="false" nc:CanSearchMessages="true" iscontainer="true"
>isempty="false">
>
>For some reason the tree gets very unhappy if I have an empty rule at the
>beginning for skipping these nodes. I'm still playing around with that. 
Maybe if you add nc:CanFileMessageOnServer="true" to all of the other conditions
instead? Also, you shouldn't need iscontainer and/or isempty as the tree builder
should take care of that.

Comment 27

[Scott MacGregor]

(In reply to comment #26)
> (In reply to comment #25)
> >4) I'm still having problems using just .ruleactiontarget for the base class
> >declaration for all the generic action targets in messenger.css for some reason
> >things get very broken if I don't list each type separately. Still working on
> >it.
> WFM - I just commented out the rest of the selectors as a test.

Interesting, I wonder why that is. When I comment out the other selectors for
.ruleactiontarget, I can create new filter actions just fine, but I am unable to
edit an existing filter and see the actions get built correctly. I get all sorts
of errors because the xbl content is not getting generated to properly reflect
the right widgett anymore. Does it still work for you when you are editing an
existing filter with a bunch of actions?

Comment 28

[Scott MacGregor]

Attachment: udpated with the latest review comments

1) Added seamonkey jar.mn and messenger.css changes
2) Used XML comments so we don't have to run the pre-processor on
searchWidgets.xml
3) Fixed tree template builder rules so news servers don't get listed
4) Restored locationpopup to mailWidgets.xml
5) I did not include the ruleactiontarget CSS optimization because that still
breaks things badly for me when editing filters.

The only odd behavior I see is with the new folder menu picker. If the popup
happens to come up above the list box (instead of opening below it), I'm
finding that I can't actually use the mouse to select a folder. If I move the
dialog closer to the top of my screen so the popup is forced to open below the
list box then it works fine.

Comment 29

[Scott MacGregor]

Comment on attachment 190434 [details] [diff] [review]
take 2

clearing a now obsolete review request

Comment 30

[[e-mail]]

Comment on attachment 190913 [details] [diff] [review]
udpated with the latest review comments

>+          <rule nc:CanFileMessagesOnServer="true" nc:CanFileMessages="true" nc:Virtual="false">
My virtual folder was still showing up so this probably needs tweaking
slightly.

>+.ruleactiontarget[type="markasread"], .ruleactiontarget[type="deletemessage"], 
>+.ruleactiontarget[type="ignorethread"], .ruleactiontarget[type="watchthread"], .ruleactiontarget[type="markasflagged"],
>+.ruleactiontarget[type="deletefrompopserver"], .ruleactiontarget[type="leaveonpopserver"], .ruleactiontarget[type="fetchfrompopserver"] {
Interestingly .ruleactiontarget[type] works, I suspect this could do with a
follow-up bug, also one for the weirdness with the dropdowns which I also
noticed.

>-function SetFolderPicker(uri,pickerID)
>+function SetFolderPickerWithEl(uri, picker)
I would have been tempted to call it SetFolderPickerElement.

>+    var listItem = gFilterActionList.getItemAtIndex(index);
>+    if (!listItem.validateAction())
...
>+    var listItem = gFilterActionList.getItemAtIndex(index);
>+    listItem.saveToFilter(gFilter);
Technically a redeclaration of var listItem (although these warnings were
disabled on the trunk).

>+  This file has the following external dependencies:
>+      -gFilterActionStrings from FilterEditor.js
>+      -gFilterList from FilterEditor.js
>+      -gPromptService from FilterEditor.js
>+      -gMessengerBundle from msgFolderPickerOverlay.js
>+      -GetMsgFolderFromUri() from msgFolderPickerOverlay.js 
SetFolderPickerElement too ;-)

>+.ruleactionitem {
>+  min-width: 13em;
>+}
This isn't quite wide enough for "Delete from POP3 server" - the layout jiggles
when you select this action. Maybe keep the 15em from the original patch?

Comment 31

[Scott MacGregor]

Attachment: [patch checked in]updated patch with the last set of comments

carrying forward Neil's review. Thanks a lot Neil.

Comment 32

[David :Bienvenu]

Comment on attachment 191230 [details] [diff] [review]
[patch checked in]updated patch with the last set of comments

this looks excellent

+	   // now that we've changed the labels for each menu list. We need to
use the current selectedIndex
+	   // (if its defined) to handle the case where we were initialized
with a filter action already.

should be "if it's defined..."

Comment 33

[chris hofmann]

Comment on attachment 191230 [details] [diff] [review]
[patch checked in]updated patch with the last set of comments

a=chofmann

Comment 34

[Scott MacGregor]

woo hoo

Comment 35

[Mike Cowperthwaite]

I'm not sure if this is a result of this fix, or a problem that I simply found 
when I was testing this, but: the folder-picker in the Move/Copy actions does 
not allow selection of a folder.  (I'm not sure when that folder-picker was 
changed to a single-level menu, but I like the tiered menu better.)

Comment 36

[Mike Cowperthwaite]

Also, if you create a filter using Create Filter From Message, the default 
action is    Move To | <account>    which is illegal, but (per comment 35) not 
changeable; worse, when you click OK, it's saved as-is rather than being parsed 
as an illegal destination.

I also note there is an error thrown in the JS console when the Filter Rules 
window first opens via Create Filter From Message.

Comment 37

[Scott MacGregor]

Attachment: address two follow up issues by mcow

1) the if branch for initializing the dialog when called from "Create Filter
From Message" was assuming there was a default action row already present in
the dialog.  That's no longer true with the new design where the actions are
added dynamically after the window has loaded. Removing this line gets rid of
the JS error.

2) searchWidgets.xml --> Make sure we are dealing with a folder which can have
messages copied into it otherwise throw up an error to the user informing him
to select a valid folder.

Comment 38

[[e-mail]]

Comment on attachment 191527 [details] [diff] [review]
address two follow up issues by mcow

>-        // Clear the default action added above now that the dialog is initialized.
>-        gFilter.clearActionList();
Right, but don't forget to clear the list in saveFilter.

>-                if (!actionTarget.uri || actionTarget.uri == "") 
>+                var msgFolder = GetMsgFolderFromUri(actionTarget.uri);
I'd prefer it if the uri was (still) null-checked somewhere.

Comment 39

[Scott MacGregor]

(In reply to comment #38)
> (From update of attachment 191527 [details] [diff] [review] [edit])
> >-        // Clear the default action added above now that the dialog is
initialized.
> >-        gFilter.clearActionList();
> Right, but don't forget to clear the list in saveFilter.

I didn't understand this comment Neil. Currently, in saveFilter, the action list
is cleared if we are saving an existing filter. New filters, there are no
actions to clear so we don't clear anything.

Comment 40

[Scott MacGregor]

Attachment: [patch checked in]updated tweak

carrying forward neil's r. 

I addressed his two comments.

Comment 41

[Markus Faßbender]

I couldn't mark the row. 

When I click on a condition-row, than this row get a colored background. This
doesn't function with the actions.

Comment 42

[Mike Cowperthwaite]

(In reply to comment #25)
> 6) I was hoping to try this out for a little while without the buttons for new
> folder as I thought it cluttered things up. I wanted to see how many people
> notice first before adding it back.

Bug 308660 and bug 330942.