Use a ui::MenuModel in views::Combobox

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 7d527a9b0108e80457e2fd0b838195017d596376..b62545b4c4c5cf26deee75e762595d82a284dd8c 100644
--- a/ui/views/controls/combobox/combobox.cc
+++ b/ui/views/controls/combobox/combobox.cc
@@ -4,33 +4,27 @@
 
 #include "ui/views/controls/combobox/combobox.h"
 
-#include "base/bind.h"
 #include "base/logging.h"
-#include "base/strings/utf_string_conversions.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/ime/input_method.h"
 #include "ui/base/models/combobox_model.h"
+#include "ui/base/models/combobox_model_observer.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/events/event.h"
-#include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/animation/throb_animation.h"
 #include "ui/gfx/canvas.h"
-#include "ui/gfx/image/image.h"
 #include "ui/gfx/scoped_canvas.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/native_theme/common_theme.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/resources/grit/ui_resources.h"
-#include "ui/views/background.h"
 #include "ui/views/color_constants.h"
 #include "ui/views/controls/button/custom_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/combobox/combobox_listener.h"
 #include "ui/views/controls/focusable_border.h"
-#include "ui/views/controls/menu/menu_item_view.h"
+#include "ui/views/controls/menu/menu_config.h"
 #include "ui/views/controls/menu/menu_runner.h"
-#include "ui/views/controls/menu/menu_runner_handler.h"
-#include "ui/views/controls/menu/submenu_view.h"
 #include "ui/views/controls/prefix_selector.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/mouse_constants.h"
@@ -224,6 +218,135 @@ void PaintArrowButton(
 // static
 const char Combobox::kViewClassName[] = "views/Combobox";
 
+// Adapts a ui::ComboboxModel to a ui::MenuModel.
+class Combobox::ComboboxMenuModelAdapter : public ui::MenuModel,
+                                           public ui::ComboboxModelObserver {
+ public:
+  ComboboxMenuModelAdapter(Combobox* owner, ui::ComboboxModel* model)
+      : owner_(owner), model_(model) {
+    model_->AddObserver(this);
+  }
+
+  ~ComboboxMenuModelAdapter() override { model_->RemoveObserver(this); }
+
+  // Finds the size of the largest menu label or, for STYLE_ACTION, the size of
+  // the selected label.
+  gfx::Size GetContentSize() const {
+    const gfx::FontList& font_list = GetFontList();
+    const int num_items = GetItemCount();

[sky, 2015/09/03 17:52:27]

move into for loop.

[tapted, 2015/09/04 00:46:54]

Done.

+
+    int width = 0;
+    for (int i = 0; i < num_items; ++i) {
+      if (model_->IsItemSeparatorAt(i))
+        continue;
+
+      if (owner_->style_ != STYLE_ACTION || i == owner_->selected_index_)
+        width = std::max(width, gfx::GetStringWidth(GetLabelAt(i), font_list));
+    }
+    return gfx::Size(width, font_list.GetHeight());
+  }
+
+ private:
+  bool UseCheckmarks() const {
+    return owner_->style_ != STYLE_ACTION &&
+           MenuConfig::instance(owner_->GetNativeTheme())
+               .check_selected_combobox_item;
+  }
+
+  // Overridden from MenuModel:
+  bool HasIcons() const override { return false; }
+
+  int GetItemCount() const override { return model_->GetItemCount(); }
+
+  ItemType GetTypeAt(int index) const override {
+    if (model_->IsItemSeparatorAt(index)) {
+      // In action menus, disallow <item>, <separator>, ... since that would put
+      // a separator at the top of the menu.
+      DCHECK(index != 1 || owner_->style_ != STYLE_ACTION);
+      return TYPE_SEPARATOR;
+    }
+    return UseCheckmarks() ? TYPE_CHECK : TYPE_COMMAND;
+  }
+
+  ui::MenuSeparatorType GetSeparatorTypeAt(int index) const override {
+    return ui::NORMAL_SEPARATOR;
+  }
+
+  int GetCommandIdAt(int index) const override {
+    return index + kFirstMenuItemId;
+  }
+
+  base::string16 GetLabelAt(int index) const override {
+    // Inserting the Unicode formatting characters if necessary so that the
+    // text is displayed correctly in right-to-left UIs.
+    base::string16 text = model_->GetItemAt(index);
+    base::i18n::AdjustStringForLocaleDirection(&text);
+    return text;
+  }
+
+  bool IsItemDynamicAt(int index) const override { return true; }
+
+  const gfx::FontList* GetLabelFontListAt(int index) const override {
+    return &GetFontList();
+  }
+
+  bool GetAcceleratorAt(int index,
+                        ui::Accelerator* accelerator) const override {
+    return false;
+  }
+
+  bool IsItemCheckedAt(int index) const override {
+    return UseCheckmarks() && index == owner_->selected_index_;
+  }
+
+  int GetGroupIdAt(int index) const override { return -1; }
+
+  bool GetIconAt(int index, gfx::Image* icon) override { return false; }
+
+  ui::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const override {
+    return nullptr;
+  }
+
+  bool IsEnabledAt(int index) const override {
+    return model_->IsItemEnabledAt(index);
+  }
+
+  bool IsVisibleAt(int index) const override {
+    // When STYLE_ACTION is used, the first item is not added to the menu. It is
+    // assumed that the first item is always selected and rendered on the top of
+    // the action button.
+    return index > 0 || owner_->style_ != STYLE_ACTION;
+  }
+
+  void HighlightChangedTo(int index) override {}
+
+  void ActivatedAt(int index) override {
+    owner_->selected_index_ = index;
+    owner_->OnPerformAction();
+  }
+
+  void ActivatedAt(int index, int event_flags) override { ActivatedAt(index); }
+
+  MenuModel* GetSubmenuModelAt(int index) const override { return nullptr; }
+
+  void SetMenuModelDelegate(
+      ui::MenuModelDelegate* menu_model_delegate) override {}
+
+  ui::MenuModelDelegate* GetMenuModelDelegate() const override {
+    return nullptr;
+  }
+
+  // Overridden from ComboboxModelObserver:
+  void OnComboboxModelChanged(ui::ComboboxModel* model) override {
+    owner_->ModelChanged();
+  }
+
+  Combobox* owner_;           // Weak. Owns this.
+  ui::ComboboxModel* model_;  // Weak.
+
+  DISALLOW_COPY_AND_ASSIGN(ComboboxMenuModelAdapter);
+};
+
 ////////////////////////////////////////////////////////////////////////////////
 // Combobox, public:
 
@@ -233,13 +356,11 @@ Combobox::Combobox(ui::ComboboxModel* model)
       listener_(NULL),
       selected_index_(model_->GetDefaultIndex()),
       invalid_(false),
-      menu_(NULL),
-      dropdown_open_(false),
+      menu_model_adapter_(new ComboboxMenuModelAdapter(this, model)),
       text_button_(new TransparentButton(this)),
       arrow_button_(new TransparentButton(this)),
       weak_ptr_factory_(this) {
-  model_->AddObserver(this);
-  UpdateFromModel();
+  ModelChanged();
   SetFocusable(true);
   UpdateBorder();
 
@@ -272,8 +393,6 @@ Combobox::Combobox(ui::ComboboxModel* model)
 }
 
 Combobox::~Combobox() {
-  model_->RemoveObserver(this);
-
   if (GetInputMethod() && selector_.get()) {
     // Combobox should have been blurred before destroy.
     DCHECK(selector_.get() != GetInputMethod()->GetTextInputClient());
@@ -295,13 +414,13 @@ void Combobox::SetStyle(Style style) {
     selected_index_ = 0;
 
   UpdateBorder();
-  UpdateFromModel();
+  content_size_ = menu_model_adapter_->GetContentSize();
   PreferredSizeChanged();
 }
 
 void Combobox::ModelChanged() {
-  selected_index_ = std::min(0, model_->GetItemCount());
-  UpdateFromModel();
+  selected_index_ = std::min(model_->GetDefaultIndex(), model_->GetItemCount());

[tapted, 2015/09/03 12:52:26]

The old code did `index_ = std::min(0, itemcount)`, which I think is the same as
`index_ = 0`. using DefaultIndex allows ModelChanged() to be called from the
constructor and helps testing (also added coverage for this).

[sky, 2015/09/03 17:52:27]

I suspect the old code really wanted something like:
index_ = std::max(0, std::min(index_, model_->GetItemCount() - 1));
In other words, try to maintain the same selected index, unless it's beyond the
length of the new model.

[tapted, 2015/09/04 00:46:54]

Ah, yeah using GetItemCount() would only make sense if the count was 0. So I
guess this currently supports a model that has a default index that isn't zero
before adding any items. But if it's adding items it would have to call
ModelChanged() again anyway.

So, I think this could just be 

selected_index_ = model_->GetDefaultIndex();

Unfortunately, I don't think we can DCHECK_LT since returning 0 is probably the
best you can do if you haven't added items yet. So maybe better to keep the
std::min.

[sky, 2015/09/04 14:34:17]

I think GetDefaultIndex() is used during construction, but if the content of the
model changes we try to preserve the selected index, not reset to the default.
Like I outlined above.

Your patch always tries to use the default. That doesn't seem right.

[tapted, 2015/09/07 00:37:27]

Sorry - misunderstood your comment. Changed this to try to maintain the current
selection.

I think it should check separators so that it's consistent with a
`DCHECK(!model()->IsItemSeparatorAt(new_index));` in OnKeyPressed(). So it
includes logic for that. It doesn't properly support a model-change that alters
"GetDefaultIndex()", but there's no current use case for that.

Also added a test.

+  content_size_ = menu_model_adapter_->GetContentSize();
   PreferredSizeChanged();
 }
 
@@ -366,23 +485,6 @@ void Combobox::Layout() {
   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);
-  OnPerformAction();
-}
-
-bool Combobox::GetAccelerator(int id, ui::Accelerator* accel) const {
-  return false;
-}
-
 int Combobox::GetRowCount() {
   return model()->GetItemCount();
 }
@@ -415,8 +517,8 @@ gfx::Size Combobox::GetPreferredSize() const {
                         Textfield::kTextPadding,
                         Textfield::kTextPadding);
   int total_width = std::max(kMinComboboxWidth, content_size_.width()) +
-      insets.width() + GetDisclosureArrowLeftPadding() +
-      ArrowSize().width() + GetDisclosureArrowRightPadding();
+                    insets.width() + GetDisclosureArrowLeftPadding() +
+                    ArrowSize().width() + GetDisclosureArrowRightPadding();
   return gfx::Size(total_width, content_size_.height() + insets.height());
 }
 
@@ -430,7 +532,7 @@ bool Combobox::SkipDefaultKeyEventProcessing(const ui::KeyEvent& e) {
       e.IsShiftDown() || e.IsControlDown() || e.IsAltDown()) {
     return false;
   }
-  return dropdown_open_;
+  return menu_runner_;
 }
 
 bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
@@ -500,7 +602,6 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
   }
 
   if (show_menu) {
-    UpdateFromModel();
     ShowDropDownMenu(ui::MENU_SOURCE_KEYBOARD);
   } else if (new_index != selected_index_ && new_index != kNoSelection &&
              style_ != STYLE_ACTION) {
@@ -565,11 +666,6 @@ void Combobox::GetAccessibleState(ui::AXViewState* state) {
   state->count = model_->GetItemCount();
 }
 
-void Combobox::OnComboboxModelChanged(ui::ComboboxModel* model) {
-  DCHECK_EQ(model, model_);
-  ModelChanged();
-}
-
 void Combobox::ButtonPressed(Button* sender, const ui::Event& event) {
   if (!enabled())
     return;
@@ -595,44 +691,6 @@ void Combobox::ButtonPressed(Button* sender, const ui::Event& event) {
   }
 }
 
-void Combobox::UpdateFromModel() {
-  const gfx::FontList& font_list = Combobox::GetFontList();
-
-  menu_ = new MenuItemView(this);
-  // MenuRunner owns |menu_|.
-  dropdown_list_menu_runner_.reset(new MenuRunner(menu_, MenuRunner::COMBOBOX));
-
-  int num_items = model()->GetItemCount();
-  int width = 0;
-  bool text_item_appended = false;
-  for (int i = 0; i < num_items; ++i) {
-    // When STYLE_ACTION is used, the first item and the following separators
-    // are not added to the dropdown menu. It is assumed that the first item is
-    // always selected and rendered on the top of the action button.
-    if (model()->IsItemSeparatorAt(i)) {
-      if (text_item_appended || style_ != STYLE_ACTION)
-        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);
-
-    if (style_ != STYLE_ACTION || i > 0) {
-      menu_->AppendMenuItem(i + kFirstMenuItemId, text, MenuItemView::NORMAL);
-      text_item_appended = true;
-    }
-
-    if (style_ != STYLE_ACTION || i == selected_index_)
-      width = std::max(width, gfx::GetStringWidth(text, font_list));
-  }
-
-  content_size_.SetSize(width, font_list.GetHeight());
-}
-
 void Combobox::UpdateBorder() {
   scoped_ptr<FocusableBorder> border(new FocusableBorder());
   if (style_ == STYLE_ACTION)
@@ -758,14 +816,6 @@ void Combobox::PaintButtons(gfx::Canvas* canvas) {
 }
 
 void Combobox::ShowDropDownMenu(ui::MenuSourceType source_type) {
-  if (!dropdown_list_menu_runner_.get())
-    UpdateFromModel();
-
-  // Extend the menu to the width of the combobox.
-  SubmenuView* submenu = menu_->CreateSubmenu();
-  submenu->set_minimum_preferred_width(
-      size().width() - (kMenuBorderWidthLeft + kMenuBorderWidthRight));
-
   gfx::Rect lb = GetLocalBounds();
   gfx::Point menu_position(lb.origin());
 
@@ -788,15 +838,19 @@ void Combobox::ShowDropDownMenu(ui::MenuSourceType source_type) {
     original_state = arrow_button_->state();
     arrow_button_->SetState(Button::STATE_PRESSED);
   }
-  dropdown_open_ = true;
   MenuAnchorPosition anchor_position =
       style_ == STYLE_ACTION ? MENU_ANCHOR_TOPRIGHT : MENU_ANCHOR_TOPLEFT;
-  if (dropdown_list_menu_runner_->RunMenuAt(
-          GetWidget(), NULL, bounds, anchor_position, source_type) ==
-      MenuRunner::MENU_DELETED) {
+
+  // Allow |menu_runner_| to be set by the testing API.
+  if (!menu_runner_) {

[sky, 2015/09/03 17:52:27]

This makes me nervous, in particular what happens if ShowDropDownMenu is invoked
recursively? Maybe protect against that?

[tapted, 2015/09/04 00:46:54]

Done.

+    menu_runner_.reset(
+        new MenuRunner(menu_model_adapter_.get(), MenuRunner::COMBOBOX));
+  }
+  if (menu_runner_->RunMenuAt(GetWidget(), nullptr, bounds, anchor_position,
+                              source_type) == MenuRunner::MENU_DELETED) {
     return;
   }
-  dropdown_open_ = false;
+  menu_runner_.reset();
   if (arrow_button_)
     arrow_button_->SetState(original_state);
   closed_time_ = base::Time::Now();
@@ -820,14 +874,6 @@ void Combobox::OnPerformAction() {
     selected_index_ = 0;
 }
 
-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_NORMAL:
@@ -874,4 +920,8 @@ PrefixSelector* Combobox::GetPrefixSelector() {
   return selector_.get();
 }
 
+ui::MenuModel* Combobox::GetMenuModelForTest() {
+  return menu_model_adapter_.get();
+}
+
 }  // namespace views