Last Comment Bug 777287 - try to remove some unneeded XUL elements in the filter editor dialog
: try to remove some unneeded XUL elements in the filter editor dialog
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
Depends on: 183994 202036 743974
Blocks: 444793
  Show dependency treegraph
 
Reported: 2012-07-25 03:26 PDT by :aceman
Modified: 2012-10-19 17:19 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot of problems (9.13 KB, image/png)
2012-07-25 03:26 PDT, :aceman
no flags Details
screenshot after patch (Win XP) (7.51 KB, image/png)
2012-07-25 04:03 PDT, :aceman
bwinton: ui‑review+
jh: feedback+
Details
WIP patch (4.36 KB, patch)
2012-07-25 05:09 PDT, :aceman
no flags Details | Diff | Review
patch v2 (16.54 KB, patch)
2012-07-25 13:02 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Review
patch v3 (18.04 KB, patch)
2012-07-26 11:41 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Review
patch v4 (18.12 KB, patch)
2012-10-07 07:51 PDT, :aceman
neil: review+
mkmelin+mozilla: review+
Details | Diff | Review
patch v5 (18.12 KB, patch)
2012-10-19 13:10 PDT, :aceman
acelists: review+
Details | Diff | Review

Description :aceman 2012-07-25 03:26:21 PDT
Created attachment 645691 [details]
screenshot of problems

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.
Comment 1 :aceman 2012-07-25 04:03:44 PDT
Created attachment 645702 [details]
screenshot after patch (Win XP)

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.
Comment 2 neil@parkwaycc.co.uk 2012-07-25 04:12:04 PDT
(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 :-(
Comment 3 :aceman 2012-07-25 05:09:39 PDT
Created attachment 645723 [details] [diff] [review]
WIP patch

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.
Comment 4 :aceman 2012-07-25 05:18:07 PDT
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 5 Blake Winton (:bwinton) (:☕️) 2012-07-25 07:07:43 PDT
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.
Comment 6 Kent James (:rkent) 2012-07-25 09:15:31 PDT
I don't see any reason why an extension would use these elements that you want to remove.
Comment 7 :aceman 2012-07-25 13:02:17 PDT
Created attachment 645861 [details] [diff] [review]
patch v2

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 8 Philip Chee 2012-07-25 23:02:02 PDT
Comment on attachment 645702 [details]
screenshot after patch (Win XP)

Jens is more familiar with our filter editor UI. r?->jh@junetz.de
Comment 9 Blake Winton (:bwinton) (:☕️) 2012-07-26 09:04:18 PDT
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.)
Comment 10 Philip Chee 2012-07-26 11:20:33 PDT
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))
Comment 11 :aceman 2012-07-26 11:28:46 PDT
Philip, I am not sure how that is related. Can you explain?
Comment 12 :aceman 2012-07-26 11:41:00 PDT
Created attachment 646231 [details] [diff] [review]
patch v3

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 13 Blake Winton (:bwinton) (:☕️) 2012-07-30 11:58:51 PDT
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 14 Jens Hatlak (:InvisibleSmiley) 2012-08-05 12:00:06 PDT
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).
Comment 15 :aceman 2012-08-05 13:31:41 PDT
Yes, that is intended. 1. and 2. should not produce any visible changes.
Comment 16 :aceman 2012-08-06 00:14:32 PDT
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.
Comment 17 :aceman 2012-09-13 00:52:38 PDT
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 18 :aceman 2012-10-07 07:51:38 PDT
Created attachment 668912 [details] [diff] [review]
patch v4
Comment 19 Magnus Melin 2012-10-19 12:57:18 PDT
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
Comment 20 :aceman 2012-10-19 13:10:44 PDT
Created attachment 673387 [details] [diff] [review]
patch v5

Thanks.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-10-19 17:19:34 PDT
https://hg.mozilla.org/comm-central/rev/6fcff08e997e

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