Notifications: Retain button hover state during content updates.

ui/message_center/views/notification_view.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/views/notification_view.h"
 
 #include "base/command_line.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "grit/ui_resources.h"
@@ skipping 433 matching lines @@
 }
 
 views::View* NotificationView::GetEventHandlerForRect(const gfx::Rect& rect) {
   // TODO(tdanderson): Modify this function to support rect-based event
   // targeting. Using the center point of |rect| preserves this function's
   // expected behavior for the time being.
   gfx::Point point = rect.CenterPoint();
 
   // Want to return this for underlying views, otherwise GetCursor is not
   // called. But buttons are exceptions, they'll have their own event handlings.
-  std::vector<views::View*> buttons(action_buttons_);
+  std::vector<View*> buttons(action_buttons_.begin(), action_buttons_.end());

[Jun Mukai, 2014/05/13 07:23:23]

I think omitting views:: namespace leads to a compile error?

[dewittj, 2014/05/20 18:32:13]

Pretty sure I compiled this :) but I'll check

[dewittj, 2014/05/21 21:13:30]

Done.

   buttons.push_back(close_button());
 
   for (size_t i = 0; i < buttons.size(); ++i) {
     gfx::Point point_in_child = point;
     ConvertPointToTarget(this, buttons[i], &point_in_child);
     if (buttons[i]->HitTestPoint(point_in_child))
       return buttons[i]->GetEventHandlerForPoint(point_in_child);
   }
 
   return this;
@@ skipping 230 matching lines @@
   if (!notification.image().IsEmpty()) {
     gfx::Size image_size(kNotificationPreferredImageWidth,
                          kNotificationPreferredImageHeight);
     image_view_ = MakeNotificationImage(notification.image(), image_size);
     bottom_view_->AddChildView(image_view_);
   }
 }
 
 void NotificationView::CreateOrUpdateActionButtonViews(
     const Notification& notification) {
-  for (size_t i = 0; i < separators_.size(); ++i)
-    delete separators_[i];
-  separators_.clear();
+  std::vector<ButtonInfo> buttons = notification.buttons();

[Jun Mukai, 2014/05/13 07:23:23]

const &?

[dewittj, 2014/05/20 18:32:13]

I don't like marking the result of a method call as const &.  It's not too big
of a copy anyway.  If you feel strongly I can update.

+  bool new_buttons = action_buttons_.size() != buttons.size();
 
-  for (size_t i = 0; i < action_buttons_.size(); ++i)
-    delete action_buttons_[i];
-  action_buttons_.clear();
+  if (new_buttons || buttons.size() == 0) {

[Jun Mukai, 2014/05/13 07:23:23]

Does this means that if the hover state will be lost if the number of buttons
has changed?
Consider the case like:
- notification created, with button A and B
- mouse hover on A
- notification updated to button A (button B removed)


There is a similar but a bit trickier scenario like:
- notification updated to button B (button A removed)
in this case -- I personally think the button B should get the hover background,
because the mouse cursor still exists there.


Considering this, it would be better to invoke
GetWidget()->SynthesizeMouseMoveEvent() which rerun a dummy mouse move event for
this purpose.
MessageCenterView already has this for refocusing the close button: see
message_center_view.cc for the details.

[dewittj, 2014/05/20 18:32:13]

It does mean this.  However for frequent updates, SynthesizeMouseEvent is not
sufficient to prevent flicker.  I'll try adding a SynthesizeMouseEvent if the
number of buttons changes.

+    for (size_t i = 0; i < separators_.size(); ++i)
+      delete separators_[i];
+    separators_.clear();

[Jun Mukai, 2014/05/13 07:23:23]

I think STLDeleteElements(&separators_); (in base/stl_util.h) would be more
concise.

[dewittj, 2014/05/20 18:32:13]

Will do!

+
+    for (size_t i = 0; i < action_buttons_.size(); ++i)
+      delete action_buttons_[i];
+    action_buttons_.clear();

[Jun Mukai, 2014/05/13 07:23:23]

ditto

[dewittj, 2014/05/21 21:13:30]

Done.

+  }
 
   DCHECK(bottom_view_);
   DCHECK_EQ(this, bottom_view_->parent());
 
-  std::vector<ButtonInfo> buttons = notification.buttons();
   for (size_t i = 0; i < buttons.size(); ++i) {
-    views::View* separator = new views::ImageView();
-    separator->SetBorder(MakeSeparatorBorder(1, 0, kButtonSeparatorColor));
-    separators_.push_back(separator);
-    bottom_view_->AddChildView(separator);
-    NotificationButton* button = new NotificationButton(this);
     ButtonInfo button_info = buttons[i];
-    button->SetTitle(button_info.title);
-    button->SetIcon(button_info.icon.AsImageSkia());
-    action_buttons_.push_back(button);
-    bottom_view_->AddChildView(button);
+    if (new_buttons) {
+      views::View* separator = new views::ImageView();
+      separator->SetBorder(MakeSeparatorBorder(1, 0, kButtonSeparatorColor));
+      separators_.push_back(separator);
+      bottom_view_->AddChildView(separator);
+      NotificationButton* button = new NotificationButton(this);
+      button->SetTitle(button_info.title);
+      button->SetIcon(button_info.icon.AsImageSkia());
+      action_buttons_.push_back(button);
+      bottom_view_->AddChildView(button);
+    } else {
+      action_buttons_[i]->SetTitle(button_info.title);
+      action_buttons_[i]->SetIcon(button_info.icon.AsImageSkia());
+      action_buttons_[i]->SchedulePaint();
+      action_buttons_[i]->Layout();
+    }
   }
 }
 
 int NotificationView::GetMessageLineLimit(int title_lines, int width) {
   // Image notifications require that the image must be kept flush against
   // their icons, but we can allow more text if no image.
   int effective_title_lines = std::max(0, title_lines - 1);
   int line_reduction_from_title = (image_view_ ? 1 : 2) * effective_title_lines;
   if (!image_view_) {
     // Title lines are counted as twice as big as message lines for the purpose
@@ skipping 25 matching lines @@
 
   return message_line_limit;
 }
 
 int NotificationView::GetMessageHeight(int width, int limit) {
   return message_view_ ?
          message_view_->GetSizeForWidthAndLines(width, limit).height() : 0;
 }
 
 }  // namespace message_center