Some minor cleanup items. 1. The "Filters for:" menu should appear to the right of its label, not below it. 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.
Just for reference, the UI cleanup bug for Filter Rules is bug 45038
QA Contact: esther → laurel
Taking, I'm working on a fix.
Assignee: naving → hwaara
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!
http://www.mozilla.org/mailnews/specs/filters/images/FilterA1.gif 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.
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?
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.
Nice job! Thanks. :-) File a separate bug on the read/unread green stars being replaced with checkboxes issue if nec, thanks.
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 http://bugzilla.mozilla.org/show_bug.cgi?id=69860
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.
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 height. 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.
bug 82754 filed about the checkbox icon.
"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.
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
*** Bug 43697 has been marked as a duplicate of this bug. ***
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="http://home.netscape.com/NC-rdf#" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" orient="vertical" onload="onLoad();" - 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. r=bhuvan ps : Don't forget to fix 'Delete..' issue before you checkin.
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...
Status: NEW → ASSIGNED
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
fix checked in.
Status: ASSIGNED → RESOLVED
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" src="chrome://messenger/locale/filter.properties"/> 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.
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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.