Redesign notifier settings view.

ui/message_center/views/notifier_settings_view.cc

Index: ui/message_center/views/notifier_settings_view.cc
diff --git a/ui/message_center/views/notifier_settings_view.cc b/ui/message_center/views/notifier_settings_view.cc
index 208a602d84d57fd8c095ec28f700fa4582b25c2d..facca489a923bcf2346f29fa1650afe24c287f46 100644
--- a/ui/message_center/views/notifier_settings_view.cc
+++ b/ui/message_center/views/notifier_settings_view.cc
@@ -7,6 +7,8 @@
 #include "grit/ui_strings.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/canvas.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/size.h"
 #include "ui/message_center/message_center_constants.h"
@@ -16,6 +18,7 @@
 #include "ui/views/controls/button/custom_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
+#include "ui/views/controls/scroll_view.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/widget/widget.h"
 
@@ -28,9 +31,66 @@ namespace {
 
 const int kSpaceInButtonComponents = 16;
 const int kMarginWidth = 16;
+const int kMinimumWindowWidth = 320;
+const int kMinimumWindowHeight = 480;
+const int kEntryHeight = kMinimumWindowHeight / 10;
+const SkColor kSeparatorColor = SkColorSetRGB(0xcc, 0xcc, 0xcc);
 
 NotifierSettingsView* settings_view_ = NULL;
 
+// The layout manager to guarantee the 48px height and place the view at the
+// middle.  It also guarantee the left margin.
+class NotifierSettingsEntryLayoutManager : public views::LayoutManager {

[dharcourt, 2013/03/13 23:33:56]

Nit: Since this class is private to NotifierSettingsView, the NotifierSettings
prefix in its name is IMHO redundant and the code might be more readable without
it. The Manager suffix might also be redundant since views LayoutManager
subclasses don't have it (BoxLayout, GridLayout, ...).

[dharcourt, 2013/03/13 23:33:56]

Suggestion: Having an EntryView that implements the same Layout() and
GetPreferredSize() methods might be simpler than using this EntryLayout  with
generic views::View. If you made EntryView's constructor take a child view as an
argument it could be used with a single line of code like
"contents_view->AddChildView(new EntryView(top_label))" instead of the current
4, and it would minimize the risk of ending up with 0 or more than 1 children to
lay out. Your call.

[Jun Mukai, 2013/03/14 06:47:48]

Changed to EntryView.  The code gets more concise. Thanks!

+ public:
+  NotifierSettingsEntryLayoutManager() {}
+  virtual ~NotifierSettingsEntryLayoutManager() {}

[dharcourt, 2013/03/13 23:33:56]

Nit: Chromium style recommends against inlining even empty constructors like the
one above, and prohibits inlining virtual functions like the destructor above
(http://dev.chromium.org/developers/coding-style#TOC-Inline-functions).

[Jun Mukai, 2013/03/14 06:47:48]

fixed...

+
+  virtual void Layout(views::View* host) OVERRIDE;
+  virtual gfx::Size GetPreferredSize(views::View* host) OVERRIDE;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NotifierSettingsEntryLayoutManager);
+};
+
+void NotifierSettingsEntryLayoutManager::Layout(views::View* host) {
+  if (!host->has_children())
+    return;
+
+  views::View* child = host->child_at(0);
+  gfx::Size size = child->GetPreferredSize();
+  int y = 0;
+  if (size.height() < host->height())
+    y = (host->height() - size.height()) / 2;
+  child->SetBounds(
+      kMarginWidth, y, host->width() - kMarginWidth, size.height());
+}
+
+gfx::Size NotifierSettingsEntryLayoutManager::GetPreferredSize(
+    views::View* host) {
+  DCHECK_EQ(1, host->child_count());

[dharcourt, 2013/03/13 23:33:56]

Q: Why is this a DCHECK whereas a similar check in Layout() isn't? Are there
situations where a particular layout will have 0 children in its Layout() but 1
in GetPreferredSize()?

[Jun Mukai, 2013/03/14 06:47:48]

Added DCHECK to both.

+  gfx::Size size = host->child_at(0)->GetPreferredSize();
+  size.ClampToMin(gfx::Size(kMinimumWindowWidth, kEntryHeight));
+  return size;
+}
+
+// The horizontal bar between title and scroller.
+class SeparatorView : public views::View {

[dharcourt, 2013/03/13 23:33:56]

Suggestion: It's kind of a hack, but you could replace this separator class with
a null-image ImageView with a 1 pixel colored border. Search for
kButtonSeparatorColor in notification_view.cc for an example. It would remove a
few lines of code but whether it would simplify the code or make it more complex
is an open question. Your call.

[Jun Mukai, 2013/03/14 06:47:48]

Thinking about this more, I decided to change it to a views::Border so I don't
need to care about its layout.
Unfortunately this kind of border (slightly short from the edge) isn't supported
in views::Border, so I still keep this as a class here.

+ public:
+  SeparatorView() {}
+  virtual ~SeparatorView() {}
+
+  virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SeparatorView);
+};
+
+void SeparatorView::OnPaint(gfx::Canvas* canvas) {
+  canvas->DrawLine(gfx::Point(kMarginWidth, 0),
+                   gfx::Point(width() - kMarginWidth, 0),
+                   kSeparatorColor);
+}
+
 }  // namespace
 
 NotifierSettingsDelegate* ShowSettings(NotifierSettingsProvider* provider,
@@ -152,42 +212,65 @@ NotifierSettingsView::NotifierSettingsView(
     : delegate_(delegate) {
   DCHECK(delegate_);
 
-  SetLayoutManager(new views::BoxLayout(
-      views::BoxLayout::kVertical, kMarginWidth, kMarginWidth, kMarginWidth));
   set_background(views::Background::CreateSolidBackground(SK_ColorWHITE));
 
-  views::Label* top_label = new views::Label(
-      l10n_util::GetStringUTF16(IDS_MESSAGE_CENTER_FOOTER_TITLE));
+  title_entry_ = new views::View();
+  title_entry_->SetLayoutManager(new NotifierSettingsEntryLayoutManager());
+  gfx::Font title_font =
+      ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::MediumFont);
+  views::Label* title_label = new views::Label(
+      l10n_util::GetStringUTF16(IDS_MESSAGE_CENTER_SETTINGS_BUTTON_LABEL),
+      title_font);
+  title_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  title_label->SetMultiLine(true);
+  title_entry_->AddChildView(title_label);
+  AddChildView(title_entry_);
+
+  separator_ = new SeparatorView();
+  AddChildView(separator_);
+
+  scroller_ = new views::ScrollView();
+  // TODO(mukai): set kennedy scroll bar here.
+  AddChildView(scroller_);
+
+  views::View* contents_view = new views::View();
+  contents_view->SetLayoutManager(new views::BoxLayout(
+      views::BoxLayout::kVertical, 0, 0, 0));
+
+  views::View* top_entry = new views::View();
+  top_entry->SetLayoutManager(new NotifierSettingsEntryLayoutManager());
+  views::Label* top_label = new views::Label(l10n_util::GetStringUTF16(
+      IDS_MESSAGE_CENTER_SETTINGS_DIALOG_DESCRIPTION));
   top_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  AddChildView(top_label);
-
-  views::View* items = new views::View();
-  items->SetLayoutManager(new views::BoxLayout(
-      views::BoxLayout::kVertical, 0, 0, kMarginWidth));
-  items->set_border(views::Border::CreateEmptyBorder(0, kMarginWidth, 0, 0));
-  AddChildView(items);
+  top_label->SetMultiLine(true);
+  top_label->SizeToFit(kMinimumWindowWidth - kMarginWidth);
+  top_entry->AddChildView(top_label);
+  contents_view->AddChildView(top_entry);
 
   std::vector<Notifier*> notifiers;
   delegate_->GetNotifierList(&notifiers);
   for (size_t i = 0; i < notifiers.size(); ++i) {
     NotifierButton* button = new NotifierButton(notifiers[i], this);
-    items->AddChildView(button);
+    views::View* button_entry = new views::View();
+    button_entry->SetLayoutManager(new NotifierSettingsEntryLayoutManager());
+    button_entry->AddChildView(button);
+    contents_view->AddChildView(button_entry);
     buttons_.insert(button);
   }
+  scroller_->SetContents(contents_view);
+
+  gfx::Size contents_size = contents_view->GetPreferredSize();
+  if (kMinimumWindowWidth <

[dharcourt, 2013/03/13 23:33:56]

Typo? Shouldn't this be kMinimumWindowHeight, and shouldn't the height being
compared include 1 pixel for the separator?

[Jun Mukai, 2013/03/14 06:47:48]

Aww, right. fixed.  Thanks!

+      title_entry_->GetPreferredSize().height() + contents_size.height()) {
+    contents_size.Enlarge(-scroller_->GetScrollBarWidth(), 0);
+  }
+  contents_view->SetBoundsRect(gfx::Rect(contents_size));
 }
 
 NotifierSettingsView::~NotifierSettingsView() {
   settings_view_ = NULL;
 }
 
-bool NotifierSettingsView::CanResize() const {
-  return true;
-}
-
-string16 NotifierSettingsView::GetWindowTitle() const {
-  return l10n_util::GetStringUTF16(IDS_MESSAGE_CENTER_SETTINGS_BUTTON_LABEL);
-}
-
 void NotifierSettingsView::WindowClosing() {
   if (delegate_)
     delegate_->OnNotifierSettingsClosing();
@@ -197,6 +280,18 @@ views::View* NotifierSettingsView::GetContentsView() {
   return this;
 }
 
+void NotifierSettingsView::Layout() {
+  int title_height = title_entry_->GetPreferredSize().height();
+  title_entry_->SetBounds(0, 0, width(), title_height);
+  separator_->SetBounds(0, title_height,
+                        width() - scroller_->GetScrollBarWidth(), 1);
+  scroller_->SetBounds(0, title_height, width(), height() - title_height);

[dharcourt, 2013/03/13 23:33:56]

Typo? Shouldn't y be (title_height + 1) and height (height - title_height - 1)
to account for the separator?

[Jun Mukai, 2013/03/14 06:47:48]

Since separator_ is gone (it now belongs to scroller_), the current code is
correct.

+}
+
+gfx::Size NotifierSettingsView::GetMinimumSize() {
+  return gfx::Size(kMinimumWindowWidth, kMinimumWindowHeight);
+}
+
 void NotifierSettingsView::ButtonPressed(views::Button* sender,
                                          const ui::Event& event) {
   std::set<NotifierButton*>::iterator iter = buttons_.find(