Closed Bug 79258 Opened 23 years ago Closed 23 years ago

Message Filters: UI Cleanup

Categories

(MailNews Core :: Filters, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.2

People

(Reporter: jglick, Assigned: hwaara)

References

Details

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

Attachments

(4 files)

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
Keywords: polish
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!
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?
Attached image example
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.
Attached patch fixSplinter Review
bug 82754 filed about the checkbox icon.
sr=hewitt
"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
Closed: 23 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.
Thank you.
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
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: