Fix 18949 on GTK ("Options" window does not fit a small display).

gtk: fix 18949 on GTK ("Options" window does not fit a small display).

Change the layout of content and general page. Use the horizontal title and content layout for each option group.

BUG=18949
TEST=Open the option window, check the content and general pages.

Patch by Ningxin Hu <ningxin.hu@intel.com>;.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49291

[ningxin.hu, 2010-05-06 01:41:39]

As you suggestion, I composed this patch. Could you please review? Your
suggestions are very appreciated. 

Thanks in advance.

regards,
ningxin

[Evan Martin, 2010-05-06 01:45:34]

We intentionally use the layout we do to match other GNOME apps.

[ningxin.hu, 2010-05-11 02:32:22]

On 2010/05/06 01:45:34, Evan Martin wrote:
> We intentionally use the layout we do to match other GNOME apps.

But if using GNOME HIG suggested layout, the height is too large for 1024*600
screen. I used to suggest to add scroll bar to other option pages like the
advanced page. But you guys rejected it also. 

Could you please share any ideas how to resolve these? It is really bad user
experience on netbook. Thanks.

[yuzhiqiang, 2010-05-18 17:03:21]

On 2010/05/06 01:45:34, Evan Martin wrote:
> We intentionally use the layout we do to match other GNOME apps.

Is it acceptable to adjust the UI ONLY when the screen size is less than 10'
(the UI change could be either adding a scroll bar and laying the control side
by side)?  in this way, user won't notice any UI change in a bigger screen and
at the same time, better user experience could be achieved in the devices with
small screen.

[Evan Martin, 2010-05-18 17:23:02]

On 2010/05/18 17:03:21, yuzhiqiang wrote:
> On 2010/05/06 01:45:34, Evan Martin wrote:
> > We intentionally use the layout we do to match other GNOME apps.
> 
> Is it acceptable to adjust the UI ONLY when the screen size is less than 10'
> (the UI change could be either adding a scroll bar and laying the control side
> by side)?  in this way, user won't notice any UI change in a bigger screen and
> at the same time, better user experience could be achieved in the devices with
> small screen.

I'm sorry to not give helpful responses.  I started writing a mail to ask for
help on chromium-dev, but I realize I don't understand the problem.  Why is the
dialog too tall?

From the screenshot on issue 18949, it seems that the middle tab is short enough
to fit on screen.  So is it that the first tab is too tall?  I think the "Open
the following pages:" section is the part that is too tall.  Maybe we can just
make that shorter?

[Evan Martin, 2010-05-18 17:25:34]

+erg (I am going on vacation in a few days and I don't want to forget this
patch, so maybe he will have opinions too)


How many pixels do we need to shorten the dialog by?

[Elliot Glaysher, 2010-05-19 00:34:59]

On 2010/05/18 17:25:34, Evan Martin wrote:
> +erg (I am going on vacation in a few days and I don't want to forget this
> patch, so maybe he will have opinions too)

I patched this in. It very obviously breaks with the GNOME HIG, but I can't
really think of a better way to do this.

I would be semi-OK with this if:
- This was fallback behaviour that only triggered when we detected that we
couldn't fit the window on the screen. Normal desktop users should see the
normal layout.
- Small spacing problems were fixed. (The combobox and manage button for
"Default Search" are huge, check your boxes).
- Think about right aligning the section labels. IMHO, the left aligned labels
look bad. The Apple HIG has a section on simple preference windows[1], where
labels are placed to the left of the control groups they describe, and says they
should be right aligned. This is a judgement call, admittedly...

[1] 
http://developer.apple.com/mac/library/documentation/userexperience/conceptua...

> How many pixels do we need to shorten the dialog by?

In Radiance, roughly 65. YMMV by theme.

[Evan Martin, 2010-05-19 10:30:16]

I think I'm also ok with doing it conditionally if the screen is too short.

[Evan Martin, 2010-05-19 10:32:12]

On 2010/05/19 10:30:16, Evan Martin wrote:
> I think I'm also ok with doing it conditionally if the screen is too short.

You can probably do it with an abstract base class of OptionsLayoutBuilderGtk
and OptionsTableBuilderGtk, and then choose one or the other after considering
gdk_screen_get_height(gtk_widget_get_screen(parent_window))

[ningxin.hu, 2010-05-31 06:50:32]

Thanks for your suggestions. And sorry for late response. 
As patch set 3, updated by evan and erg's comments:
1. Added an abstract layout builder class OptionsLayoutBuilderGtk. Added an
OptionsLayoutBuilderFactoryGtk to create concrete option layout builder class
based on screen height. Normally, it just creates the
DefaultOptionsLayoutBuilderGtk which is same as chrome default one. Only if the
screen height is compacted (<= 600), it creates the
CompactOptionsLayoutBuilderGtk which uses horizontal layout.
The 'automatic' parameter controls whether to choose automatically. Now only
content and general pages use automatic selection.
2. Only allow x expandable widget in compact layout builder to fix the size
issue of 'default search' combo box and button.
3. Right aligned labels in compact layout builder.

Your comments and review are highly appreciated.

[Elliot Glaysher, 2010-06-01 17:09:39]

http://codereview.chromium.org/1969006/diff/14001/15008
File chrome/browser/gtk/options/options_layout_gtk.cc (right):

http://codereview.chromium.org/1969006/diff/14001/15008#newcode101
chrome/browser/gtk/options/options_layout_gtk.cc:101: gint screen_height =
gdk_screen_get_height(gdk_screen_get_default());
So this works only in the explicit case where the screen is 600 pixels high
instead of actually measuring the size of the resulting GTK layout and then
adjusting everything accordingly.

I am not sure how pedantic to be about this; doing an explicit check like this
feels hacky and perhaps actual measurements on the size of the dialog as laid
out should trigger the change, but there's certainly lots of examples of us
doing hacky stuff in the GTK interface. I guess I'm fine with this if tony (who
wrote the original version of this class) and evanm are fine with this.

http://codereview.chromium.org/1969006/diff/14001/15009
File chrome/browser/gtk/options/options_layout_gtk.h (right):

http://codereview.chromium.org/1969006/diff/14001/15009#newcode32
chrome/browser/gtk/options/options_layout_gtk.h:32: static
OptionsLayoutBuilderGtk* CreateOptionsLayoutBuilder(bool automatic = false);
1) Search your entire patch and make sure all lines are <80 chars. This happens
every time you call this method.
2) There's no reason for this factory class to exist. Move this static method
into OptionsLayoutBuilderGtk.

[ningxin.hu, 2010-06-02 05:14:08]

Hi Elliot,

Thanks for your comments.

The main purpose of this patch is to let the option window fit on netbook's
screen. 
The mainstream netbook screen is 10 inch. The 10 inch screen's resolution is
normally 1024X600. Some netbooks have less height, such as HP mini is 1024X576. 
However, the default option window height is 659 pixels, it is out of 10 inch
screen. By using horizontal layout as this patch, the option window height
shrinks to 561 pixels. It fits into netbook screen.
For screen bigger than 10 inch, such as 12 inch, the height is normally 768, so
option window shows as default layout.
For even smaller screen, such as 7 inch or 5 inch, I don't think this patch can
work. Actually, the option window UI need to be re-designed for that form
factor, such as using scrolled window, or putting it into a tab. (BTW, I used to
propose a patch to use automatic scrolled window for all option pages, so it can
fit for even small screen. But chromium UI guys reject this design.)
So it just simply checks the screen height with 600 pixels.

As your suggestion, I updated the patch set to 4, removed the redundant
OptionsLayoutBuilderFactoryGtk class and made sure line length is less than 80. 

Thanks for your review.

[Evan Martin, 2010-06-07 23:32:23]

This looks pretty good to me; I only have minor style comments.

I think you need to sync against the current tree now because you are now in the
AUTHORS file (so the AUTHORS part of the patch isn't needed).

http://codereview.chromium.org/1969006/diff/19001/20008
File chrome/browser/gtk/options/options_layout_gtk.cc (right):

http://codereview.chromium.org/1969006/diff/19001/20008#newcode9
chrome/browser/gtk/options/options_layout_gtk.cc:9: const int
kCompactScreenHeight = 600;
Can you add a comment above this describing what it means?  E.g. "If the screen
is shorter than this, we will use a more compact option layout."

http://codereview.chromium.org/1969006/diff/19001/20008#newcode24
chrome/browser/gtk/options/options_layout_gtk.cc:24: GtkWidget* page_;
I guess you can put this (and the get_page_widget()) function into the
superclass.

http://codereview.chromium.org/1969006/diff/19001/20008#newcode90
chrome/browser/gtk/options/options_layout_gtk.cc:90: expandable ? GTK_FILL :
GtkAttachOptions(GTK_FILL|GTK_EXPAND),
Can you wrap this to fit in 80 columns?

http://codereview.chromium.org/1969006/diff/19001/20008#newcode97
chrome/browser/gtk/options/options_layout_gtk.cc:97:
OptionsLayoutBuilderGtk::Create(bool automatic) {
Can you fit this on one line?

http://codereview.chromium.org/1969006/diff/19001/20009
File chrome/browser/gtk/options/options_layout_gtk.h (right):

http://codereview.chromium.org/1969006/diff/19001/20009#newcode21
chrome/browser/gtk/options/options_layout_gtk.h:21: bool expandable) = 0;
The "bool" should line up with the "const" above it (both in the same column, to
the right of the open paren).

http://codereview.chromium.org/1969006/diff/19001/20009#newcode29
chrome/browser/gtk/options/options_layout_gtk.h:29: static
OptionsLayoutBuilderGtk* Create(bool automatic = false);
Our style guide doesn't allow default variables.  How about making Create() do
the automatic=false behavior and then add another function like
CreateOptionallyCompactLayout() or somethign?

[ningxin.hu, 2010-06-08 19:06:50]

evan,

Thanks a lot for you comments. 

I updated the patches according to your suggestions. 
Besides that, I merged with trunk at svn49167
(gitca0bcab87b746d2e3c8ad44be7981d1ddaaccdc1). A new API AddWidget is added into
OptionsLayoutBuilderGtk. I updated the patches for that change.

And I found some trunk changes are also included in the patch set. It might
impact your review. How do I resolve this? Need I request a new review branched
from trunk?

Thanks.

http://codereview.chromium.org/1969006/diff/19001/20008
File chrome/browser/gtk/options/options_layout_gtk.cc (right):

http://codereview.chromium.org/1969006/diff/19001/20008#newcode9
chrome/browser/gtk/options/options_layout_gtk.cc:9: const int
kCompactScreenHeight = 600;
On 2010/06/07 23:32:24, Evan Martin wrote:
> Can you add a comment above this describing what it means?  E.g. "If the
screen
> is shorter than this, we will use a more compact option layout."

Done.

http://codereview.chromium.org/1969006/diff/19001/20008#newcode24
chrome/browser/gtk/options/options_layout_gtk.cc:24: GtkWidget* page_;
On 2010/06/07 23:32:24, Evan Martin wrote:
> I guess you can put this (and the get_page_widget()) function into the
> superclass.

Done.

http://codereview.chromium.org/1969006/diff/19001/20008#newcode90
chrome/browser/gtk/options/options_layout_gtk.cc:90: expandable ? GTK_FILL :
GtkAttachOptions(GTK_FILL|GTK_EXPAND),
On 2010/06/07 23:32:24, Evan Martin wrote:
> Can you wrap this to fit in 80 columns?

Done.

http://codereview.chromium.org/1969006/diff/19001/20008#newcode97
chrome/browser/gtk/options/options_layout_gtk.cc:97:
OptionsLayoutBuilderGtk::Create(bool automatic) {
On 2010/06/07 23:32:24, Evan Martin wrote:
> Can you fit this on one line?

Done.

http://codereview.chromium.org/1969006/diff/19001/20009
File chrome/browser/gtk/options/options_layout_gtk.h (right):

http://codereview.chromium.org/1969006/diff/19001/20009#newcode21
chrome/browser/gtk/options/options_layout_gtk.h:21: bool expandable) = 0;
On 2010/06/07 23:32:24, Evan Martin wrote:
> The "bool" should line up with the "const" above it (both in the same column,
to
> the right of the open paren).

Done.

http://codereview.chromium.org/1969006/diff/19001/20009#newcode29
chrome/browser/gtk/options/options_layout_gtk.h:29: static
OptionsLayoutBuilderGtk* Create(bool automatic = false);
On 2010/06/07 23:32:24, Evan Martin wrote:
> Our style guide doesn't allow default variables.  How about making Create() do
> the automatic=false behavior and then add another function like
> CreateOptionallyCompactLayout() or somethign?

Done.

[Evan Martin, 2010-06-08 20:54:56]

Looks good.  Screenshot for the curious:

http://i.imgur.com/TlmwZ.png

I'll land this once the tree is open again.

[ningxin.hu, 2010-06-09 02:46:40]

On 2010/06/08 20:54:56, Evan Martin wrote:
> Looks good.  Screenshot for the curious:
> 
> http://i.imgur.com/TlmwZ.png
> 
> I'll land this once the tree is open again.

Thanks.

[Evan Martin, 2010-06-09 20:07:30]

r49291.  Thanks for the patch, and sorry it took so long!