Updated multiple item notification view to match latest mockups

ui/message_center/message_view_multiple.cc

Index: ui/message_center/message_view_multiple.cc
diff --git a/ui/message_center/message_view_multiple.cc b/ui/message_center/message_view_multiple.cc
index c6cfc1f1a90130abbeed4de93a61a991f2957442..3233576336dc371ea2b54188c9d1f3a6af64a895 100644
--- a/ui/message_center/message_view_multiple.cc
+++ b/ui/message_center/message_view_multiple.cc
@@ -20,53 +20,53 @@
 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;
-
-// 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);
+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;
+
+// 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;
@@ -83,25 +83,27 @@ class ItemView : public views::View {
   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.
@@ -109,7 +111,8 @@ void ItemView::GetAccessibleState(
 
 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;
   }
@@ -122,16 +125,19 @@ void ItemView::OnPaint(gfx::Canvas* 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());
-
-  int x = font.GetStringWidth(item_.title) + kNotificationItemInternalPadding;
-  if (x < width()) {
+  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 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());
   }
 }
 
@@ -144,19 +150,26 @@ gfx::Font ItemView::GetDefaultFont() {
 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);
 };
@@ -169,6 +182,11 @@ 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 {
@@ -176,133 +194,86 @@ namespace message_center {
 MessageViewMultiple::MessageViewMultiple(
     NotificationList::Delegate* list_delegate,
     const NotificationList::Notification& notification)
-    : MessageView(list_delegate, notification) {}
-
-MessageViewMultiple::MessageViewMultiple() {}
+    : MessageView(list_delegate, notification) {
+}
 
-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,
-                     0, views::GridLayout::FIXED,
-                     kNotificationPadding3Width + kNotificationColumn3Width,
-                     kNotificationPadding3Width + kNotificationColumn3Width);
-  columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING,
+                     // Text + padding (kTextToClosePadding).
+  columns->AddColumn(views::GridLayout::LEADING, 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());
-
-  // First row: Title.
+  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. 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, kNotificationColumn4Top));
+  layout->AddView(new BoxView(1, kCloseTopPadding));
 
-  // 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