Closed
Bug 205039
Opened 21 years ago
Closed 12 years ago
Filter Rules window needs a splitter
Categories
(MailNews Core :: Filters, enhancement)
MailNews Core
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 14.0
People
(Reporter: mcow, Assigned: aceman)
References
Details
(Whiteboard: [filter-mgmt])
Attachments
(3 files, 6 obsolete files)
The Search Messages window has a horizontal splitter between the criteria section and the results display. Filter Rules would be greatly more usable if it had the same sort of splitter between the criteria and action sections. Currently (1.4b), the two sections resize to the same height as the window resizes. (This is an improvement over 1.3, where only the criteria section resized.) The criteria section often has only one or two criteria, whereas the action section always contains a fixed number of items that's typically larger than the window allows for. With a splitter, the window could be sized smaller while minimizing unused space in the criteria area. Spacing between the items in the action area could be compressed some, too.
Updated•20 years ago
|
Product: Browser → Seamonkey
Updated•19 years ago
|
Assignee: sspitzer → mail
Updated•16 years ago
|
Assignee: mail → nobody
Component: MailNews: Message Display → Filters
Product: SeaMonkey → MailNews Core
QA Contact: esther → filters
Hardware: PC → All
Comment 1•13 years ago
|
||
comment 0 is still true. The two areas are always of equal height
Keywords: uiwanted
Whiteboard: [filter-mgmt]
I do not understand this. The splitter is draggable. And what is the action area? Is it the results display?
Comment 3•13 years ago
|
||
(In reply to aceman from comment #2) > I do not understand this. The splitter is draggable. > And what is the action area? Is it the results display? right. Two areas - filter criteria at top - filter "results"/actions at bottom I don't see a splitter using windows trunk
Ah, didn't notice he went from "Search messages" to "edit filter" window :)
The patch only adds the splitter. The vertical spacing between rules and between actions probably depends on the theme used. My actions (in linux) are much more "compressed" together than on Wayne's screenshot (Win 7).
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #572995 -
Flags: ui-review?(clarkbw)
Attachment #572995 -
Flags: review?(dbienvenu)
Comment 6•13 years ago
|
||
Comment on attachment 572995 [details] [diff] [review] patch adding the same <splitter> into filterDialog I'm not really the person you want reviewing front end code unless you're desperate. But bwinton can probably r+ it if he ui+'s it.
Attachment #572995 -
Flags: ui-review?(clarkbw)
Attachment #572995 -
Flags: ui-review?(bwinton)
Attachment #572995 -
Flags: review?(dbienvenu)
Attachment #572995 -
Flags: review?
Comment 7•13 years ago
|
||
Comment on attachment 572995 [details] [diff] [review] patch adding the same <splitter> into filterDialog Once you've got ui-review please rerequest normal review.
Attachment #572995 -
Flags: review?
Comment 8•13 years ago
|
||
Comment on attachment 572995 [details] [diff] [review] patch adding the same <splitter> into filterDialog Close, but the splitter should go all the way across the window, as shown in http://dl.dropbox.com/u/2301433/Space.png Thanks, Blake.
Attachment #572995 -
Flags: ui-review?(bwinton) → ui-review-
That is a padding that is in the whole Edit Filter dialog. Compare how close the white areas are to the window border in the Edit Filter dialog and in the Search Messages dialog. This padding can be fixed in the theme. But now I notice the window does not have the resize triangle in the corner as others have. Should the <dialog> be changed to <window>? That may have other consequences (I tried exchanging the XUL tag: it fixed the padding, but caused other problems).
Assignee | ||
Comment 10•13 years ago
|
||
Is this better?
Attachment #572995 -
Attachment is obsolete: true
Attachment #574460 -
Flags: review?(bwinton)
Comment 11•13 years ago
|
||
Comment on attachment 574460 [details] [diff] [review] patch, adding <splitter> into filterDialog, padding fix Review of attachment 574460 [details] [diff] [review]: ----------------------------------------------------------------- Aside from the things I mention below, I'm pretty happy with it, so I'm going to say r=me with those things fixed. Thanks, Blake. ::: mailnews/base/search/content/FilterEditor.xul @@ -106,4 +106,4 @@ > > > > </vbox> > > > > - <separator/> > > + <splitter id="gray_horizontal_splitter" collapse="after" persist="state"/> So, I think we don't want it to collapse all the way, because there will always be an action to see. ::: mail/themes/qute/mail/filterDialog.css @@ -53,1 +53,5 @@ > > - list-style-image: url("chrome://messenger/skin/icons/check.gif"); > > + list-style-image: url("chrome://messenger/skin/icons/check.gif"); > > +} > > + > > +#FilterEditor { > > + padding: 1px 0px; While this looks good in Aero, it leaves an odd gap in Classic mode. I recommend having "padding: 0px 0px" here, and "padding: 1px 0px" just for the Aero style.
Attachment #574460 -
Flags: review?(bwinton) → review+
Assignee | ||
Comment 12•13 years ago
|
||
I can only test on Linux. Which one is the Aero theme? From your comment I would infer pinstripe. Also, I think you repeatedly gave r+ to patches that were to be modified. How can I do that and carry over the r+? Should I just make a new attachment and request review again? Or what do you mean exactly. Thanks.
Comment 13•13 years ago
|
||
The Aero theme is the Windows Glass stuff. Richard Marti (cc-ed) should be able to help you with that. (Pinstripe is the Mac theme.) For the r+, you can make a new attachment, and just give it a review+ yourself, and say in the comment for the attachment something like "carrying forward r=bwinton". The r+ means that I trust that you can make this change without me having to re-review it. (If you feel like it's gotten bigger than you're comfortable with, you should feel free to re-request review anyways, of course. :) Thanks, Blake.
Assignee | ||
Comment 14•13 years ago
|
||
Thanks for explanation. Would "padding: 0px" be ugly if used on Aero too? To keep the styles the same across themes.
Comment 15•13 years ago
|
||
I'm not sure… Lemme give it a try… So with 0px, Aero looks like http://dl.dropbox.com/u/2301433/Glass/Filter0pxAero.png But, Classic looks like http://dl.dropbox.com/u/2301433/Glass/Filter0pxClassic.png so it looks like the 0px doesn't fix the problem in Classic… :( Richard, could you take a quick stab a fixing this? Thanks, Blake.
Comment 16•13 years ago
|
||
You could also instead of reducing the dialogs padding, give the splitter a negative margin. On Linux and Windows: #gray_horizontal_splitter { -moz-margin-start: -8px; -moz-margin-end: -10px; } and on Mac: #gray_horizontal_splitter { -moz-margin-start: -14px; -moz-margin-end: -14px; }
Comment 17•13 years ago
|
||
I've seen when pulling the splitter on top the filter name box will be squeezed.
Assignee | ||
Comment 18•13 years ago
|
||
Is that margin setting universal for Aero and Classic? Also, without the reducing of padding, the splitter may seem ugly, extending past the other content (on left and right). I think the ugliness problems stem from the fact, that this dialog is a <dialog>. The users want the splitter because they see it in Search messages window. But that one is defined via <window> and seems to have different styles and behaviour.
Comment 19•13 years ago
|
||
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #15) > I'm not sure… Lemme give it a try… > > So with 0px, Aero looks like > http://dl.dropbox.com/u/2301433/Glass/Filter0pxAero.png > But, Classic looks like > http://dl.dropbox.com/u/2301433/Glass/Filter0pxClassic.png > so it looks like the 0px doesn't fix the problem in Classic… :( The search Messages window looks the same under Classic. The splitter has always a small gap because the window itself has a border which is not visible. Classic uses no inset border style and the content flows directly into the border. So I would say 0px is okay also under Classic.
Assignee | ||
Comment 20•13 years ago
|
||
Actually, the patch has a padding of "1px 0px". I think 1px applies to the top and bottom padding, not to the edges. So the border (left right) disconnect will not be fixed. Maybe with "1px -1px". Yes, the Classic padding Search messages is the same as I have proposed, the splitter is disconnected from the window edges (as in the screenshot). So, bwinton, what should I do? 1px or 0px on top+bottom of the splitter? The left+right can be left on 0px for consistency. (In reply to Richard Marti [:paenglab] from comment #17) > I've seen when pulling the splitter on top the filter name box will be > squeezed. I'll check that out.
Comment 22•12 years ago
|
||
Let's go with Richard's suggestion, and use 0px padding for all the sides, on Classic and Aero. (You can carry-forward the r=me.) Thanks, Blake.
Assignee | ||
Comment 23•12 years ago
|
||
I removed the possibility to completely collapse the Action part as you requested in comment 11. I set the padding to 0px on all sides on qute theme. I fixed the squezing of the filter name box when the splitter is dragged to the top (comment 17) by wrapping it in a vbox. I don't know if that can have other side effects. So please review again.
Attachment #574460 -
Attachment is obsolete: true
Attachment #606978 -
Flags: ui-review?(bwinton)
Attachment #606978 -
Flags: review?(bwinton)
Attachment #606978 -
Flags: feedback?(richard.marti)
Assignee | ||
Comment 24•12 years ago
|
||
As the theme files I changed were in /mail but the filter dialog is defined in /mailnews, does this also need corresponding changes in /suite themes?
Comment 25•12 years ago
|
||
(In reply to :aceman from comment #24) > As the theme files I changed were in /mail but the filter dialog is defined > in /mailnews, does this also need corresponding changes in /suite themes? As far as I can see, yes it would.
Comment 26•12 years ago
|
||
Comment on attachment 606978 [details] [diff] [review] patch v2 This patch looks good under Windows XP and W7 also in Classic. Under Mac I think you don't need the padding: 0px for #FilterEditor. The splitter has no borders and also no problems with the gap to the window border. And because under Mac the dialog is borderless bigger padding would IMHO look better. Under Linux I propose on bottom a padding to not cut the OK button with the window resizer. (see attachment in next comment). All in all this is looking good and no more sqeezing is visible.
Attachment #606978 -
Flags: feedback?(richard.marti) → feedback+
Assignee | ||
Comment 27•12 years ago
|
||
Ian, please forward to the proper Seamonkey theme guy. I don't know how its themes work, I couldn't see such separation of themes for Win/Mac/Linux as TB has. Maybe you just separate classic/modern theme that is shared for all OSes. I can't test the change on SM so please give it to the proper guy. Thanks.
Attachment #606978 -
Attachment is obsolete: true
Attachment #606992 -
Flags: ui-review?(bwinton)
Attachment #606992 -
Flags: review?(iann_bugzilla)
Attachment #606992 -
Flags: feedback?(richard.marti)
Attachment #606978 -
Flags: ui-review?(bwinton)
Attachment #606978 -
Flags: review?(bwinton)
Comment 28•12 years ago
|
||
Comment 29•12 years ago
|
||
Comment on attachment 606992 [details] [diff] [review] patch v3 f+ with the comments from comment 26 I can't give feedback to the suite as I don't use it.
Attachment #606992 -
Flags: feedback?(richard.marti) → feedback+
Assignee | ||
Comment 30•12 years ago
|
||
I have no resizer in that dialog on Linux. How comes?
Comment 31•12 years ago
|
||
Different Linux theme?
Assignee | ||
Comment 32•12 years ago
|
||
Or you have some special settings? Can anybody else see the resizer on any system? See also bug 737796.
Comment 33•12 years ago
|
||
I played on my Ubuntu with Kubuntu and other themes. It think this is a fallout of this. The Ubuntu system programs have no resizer but the GTK2 apps like TB have. Before I haven't seen this. On Windows I have no resizer.
Comment 34•12 years ago
|
||
Comment on attachment 606992 [details] [diff] [review] patch v3 stefanh, could you feedback from the OSX side of things?
Attachment #606992 -
Flags: feedback?(stefanh)
Comment 35•12 years ago
|
||
Comment on attachment 606992 [details] [diff] [review] patch v3 I wonder why we're using this id: + <splitter id="gray_horizontal_splitter" persist="state"/> (mxr tells me that we have that on a few more splitters with this id, but we never use it afaics)
Attachment #606992 -
Flags: feedback?(stefanh) → feedback+
Assignee | ||
Comment 36•12 years ago
|
||
"Copy & paste + do not touch unknown code in case it is important"-reason :) So what about the padding on OS X?
Comment 37•12 years ago
|
||
Comment on attachment 606992 [details] [diff] [review] patch v3 (In reply to :aceman from comment #36) > So what about the padding on OS X? Oops: +#FilterEditor { + padding: 0px; +} + SeaMonkey don't want this on mac. I'm not a thunderbird peer, but I'm sure thunderbird doesn't want this either.
Assignee | ||
Comment 38•12 years ago
|
||
(In reply to Stefan [:stefanh] from comment #35) > + <splitter id="gray_horizontal_splitter" persist="state"/> > > (mxr tells me that we have that on a few more splitters with this id, but we > never use it afaics) If you think this id is unused please file the bug and assign it to me, I'll look at it. Thanks.
Assignee | ||
Comment 39•12 years ago
|
||
Thanks, I hope I covered all the comments.
Attachment #606992 -
Attachment is obsolete: true
Attachment #609461 -
Flags: ui-review?(bwinton)
Attachment #606992 -
Flags: ui-review?(bwinton)
Attachment #606992 -
Flags: review?(iann_bugzilla)
Attachment #609461 -
Flags: feedback?(stefanh)
Attachment #609461 -
Flags: feedback?(richard.marti)
Comment 40•12 years ago
|
||
Comment on attachment 609461 [details] [diff] [review] patch v4 The patch looks good for the Thunderbird part. nit in /mail/themes/gnomestripe/mail/filterDialog.css: +#FilterEditor { + padding-top: 1px; padding-left: 0; padding-right: 0; I don't think Blake likes the three definitions in one line. Better put every one in their own line.
Attachment #609461 -
Flags: feedback?(richard.marti) → feedback+
Comment 41•12 years ago
|
||
Comment on attachment 609461 [details] [diff] [review] patch v4 Looks good in SeaMonkey (on Mac), thanks! Just another nit re the TB gnomestripe file: In this case it doesn't really matter since left and right values are equal, but Blake might prefer -moz-padding-start/end.
Attachment #609461 -
Flags: feedback?(stefanh) → feedback+
Comment 42•12 years ago
|
||
Comment on attachment 609461 [details] [diff] [review] patch v4 Yeah, this looks pretty good to me (at least on Mac, since I haven't set up my Windows/Linux desktop yet). I did see some disappointing behaviour when I resized the window wider, and then narrower, but I don't think that's related to this patch. So, ui-r=me! (And the two previous commenters have successfully read my mind. :) Thanks, Blake.
Attachment #609461 -
Flags: ui-review?(bwinton) → ui-review+
Assignee | ||
Comment 43•12 years ago
|
||
Ok, I can fix comment 40, but I do not understand what comment 41 meant. I do not know how to use -moz-padding-start/end . Please tell me.
Comment 44•12 years ago
|
||
(In reply to :aceman from comment #43) > Ok, I can fix comment 40, but I do not understand what comment 41 meant. I > do not know how to use -moz-padding-start/end . Please tell me. To help with locales that read in a direction other than left to right it is best to use -moz-padding-start/end instead of padding-left/right (I believe)
Assignee | ||
Comment 45•12 years ago
|
||
Thanks for the hints, patch fixed.
Attachment #609461 -
Attachment is obsolete: true
Attachment #615846 -
Flags: ui-review+
Attachment #615846 -
Flags: review?(mconley)
Comment 46•12 years ago
|
||
Comment on attachment 615846 [details] [diff] [review] patch v5 Review of attachment 615846 [details] [diff] [review]: ----------------------------------------------------------------- With the nit I found fixed, r=me. ::: mailnews/base/search/content/FilterEditor.xul @@ +72,5 @@ > > + <vbox> > + <hbox align="center"> > + <label value="&filterName.label;" accesskey="&filterName.accesskey;" control="filterName"/> > + <textbox flex="50%" id="filterName"/> I'm pretty sure this flex value is wrong - I don't believe you can just toss in a percentage like that. I'm pretty certain it has to be an integer. Replacing the "50%"'s here with "1" seemed to do the job.
Attachment #615846 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 47•12 years ago
|
||
Ooooh, how I like it having to fix inherited code so late in review process ;) https://developer.mozilla.org/en/XUL/Attribute/flex, yes flex needs to be an integer. Using "1" has the same effect as current code. I wonder why the spacer is even there, it just uses space that could be used for the filter name.
Attachment #615846 -
Attachment is obsolete: true
Attachment #616224 -
Flags: review+
Keywords: checkin-needed
Comment 48•12 years ago
|
||
http://hg.mozilla.org/comm-central/rev/b03573ab65f8
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•