450302 - There is no "filter searching" (to find a filter)

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[eddie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: 2.0.0.16



There is no "filter searching" ; I have hundreds filters and when I unsubscribe from a newsletter I need to find that filter among hundreds of filters to delete the no longer needed filter.... (because f I don't delete the unnecessary ones it will be a huge resource consumed when downloading messages) -- so there is a need for filter searching -- or is there any?...

Reproducible: Always

Steps to Reproduce:
1. There is no filter searching
2.
3.



there is no filter searching.

Comment 7

[Wayne Mery (:wsmwk)]

I haven't tried it, but https://addons.mozilla.org/en-US/thunderbird/addon/filter-of-filters/ is cited in https://getsatisfaction.com/mozilla_messaging/topics/sorting_400x_tb_message_filters_suggestion

Comment 8

[Werner Warweg]

Thank you for this link....
But it's not compatible to actual version of TB

Comment 10

[Wayne Mery (:wsmwk)]

(In reply to Werner Warweg from comment #8)
> Thank you for this link....
> But it's not compatible to actual version of TB

It claims compatibility with versions 3-10. Works for me using version 13, but only searches on filter *name*. If it doesn't work for you, the place to comment is with the addon author/website.

Comment 11

[Herbert Eppel]

Splendid, thanks for pointing out the Filter Of Filters add-on - I had been looking for something like this for some time! See discussion at http://groups.google.com/group/mozilla.support.thunderbird/browse_thread/thread/83c4153e714101d3/

Comment 12

[Axel Grude [:realRaven]]

I have written a similar functionality in my latest release of QuickFOlders (3.1) which is going to be released this weekend. Currently working on SeaMonkey compatibility. I also added buttons for moving filters to top or bottom of list. If I get time I will turn this into a patch, will post screen shot next.

Comment 13

[Axel Grude [:realRaven]]

Attachment: Preview of the Patched filter list dialog

This shows the modified overlay, including the search box (in this example I am using "Moz"); this filters the list currently just by name. Filter persists even if you change the server in the dropdown. Also move to top and move to bottom for a quick filter positioning fix. 

One major problem is the modified behavior of move Up / move Down, while the filter is applied; I would expect it to move the selected folder to next free position after the target folder (even if this means that several hidden folders must be skipped). Having no visual feedback during onUp / onDown would only frustrate the users.

Comment 14

[John David Galt]

I would want it to filter on all fields, or at least on the strings that the filter itself looks for.  That's where duplicates are most likely to occur; the existing product detects duplicate filter names when you try to create them.

Comment 15

[John David Galt]

(corrected typo)
I would want it to _search_ on all fields, or at least on the strings that the filter itself looks for.  That's where duplicates are most likely to occur; the existing product detects duplicate filter names when you try to create them.

Comment 16

[Axel Grude [:realRaven]]

It would be nice to react to more fields, but for a first pass a name filter will cover a lot of the use cases. If I filter for AMO or Moz i am very likely to get filters that have to with Addons or Mozilla, so at least from my perspective this is already usability win. 

When we're talking about filter values, apart from the condition values some of the filter action values (such as target folder, or tag as ...) would probably be desirable; this is where it can get easily confusing. 

To the average user this is not always obvious, so filtering by name would probably be the easiest for a version 1.0, thus mimicking the behavior of the password manager were only fields that can be seen (including the password) can be filtered for. Hidden fields such as name of the password form field are not considered.

Comment 17

[Ludovic Hirlimann [:Usul]]

Comment on attachment 615087 [details]
Preview of the Patched filter list dialog

Blake what do you think of those designs ?

Comment 18

[Axel Grude [:realRaven]]

Attachment: dialog including count of (filtered) items

Added a count label beside the search box, which is probably better than a 'find' label. If all items are visible, it reads "x items", if items are filter it reads "x of y". A preview version to test look & feel can be found in my extension QuickFolders, here:

http://downloads.mozdev.org/quickfolders/QuickFolders-tb-pb-sm-3.2pre2.xpi

It also includes the "To Top" "To Bottom" buttons, should I remove them for this patch and add to another bug?

Comment 19

[:aceman]

I'd say discussion about Move to Top/Bottom should be moved to other bug, like bug 668995, if no better is found.

But for those deciding about it, I must note that it is true that this bug is only about filtering of filters without any management, on the other hand, code for the existing Move up/down buttons must be adapted to work in the new filtered view. So discussion about filter management (movement) is relevant here too.
(If it is not decided that pressing any of the buttons just resets the filter and exposes all filters again.)

Comment 20

[Axel Grude [:realRaven]]

(In reply to :aceman from comment #19)

> But for those deciding about it, I must note that it is true that this bug
> is only about filtering of filters without any management, on the other
> hand, code for the existing Move up/down buttons must be adapted to work in
> the new filtered view. 

True, in my test inmplementation MoveUp / MoveDown are already re-written to work on the "visible" folders, which means that they can be skipping invisible folders while re-arranging, as I presume the filtered list shows "related" Filters, and re-arranging them is probably desired to work on the order of the filtered sub-set.

E.g. consider this filter list:

1-AMO Editors
2-test a
3-test b
4-AMO-Internal
5-test c
6-test d

In my implementation, if you filter for AMO you will see 2 items:
1-AMO Editors
4-AMO-Internal

When the user highlights 1-AMO-Editors and then pushed Move-Down, this will be re-sorted as follows:
4-AMO-Internal
1-AMO Editors

removing the search value the resulting full list is now:
2-test a
3-test b
4-AMO-Internal
1-AMO Editors
5-test c
6-test d

this achieves: execution of 1-AMO Editors _after_ 4-AMO-Internal with just one click. It would not make sense to have "invisible" position changes and removing the filter would defeat the purpose... If the filter can be extended later for including value attributes then the relation with "related filters" can be tightened, and MoveUp / MoveDown will experience even higher value. Also, it might make the implementation of "move 10 up" / "move 10 down" less necessary?

On another note, maybe MoveTop/MoveBottom buttons should function in a similar fashion and place the filter at the beginning / end of the _visible_ list as well; at the moment they always move the filter to start / end of the full list.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 615635 [details]
dialog including count of (filtered) items

I like the filter box, and I like the "x of y".

I think the current design (based on the screenshot) is a little too crowded and doesn't really say what the box is for.  What about putting them on a new line, so we get something like:  http://dl.dropbox.com/u/2301433/Screenshots/TBFilterFilter.png ?

So, uh, I'm not entirely sure how to mark this.  I guess it's ui-r+ for the idea, and ui-r=me with the tweaks I've suggested?  :)

Thanks,
Blake.

Comment 22

[Axel Grude [:realRaven]]

Not sure about the improvement of this - it seems like a colossal waste of space. Are you sure the other one isn't easier to use? It seems like the filter and server elements both above the list are easier to handle, and they conform to similar dialog boxes such as browser history in firefox. I think for usability the List's length should be maximizes, especially since the sorting order of items is relevant.

If the search box gets narrow, there is no harm, as the dialog can be widened for people who have lots of filters and find it necessary to enter long search terms. For me as power user the filter below (and to an extent the label above the list as well) is too much waste of vertical screen space, therefore I deliberately moved it up from the original design.  I also had added a tooltip, although I find the trailing 0 items / 40 items / 3 of 15 pretty self explanatory. And I wouldn't expect a huge amount of characters from a search string; from experience even if you have 100 filters, you can pretty much get a usable subset fitting on one page with 4 to 6 letters.

Comment 23

[Axel Grude [:realRaven]]

Attachment: Alternative Suggestion to win back space

As a third option: How about moving the filter button down, this should provide ample space for a long server name and filtering term; plus the filtering User Interface elements somehow are binding together.

The filter button also works nicely on top of the other buttons and on the opposite side of the list label. what do you think?

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 616724 [details]
Alternative Suggestion to win back space

You say "waste of space", I say "readable and uncluttered".  :)

I suspect this is just one of those philosophical differences, and it seems like it will be hard for each of us to convince the other that their viewpoint is right.  (I mean, I do believe that the way you have it works very well for you, and probably for other advanced users, but I don't think that all Thunderbird's users are advanced, and I feel we have to take into account the newer users, too, and present them with a less confusing and scary UI.)

(As well, the fact that in your latest screenshot you've had to cut off the newsgroup name sort of reinforces my feeling that that should really take up the whole row, and the filter stuff should be on a separate row.)

Thanks,
Blake.

Comment 25

[Axel Grude [:realRaven]]

I forgot to mention: since this is not an ordinary textbox, it has an emptyText attribute, so you can put the "filters which contain..." inside, although I think the search icon and "find filters..." is simple and sufficient. If the user really needs the fact of what exactly is searched a tool tip should be sufficient.

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Ah, I do like the emptyText idea.  We'll have to figure out the wording, though, since I'm not really fond of "find filters…", but I can't quite put my finger on why.  (Firefox uses "Search History" and "Search Bookmarks", so I think I could live with "Search Filters", but there might be something better that I haven't thought of.)

Comment 27

[Axel Grude [:realRaven]]

I agree about the wording, this could be optimized. Of course "find filters" is better than "filter the filters".

As regards the abbreviated newsgroup name, the ellipsis happened because of a limitation in the theme [Charamel] I am using, as there the dropdown is fixed size. I actually stretched the textbox portion with paintshop, and forgot to extend the string. So the limitation of the width would have been there even without the search text beside it. I think there can be a good minimal width and maybe some flex attributes in favor of  the server dropdown, the minimal width of a search box is so that the hourglass icon is shown plus one or two characters.

Also as regards complete labeling, we don't have the same requirements for other search boxes (e.g. search messages) that can be placed anywhere in the user interface? The search messages box on the toolbar simply says "Search... <CTRL+K> " and that doesn't make it harder to use. 

Plus: the filter is directly to the right of the drop down (which also selects a subset from ALL filters) within the filter list, which imo provides enough context. Anything intrinsically wrong with putting extra info [what is exactly searched] into the tooltip?

As far as aestetics go this is probably something to argue about (in my view the two white text boxes above each other look more cluttered than beside each other). The [Filter Log] could be the same width as the other buttons to tidy up the dialog further.

Comment 28

[Axel Grude [:realRaven]]

I am just trying to create an overlay in my extension that pushes the button down, but it is not quite so easy, as there is an unnamed grid and I would have to move the filter button into the 2nd row, so I'll do another version with paintshop, this time with some aesthetic improvements, using Thunderbird default theme under Windows XP.

Comment 29

[Axel Grude [:realRaven]]

Attachment: Another Suggestion with Filter button moved down

Ok here is another version, with a little dose of DOM manipulation, plus set min-width of 20em on the server dropdown and the log button moved down, like I said. No photoshop trickery here, but hopefully tidy enough. I also changed the emptyText to something more instructive as suggested.

Comment 30

[Axel Grude [:realRaven]]

Attachment: with search term

same dialog with an active search. filtering is immediate.

Comment 31

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 616754 [details]
Another Suggestion with Filter button moved down

Better, but still not quite there.

(Axel and I are emailing suggestions back and forth, to try and iterate on it a little more before coming back to this bug with a compromise solution.  I think we're making progress.)

Comment 32

[Axel Grude [:realRaven]]

Attachment: final (?) screenshot

After some discussion and analyzing the structure of the dialog we agreed on this variant. Since [Filter Log] more related to running the filters it was moved down. 

Note that the count is now closer to the list and by default displays "n items" but changes to "n of m" if the list is filtered. Also the emptyString of the search box has been made more precise.

Comment 33

[Axel Grude [:realRaven]]

Attachment: final screenshot (2)

Previous image was broken, sorry.

After some discussion and analyzing the structure of the dialog we agreed on this variant. Since [Filter Log] more related to running the filters it was moved down. 

Note that the count is now closer to the list and by default displays "n items" but changes to "n of m" if the list is filtered. Also the emptyString of the search box has been made more precise.

Comment 34

[Magnus Melin [:mkmelin]]

Please capitalize Search.

Comment 35

[Axel Grude [:realRaven]]

Attachment: My first crack at the patch

Here is my first attempt at the patch. Haven't put that through a test compile yet as my makefile gets stuck somewhere, what do you think of this as a first attempt?

I have tested the code in my extension QuickFolders, but this is somewhat re-factored. I left a few stubs for porting to Mozilla;  I would need somebody to show me how to rebuild the treeView to make it work there as well. For the moment I am happy if we can get this to land in Tb though.

Note: moving filters up and down must work in "searched" mode as well. The output conforms to the final screenshot (617204) minus CSS niceties (arrows). I included the new "to top" / "to bottom" buttons but they are relatively trivial and can be removed if you want them for a different bug. However, I would expect them to be improved in future versions to move the filter "just underneath" the last visible filter rather than at the absolute end, so there is some dependencies here.

Comment 36

[:aceman]

Comment on attachment 617468 [details] [diff] [review]
My first crack at the patch

I have some points to the patch (I am no reviewer just somebody also doing patches in filters):
- I am not sure why you replace whole FilterListDialog.xul, complete with the licence. Is that intentional?
- I do not like how you remove and then insert a filter to move it. I do not know if it causes any problems but it is strange. I could try to make you a MoveFilterTo(<absolute position>) function what you could use and simplify the code. It would use knowledge from bug 668995 and I will also use the function in that bug.
- There is localisation support for doing plurals properly (your filterCountItems, filterCountSingleItem, filterCountVisibleOfTotal), see https://developer.mozilla.org/en/Localization_and_Plurals . Please update the code.
- I think the suite specific parts should be removed from this TB specific patch.
- the Move to Top/Bottom buttons were rejected so far by bwinton (in bug 668995) but maybe he changes his mind:)
- the reorderBottomButton.toolTip probably should not say "execute" but "move to bottom".

I have not yet tried to apply and run the patch, I just spotted this by reading it. Otherwise thanks for doing this!

Comment 37

[Axel Grude [:realRaven]]

Attachment: patch 1.1

I have fixed the problem with the xul file, my Diff utility (ExamDiff) somehow turned the file from utf-8 into ASCII. it should be much easier to review now. the XUL file isn't dramatically different, I only moved one button, added 2 buttons and a search box, and that's it.

Comment 38

[:aceman]

IanN, can you tell us what to do with the seamonkey specific code here? The patch is from an extension that caters for both SM and TB but the patch here probably will need to be cleaned up to use TB code only. Will the stripped out SM code be usable to anyone trying to port this to SM someday?

Comment 39

[Axel Grude [:realRaven]]

I have some more improvements for the tooltips which I am going to put into one of the future patches:

<!ENTITY reorderButtonTop.toolTip "Rearrange filter so it executes before all others">
<!ENTITY reorderButtonBottom.toolTip "Rearrange filter so it executes after all others">

this makes is possible to be flexible in the implementation, to either move to absolute top/bottom, or after (before) the last (first) visible filter (which I would find more elegant).

Comment 40

[Axel Grude [:realRaven]]

Attachment: Working Patch, tested on yesterday's Daily

I compiled Thunderbird 14 daily successfully yesterday, and fixed all apparent bugs / removed vestigial code. Testing it on DailyBird today, I think this is pretty solid. If you want I can remove the SeaMonkey stubs, although they shouldn't interfere too much.

Comment 41

[:aceman]

OK, you addressed the first of my problems in comment 36. What about the rest? :)

Comment 42

[Philip Chee]

Comment on attachment 617682 [details]
patch 1.1

Code review only.

>  function onUp(event)
>  {
> +  var searchBox = document.getElementById("searchBox");
> +  if (searchBox.value) {
> +    var filtersList = gCurrentFilterList; 
Just use gCurrentFilterList directly;

> +    var list = getFilterListElement();
> +    if (getSelectedCount(list) != 1)
> +      return;
> +
> +    var activeFilter = getSelectedFilterAt(list, 0); 
> +    if (activeFilter) try {
try should be on the next line. Also why do you expect this to throw?

> +      var nextIndex = list.selectedIndex-1;
Space around operators: list.selectedIndex - 1;

> +      var nextFilter = list.getItemAtIndex(nextIndex)._filter;
Use let in the above two lines and in the rest of the function.

> +      rebuildFilterList();  
> +
> +      // assumption: item stays selected even after removing the search condition
> +      var newIndex = list.selectedIndex-1; 
Space around operators: list.selectedIndex - 1;

> +      filtersList.removeFilter(activeFilter);
> +
> +      // insert before next visible item
> +      // go up from selected index until finding the correct filter name
> +      while (nextFilter.filterName!=list.getItemAtIndex(newIndex)._filter.filterName && nextIndex<list.itemCount)
Space around operators.

> +        newIndex--;
> +      filtersList.insertFilterAt(newIndex, activeFilter);
> +      rebuildFilterList();  
> +      list.selectedIndex = newIndex;
> +
Unnecessary blank line.

> +    }
> +    catch (ex) {
> +      dump(ex + "\n");
Use Components.utils.reportError() and make the error message more informative.

> +    }
> +    onFindFilter(true, false);
> +  }
> +  else
>      moveCurrentFilter(Components.interfaces.nsMsgFilterMotion.up);
>  }
>  
>  function onDown(event)
This is almost to unUp(). Consider consolidating these two into a single function e.g.
function onMove(aEvent, aUp); where aUp is a boolean.

>  {
> +  var searchBox = document.getElementById("searchBox");
> +  if (searchBox.value) {
> +    var filtersList = gCurrentFilterList;
> +    var list = getFilterListElement();
> +    if (getSelectedCount(list) != 1)
> +      return;
> +
> +    var activeFilter = getSelectedFilterAt(list, 0); 
> +    if (activeFilter) try {
> +      var nextIndex = list.selectedIndex+1;
> +      var nextFilter = list.getItemAtIndex(nextIndex)._filter;
> +      rebuildFilterList();  
> +
> +      // assumption: item stays selected even after removing the search condition
> +      var newIndex = list.selectedIndex+1; 
> +      filtersList.removeFilter(activeFilter);
> +
> +
> +      // insert after next visible item
> +      // go down from selected index until finding the correct filter name
> +      while (nextFilter.filterName!=list.getItemAtIndex(newIndex)._filter.filterName && nextIndex<list.itemCount)
> +        newIndex++;
> +      filtersList.insertFilterAt(newIndex, activeFilter);
> +      rebuildFilterList();  
> +      list.selectedIndex = newIndex;
> +
> +    }
> +    catch (ex) {
> +      dump(ex + "\n");
> +    }
> +    onFindFilter(true, false);
> +  }
> +  else
>      moveCurrentFilter(Components.interfaces.nsMsgFilterMotion.down);
>  }
>  
> +/* Added functions for moving Filter to top / bottom for long filter lists */
> +function onTop(evt) 
Trailing whitespace.
function onTop(aEvent)
Also consider consolidating with onBottom()

> +{
> +  var filtersList = gCurrentFilterList; 
> +  var list = getFilterListElement();
> +  try {
> +    if (getSelectedCount(list) != 1)  // shouldn't happen if we disabled buttons correctly...
> +      return; 
> +
> +    var activeFilter = getSelectedFilterAt(list, 0); 
> +    if (activeFilter) {
> +      filtersList.removeFilter(activeFilter);
> +      filtersList.insertFilterAt(0, activeFilter);
> +      rebuildFilterList(gCurrentFilterList);
> +      onFindFilter(true, false); // re-filter list
> +      document.getElementById("reorderTopButton").disabled=true;
Space around operators.

> +    }
> +  }
> +  catch(ex) {
> +    dump(ex + "\n");
Use Components.utils.reportError() and make the error message more informative.

> +  }
> +} 
> +  
> +/* Added functions for moving Filter to top / bottom for long filter lists */
> +function onBottom(evt) 
> +{
> +  var filtersList = gCurrentFilterList;
> +  var list =getFilterListElement();
> +  try {
> +    if (getSelectedCount(list) != 1) // shouldn't happen if we disabled buttons correctly...
> +      return; 
> +
> +    var activeFilter = getSelectedFilterAt(list, 0); 
> +    if (activeFilter) {
> +      filtersList.removeFilter(activeFilter);
> +      filtersList.insertFilterAt(filtersList.filterCount, activeFilter);
> +      rebuildFilterList(gCurrentFilterList);
> +      onFindFilter(true, false);
> +      document.getElementById("reorderBottomButton").disabled=true;
> +    }
> +  }
> +  catch(ex) {
> +    dump(ex + "\n");
> +  }
> +}
> +
> +
> +
Unnecessary blank lines.

> -  // Save scroll position so we can try to restore it later.
> +  // Save scroll position so we can try to restore it later. (to test: Might not work when filtered!)
Should limit comments to < 80 cols right margin.

>    catch (ex) {
>      dump(ex + "\n");
>    }
>    return msgFolder;
>  }
> +
> +
> +
Unnecessary blank lines.

> +function onFindFilter(whenNotEmpty, focusSearchBox) 
> +{
> +  var searchBox = document.getElementById("searchBox");
> +  var filterList = getFilterListElement();
> +  var keyWord = searchBox.value;
var keyWord = searchBox.value.keyWord.toLocaleLowerCase();

> +  if (!keyWord && whenNotEmpty) {
> +    updateCountBox();
> +    return;
> +  }
> +  rebuildFilterList(gCurrentFilterList);
> +  if (!keyWord) {
> +    searchBox.focus();
> +    updateCountBox();
> +    return;
> +  }
> +
> +  keyWord = keyWord.toLocaleLowerCase();
See previous comment.

> +  var rows = getListElementCount(filterList); 
> +  
> +  for(var i=rows-1;i>=0;i--){
Space around operators.

> +    var matched = true;
> +    item = filterList.getItemAtIndex(i);
> +    title = item.firstChild.getAttribute("label");
> +    if(title.toLocaleLowerCase().indexOf(keyWord) == -1){
Need more space around the if () clause.
if (title.toLocaleLowerCase().indexOf(keyWord) < 0) {

> +      matched = false;
> +      filterList.removeChild(item);
> +    }
> +      
Unnecessary blank line.

> +  }
> +  updateCountBox();
> +  if (focusSearchBox)
> +    searchBox.focus();
> +
Unnecessary blank line.

> +} 
> +
> +function onSelectFilter(evt) 
function onSelectFilter(aEvent)
Trailing whitespace. And use aEvent for new code.

> +{
> +  var list = getFilterListElement();
> +  var numFiltersSelected = getSelectedCount(list);
> +  var oneFilterSelected = (numFiltersSelected == 1);
> +  var buttonTop = document.getElementById("reorderTopButton");
> +  var buttonBottom = document.getElementById("reorderBottomButton");
> +  var upDisabled = !(oneFilterSelected && 
> +                     getSelectedFilterAt(list, 0) != list.childNodes[1]);
> +  if (list.currentIndex == 0) // SM
> +    upDisabled = true;
> +  buttonTop.disabled = upDisabled;
> +  var downDisabled = (!oneFilterSelected 
> +      || list.currentIndex == getListElementCount(list)-1);
Space around operators. Also weird indentation.

> +  buttonBottom.disabled = downDisabled;
> +} 
> +
> +
too many blank lines.

> +/* function to display "1 item",  "11 items" or "4 of 10" if list is filtered via search box */
> +function updateCountBox()
> +{
> +  var countBox = document.getElementById("countBox");
> +  var sum = gCurrentFilterList.filterCount;
> +  var filterList = getFilterListElement();
> +  var len = getListElementCount(filterList); 
> +  
> +  // embarrassment saver:
> +  if (len>sum) 
Space around operators.

> +    len=sum;
Space around operators.

> +  
> +  var bundle = document.getElementById("bundle_filter");
> +
> +  if (len==sum) // "N items"
> +    countBox.value = 
> +      (len == 1)
> +      ? bundle.getString("filterCountSingleItem") // document.getElementById ('qf-FilterCount-1-item').value
Bad indentation.

> +      : len.toString() + " " + bundle.getString("filterCountItems"); // document.getElementById ('qf-FilterCount-items').value;
This will work on very few locales. Use the built in bundle.getFormattedString() instead.

    countBox.value = 
      (len == 1) ? bundle.getString("filterCountSingleItem")
                 : bundle.getFormattedString("filterCountItems", [len]);

> +  else // "N of M"
> +    countBox.value = bundle.getString("filterCountVisibleOfTotal") // document.getElementById ('qf-FilterCount-n-of-m').value
> +      .replace('{0}', len.toString())
> +      .replace('{1}', sum.toString());
countBox.value = bundle.getFormattedString("filterCountVisibleOfTotal", [len, sum]);
Also consider using Components.utils.import("resource://gre/modules/PluralForm.jsm");

> +  
> +} 
> +
> +
> +
Too many blank lines.

> +// helper functions, for porting to Suite later.
You should leave out Suite specific code in a Thunderbird only patch. You can always file a bug in SeaMonkey with your notes on how to adapt this to Suite.

> +        <textbox id="searchBox"
> +            flex="7"
Why 7 and not 6 or 8 or something else?

> +            type="search"
> +            oncommand="QuickFolders.Filter.onFindFilter(false, true);"
> +            emptytext="&searchBox.emptyText;"
> +            tooltiptext="&searchBox.toolTip;"
> +            isempty="true"
> +            timeout="300"/>
Why 300?

> -        <label control="filterTree">&filterHeader.label;</label>
> +        <hbox>
> +          <label control="filterTree" id="filterListLabel">&filterHeader.label;</label>
id normally comes first.

> -          <button id="reorderUpButton" label="&reorderUpButton.label;" accesskey="&reorderUpButton.accesskey;" 
> +
> +        <button id="reorderTopButton" label="&reorderTopButton;" accesskey="&reorderTopButton.accessKey;"
> +            tooltiptext="&reorderTopButton.toolTip;"
Wrong indentation. Line up all the attributes under id. And if you are going to wrap the attributes, move accesskey to the next line.

> +            oncommand="QuickFolders.Filter.onTop(event);"/>
And where is QuickFolders defined?

> +          <button id="reorderBottomButton" label="&reorderBottomButton;" accesskey="&reorderBottomButton.accessKey;"
> +                  tooltiptext="&reorderBottomButton.toolTip;"
> +                  oncommand="QuickFolders.Filter.onBottom(event);"/>
Where is QuickFolders defined?

> -          <button id="runFiltersButton" 
> -                  label="&runFilters.label;" 
> -                  accesskey="&runFilters.accesskey;" 
> -                  runlabel="&runFilters.label;" 
> -                  runaccesskey="&runFilters.accesskey;" 
> -                  stoplabel="&stopFilters.label;" 
> -                  stopaccesskey="&stopFilters.accesskey;"
> -                  oncommand="runSelectedFilters();" disabled="true"/>

> +            <button label="&viewLogButton.label;" accesskey="&viewLogButton.accesskey;" oncommand="viewLog();"/>
This section wraps code to under 80 columns.

> +<!ENTITY reorderTopButton "To Top">
Move to top.

> +<!ENTITY reorderTopButton.accessKey "o">
> +<!ENTITY reorderTopButton.toolTip "executes filter before all others">
>  <!ENTITY reorderUpButton.label "Move Up">
Move up.

>  <!ENTITY reorderUpButton.accesskey "U">
>  <!ENTITY reorderDownButton.label "Move Down">
>  <!ENTITY reorderDownButton.accesskey "D">
> +<!ENTITY reorderBottomButton "To Bottom">
Move to bottom.

> +<!ENTITY searchBox.emptyText "Search filters by name…">
> +<!ENTITY searchBox.toolTip "Enter a keyword to search filters by name">
If you have emptytext, a tooltip is redundant.

> +
> +
> +
Too many blank line.


> +filterCountVisibleOfTotal={0} of {1}
filterCountVisibleOfTotal=%1$S of %2$S
Also consider using Components.utils.import("resource://gre/modules/PluralForm.jsm");

> +filterCountItems=items
> +filterCountSingleItem=1 item
>  
>  # for junk mail logging / mail filter logging
>  # LOCALIZATION NOTE(junkLogDetectStr)
>  # %1$S=author, %2$S=subject, %3$S=date
>  junkLogDetectStr=Detected junk message from %1$S - %2$S at %3$S
>  # LOCALIZATION NOTE(logMoveStr)
>  # %1$S=message id, %2$S=folder URI
>  logMoveStr=moved message id = %1$S to %2$S

Comment 43

[Axel Grude [:realRaven]]

Thanks Phil, will tidy up! Lovely!

AS regards To Top / Move To Top I stuck with the original but I can change it if that's better. Sorry about the QuickFolders reference I was sure I had removed all references :(

Axel

Comment 44

[Axel Grude [:realRaven]]

Attachment: Patch 3 addressing Phil's feedback

Comment 45

[Axel Grude [:realRaven]]

Attachment: Patch 3

here are the fixes, according to Phil Chee's review

Comment 46

[:aceman]

Comment on attachment 618425 [details] [diff] [review]
Patch 3

Well, you still haven't addressed many of Phillips comments (like merging the similar functions, there are still 'var+1' without spaces, still vars instead of lets, etc). And some of mine ;)
The plurals are still wrong, you can see here what you need to do: https://bug719456.bugzilla.mozilla.org/attachment.cgi?id=600575 (subscribe-OPMLImportFoundFeeds=(out of #1 entry found);(out of #1 total entries found)).

But the main point: yes, the patch does work as advertised:) The filter manager gets nicely crowded with features!

Comment 47

[:aceman]

Also, I am not sure what do you use 'matched' variable in onFindFilter() for.
And it seems you completely remove rows from the list that do not match the keyword. Then when you want to do some movement of filter, you rebuild the list, make the operation and apply the filter again.
I wonder if just making the rows hidden (e.g. display:none) would be safer (for code not expecting any filters missing in the list) and simpler in some operations. On the other hand, some other code (playing with and moving the selection) would need to be made aware of hidden rows so that is not simply does things like row.previousElementSibling .

But I have learned some stuff from your patch that I'd like to apply throughout the file in a new bug (like list.getItemAtIndex()).

Comment 48

[Axel Grude [:realRaven]]

(In reply to :aceman from comment #47)
> Also, I am not sure what do you use 'matched' variable in onFindFilter() for.
probabl a brain fart - the code was a lot more complicated when I tried to cater for Thunderbird / Postbox+SeaMonkey.
> And it seems you completely remove rows from the list that do not match the
> keyword. Then when you want to do some movement of filter, you rebuild the
> list, make the operation and apply the filter again.
It is the safest way. Also in SeaMonkey one should do a similar thing with the treeView (removing non-matches after rebuilding)

> I wonder if just making the rows hidden (e.g. display:none) would be safer
> (for code not expecting any filters missing in the list) and simpler in some
> operations. 
Maybe. I had thought about it but was afraid of the side effects ;) I actually believe this route is safer and there is no great performance problem unless you have 1000s of filters. I would say 50 to 100 filters / account would be a reasonable number for a power user.

> On the other hand, some other code (playing with and moving the
> selection) would need to be made aware of hidden rows so that is not simply
> does things like row.previousElementSibling .
yes exactly - I think moving could become a bit of a nightmare.

> But I have learned some stuff from your patch that I'd like to apply
> throughout the file in a new bug (like list.getItemAtIndex()).
sure, knock yourself out :)

What I am really interested in is to port a streamlined version of my filter wizard (which is part of QuickFolders) into Thunderbird. If it wasn't too hidden and easy to use then it has tremendous potential for  encouraging users to create their own filters. At the moment I an create a new "mail-sorting" filter in 15 seconds: drag, click, click. this would be a cool feature for Tb core!

Comment 49

[John David Galt]

There is no point to a fix if it *doesn't* support thousands of filters.

Comment 50

[:aceman]

Axel&John, I believe the solution implemented in the patch is more performant in case of 1000s of filters when scrolling. Axel says the same in the comment 48. I doubt it is at the move up/down operations, which now need TWO full rebuilds of the list and a third pass at applying the filter again.

I just question the safeness of the list temporarily not representing (having all items of) the gCurrentFilterList array.

Axel, notice bug 103684 landed, you will probably need to update your patch now to accommodate the code change and maybe the functionality. That should be the last of my changes for now, I'll stop changing the file until you finish your patch here:)

Also there are these considerations for you:
- What happens if a new filter is created and it does not match the current search criteria?
- What happens if a filter is edited and the new name does not match the search criteria?

Either it gets hidden and the user may wonder or you leave it visible in the list even though it does not match (this could be smart solution:)) Or maybe you should clear the search in these cases. This needs UI decision.

Comment 51

[Axel Grude [:realRaven]]

(In reply to John David Galt from comment #49)
> There is no point to a fix if it *doesn't* support thousands of filters.

True. Fancy writing a test case? :P

Comment 52

[:aceman]

I've seen a filter generator in some bug if you are interested :)
It creates 100s of filters instantly.

Comment 53

[Axel Grude [:realRaven]]

(In reply to :aceman from comment #50)
> Axel&John, I believe the solution implemented in the patch is more
> performant in case of 1000s of filters when scrolling. Axel says the same in
> the comment 48. I doubt it is at the move up/down operations, which now need
> TWO full rebuilds of the list and a third pass at applying the filter again.

there is certainly space for optimization here. I will look into it.

> 
> I just question the safeness of the list temporarily not representing
> (having all items of) the gCurrentFilterList array.
> 
> Axel, notice bug 103684 landed, you will probably need to update your patch
> now to accommodate the code change and maybe the functionality. That should
> be the last of my changes for now, I'll stop changing the file until you
> finish your patch here:)

thanks, will do.
> 
> Also there are these considerations for you:
> - What happens if a new filter is created and it does not match the current
> search criteria?
> - What happens if a filter is edited and the new name does not match the
> search criteria?
I think clicking Edit / New should just discard the filter, without losing the scroll position - lets have another UI review after the next patch. Did you take care of retaining scroll position in bug 103684? I will probably mail you directly with some questions on that one. (e.g. do you sort and does this affect execution order?) - you can mail me directly to keep S/N on this bug.

- re: filter generator, yes if you can provide the link I will put a test function in FilterListDialog.js. (Do you usually use JS Debugger to run test functions?)

Comment 54

[:aceman]

Generator here:
https://bug444793.bugzilla.mozilla.org/attachment.cgi?id=514339

Paste the output text into the msgFilterRules.dat file in the Local Directory holding the mail files.

Comment 55

[Axel Grude [:realRaven]]

Attachment: Patch 4

New patch incorporating aceman's changes from Bug bug 103684 - aceman, hope I got them all please check.

Comment 56

[:aceman]

Comment on attachment 618989 [details] [diff] [review]
Patch 4

Sorry, I am not authorized to do reviews. But I do feedback soon.

Comment 57

[:aceman]

Comment on attachment 618989 [details] [diff] [review]
Patch 4

OK, it seems to work fine. After New or Edit, the search is reset. In the Edit case, the search field is not cleared properly (but the search is reset, all filters shown). Try to fix that. Otherwise feedback+.

But you still haven't addressed many of Philip's comments. You can contact me on IRC and we can go through them.

Comment 58

[Axel Grude [:realRaven]]

Attachment: Latest Patch

I believe I have addressed all items previously flagged by Phil Chee; the change set includes a unified moveFilter function to streamline the code - I added an enum msgMoveMotion for this as i did not want to expand the XPCIM interface one (Ci.nsMsgFilterMotion)

Comment 59

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 620255 [details] [diff] [review]
Latest Patch

So I'm not the best reviewer for this.  My script says that mkmelin, bienvenu, or Standard8 would be better choices.  Picking one at random, I've redirected your review request to mkmelin.  ;)

But while I'm here, I can do the ui-review.

And on that note, I mostly like it!

A couple of nits:
The "Enabled filters..." label should probably get cropped when the window gets too small.  (I've played around a little, and didn't see how to get that working.  clokep might know though, so I'm asking him for feedback.)

I would like a little more vertical space in the header in Windows (Mac seems better for some reason), but if you're set against it, I'm not going to insist.

So, ui-r=me with those fixed.

Thanks,
Blake.

Comment 60

[Magnus Melin [:mkmelin]]

Comment on attachment 620255 [details] [diff] [review]
Latest Patch

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

This is working quite nicely, thx!

Please make sure there is no trailing spaces (on lines you touch).

::: mail/base/content/FilterListDialog.js
@@ +271,5 @@
>    let selectedFilter = currentFilter();
>    // if no filter is selected use the first position, starting at 1
>    let position = 1;
>    if (selectedFilter) {
> +    // get the position in the unfiltered list - this is where the new filter should be inserted!

Capitalize sentence.

@@ +303,3 @@
>    }
> +  else
> +    onFindFilter(true, false); // if no filter created, let's search again.

Style nit: curly braces here (if one of the if-elses need it, they should all have it. Then you can also move the comment up a row.

@@ +371,5 @@
> +function onBottom(evt) {
> +  moveFilter(msgMoveMotion.Bottom);
> +}
> +
> +function moveFilter(motion) {

For all these functions please add a short docxygen/javadoc style documentation saying what they do, and documenting parameters.

@@ +411,5 @@
> +  }
> +
> +  var searchBox = document.getElementById("searchBox");
> +  if (searchBox.value) {
> +    if (activeFilter) try {

Curly brackets for the if block, try on its own row.

@@ +441,5 @@
> +      list.selectedIndex = newIndex;
> +
> +    }
> +    catch (ex) {
> +      Components.utils.reportError("moveFilter(" + motion + ") failed  - " + ex + "\n");

... though there probably shouldn't be a try-catch block at all. Might as well let it break if it breaks - as there should be no expected exceptions here, right?

@@ +809,5 @@
>  }
> +
> +
> +
> +function onFindFilter(whenNotEmpty, focusSearchBox) 

I do think the whole whenNotEmpty is confusing, and would like to see it done in separate functions or something.

@@ +828,5 @@
> +  }
> +
> +  var rows = getListElementCount(filterList); 
> +  
> +  for(var i = rows - 1; i >= 0; i--){

Prefer to use let instead of var (elsewhere too).

@@ +845,5 @@
> +} 
> +
> +
> +
> +/* function to display "1 item",  "11 items" or "4 of 10" if list is filtered via search box */

Doxgen style please, like 
/**
 * Update the item count box label to display e.g. "1 item", "11 items" or "4 of 10".
 */

@@ +855,5 @@
> +  var len = getListElementCount(filterList); 
> +  
> +  // embarrassment saver:
> +  if (len > sum) 
> +    len = sum;

I'd ditch this, if there's a bug it better be found and fixed than hidden.

@@ +859,5 @@
> +    len = sum;
> +  
> +  var bundle = document.getElementById("bundle_filter");
> +
> +  if (len==sum) // "N items"

spaces around ==

@@ +866,5 @@
> +  else // "N of M"
> +    countBox.value = bundle.getFormattedString("filterCountVisibleOfTotal", [len, sum]);
> +}
> +
> +// helper functions, for porting to Suite later.

doxygen style, here and elsewhere

@@ +876,5 @@
> +
> +/* for suite, we need list.view.rowCount */
> +function getListElementCount(list) {
> +  if (typeof list.getRowCount !== "undefined")
> +    return list.getRowCount();

This do seem fairly error-prone, and i think it's probably not worth using helper functions to help suite porting.

@@ +898,5 @@
> +  // suite uses a tree view: - probably need to use list.view.selection.getRangeAt
> +  return null;
> +}
> +
> +// obsolete

then remove it

::: mail/base/content/FilterListDialog.xul
@@ +86,5 @@
> +        <textbox id="searchBox"
> +                 flex="1"
> +                 type="search"
> +                 oncommand="onFindFilter(false, true);"
> +                 emptytext="&searchBox.emptyText;"

emptytext is deprecated, use placeholder

@@ +103,5 @@
>        <row>
> +        <hbox>
> +          <label id="filterListLabel" control="filterList">&filterHeader.label;</label>
> +          <spacer/>
> +          <label id="countBox" flex="1" style="text-align:right;"/>

don't use style; maybe align="right" or something?

Comment 61

[Patrick Cloke [:clokep]]

Comment on attachment 620255 [details] [diff] [review]
Latest Patch

Sorry this took so long for me to get to...I'm assuming bwinton is talking about the #filterListLabel? That's what I took a look at anyway.

>diff --git a/mail/base/content/FilterListDialog.xul b/mail/base/content/FilterListDialog.xul
>@@ -66,45 +66,51 @@
[...snip...]
>   <grid flex="1">
>     <columns>
>       <column flex="1"/>
>       <column/>
>     </columns>
>     <rows>
>       <row>
>         <separator class="thin"/>
>       </row>
>       <row>
>-        <label control="filterTree">&filterHeader.label;</label>
>+        <hbox>
>+          <label id="filterListLabel" control="filterList">&filterHeader.label;</label>
First of all, if you want it to crop (instead of wrap), use value="&filterHeader.label;" instead of putting it as the content. You'll also want to add crop="end" and add a flex number that's higher than the other elements in the container (I used 1 and it seemed to work fine, more on that later). [Aside: the flex property is confusing, people generally consider it the element that will expand to be the biggest (and I think this is what the documents say), but in my experience, it's actually the element that will expand OR contract the most, i.e. the most flexible.]

These changes should let it get cropped until the window is really just too small to do anything and things start getting cut off.


>+          <spacer/>
I'm pretty sure you can get rid of this now (it worked without it in my tests at least).

>+          <label id="countBox" flex="1" style="text-align:right;"/>
Remove the flex attribute from here -- you want the other label to flex more than this. So now you've removed the spacer and flex from this label, the only flexible element left is the first label, so that should do all the moving. I think you can also just not care about aligning right as all the "extra" space should be taken up by the other label.

>+        </hbox>
>       </row>

Also, I found it a bit strange to put the count here, you end up with a lot of "empty" space kind of hanging to the right of it; but that's bwinton's call.

Anyway, it's definitely a cool patch, can't wait to see it in action! Let me know if anything I said doesn't make sense.

Comment 62

[Axel Grude [:realRaven]]

Attachment: Patch 6

Comment 63

[Axel Grude [:realRaven]]

Comment on attachment 646860 [details] [diff] [review]
Patch 6

I think I have addressed all open items with this one, also simplified that onFindFilter function and added doxygen style comments. The label resizes nicely now with the crop; good trick to moce the text into the value attribute (I might even try to do that from my addons, they actually overlay a spacer at the moment)

Comment 64

[Axel Grude [:realRaven]]

Comment on attachment 646860 [details] [diff] [review]
Patch 6

When I submitted this I forgot to invalidate the other patches, can somebody do this after the fact?

Comment 65

[Patrick Cloke [:clokep]]

Comment on attachment 620255 [details] [diff] [review]
Latest Patch

(In reply to Axel Grude from comment #64)
> Comment on attachment 646860 [details] [diff] [review]
> Patch 6
> 
> When I submitted this I forgot to invalidate the other patches, can somebody
> do this after the fact?

In Details for the patch, click "Edit details" and then there's an obsolete checkbox. :)

Comment 66

[Blake Winton (:bwinton) (:☕️)]

Hey Axel,

given that the previous version got a ui-r+ from me, do you still need me to ui-review this patch, or are you comfortable just carrying the previous review forward?

Thanks,
Blake.

Comment 67

[Axel Grude [:realRaven]]

Comment on attachment 646860 [details] [diff] [review]
Patch 6

carrying forward UI +r from bwinton

Comment 68

[Magnus Melin [:mkmelin]]

Comment on attachment 646860 [details] [diff] [review]
Patch 6

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

Getting there! Mostly some nits, r- since i'd like to take another look when the nits are addressed

::: mail/base/content/FilterListDialog.js
@@ +113,1 @@
>   *

File a followup bug to do this?

@@ +239,5 @@
>    let selectedFilter = currentFilter();
>    // if no filter is selected use the first position, starting at 1
>    let position = 1;
>    if (selectedFilter) {
> +    // Get the position in the unfiltered list - this is where the new filter should be inserted!

Wrap @80

@@ +259,5 @@
>    window.openDialog("chrome://messenger/content/FilterEditor.xul", "FilterEditor", "chrome,modal,titlebar,resizable,centerscreen", args);
>  
>    if ("refresh" in args && args.refresh) {
> +    // On success: reset the search box!
> +    document.getElementById("searchBox").value="";

add spaces to make it value = "";

@@ +305,5 @@
>  
>    if (checkValue.value)
>       prefBranch.setBoolPref("mailnews.filters.confirm_delete", false);
>  
> +  // Save filter position before the first selected one

sentences should have . at the end (here and below)

@@ +335,5 @@
> +function onDown(event) {
> +  moveFilter(msgMoveMotion.Down);
> +}
> +
> +/* Added functions for moving Filter to top / bottom for long filter lists */

Should be something like 
/**
 * Move filter to bottom for long filter lists.
 */

@@ +343,5 @@
> +
> +/* Added functions for moving Filter to top / bottom for long filter lists */
> +function onBottom(evt) {
> +  moveFilter(msgMoveMotion.Bottom);
> +}

Should be something like 
/**
 * Move filter to top for long filter lists.
 */

@@ +347,5 @@
> +}
> +
> +/**
> +  * Moves a singular selected filter up or down either 1 increment or to the top/bottom
> +  * This acts on the visible filter list only which means that:

drop one space from star indention

/**
 * text....

@@ +433,5 @@
> +
> +      }
> +      catch (ex) {
> +        Components.utils.reportError("moveFilter(" + motion + ") failed  - " + ex + "\n");
> +      }

No need for the try-catch, right?

@@ +837,5 @@
> +
> +  // rematch everything in the list, remove what doesn't match the search box
> +  let rows = getListElementCount(filterList);
> +
> +  for(let i = rows - 1; i >= 0; i--){

for (let i = rows - 1; i >= 0; i--) {

@@ +854,5 @@
> +}
> +
> +
> +
> +/* function to display "1 item",  "11 items" or "4 of 10" if list is filtered via search box */

Doxygen/javadoc style please

@@ +876,5 @@
> + * Suite Compatibility Section
> + *
> + * These are meta functions that are designed to help porting the filter patch to SeaMonkey later
> + *
> + */

Like i said earlier, i don't think these are very useful. But if you really want to keep them, at least they shouldn't try to do the suite things. (You're thinking about it like an extension author i guess?)

::: mail/base/content/FilterListDialog.xul
@@ +91,2 @@
>                    oncommand="onEditFilter();"/>
> +          <button id="deleteButton" label="&deleteButton.label;" accesskey="&deleteButton.accesskey;"

If you're touching it, line up the attributes. (here and above)

Comment 69

[Axel Grude [:realRaven]]

Before I submit the next patch:

(In reply to Magnus Melin from comment #68)
> Comment on attachment 646860 [details] [diff] [review]
> Patch 6
> 
> Review of attachment 646860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Getting there! Mostly some nits, r- since i'd like to take another look when
> the nits are addressed
> 
> @@ +876,5 @@
> > + * Suite Compatibility Section
> > + *
> > + * These are meta functions that are designed to help porting the filter patch to SeaMonkey later
> > + *
> > + */
> 
> Like i said earlier, i don't think these are very useful. But if you really
> want to keep them, at least they shouldn't try to do the suite things.
> (You're thinking about it like an extension author i guess?)
Yes, exactly. I am still struggling to make the filter search work on SM, but unfortunately it is not easy with tree views. I think having some hook-in points will be useful for extension authors as well though. For the moment I just inserted comments with the suite parts that would work. At least getSelectedFilterAt has to be ported to work with a treeview.

Is removing the functions a hard requirement for getting this patch landed? guess it *would* make the code more readable.

> 
> ::: mail/base/content/FilterListDialog.xul
> @@ +91,2 @@
> >                    oncommand="onEditFilter();"/>
> > +          <button id="deleteButton" label="&deleteButton.label;" accesskey="&deleteButton.accesskey;"
> 
> If you're touching it, line up the attributes. (here and above)
hmm, not sure that I touched the delete button. But I could line up attributes for these 3 to make it consistent? don't know what the policy is as regard beatifying code that I didn't touch?

Comment 70

[:aceman]

Maybe you could file a new bug called "port bug 450302 to Seamonkey" and attach the SM code that you have to remove now into that bug.

Comment 71

[Philip Chee]

Yeah that would be best.

Comment 72

[Axel Grude [:realRaven]]

Attachment: patch

Another one, after MkMelin's last review. removed the SeaMonkey stuff completely if it makes it easier...

Comment 73

[Magnus Melin [:mkmelin]]

(In reply to Axel Grude [:realRaven] from comment #69)
> > > +          <button id="deleteButton" label="&deleteButton.label;" accesskey="&deleteButton.accesskey;"
> > 
> > If you're touching it, line up the attributes. (here and above)
> hmm, not sure that I touched the delete button. But I could line up
> attributes for these 3 to make it consistent? don't know what the policy is
> as regard beatifying code that I didn't touch?

Well, patch says you touched it (fixing whitespace). 
I think the policy is you can fix things nearby what you're touching if you want. (For instance a function might be hard to read otherwise.) But don't mix larger style fixes with other fixes as that makes the patch harder to review.

Comment 74

[Magnus Melin [:mkmelin]]

Comment on attachment 648712 [details] [diff] [review]
patch

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

Thx for he patch. Some minor style nits to fix still, but r=mkmelin! Looking forward to the patch landing.

::: mail/base/content/FilterListDialog.js
@@ +239,5 @@
>    let selectedFilter = currentFilter();
>    // if no filter is selected use the first position, starting at 1
>    let position = 1;
>    if (selectedFilter) {
> +    // Get the position in the unfiltered list 

trailing space

@@ +264,3 @@
>      rebuildFilterList(gCurrentFilterList);
>  
> +    // Select the new filter, it is at the position of previous selection

dot at the end of the sentence

@@ +368,5 @@
> + *   this is currently moving to the top/bottom of the absolute list
> + *   but it would be better if it moved "just as far as necessary"
> + *   which would further "compact" related filters
> + *
> + * @parameter: motion

@param motion

@@ +372,5 @@
> + * @parameter: motion
> + *   msgMoveMotion.Up, msgMoveMotion.Down, msgMoveMotion.Top, msgMoveMotion.Bottom
> + */
> +function moveFilter(motion) {
> +  // At the moment, do not allow moving groups of filters

dot at the end of the sentence

@@ +538,5 @@
>    for (var i = 0; i < list.selectedItems.length; i++)
>      selectedNames.push(list.selectedItems[i]._filter.filterName);
>  
>    // Save scroll position so we can try to restore it later.
> +  // (to test: Might not work when filtered!)

So, does it? Remove or add an XXX comment that it's not working for that case

@@ +681,5 @@
>  
>      return null;
>  }
>  
>  /** if the selected server cannot have filters, get the default server

Please move this first text down a line, and make it sentences.

@@ +857,5 @@
> +    searchBox.focus();
> +}
> +
> +/**
> + * display "1 item",  "11 items" or "4 of 10" if list is filtered via search box.

Capitalize

::: mail/base/content/FilterListDialog.xul
@@ +66,5 @@
>          <separator class="thin"/>
>        </row>
>        <row>
> +        <hbox>
> +          <label id="filterListLabel" control="filterList" value="&filterHeader.label;" crop="end" flex="1"></label>

https://bugzilla.mozilla.org/attachment.cgi?id=648712

@@ +67,5 @@
>        </row>
>        <row>
> +        <hbox>
> +          <label id="filterListLabel" control="filterList" value="&filterHeader.label;" crop="end" flex="1"></label>
> +          <label id="countBox" />

No space before />

::: mail/locales/en-US/chrome/messenger/FilterListDialog.dtd
@@ +10,5 @@
>  <!ENTITY editButton.label "Edit…">
>  <!ENTITY editButton.accesskey "E">
>  <!ENTITY deleteButton.label "Delete">
>  <!ENTITY deleteButton.accesskey "t">
> +<!ENTITY reorderTopButton "Move to top">

"Top" and "Bottom" in the button labels should be capitalized

@@ +18,2 @@
>  <!ENTITY reorderUpButton.accesskey "U">
> +<!ENTITY reorderDownButton.label "Move down">

(This change wouldn't be ok, as keys always have to change if we change the text, otherwise localizers can't keep up:( )

Comment 75

[Axel Grude [:realRaven]]

Attachment: Final ? patch

reusing UI+ review from bwinton
mkmelin suggested to r+ pending these last style nits

Comment 76

[Magnus Melin [:mkmelin]]

Comment on attachment 648873 [details] [diff] [review]
Final ? patch

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

::: mail/locales/en-US/chrome/messenger/FilterListDialog.dtd
@@ +10,5 @@
>  <!ENTITY editButton.label "Edit…">
>  <!ENTITY editButton.accesskey "E">
>  <!ENTITY deleteButton.label "Delete">
>  <!ENTITY deleteButton.accesskey "t">
> +<!ENTITY reorderTopButton "Move to top">

Move to Top

@@ +13,5 @@
>  <!ENTITY deleteButton.accesskey "t">
> +<!ENTITY reorderTopButton "Move to top">
> +<!ENTITY reorderTopButton.accessKey "o">
> +<!ENTITY reorderTopButton.toolTip "Rearrange filter so it executes before all others">
> +<!ENTITY reorderUpButton.label "Move up">

Move Up

@@ +18,2 @@
>  <!ENTITY reorderUpButton.accesskey "U">
> +<!ENTITY reorderDownButton.label "Move down">

Move Down

@@ +20,2 @@
>  <!ENTITY reorderDownButton.accesskey "D">
> +<!ENTITY reorderBottomButton "Move to bottom">

Move to Bottom

Comment 77

[Axel Grude [:realRaven]]

Attachment: fixed dtd

fixing the locale. Had it fixed in a local file not in the tree :) I will leave adding the checkin-needed flag to you, if that's ok :)

keeping UI flag from bwinton, giving r? to mkmelin for the final step.

Comment 78

[Magnus Melin [:mkmelin]]

Comment on attachment 648985 [details] [diff] [review]
fixed dtd

This isn't a patch...

Comment 79

[Axel Grude [:realRaven]]

Attachment: patch, again

very embarrassing! the last file was brought to you courtesy of lazarus (I tried to restore the form to find out who had ui-reviewed last time) and this somehow managed to put in a file from a different bug. apologies for the patch-spam.

the file I tried to upload is called patch-8.1.txt

Comment 80

[Magnus Melin [:mkmelin]]

http://hg.mozilla.org/comm-central/rev/6856915ce62b