Notifications: Retain button hover state during content updates.

ui/message_center/views/notification_view.cc

Index: ui/message_center/views/notification_view.cc
diff --git a/ui/message_center/views/notification_view.cc b/ui/message_center/views/notification_view.cc
index 0884205e68f3077fa66d02e31638a5e3f820d205..76adbb8fe2d27f87f29d5c9786432a7b83795770 100644
--- a/ui/message_center/views/notification_view.cc
+++ b/ui/message_center/views/notification_view.cc
@@ -451,7 +451,7 @@ views::View* NotificationView::GetEventHandlerForRect(const gfx::Rect& rect) {
 
   // 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) {
@@ -702,29 +702,40 @@ void NotificationView::CreateOrUpdateImageView(
 
 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();
+
+  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();
+    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();
+    }
   }
 }