Combobox: Rename styles to STYLE_NORMAL and STYLE_ACTION and modify behaviors

ui/views/controls/combobox/combobox.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/views/controls/combobox/combobox.h"
 
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "grit/ui_resources.h"
 #include "ui/base/accessibility/accessible_view_state.h"
@@ skipping 201 matching lines @@
 }  // namespace
 
 // static
 const char Combobox::kViewClassName[] = "views/Combobox";
 
 ////////////////////////////////////////////////////////////////////////////////
 // Combobox, public:
 
 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),
       disclosure_arrow_(ui::ResourceBundle::GetSharedInstance().GetImageNamed(
           IDR_MENU_DROPARROW).ToImageSkia()),
       dropdown_open_(false),
       text_button_(new TransparentButton(this)),
       arrow_button_(new TransparentButton(this)) {
   model_->AddObserver(this);
   UpdateFromModel();
@@ skipping 36 matching lines @@
 const gfx::FontList& Combobox::GetFontList() {
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   return rb.GetFontList(ui::ResourceBundle::BaseFont);
 }
 
 void Combobox::SetStyle(Style style) {
   if (style_ == style)
     return;
 
   style_ = style;
+  if (style_ == STYLE_ACTION)
+    selected_index_ = 0;
 
   UpdateBorder();
+  UpdateFromModel();
   PreferredSizeChanged();
 }
 
 void Combobox::ModelChanged() {
   selected_index_ = std::min(0, model_->GetItemCount());
   UpdateFromModel();
   PreferredSizeChanged();
 }
 
 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);
       return true;
     }
   }
   return false;
 }
 
 void Combobox::SetAccessibleName(const base::string16& name) {
@@ skipping 17 matching lines @@
 }
 
 void Combobox::Layout() {
   PrefixDelegate::Layout();
 
   gfx::Insets insets = GetInsets();
   int text_button_width = 0;
   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;
       break;
     }
   }
 
   int arrow_button_x = std::max(0, text_button_width);
   text_button_->SetBounds(0, 0, std::max(0, text_button_width), height());
   arrow_button_->SetBounds(arrow_button_x, 0, arrow_button_width, height());
 }
 
 bool Combobox::IsItemChecked(int id) const {
   return false;
 }
 
 bool Combobox::IsCommandEnabled(int id) const {
   return model()->IsItemEnabledAt(MenuCommandToIndex(id));
 }
 
 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) {
   return false;
 }
 
 int Combobox::GetRowCount() {
   return model()->GetItemCount();
 }
 
@@ skipping 78 matching lines @@
       new_index = GetAdjacentIndex(model(), 1, -1);
       break;
 
     // Move to the previous item if any.
     case ui::VKEY_UP:
       new_index = GetAdjacentIndex(model(), -1, selected_index_);
       break;
 
     // 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);
       } else {
         return false;
       }
       break;
 
     // 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;
 
     default:
       return false;
   }
 
   if (show_menu) {
     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;
 }
 
 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;
     }
   }
 }
 
 void Combobox::OnFocus() {
   GetInputMethod()->OnFocus();
   View::OnFocus();
@@ skipping 38 matching lines @@
     ui::MenuSourceType source_type = ui::MENU_SOURCE_MOUSE;
     if (event.IsKeyEvent())
       source_type = ui::MENU_SOURCE_KEYBOARD;
     else if (event.IsGestureEvent() || event.IsTouchEvent())
       source_type = ui::MENU_SOURCE_TOUCH;
     ShowDropDownMenu(source_type);
   }
 }
 
 void Combobox::UpdateFromModel() {
-  int max_width = 0;
   const gfx::FontList& font_list = Combobox::GetFontList();
 
   MenuItemView* menu = new MenuItemView(this);
   // MenuRunner owns |menu|.
   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();
       continue;
     }
 
     base::string16 text = model()->GetItemAt(i);
 
     // Inserting the Unicode formatting characters if necessary so that the
     // text is displayed correctly in right-to-left UIs.
     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);
   SetBorder(border.PassAs<Border>());
 }
 
 void Combobox::AdjustBoundsForRTLUI(gfx::Rect* rect) const {
   rect->set_x(GetMirroredXForRect(*rect));
 }
 
@@ skipping 31 matching lines @@
   gfx::Rect arrow_bounds(arrow_x,
                          height() / 2 - disclosure_arrow_->height() / 2,
                          disclosure_arrow_->width(),
                          disclosure_arrow_->height());
   AdjustBoundsForRTLUI(&arrow_bounds);
 
   canvas->DrawImageInt(*disclosure_arrow_, arrow_bounds.x(), arrow_bounds.y());
 }
 
 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()) {
     canvas->Translate(gfx::Vector2d(width(), 0));
     canvas->Scale(-1, 1);
   }
 
   bool focused = HasFocus();
   const std::vector<const gfx::ImageSkia*>& arrow_button_images =
       menu_button_images_[focused][
@@ skipping 101 matching lines @@
 int Combobox::MenuCommandToIndex(int menu_command_id) const {
   // (note that the id received is offset by kFirstMenuItemId)
   // Revert menu ID offset to map back to combobox model.
   int index = menu_command_id - kFirstMenuItemId;
   DCHECK_LT(index, model()->GetItemCount());
   return index;
 }
 
 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();
   return 0;
 }
 
 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();
   return 0;
 }
 
 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