Message Filters: UI Cleanup

VERIFIED FIXED in mozilla0.9.2


MailNews Core
17 years ago
10 years ago


(Reporter: jglick, Assigned: Håkan Waara)




Firefox Tracking Flags

(Not tracked)


(Whiteboard: r=bhuvan, sr=hewitt, waiting for trunk unfreeze)


(4 attachments)



17 years ago
Some minor cleanup items.

1. The "Filters for:" menu should appear to the right of its label, not below 
2. "Description" should be replaced with "Filter Name" (since that is what is 
being displayed).
3. The second column should be titled "Enabled".  Currently it is blank.
4. A checkbox (see Subscription dialog) should be used under the "Enabled" 
column, not the green star.  The green star means read/unread.
5. The checkbox (#4) should be centered under the "Enabled" column.
6. The descriptive text should be "Enabled filters are run automatically in the 
order shown below."
7. Are the arrows on the Move Up/Move Down buttons necessary? They make all the 
other buttons wider and aren't very attractive visually.

Comment 1

17 years ago
Just for reference, the UI cleanup bug for Filter Rules is bug 45038
QA Contact: esther → laurel


17 years ago
Keywords: polish

Comment 2

17 years ago
Taking, I'm working on a fix.
Assignee: naving → hwaara
Keywords: ui
hwaara, to speed up the review process, please break up your fix in pieces.

as you know, smaller fixes can get reviewed / tested faster.

take a single task on this list, attach a patch, get it reviewed and landed (and
out of your tree), and repeat.

we can keep this bug open as long as we need to, and continue to check in your
improvements in increments.

divide and conquer!

Comment 4

17 years ago

Note: "Run Filter(s) Now" (hence text: "Filters can also be run manually by 
highlighting the filter(s) and selecting "Run Filter(s) Now") and "Filter Log" 
features are not currently implemented.

Comment 5

17 years ago
Hey, I have all the nits plus some more (except for the checkbox vs. green thing
image) fixed.

I don't know whether that is unfortunate or not. Seth, is it OK?

Comment 6

17 years ago
Created attachment 36148 [details]

Comment 7

17 years ago
Created attachment 36150 [details]
Screenshot of current fix

Comment 8

17 years ago
Jennifer, Scott, Seth, Anyone: The latest screenshot includes all fixes
originally pointed out except for the green image.

What do you all think? Any other things you want fixed, UI-wise?

My only current nit is that the dialog is too high. I will fix that before I
attach a patch here.

Comment 9

17 years ago
Nice job! Thanks.   :-)

File a separate bug on the read/unread green stars being replaced with 
checkboxes issue if nec, thanks.

Comment 10

17 years ago
That looks really nice.  There's another bug out there (doesn't need to go with
this bug, but I thought you might be interested in investigating since you are
already in this dialog) that would also make this window nicer. See
if you've already got a patch, great!  attach it and we'll work on getting it in
for 0.9.2 (too late for 0.9.1)

next time you've got a bug like this, feel free to divide and conquer to speed
up the process.

Comment 12

17 years ago
Created attachment 36157 [details]
final screenshot of the UI

Comment 13

17 years ago
None of you people acually noticed that I missed a few wording changes in the
last screenshot. :)   Anyway, those are fixed now and I am satisfied with the

So here's what my coming patch fixes:
* The columns are now resizable
* All jglick's nits
* Renamed "Delete" to "Delete..." because it brings up a dialog
* Removed lots of old code cruft

Patch coming up.

Comment 14

17 years ago
Created attachment 36158 [details] [diff] [review]

Comment 15

17 years ago
bug 82754 filed about the checkbox icon.

Comment 16

17 years ago

Comment 17

17 years ago
"Delete" shouldn't be "Delete...".  Elipsis are only used when additional input 
from the user is needed. Confirmation dialogs are not included in that category. 
Not necessary here.  None of our other Delete buttons have elipsis.

Comment 18

17 years ago
Bhuvan can you test and review this patch?  Thanks!

Jglick: I will fix that before I land my patch.

The new bug about checkbox-image is 82758. Scratch that earlier comment.
OS: Windows 98 → All

Comment 19

17 years ago
*** Bug 43697 has been marked as a duplicate of this bug. ***

Comment 20

17 years ago
reviewed and tested the patch. Looks good. nice work.

Only thing I want to suggest is that it will be nice if we can persist on the 
window sizes (height, width) incase user resizes the window. It's not a major 
issue..but it is a pretty simple fix. Two additional items this patch to 
enable the persistence are id="filterListDialog" and persist="width height". 

+<window class="dialog" id="filterListDialog"
+  xmlns:NC=""
-  style="width: 34em;"
+  width="440"
+  height="320"
+  persist="width height"

Please get some feedback on this. Then, if needed, alter the patch to 
incorporate the above.


ps : Don't forget to fix 'Delete..' issue before you checkin. 

Comment 21

17 years ago
Thanks Bhuvan! I will incorporate your suggestions in my tree now, and then
checkin with that.

Now all I am waiting for is 0.9.2 tree opening...
Whiteboard: r=bhuvan, sr=hewitt, waiting for trunk unfreeze
Target Milestone: --- → mozilla0.9.2
a=dbaron for trunk checkin assuming comments above are dealt with

Comment 23

17 years ago
fix checked in.
Last Resolved: 17 years ago
Resolution: --- → FIXED
eye balling what actually got checked in (which is not the same as the fix 
above), it looks like this line got removed from FilterListDialog.xul:

<stringbundle id="bundle_filter" 
that would break the confirmation code that gets run when you confirm the 
delete of a filter.

I'll check my build, and if it's busted I'll fix it.

Comment 25

17 years ago
Oops (how'd that happen?). Sorry about that!

r=hwaara on reverting that line back

no worries.

I've fixed it by adding that line back to the .xul file.

fix checked in.

Comment 27

17 years ago
Thank you.

Comment 28

17 years ago
OK using june14 commercial trunk builds: win98, mac OS 9.0 and linux rh6.2.
Checked in both classic, new modern themes.
Generally checked for changes mentioned.
Any other issues will be logged separately.
Marking verified.
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.