So propose to do these optimizations in the current filter edit dialog (see the screenshot): 1. There are some hidden columns/listcells in the rules list. Those were added in bug 183994 but I can't find out the reason. Also, the editor has changed a bit in the meanwhile (and also core toolkit bugs). I tried to remove those cells and could see any problems. 2. The Add/remove buttons are inside a hbox element that serves no purpose. I'll put them directly inside the containing listcell. The usefulness of this hbox in the rules and also the actions list is already questioned in in bug 743974 comment 16. 3. The is a small gap on the top of the scrollbar, that is not there on the bottom. There was already intention to fix that in bug 183994, but I am not sure why it did not happen. Maybe it was reverted as the fields/buttons are then too close to the border of the listbox? I'll attach a new screenshot of the results. Removing the superfluous elements that repeat for each row may help performance in bug 444793.

The only visible change is the gap above the scrollbars. I see the add/remove buttons in the actions list may be too close to the listbox border (the search term rules could be acceptable). So if it is bad I drop this from this bug and create a new one where I'd like to align also other elements better: e.g. search term listitems are 27px high (Win XP), the action listitems are 25px, therefore they have less padding and the ugliness appears. Also textboxes do not align (vertically) with the menulists. I can play with all this in another bug if you want.

(In reply to aceman) > 1. There are some hidden columns/listcells in the rules list. Those were > added in bug 183994 but I can't find out the reason. Perhaps http://www-archive.mozilla.org/mailnews/specs/search/#Mail (although presumably the extra columns were provided for l10n purposes.) > 2. The Add/remove buttons are inside a hbox element that serves no purpose. The hbox serves to simplify the code that generates the listcells because it was only designed to put one element in each cell. I didn't look to see how hard it would be to rewrite this. > 3. The is a small gap on the top of the scrollbar, that is not there on the > bottom. Indeed, that was added by bug 195224, in a patch that I even reviewed, and I didn't notice it :-(

I couldn't find anything relevant at that archive link... I am not sure how l10n could use the cells, they had no IDs, no entities that could be translatable. Maybe extensions? Rkent could know. The patch for 1 and 2 is quite easy and a fine code removal. For 2 it would also contain the equivalent trivial removal of hbox in searchWidgets.xml.

But I am now not sure if the code "searchTermObj.booleanNodes = searchrow.childNodes;" actually does anything now (or whether it did before the patch). It was collecting the hidden cells, which I now broke. But I also inspected msgFilterRules.dat and it seems the filters are saved correctly even after the patch. Maybe this is a remnant (temporarily hidden until it gets fixed) of the option to have a different boolean operator for each rule (like it was before bug 183994).

Comment on attachment 645702 [details] screenshot after patch (Win XP) Yeah, that looks better to me. I'ld like the patch to be ui-reviewed on other systems, to make sure nothing else breaks, but ui-r=me for this screenshot. Thanks, Blake.

I don't see any reason why an extension would use these elements that you want to remove.

This is a real patch for all platforms and Seamonkey too. It also fixes the padding issues and textbox height (it makes the listitems the same height in the rules and the actions lists) I mentioned in comment 1. That makes the padding on top (from the buttons to the border of the listbox) in the actions list the same as in the rules list, so it is even better than in the screenshot. I have also checked the Filter editor, Search messages dialog, Search addresses dialog and Saved search editor. It seems they all share the xul and css so are fixed at once.

Comment on attachment 645702 [details] screenshot after patch (Win XP) Jens is more familiar with our filter editor UI. r?->jh@junetz.de

Comment on attachment 645861 [details] [diff] [review] patch v2 On OS X, it's very close. There's a little too much space in a couple of places, and too little in other places, as shown in http://dl.dropbox.com/u/2301433/Screenshots/filterSpacing.png but aside from that, I like it. ui-r=me with those fixed. (As a side note, Windows 7 has enough space after the "Blogs & News Feeds", but too much after the "-" button.)

Might be a DUP but I'll just set a dependency for Bug 202036 first ( Scroll a criteria list and a dark highlight appears (Filter Rules, Search Messages, Search Addresses))

Philip, I am not sure how that is related. Can you explain?

The "too much space" after "-" button is intentional, it is reserved for the scrollbar when it appears. The small space between a dropdown menu and the "+" button on OS X should not be caused by this patch (but was probably caused by bug 743974 where we allowed the dropdown menu to extend until the button) but I attach a new version that attempts to fix it. Also check the scrollbar on OS X if it does not have the 2px gap on top, that is intended to be removed by this bug (the patch v2 was wrongly missing that fix for TB OS X).

Comment on attachment 646231 [details] [diff] [review] patch v3 Okay, that fixes the one thing, and explains the other, so ui-r=me. Thanks, Blake.

Comment on attachment 645702 [details] screenshot after patch (Win XP) Comparing the "screenshot of problems" attachment to what I see on Win 7 with patch v3 applied, I can only see point 3 being addressed, i.e. the space above the scroll bar is gone. The rest seems untouched to me, but nothing looks broken either. If that is intended, f=me (I only checked the Filter Rules and Search Messages dialogues, and only on Win 7).

Yes, that is intended. 1. and 2. should not produce any visible changes.

It seems the patch also removes the small gap (padding-top:2px) above the list header in the filter list window (not only the filter editor window). That seems to be a good thing.

Neil? I also wonder if the line + searchTermObj.booleanNodes = searchrow.childNodes; is still needed. I do not see any reference to booleanNodes in the rest of comm-central. Also even until now it was collecting the values of the empty nodes so its value was basically useless, I think.

Comment on attachment 668912 [details] [diff] [review] patch v4 Review of attachment 668912 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/search/content/searchTermOverlay.js @@ +403,5 @@ > + * Creates a <listitem> using the array children as the children > + * of each listcell. > + * @param aChildren An array of XUL elements to put into the listitem. > + * Each array member is put into a separate listcell. > + * If the member itself is an array of elements, nit: trailing space

Thanks.