Last Comment Bug 205039 - Filter Rules window needs a splitter
: Filter Rules window needs a splitter
Status: RESOLVED FIXED
[filter-mgmt]
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 743974 746312
  Show dependency treegraph
 
Reported: 2003-05-09 08:54 PDT by Mike Cowperthwaite
Modified: 2012-04-18 16:01 PDT (History)
9 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
TB filter pane windows trunk (133.00 KB, image/png)
2011-10-24 14:29 PDT, Wayne Mery (:wsmwk, NI for questions)
no flags Details
patch adding the same <splitter> into filterDialog (772 bytes, patch)
2011-11-08 13:50 PST, :aceman
bwinton: ui‑review-
Details | Diff | Review
patch, adding <splitter> into filterDialog, padding fix (2.59 KB, patch)
2011-11-14 15:46 PST, :aceman
bwinton: review+
Details | Diff | Review
patch v2 (3.86 KB, patch)
2012-03-18 09:22 PDT, :aceman
richard.marti: feedback+
Details | Diff | Review
patch v3 (5.36 KB, patch)
2012-03-18 11:59 PDT, :aceman
richard.marti: feedback+
stefanh: feedback+
Details | Diff | Review
Resizer cuts the button (4.95 KB, image/png)
2012-03-18 11:59 PDT, Richard Marti (:Paenglab)
no flags Details
patch v4 (4.29 KB, patch)
2012-03-26 13:34 PDT, :aceman
bwinton: ui‑review+
stefanh: feedback+
richard.marti: feedback+
Details | Diff | Review
patch v5 (4.30 KB, patch)
2012-04-17 13:23 PDT, :aceman
mconley: review+
acelists: ui‑review+
Details | Diff | Review
patch v6 (4.31 KB, patch)
2012-04-18 11:23 PDT, :aceman
acelists: review+
Details | Diff | Review

Description Mike Cowperthwaite 2003-05-09 08:54:22 PDT
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.
Comment 1 Wayne Mery (:wsmwk, NI for questions) 2011-10-24 14:02:51 PDT
comment 0 is still true. The two areas are always of equal height
Comment 2 :aceman 2011-10-24 14:22:39 PDT
I do not understand this. The splitter is draggable.
And what is the action area? Is it the results display?
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2011-10-24 14:29:05 PDT
Created attachment 569186 [details]
TB filter pane windows trunk

(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
Comment 4 :aceman 2011-10-24 14:37:20 PDT
Ah, didn't notice he went from "Search messages" to "edit filter" window :)
Comment 5 :aceman 2011-11-08 13:50:18 PST
Created attachment 572995 [details] [diff] [review]
patch adding the same <splitter> into filterDialog

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).
Comment 6 David :Bienvenu 2011-11-08 13:52:03 PST
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.
Comment 7 Mark Banner (:standard8) 2011-11-10 00:17:48 PST
Comment on attachment 572995 [details] [diff] [review]
patch adding the same <splitter> into filterDialog

Once you've got ui-review please rerequest normal review.
Comment 8 Blake Winton (:bwinton) (:☕️) 2011-11-14 13:48:17 PST
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.
Comment 9 :aceman 2011-11-14 15:06:28 PST
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).
Comment 10 :aceman 2011-11-14 15:46:18 PST
Created attachment 574460 [details] [diff] [review]
patch, adding <splitter> into filterDialog, padding fix

Is this better?
Comment 11 Blake Winton (:bwinton) (:☕️) 2011-11-30 08:35:22 PST
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.
Comment 12 :aceman 2011-11-30 12:17:08 PST
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 Blake Winton (:bwinton) (:☕️) 2011-11-30 12:42:54 PST
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.
Comment 14 :aceman 2011-11-30 12:55:41 PST
Thanks for explanation.
Would "padding: 0px" be ugly if used on Aero too? To keep the styles the same across themes.
Comment 15 Blake Winton (:bwinton) (:☕️) 2011-11-30 13:50:35 PST
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 Richard Marti (:Paenglab) 2011-11-30 13:53:42 PST
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 Richard Marti (:Paenglab) 2011-11-30 13:57:11 PST
I've seen when pulling the splitter on top the filter name box will be squeezed.
Comment 18 :aceman 2011-11-30 14:14:36 PST
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 Richard Marti (:Paenglab) 2011-12-01 00:08:40 PST
(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.
Comment 20 :aceman 2011-12-01 07:53:44 PST
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 21 :aceman 2012-03-17 06:12:12 PDT
Bwinton, what should I do now?
Comment 22 Blake Winton (:bwinton) (:☕️) 2012-03-17 08:25:38 PDT
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.
Comment 23 :aceman 2012-03-18 09:22:11 PDT
Created attachment 606978 [details] [diff] [review]
patch v2

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.
Comment 24 :aceman 2012-03-18 09:40:33 PDT
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 Ian Neal 2012-03-18 11:16:15 PDT
(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 Richard Marti (:Paenglab) 2012-03-18 11:59:10 PDT
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.
Comment 27 :aceman 2012-03-18 11:59:32 PDT
Created attachment 606992 [details] [diff] [review]
patch v3

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.
Comment 28 Richard Marti (:Paenglab) 2012-03-18 11:59:55 PDT
Created attachment 606993 [details]
Resizer cuts the button
Comment 29 Richard Marti (:Paenglab) 2012-03-18 12:07:41 PDT
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.
Comment 30 :aceman 2012-03-18 12:09:24 PDT
I have no resizer in that dialog on Linux. How comes?
Comment 31 Richard Marti (:Paenglab) 2012-03-18 12:18:13 PDT
Different Linux theme?
Comment 32 :aceman 2012-03-21 09:45:37 PDT
Or you have some special settings?
Can anybody else see the resizer on any system?
See also bug 737796.
Comment 33 Richard Marti (:Paenglab) 2012-03-21 10:12:59 PDT
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 Ian Neal 2012-03-25 14:24:12 PDT
Comment on attachment 606992 [details] [diff] [review]
patch v3

stefanh, could you feedback from the OSX side of things?
Comment 35 Stefan [:stefanh] 2012-03-26 08:51:11 PDT
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)
Comment 36 :aceman 2012-03-26 08:55:36 PDT
"Copy & paste + do not touch unknown code in case it is important"-reason :)

So what about the padding on OS X?
Comment 37 Stefan [:stefanh] 2012-03-26 09:39:26 PDT
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.
Comment 38 :aceman 2012-03-26 10:39:31 PDT
(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.
Comment 39 :aceman 2012-03-26 13:34:48 PDT
Created attachment 609461 [details] [diff] [review]
patch v4

Thanks, I hope I covered all the comments.
Comment 40 Richard Marti (:Paenglab) 2012-03-27 08:58:48 PDT
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.
Comment 41 Stefan [:stefanh] 2012-03-27 09:50:27 PDT
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.
Comment 42 Blake Winton (:bwinton) (:☕️) 2012-04-17 09:29:35 PDT
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.
Comment 43 :aceman 2012-04-17 12:26:55 PDT
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 Ian Neal 2012-04-17 12:37:32 PDT
(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)
Comment 45 :aceman 2012-04-17 13:23:59 PDT
Created attachment 615846 [details] [diff] [review]
patch v5

Thanks for the hints, patch fixed.
Comment 46 Mike Conley (:mconley) - (Needinfo me!) 2012-04-18 08:38:03 PDT
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.
Comment 47 :aceman 2012-04-18 11:23:38 PDT
Created attachment 616224 [details] [diff] [review]
patch v6

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.
Comment 48 Ryan VanderMeulen [:RyanVM] 2012-04-18 16:01:42 PDT
http://hg.mozilla.org/comm-central/rev/b03573ab65f8

Note You need to log in before you can comment on or make changes to this bug.