Updated multiple item notification view to match latest mockups

ui/message_center/message_view_multiple.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/message_center/message_view_multiple.h"
 
 #include "base/utf_string_conversions.h"
 #include "grit/ui_resources.h"
 #include "ui/base/accessibility/accessible_view_state.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/text/text_elider.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/size.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/grid_layout.h"
 
 namespace {
 
 // Notification dimensions.
-const int kNotificationPrimaryIconSize = 64;
-const int kNotificationSecondaryIconSize = 15;
-const int kNotificationPadding1Width = 12;
-const int kNotificationColumn1Width = kNotificationPrimaryIconSize;
-const int kNotificationPadding2Width = 12;
-const int kNotificationPadding3Width = 12;
-const int kNotificationColumn3Width = 26;
-const int kNotificationPadding4Width = 10;
-const int kNotificationColumn4Width = 8;
-const int kNotificationPadding5Width = 8;
-const int kNotificationColumn1Top = 12;
-const int kNotificationColumn2Top = 9;
-const int kNotificationColumn3Top = 4;
-const int kNotificationColumn4Top = 8;
-const int kNotificationPaddingBottom = 19;
-const int kNotificationItemInternalPadding = 12;
+const int kPrimaryIconSize = 80;
+const int kIconLeftPadding = 0;
+const int kIconColumnWidth = kPrimaryIconSize;
+const int kIconToTextPadding = 15;
+const int kTextToClosePadding = 10;
+const int kCloseColumnWidth = 8;
+const int kCloseRightPadding = 6;
+const int kIconTopPadding = 0;
+const int kTextTopPadding = 9;
+const int kCloseTopPadding = 6;
+const int kIconBottomPadding = 0;
+const int kTextBottomPadding = 12;
+const int kItemTitleToDetailsPadding = 3;
 
-// The text background colors below are used only to prevent view::Label from
-// modifying the text color and will never be used for drawing. See
-// view::Label's SetEnabledColor() and SetBackgroundColor() for details.
-const SkColor kNotificationBackgroundColor = SkColorSetRGB(254, 254, 254);
-const SkColor kNotificationReadBackgroundColor = SkColorSetRGB(250, 250, 250);
-const SkColor kNotificationTitleColor = SkColorSetRGB(68, 68, 68);
-const SkColor kNotificationTitleBackgroundColor = SK_ColorWHITE;
-const SkColor kNotificationMessageColor = SkColorSetRGB(136, 136, 136);
-const SkColor kNotificationMessageBackgroundColor = SK_ColorBLACK;
-const SkColor kNotificationTimeColor = SkColorSetRGB(176, 176, 176);
-const SkColor kNotificationTimeBackgroundColor = SK_ColorBLACK;
-const SkColor kNotificationItemTitleColor = SkColorSetRGB(68, 68, 68);
-const SkColor kNotificationItemMessageColor = SkColorSetRGB(136, 136, 136);
-const SkColor kNotificationSeparatorColor = SkColorSetRGB(226, 226, 226);
+// Notification colors. The text background colors below are used only to keep
+// view::Label from modifying the text color and will not actually be drawn.
+// See view::Label's SetEnabledColor() and SetBackgroundColor() for details.
+const SkColor kBackgroundColor = SkColorSetRGB(255, 255, 255);
+const SkColor kTitleColor = SkColorSetRGB(68, 68, 68);
+const SkColor kTitleBackgroundColor = SK_ColorWHITE;
+const SkColor kItemTitleColor = SkColorSetRGB(68, 68, 68);
+const SkColor kItemMessageColor = SkColorSetRGB(136, 136, 136);
+
+// Static function to create an empty border to be used as padding.
+views::Border* MakePadding(int top, int left, int bottom, int right) {
+  return views::Border::CreateEmptyBorder(top, left, bottom, right);
+}
 
 // ItemViews are responsible for drawing each MessageViewMultiple item's title
 // and message next to each other within a single column.
 class ItemView : public views::View {
  public:
   ItemView(const message_center::NotificationList::NotificationItem& item)
-    : item_(item), preferred_size_(0, 0) {
+    : item_(item),
+      size_(0, 0) {

[Jun Mukai, 2012/12/19 00:33:53]

It's not this, but I noticed that ctor/dtor is declared inline but other methods
are not.  Can you keep the consistency?
Personally I think you can declare all inline in this class.

[dharcourt, 2012/12/19 01:47:44]

I de-inlined the constructor & destructor to make things consistent. It turns
out http://goo.gl/S8DJC suggests I should never have inlined them.

     gfx::Font font = GetDefaultFont();
-    preferred_size_.Enlarge(font.GetStringWidth(item.title), 0);
-    preferred_size_.Enlarge(kNotificationItemInternalPadding, 0);
-    preferred_size_.Enlarge(font.GetStringWidth(item.message), 0);
-    preferred_size_.set_height(font.GetHeight());
+    size_.Enlarge(font.GetStringWidth(item.title), 0);
+    size_.Enlarge(kItemTitleToDetailsPadding, 0);
+    size_.Enlarge(font.GetStringWidth(item.message), 0);
+    size_.set_height(font.GetHeight());
     PreferredSizeChanged();
     SchedulePaint();
   }
 
+  virtual ~ItemView() {
+  }
+
   // Overridden from views::View.
   virtual gfx::Size GetPreferredSize() OVERRIDE;
   virtual int GetBaseline() const OVERRIDE;
   virtual bool HitTestRect(const gfx::Rect& rect) const OVERRIDE;
   virtual void GetAccessibleState(ui::AccessibleViewState* state) OVERRIDE;
   virtual bool GetTooltipText(const gfx::Point& point,
                               string16* tooltip) const OVERRIDE;
 
  protected:
   // Overridden from views::View.
   virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE;

[Jun Mukai, 2012/12/19 00:33:53]

merge line 71-76 and this line.  It doesn't make much sense some inherited
methods are public but some are protected.  I personally prefer everything in
protected.

[dharcourt, 2012/12/19 01:47:44]

I didn't know this was possible in C++. Wouldn't turning superclass public
methods into private methods be confusing, though? http://goo.gl/ipVfR suggests
some problems it could cause. I'd rather avoid doing that if possible, either by
leaving things as is or by making OnPaint() public instead of making the public
methods protected. Of course, I'd be happy to make all the methods protected if
that's a common pattern here and if it helps readability..

[Jun Mukai, 2012/12/19 22:28:41]

It is quite common among chromium code base to put inherited methods to
protected or private. I don't agree with the discussion on the stack overflow...
but anyway, here isn't the place to discuss about that.
I'd say the consistency is more important.  Please move OnPaint to public then.

[dharcourt, 2012/12/20 03:27:36]

After implementing your suggestions for using Labels in ItemView, these methods
have all gone away, so this is moot :-). Otherwise I would have been happy to
make all the methods protected for consistency with the code base (in any case,
because ItemView is a private class, changing the access modifiers would have no
significant effect except improving code readability). I'd like to discuss this
general pattern with you offline to better understand it.

 
  private:
   static gfx::Font GetDefaultFont();
 
   message_center::NotificationList::NotificationItem item_;
-  gfx::Size preferred_size_;
+  gfx::Size size_;
 
   DISALLOW_COPY_AND_ASSIGN(ItemView);
 };
 
 gfx::Size ItemView::GetPreferredSize() {
-  return preferred_size_;
+  gfx::Insets insets = GetInsets();
+  return gfx::Size(insets.left() + size_.width() + insets.right(),
+                   insets.top() + size_.height() + insets.bottom());
 }
 
 int ItemView::GetBaseline() const {
-  return GetDefaultFont().GetBaseline();
+  gfx::Insets insets = GetInsets();
+  return insets.top() + GetDefaultFont().GetBaseline();
 }
 
 bool ItemView::HitTestRect(const gfx::Rect& rect) const {
   return false;
 }
 
-void ItemView::GetAccessibleState(
-    ui::AccessibleViewState* state) {
+void ItemView::GetAccessibleState(ui::AccessibleViewState* state) {
   state->role = ui::AccessibilityTypes::ROLE_STATICTEXT;
   state->state = ui::AccessibilityTypes::STATE_READONLY;
   state->name = item_.message;  // TODO(dharcourt): Include title localizably.
 }
 
 bool ItemView::GetTooltipText(const gfx::Point& point,
                               string16* tooltip) const {
-  if (preferred_size_.width() > width()) {
+  gfx::Insets insets = GetInsets();
+  if (insets.left() + size_.width() + insets.right() > width()) {
     *tooltip = item_.message;  // TODO(dharcourt): Include title localizably.
     return true;
   }
   return false;
 }
 
 void ItemView::OnPaint(gfx::Canvas* canvas) {

[Jun Mukai, 2012/12/19 00:33:53]

Can you create views::Labels and put them to the children of this class (by
AddChildView()?)  You can align the title and message by horizontal BoxLayout. 
Then you don't need to override GetBaseline and OnPaint and all your class have
to care would be the width.

Also, after that I believe you don't need to override AccessibleState.  Label
class will take care instead of you.

[dharcourt, 2012/12/19 01:47:44]

I'm trying this out... I'll upload a patch and update this comment when I have
it working.

[dharcourt, 2012/12/20 03:27:36]

Done, and it works very well and simplifies things.

   OnPaintBackground(canvas);
   OnPaintBorder(canvas);
   OnPaintFocusBorder(canvas);
 
   gfx::Font font = GetDefaultFont();
-  int y = std::max(0, height() - preferred_size_.height()) / 2;
-  canvas->DrawStringInt(item_.title, font, kNotificationItemTitleColor,
-                        0, y, width(), preferred_size_.height());
+  gfx::Insets insets = GetInsets();
+  int x = insets.left();
+  int y = insets.top();
+  canvas->DrawStringInt(item_.title, font, kItemTitleColor,
+                        x, y, width() - x - insets.right(), size_.height());
 
-  int x = font.GetStringWidth(item_.title) + kNotificationItemInternalPadding;
-  if (x < width()) {
+  int used = x + font.GetStringWidth(item_.title) + kItemTitleToDetailsPadding;
+  int remaining = width() - used - insets.right();
+  if (remaining > 0) {
     canvas->DrawStringInt(
-        ui::ElideText(item_.message, font, width() - x, ui::ELIDE_AT_END),
-        font, kNotificationItemMessageColor,
-        x, y, width() - x, preferred_size_.height());
+        ui::ElideText(item_.message, font, remaining, ui::ELIDE_AT_END),
+        font, kItemMessageColor,
+        used, y, remaining, size_.height());
   }
 }
 
 // static
 gfx::Font ItemView::GetDefaultFont() {
   return ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::BaseFont);
 }
 
 // BoxView draws a color rectangle or just reserves some space.
 class BoxView : public views::View {

[Jun Mukai, 2012/12/19 00:33:53]

This seems a transparent view used for padding. Am I right? Then, you don't need
this, rather you can just set EmptyBorder of the size (see ui/views/border.h) to
MessageViewMultiple.

[dharcourt, 2012/12/19 01:47:44]

This is indeed a transparent view used for padding, but I can't use an
EmptyBorder to do its job: The padding is needed by an ImageButton subview of
MessageViewMultiple, and ImageButton doesn't work well with borders (the image
gets placed in the wrong place and clipped). I end up having to create a row
with a BoxView above the ImageButton to have it padded correctly relative to the
icon in another column.

[Jun Mukai, 2012/12/19 22:28:41]

ah, I've misunderstood. thanks for description.

[dharcourt, 2012/12/20 03:27:36]

Inspired by the ItemView reduction in size, and prompted by your comment, I
tried using an empty ImageView in conjunction with borders to get the padding I
need, and it works (see code changes below).

Box view is now unnecessary and a bunch of code is avoided :-).

  public:
   BoxView(int width, int height, SkColor color=SkColorSetARGB(0, 0, 0, 0))
-      : size_(width, height) {
-    if (SkColorGetA(color) > 0)
-      set_background(views::Background::CreateSolidBackground(color));
+      : size_(width, height),
+        color_(color) {
     PreferredSizeChanged();
     SchedulePaint();
   }
 
+  virtual ~BoxView() {
+  }
+
   // Overridden from View:
   virtual gfx::Size GetPreferredSize() OVERRIDE;
   virtual bool HitTestRect(const gfx::Rect& rect) const OVERRIDE;
 
+ protected:
+  // Overridden from views::View.
+  virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE;
+
  private:
   gfx::Size size_;
+  SkColor color_;
 
   DISALLOW_COPY_AND_ASSIGN(BoxView);
 };
 
 gfx::Size BoxView::GetPreferredSize() {
   return size_;
 }
 
 bool BoxView::HitTestRect(const gfx::Rect& rect) const {
   return false;
 }
 
+void BoxView::OnPaint(gfx::Canvas* canvas) {
+  if (SkColorGetA(color_) > 0)
+    canvas->FillRect(GetContentsBounds(), color_);
+}
+
 }  // namespace
 
 namespace message_center {
 
 MessageViewMultiple::MessageViewMultiple(
     NotificationList::Delegate* list_delegate,
     const NotificationList::Notification& notification)
-    : MessageView(list_delegate, notification) {}
+    : MessageView(list_delegate, notification) {
+}
 
-MessageViewMultiple::MessageViewMultiple() {}
-
-MessageViewMultiple::~MessageViewMultiple() {}
+MessageViewMultiple::~MessageViewMultiple() {
+}
 
 // TODO(dharcourt): Make this a subclass of BaseFormatView or of a
 // BaseFormatView superclass and leverage that class' SetUpView().
 void MessageViewMultiple::SetUpView() {
-
-  SkColor background = notification_.is_read ?
-      kNotificationReadBackgroundColor : kNotificationBackgroundColor;
-  set_background(views::Background::CreateSolidBackground(background));
+  set_background(views::Background::CreateSolidBackground(kBackgroundColor));
 
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
-  // Four columns (icon, messages, time, close) surrounded by padding. The icon,
-  // time, and close columns have fixed width and the message column fills up
-  // the remaining space. Inter-column padding is included within columns
-  // whenever horizontal allignment allows for it.
+  // Three columns (icon, text, close button) surrounded by padding. The icon
+  // and close button columns and the padding have fixed widths and the text
+  // column fills up the remaining space. To minimize the number of columns and
+  // simplify column spanning padding is applied to each view within columns
+  // instead of through padding columns.
   views::ColumnSet* columns = layout->AddColumnSet(0);
-  columns->AddPaddingColumn(0, kNotificationPadding1Width);
-  columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::LEADING,
+  columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::LEADING,
                      0, views::GridLayout::FIXED,
-                     kNotificationColumn1Width, kNotificationColumn1Width);
-  columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::LEADING,
-                     0, views::GridLayout::FIXED,
-                     kNotificationPadding2Width, kNotificationPadding2Width);
+                     kIconLeftPadding + kIconColumnWidth + kIconToTextPadding,
+                     kIconLeftPadding + kIconColumnWidth + kIconToTextPadding);
+                     // Padding + icon + padding.
   columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL,
                      100, views::GridLayout::USE_PREF,
                      0, 0);
-  columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING,
+                     // Text + padding (kTextToClosePadding).
+  columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::LEADING,
                      0, views::GridLayout::FIXED,
-                     kNotificationPadding3Width + kNotificationColumn3Width,
-                     kNotificationPadding3Width + kNotificationColumn3Width);
-  columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING,
-                     0, views::GridLayout::FIXED,
-                     kNotificationPadding4Width + kNotificationColumn4Width,
-                     kNotificationPadding4Width + kNotificationColumn4Width);
-  columns->AddPaddingColumn(0, kNotificationPadding5Width);
+                     kCloseColumnWidth + kCloseRightPadding,
+                     kCloseColumnWidth + kCloseRightPadding);
+                     // Close button + padding.
 
-  // First row: Primary icon.
+  // First row: Icon. This vertically spans the close button padding row, the
+  // close button row, and all item rows.
   layout->StartRow(0, 0);
-  views::ImageView* primary_icon = new views::ImageView;
-  primary_icon->SetImageSize(gfx::Size(kNotificationPrimaryIconSize,
-                                       kNotificationPrimaryIconSize));
-  primary_icon->SetImage(notification_.primary_icon);
-  primary_icon->set_border(CreateTopBorder(kNotificationColumn1Top));
-  primary_icon->SetVerticalAlignment(views::ImageView::LEADING);
-  layout->AddView(primary_icon, 1, 3 + 2 * notification_.items.size());
+  views::ImageView* icon = new views::ImageView;
+  icon->SetImageSize(gfx::Size(kPrimaryIconSize, kPrimaryIconSize));
+  icon->SetImage(notification_.primary_icon);
+  icon->SetHorizontalAlignment(views::ImageView::LEADING);
+  icon->SetVerticalAlignment(views::ImageView::LEADING);
+  icon->set_border(MakePadding(kIconTopPadding, kIconLeftPadding,
+                               kIconBottomPadding, kIconToTextPadding));
+  layout->AddView(icon, 1, 2 + notification_.items.size());
 
-  // First row: Title.
+  // First row: Title. This vertically spans the close button padding row and
+  // the close button row.
   // TODO(dharcourt): Skip the title Label when there's no title text.
-  layout->SkipColumns(1);
   views::Label* title = new views::Label(notification_.title);
   title->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   title->SetFont(title->font().DeriveFont(4));
-  title->SetEnabledColor(kNotificationTitleColor);
-  title->SetBackgroundColor(kNotificationTitleBackgroundColor);
-  title->set_border(CreateTopBorder(kNotificationColumn2Top));
+  title->SetEnabledColor(kTitleColor);
+  title->SetBackgroundColor(kTitleBackgroundColor);
+  title->set_border(MakePadding(kTextTopPadding, 0, 3, kTextToClosePadding));
   layout->AddView(title, 1, 2);
 
-  // First row: Time.
-  // TODO(dharcourt): Timestamp as "1m/5m/1h/5h/1d/5d/..." (ago).
-  views::Label* time = new views::Label(UTF8ToUTF16(""));
-  time->SetEnabledColor(kNotificationTimeColor);
-  time->SetBackgroundColor(kNotificationTimeBackgroundColor);
-  time->set_border(CreateTopBorder(kNotificationColumn3Top));
-  layout->AddView(time, 1, 2);
+  // First row: Close button padding.
+  layout->AddView(new BoxView(1, kCloseTopPadding));
 
-  // First row: Close button padding.
-  layout->AddView(new BoxView(1, kNotificationColumn4Top));
-
-  // Second row: Close button, which has to be on a row of its own because it's
-  // an ImageButton and so its padding can't be set using borders. This row is
-  // set to resize vertically to ensure the first row stays at its minimum
-  // height of kNotificationColumn4Top.
+  // Second row: Close button, which has to be on a row of its own because its
+  // top padding can't be set using empty borders (ImageButtons don't support
+  // borders). The resize factor of this row (100) is much higher than that of
+  // other rows (0) to ensure the first row's height stays at kCloseTopPadding.
   layout->StartRow(100, 0);
-  layout->SkipColumns(4);
+  layout->SkipColumns(2);
   DCHECK(close_button_);
   layout->AddView(close_button_);
 
-  // Rows for each notification item, including appropriate padding.
-  layout->AddPaddingRow(0, 3);
-  std::vector<NotificationList::NotificationItem>::const_iterator i;
-  for (i = notification_.items.begin(); i != notification_.items.end(); ++i) {
-    layout->StartRowWithPadding(0, 0, 0, 3);
-    layout->SkipColumns(2);
-    layout->AddView(new ItemView(*i));
-    layout->SkipColumns(2);
+  // One row for each notification item, including appropriate padding.
+  for (int i = 0, n = notification_.items.size(); i < n; ++i) {
+    int bottom_padding = (i < n - 1) ? 4 : (kTextBottomPadding - 2);
+    layout->StartRow(0, 0);
+    layout->SkipColumns(1);
+    ItemView* item = new ItemView(notification_.items[i]);
+    item->set_border(MakePadding(0, 0, bottom_padding, kTextToClosePadding));
+    layout->AddView(item);
+    layout->SkipColumns(1);
   }
-
-  // Rows for horizontal separator line with appropriate padding.
-  layout->StartRowWithPadding(0, 0, 0, 6);
-  layout->SkipColumns(1);
-  layout->AddView(new BoxView(1000000, 1, kNotificationSeparatorColor), 4, 1);
-
-  // Last row: Summary message with padding.
-  // TODO(dharcourt): Skip the message Label when there's no message text.
-  layout->StartRowWithPadding(0, 0, 0, 5);
-  layout->SkipColumns(2);
-  views::Label* message = new views::Label(notification_.message);
-  message->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  message->SetMultiLine(true);
-  message->SetEnabledColor(kNotificationMessageColor);
-  message->SetBackgroundColor(kNotificationMessageBackgroundColor);
-  message->set_border(CreateTopBorder(1));
-  layout->AddView(message);
-
-  // Last row: Secondary icon.
-  layout->SkipColumns(1);
-  views::ImageView* secondary_icon = new views::ImageView;
-  secondary_icon->SetImageSize(gfx::Size(kNotificationSecondaryIconSize,
-                                         kNotificationSecondaryIconSize));
-  secondary_icon->SetImage(notification_.secondary_icon.isNull() ?
-                           notification_.primary_icon :
-                           notification_.secondary_icon);
-  secondary_icon->SetVerticalAlignment(views::ImageView::LEADING);
-  layout->AddView(secondary_icon);
-
-  // Final row with the bottom padding.
-  layout->AddPaddingRow(0, kNotificationPaddingBottom);
-}
-
-views::Border* MessageViewMultiple::CreateTopBorder(int height) {
-  return views::Border::CreateEmptyBorder(height, 0, 0, 0);
 }
 
 }  // namespace message_center