Use a ui::MenuModel in views::Combobox

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/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"
 #include "ui/views/painter.h"
 #include "ui/views/resources/grit/views_resources.h"
 #include "ui/views/widget/widget.h"
 
 namespace views {
 
 namespace {
@@ skipping 173 matching lines @@
                         *arrow_button_images[1],
                         *arrow_button_images[2],
                         x, 0, arrow_button_images[0]->width(), height);
 }
 
 }  // namespace
 
 // 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:
 
 Combobox::Combobox(ui::ComboboxModel* model)
     : model_(model),
       style_(STYLE_NORMAL),
       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();
 
   // Initialize the button images.
   Button::ButtonState button_states[] = {
     Button::STATE_DISABLED,
     Button::STATE_NORMAL,
     Button::STATE_HOVERED,
     Button::STATE_PRESSED,
   };
@@ skipping 12 matching lines @@
 
   text_button_->SetVisible(true);
   arrow_button_->SetVisible(true);
   text_button_->SetFocusable(false);
   arrow_button_->SetFocusable(false);
   AddChildView(text_button_);
   AddChildView(arrow_button_);
 }
 
 Combobox::~Combobox() {
-  model_->RemoveObserver(this);
-
   if (GetInputMethod() && selector_.get()) {
     // Combobox should have been blurred before destroy.
     DCHECK(selector_.get() != GetInputMethod()->GetTextInputClient());
   }
 }
 
 // static
 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();
+  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();
 }
 
 void Combobox::SetSelectedIndex(int index) {
   if (style_ == STYLE_ACTION)
     return;
 
   selected_index_ = index;
   SchedulePaint();
 }
@@ skipping 44 matching lines @@
       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);
-  OnPerformAction();
-}
-
-bool Combobox::GetAccelerator(int id, ui::Accelerator* accel) const {
-  return false;
-}
-
 int Combobox::GetRowCount() {
   return model()->GetItemCount();
 }
 
 int Combobox::GetSelectedRow() {
   return selected_index_;
 }
 
 void Combobox::SetSelectedRow(int row) {
   int prev_index = selected_index_;
@@ skipping 12 matching lines @@
 
 gfx::Size Combobox::GetPreferredSize() const {
   // The preferred size will drive the local bounds which in turn is used to set
   // the minimum width for the dropdown list.
   gfx::Insets insets = GetInsets();
   insets += gfx::Insets(Textfield::kTextPadding,
                         Textfield::kTextPadding,
                         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());
 }
 
 const char* Combobox::GetClassName() const {
   return kViewClassName;
 }
 
 bool Combobox::SkipDefaultKeyEventProcessing(const ui::KeyEvent& e) {
   // Escape should close the drop down list when it is active, not host UI.
   if (e.key_code() != ui::VKEY_ESCAPE ||
       e.IsShiftDown() || e.IsControlDown() || e.IsAltDown()) {
     return false;
   }
-  return dropdown_open_;
+  return menu_runner_;
 }
 
 bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
   // TODO(oshima): handle IME.
   DCHECK_EQ(e.type(), ui::ET_KEY_PRESSED);
 
   DCHECK_GE(selected_index_, 0);
   DCHECK_LT(selected_index_, model()->GetItemCount());
   if (selected_index_ < 0 || selected_index_ > model()->GetItemCount())
     selected_index_ = 0;
@@ skipping 49 matching lines @@
       if (style_ != STYLE_ACTION)
         return false;
       OnPerformAction();
       break;
 
     default:
       return false;
   }
 
   if (show_menu) {
-    UpdateFromModel();
     ShowDropDownMenu(ui::MENU_SOURCE_KEYBOARD);
   } else if (new_index != selected_index_ && new_index != kNoSelection &&
              style_ != STYLE_ACTION) {
     DCHECK(!model()->IsItemSeparatorAt(new_index));
     selected_index_ = new_index;
     OnPerformAction();
   }
 
   return true;
 }
@@ skipping 44 matching lines @@
 }
 
 void Combobox::GetAccessibleState(ui::AXViewState* state) {
   state->role = ui::AX_ROLE_COMBO_BOX;
   state->name = accessible_name_;
   state->value = model_->GetItemAt(selected_index_);
   state->index = selected_index_;
   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;
 
   RequestFocus();
 
   if (sender == text_button_) {
     OnPerformAction();
   } else {
     DCHECK_EQ(arrow_button_, sender);
     // TODO(hajimehoshi): Fix the problem that the arrow button blinks when
     // cliking this while the dropdown menu is opened.
     const base::TimeDelta delta = base::Time::Now() - closed_time_;
     if (delta.InMilliseconds() <= kMinimumMsBetweenButtonClicks)
       return;
 
     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() {
-  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)
     border->SetInsets(5, 10, 5, 10);
   if (invalid_)
     border->SetColor(kWarningColor);
   SetBorder(border.Pass());
 }
 
 void Combobox::AdjustBoundsForRTLUI(gfx::Rect* rect) const {
@@ skipping 105 matching lines @@
     canvas->SaveLayerAlpha(arrow_button_hover_alpha);
     const std::vector<const gfx::ImageSkia*>& arrow_button_hovered_images =
         menu_button_images_[focused][Button::STATE_HOVERED];
     PaintArrowButton(canvas, arrow_button_hovered_images,
                      arrow_button_->x(), height());
     canvas->Restore();
   }
 }
 
 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());
 
   if (style_ == STYLE_NORMAL) {
     // Inset the menu's requested position so the border of the menu lines up
     // with the border of the combobox.
     menu_position.set_x(menu_position.x() + kMenuBorderWidthLeft);
     menu_position.set_y(menu_position.y() + kMenuBorderWidthTop);
   }
   lb.set_width(lb.width() - (kMenuBorderWidthLeft + kMenuBorderWidthRight));
 
   View::ConvertPointToScreen(this, &menu_position);
   if (menu_position.x() < 0)
     menu_position.set_x(0);
 
   gfx::Rect bounds(menu_position, lb.size());
 
   Button::ButtonState original_state = Button::STATE_NORMAL;
   if (arrow_button_) {
     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();
 
   // Need to explicitly clear mouse handler so that events get sent
   // properly after the menu finishes running. If we don't do this, then
   // the first click to other parts of the UI is eaten.
   SetMouseHandler(NULL);
 }
 
 void Combobox::OnPerformAction() {
   NotifyAccessibilityEvent(ui::AX_EVENT_VALUE_CHANGED, false);
   SchedulePaint();
 
   // This combobox may be deleted by the listener.
   base::WeakPtr<Combobox> weak_ptr = weak_ptr_factory_.GetWeakPtr();
   if (listener_)
     listener_->OnPerformAction(this);
 
   if (weak_ptr && style_ == STYLE_ACTION)
     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:
       return kDisclosureArrowLeftPadding;
     case STYLE_ACTION:
       return kDisclosureArrowButtonLeftPadding;
   }
   NOTREACHED();
   return 0;
 }
@@ skipping 26 matching lines @@
                                              ui::NativeTheme::kNormal,
                                              ignored);
 }
 
 PrefixSelector* Combobox::GetPrefixSelector() {
   if (!selector_)
     selector_.reset(new PrefixSelector(this));
   return selector_.get();
 }
 
+ui::MenuModel* Combobox::GetMenuModelForTest() {
+  return menu_model_adapter_.get();
+}
+
 }  // namespace views