Combobox: Rename styles to STYLE_NORMAL and STYLE_ACTION and modify behaviors

ui/views/controls/combobox/combobox.cc

Index: ui/views/controls/combobox/combobox.cc
diff --git a/ui/views/controls/combobox/combobox.cc b/ui/views/controls/combobox/combobox.cc
index bfbafa5ca3ae05ddaeb9c28a707fe048738d30be..ca8e6204299ca8d7fab14edf05cdb26f6e476c5a 100644
--- a/ui/views/controls/combobox/combobox.cc
+++ b/ui/views/controls/combobox/combobox.cc
@@ -219,7 +219,7 @@ const char Combobox::kViewClassName[] = "views/Combobox";
 
 Combobox::Combobox(ui::ComboboxModel* model)
     : model_(model),
-      style_(STYLE_SHOW_DROP_DOWN_ON_CLICK),
+      style_(STYLE_NORMAL),
       listener_(NULL),
       selected_index_(model_->GetDefaultIndex()),
       invalid_(false),
@@ -276,8 +276,11 @@ void Combobox::SetStyle(Style style) {
     return;
 
   style_ = style;
+  if (style_ == STYLE_ACTION)
+    selected_index_ = 0;
 
   UpdateBorder();
+  UpdateFromModel();
   PreferredSizeChanged();
 }
 
@@ -288,11 +291,17 @@ void Combobox::ModelChanged() {
 }
 
 void Combobox::SetSelectedIndex(int index) {
+  if (style_ == STYLE_ACTION)
+    return;
+
   selected_index_ = index;
   SchedulePaint();
 }
 
 bool Combobox::SelectValue(const base::string16& value) {
+  if (style_ == STYLE_ACTION)
+    return false;
+
   for (int i = 0; i < model()->GetItemCount(); ++i) {
     if (value == model()->GetItemAt(i)) {
       SetSelectedIndex(i);
@@ -330,11 +339,11 @@ void Combobox::Layout() {
   int arrow_button_width = 0;
 
   switch (style_) {
-    case STYLE_SHOW_DROP_DOWN_ON_CLICK: {
+    case STYLE_NORMAL: {
       arrow_button_width = width();
       break;
     }
-    case STYLE_NOTIFY_ON_CLICK: {
+    case STYLE_ACTION: {
       arrow_button_width = GetDisclosureArrowLeftPadding() +
           disclosure_arrow_->width() + GetDisclosureArrowRightPadding();
       text_button_width = width() - arrow_button_width;
@@ -358,6 +367,8 @@ bool Combobox::IsCommandEnabled(int id) const {
 void Combobox::ExecuteCommand(int id) {
   selected_index_ = MenuCommandToIndex(id);
   OnSelectionChanged();
+  if (style_ == STYLE_ACTION)

[sky, 2014/01/28 14:57:14]

Isn't the selectionchanged enough here? By that I mean why do we need the
OnTextButtonClicked too? Seems like this should just notify the selection
changed, then reset selection. Also, I think you have to handle the case of the
selection changed deleting this.

[hajimehoshi, 2014/01/29 05:28:38]

How about pressing enter or space key? In this case, SelectionChanged should be
called with 0? I don't think this is natural...
Oh, right. Maybe this problem is similar to
https://codereview.chromium.org/104573003? Regarding the possibility that the
consumer deletes the combobox during handling the click event, it seems to be
impossible to revert the selected index to 0.

How about adding an argument to OnComboboxTextButtonClicked, which indicates the
action index the user wants to cause? |selected_index_| wouldn't be changed and
remain 0 in this case.

[sky, 2014/01/29 17:16:56]

If the menu is open and you press space/enter SelectionChanged is invoked. So it
doesn't seem that unreasonable to me to send it for the cases you care about
too.
It just means you need a weak ref.
Actually, I think you should nuke OnComboboxTextButtonClicked and only send
SelectionChanged. Perhaps renaming SelectionChanged to PerformAction with the
index.

[hajimehoshi, 2014/01/29 21:32:30]

I mean that if the action combobox is focused and enter or space is pressed
without opening the menu, the first action will be fired, but I'm not sure
SelectionChanged with |selected_index_| 0 is appropriate in that case.

+    HandleClickEvent();
 }
 
 bool Combobox::GetAccelerator(int id, ui::Accelerator* accel) {
@@ -456,7 +467,7 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
 
     // Click the button only when the button style mode.
     case ui::VKEY_SPACE:
-      if (style_ == STYLE_NOTIFY_ON_CLICK) {
+      if (style_ == STYLE_ACTION) {
         // When pressing space, the click event will be raised after the key is
         // released.
         text_button_->SetState(Button::STATE_PRESSED);
@@ -467,7 +478,7 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
 
     // Click the button only when the button style mode.
     case ui::VKEY_RETURN:
-      if (style_ != STYLE_NOTIFY_ON_CLICK)
+      if (style_ != STYLE_ACTION)
         return false;
       HandleClickEvent();
       break;
@@ -480,19 +491,21 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
     UpdateFromModel();
     ShowDropDownMenu(ui::MENU_SOURCE_KEYBOARD);
   } else if (new_index != selected_index_ && new_index != kNoSelection) {
-    DCHECK(!model()->IsItemSeparatorAt(new_index));
-    selected_index_ = new_index;
-    OnSelectionChanged();
+    if (style_ != STYLE_ACTION) {
+      DCHECK(!model()->IsItemSeparatorAt(new_index));
+      selected_index_ = new_index;
+      OnSelectionChanged();
+    }
   }
 
   return true;
 }
 
 bool Combobox::OnKeyReleased(const ui::KeyEvent& e) {
-  if (style_ != STYLE_NOTIFY_ON_CLICK)
+  if (style_ != STYLE_ACTION)
     return false;  // crbug.com/127520
 
-  if (e.key_code() == ui::VKEY_SPACE)
+  if (e.key_code() == ui::VKEY_SPACE && style_ == STYLE_ACTION)
     HandleClickEvent();
 
   return false;
@@ -500,13 +513,13 @@ bool Combobox::OnKeyReleased(const ui::KeyEvent& e) {
 
 void Combobox::OnPaint(gfx::Canvas* canvas) {
   switch (style_) {
-    case STYLE_SHOW_DROP_DOWN_ON_CLICK: {
+    case STYLE_NORMAL: {
       OnPaintBackground(canvas);
       PaintText(canvas);
       OnPaintBorder(canvas);
       break;
     }
-    case STYLE_NOTIFY_ON_CLICK: {
+    case STYLE_ACTION: {
       PaintButtons(canvas);
       PaintText(canvas);
       break;
@@ -565,7 +578,6 @@ void Combobox::ButtonPressed(Button* sender, const ui::Event& event) {
 }
 
 void Combobox::UpdateFromModel() {
-  int max_width = 0;
   const gfx::FontList& font_list = Combobox::GetFontList();
 
   MenuItemView* menu = new MenuItemView(this);
@@ -573,6 +585,7 @@ void Combobox::UpdateFromModel() {
   dropdown_list_menu_runner_.reset(new MenuRunner(menu));
 
   int num_items = model()->GetItemCount();
+  int width = 0;
   for (int i = 0; i < num_items; ++i) {
     if (model()->IsItemSeparatorAt(i)) {
       menu->AppendSeparator();
@@ -586,15 +599,17 @@ void Combobox::UpdateFromModel() {
     base::i18n::AdjustStringForLocaleDirection(&text);
 
     menu->AppendMenuItem(i + kFirstMenuItemId, text, MenuItemView::NORMAL);
-    max_width = std::max(max_width, gfx::GetStringWidth(text, font_list));
+
+    if (style_ != STYLE_ACTION || i == selected_index_)
+      width = std::max(width, gfx::GetStringWidth(text, font_list));
   }
 
-  content_size_.SetSize(max_width, font_list.GetHeight());
+  content_size_.SetSize(width, font_list.GetHeight());
 }
 
 void Combobox::UpdateBorder() {
   scoped_ptr<FocusableBorder> border(new FocusableBorder());
-  if (style_ == STYLE_NOTIFY_ON_CLICK)
+  if (style_ == STYLE_ACTION)
     border->SetInsets(8, 13, 8, 13);
   if (invalid_)
     border->SetColor(kWarningColor);
@@ -646,7 +661,7 @@ void Combobox::PaintText(gfx::Canvas* canvas) {
 }
 
 void Combobox::PaintButtons(gfx::Canvas* canvas) {
-  DCHECK(style_ == STYLE_NOTIFY_ON_CLICK);
+  DCHECK(style_ == STYLE_ACTION);
 
   gfx::ScopedCanvas scoped_canvas(canvas);
   if (base::i18n::IsRTL()) {
@@ -768,9 +783,9 @@ int Combobox::MenuCommandToIndex(int menu_command_id) const {
 
 int Combobox::GetDisclosureArrowLeftPadding() const {
   switch (style_) {
-    case STYLE_SHOW_DROP_DOWN_ON_CLICK:
+    case STYLE_NORMAL:
       return kDisclosureArrowLeftPadding;
-    case STYLE_NOTIFY_ON_CLICK:
+    case STYLE_ACTION:
       return kDisclosureArrowButtonLeftPadding;
   }
   NOTREACHED();
@@ -779,9 +794,9 @@ int Combobox::GetDisclosureArrowLeftPadding() const {
 
 int Combobox::GetDisclosureArrowRightPadding() const {
   switch (style_) {
-    case STYLE_SHOW_DROP_DOWN_ON_CLICK:
+    case STYLE_NORMAL:
       return kDisclosureArrowRightPadding;
-    case STYLE_NOTIFY_ON_CLICK:
+    case STYLE_ACTION:
       return kDisclosureArrowButtonRightPadding;
   }
   NOTREACHED();
@@ -789,11 +804,13 @@ int Combobox::GetDisclosureArrowRightPadding() const {
 }
 
 void Combobox::HandleClickEvent() {
-  if (style_ != STYLE_NOTIFY_ON_CLICK)
+  if (style_ != STYLE_ACTION)
     return;
 
   if (listener_)
     listener_->OnComboboxTextButtonClicked(this);
+
+  selected_index_ = 0;

[sky, 2014/01/28 14:57:14]

Same worry about the possibility of this being deleted.

 }
 
 }  // namespace views