Closed Bug 205039 Opened 21 years ago Closed 12 years ago

Filter Rules window needs a splitter

Categories

(MailNews Core :: Filters, enhancement)

enhancement
Not set
normal

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.
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Assignee: mail → nobody
Component: MailNews: Message Display → Filters
Product: SeaMonkey → MailNews Core
QA Contact: esther → filters
Hardware: PC → All
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?
(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 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 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 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).
Is this better?
Attachment #572995 - Attachment is obsolete: true
Attachment #574460 - Flags: review?(bwinton)
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+
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.
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.
Thanks for explanation.
Would "padding: 0px" be ugly if used on Aero too? To keep the styles the same across themes.
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.
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;
}
I've seen when pulling the splitter on top the filter name box will be squeezed.
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.
(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.
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.
Bwinton, what should I do now?
Keywords: uiwanted
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.
Attached patch patch v2 (obsolete) — Splinter Review
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)
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?
(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 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+
Attached patch patch v3 (obsolete) — Splinter Review
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 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+
I have no resizer in that dialog on Linux. How comes?
Different Linux theme?
Or you have some special settings?
Can anybody else see the resizer on any system?
See also bug 737796.
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 on attachment 606992 [details] [diff] [review]
patch v3

stefanh, could you feedback from the OSX side of things?
Attachment #606992 - Flags: feedback?(stefanh)
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+
"Copy & paste + do not touch unknown code in case it is important"-reason :)

So what about the padding on OS X?
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.
(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.
Attached patch patch v4 (obsolete) — Splinter Review
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 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 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+
Blocks: 743974
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+
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.
(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)
Attached patch patch v5 (obsolete) — Splinter Review
Thanks for the hints, patch fixed.
Attachment #609461 - Attachment is obsolete: true
Attachment #615846 - Flags: ui-review+
Attachment #615846 - Flags: review?(mconley)
Blocks: 746312
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+
Attached patch patch v6Splinter Review
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
http://hg.mozilla.org/comm-central/rev/b03573ab65f8
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 14.0
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.

Attachment

General

Created:
Updated:
Size: