Adjust BrowserActionsContainer drag and drop to work for overflow

chrome/browser/ui/views/toolbar/browser_actions_container.cc

 // Copyright 2013 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 "chrome/browser/ui/views/toolbar/browser_actions_container.h"
 
 #include "base/compiler_specific.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ skipping 81 matching lines @@
     border->set_insets(gfx::Insets());
     return border.Pass();
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ChevronMenuButton);
 };
 
 }  // namespace
 
+////////////////////////////////////////////////////////////////////////////////
+// BrowserActionsContainer::DropPosition
+
+struct BrowserActionsContainer::DropPosition {
+  DropPosition(size_t row, size_t icon_in_row);
+
+  // The (0-indexed) row into which the action will be dropped.
+  size_t row;
+
+  // The (0-indexed) icon in the row before the action will be dropped.
+  size_t icon_in_row;
+};
+
+BrowserActionsContainer::DropPosition::DropPosition(
+    size_t row, size_t icon_in_row)
+    : row(row), icon_in_row(icon_in_row) {
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// BrowserActionsContainer
+
 // static
 bool BrowserActionsContainer::disable_animations_during_testing_ = false;
 
-////////////////////////////////////////////////////////////////////////////////
-// BrowserActionsContainer
-
 BrowserActionsContainer::BrowserActionsContainer(
     Browser* browser,
     View* owner_view,
     BrowserActionsContainer* main_container)
     : profile_(browser->profile()),
       browser_(browser),
       owner_view_(owner_view),
       main_container_(main_container),
       popup_(NULL),
       popup_button_(NULL),
       model_(NULL),
       container_width_(0),
       resize_area_(NULL),
       chevron_(NULL),
       overflow_menu_(NULL),
       suppress_chevron_(false),
       resize_amount_(0),
       animation_target_size_(0),
-      drop_indicator_position_(-1),
       task_factory_(this),
       show_menu_task_factory_(this) {
   set_id(VIEW_ID_BROWSER_ACTION_TOOLBAR);
 
   model_ = extensions::ExtensionToolbarModel::Get(browser->profile());
   if (model_)
     model_->AddObserver(this);
 
   bool overflow_experiment =
       extensions::FeatureSwitch::extension_action_redesign()->IsEnabled();
@@ skipping 215 matching lines @@
       // Ensure that any browser actions shown in the main view are hidden in
       // the overflow view.
       browser_action_views_[i]->SetVisible(false);
     }
 
     for (size_t i = main_container_->VisibleBrowserActionsAfterAnimation();
          i < browser_action_views_.size(); ++i) {
       BrowserActionView* view = browser_action_views_[i];
       size_t index = i - main_container_->VisibleBrowserActionsAfterAnimation();
       int row_index = static_cast<int>(index) / kIconsPerMenuRow;
-      int x = (index * IconWidth(true)) -
+      int x = kItemSpacing + (index * IconWidth(true)) -
           (row_index * IconWidth(true) * kIconsPerMenuRow);
       gfx::Rect rect_bounds(
           x, IconHeight() * row_index, icon_width, IconHeight());
       view->SetBoundsRect(rect_bounds);
       view->SetVisible(true);
     }
   } else {
     for (BrowserActionViews::const_iterator it = browser_action_views_.begin();
          it < browser_action_views_.end(); ++it) {
       BrowserActionView* view = *it;
@@ skipping 27 matching lines @@
 int BrowserActionsContainer::OnDragUpdated(
     const ui::DropTargetEvent& event) {
   // First check if we are above the chevron (overflow) menu.
   if (GetEventHandlerForPoint(event.location()) == chevron_) {
     if (!show_menu_task_factory_.HasWeakPtrs() && !overflow_menu_)
       StartShowFolderDropMenuTimer();
     return ui::DragDropTypes::DRAG_MOVE;
   }
   StopShowFolderDropMenuTimer();
 
-  // TODO(devlin): This calculation needs to take 'overflow' mode into account
-  // once the wrench menu becomes a drag target for browser action icons.
-
   // Figure out where to display the indicator.  This is a complex calculation:
 
   // First, we figure out how much space is to the left of the icon area, so we
-  // can calculate the true offset into the icon area.
-  int width_before_icons = ToolbarView::kStandardSpacing;
-  if (chevron_ && base::i18n::IsRTL()) {
-    width_before_icons +=
-        chevron_->GetPreferredSize().width() + kChevronSpacing;
-  }
-  int offset_into_icon_area = event.x() - width_before_icons;
+  // can calculate the true offset into the icon area. The easiest way to do
+  // this is to just find where the first icon starts.
+  int width_before_icons =
+      browser_action_views_[GetFirstVisibleIconIndex()]->x();
+
+  // If we're right-to-left, we flip the mirror the event.x() so that our
+  // calculations are consistent with left-to-right.
+  int offset_into_icon_area =
+      GetMirroredXInView(event.x()) - width_before_icons;
+
+  // Next, figure out what row we're on. This only matters for overflow mode,
+  // but the calculation is the same for both.
+  size_t row_index = event.y() / IconHeight();
+
+  // Sanity check - we should never be on a different row in the main container.
+  DCHECK(in_overflow_mode() || row_index == 0);
 
   // Next, we determine which icon to place the indicator in front of.  We want
   // to place the indicator in front of icon n when the cursor is between the
   // midpoints of icons (n - 1) and n.  To do this we take the offset into the
   // icon area and transform it as follows:
   //
   // Real icon area:
   //   0   a     *  b        c
   //   |   |        |        |
   //   |[IC|ON]  [IC|ON]  [IC|ON]
   // We want to be before icon 0 for 0 < x <= a, icon 1 for a < x <= b, etc.
   // Here the "*" represents the offset into the icon area, and since it's
   // between a and b, we want to return "1".
   //
   // Transformed "icon area":
   //   0        a     *  b        c
   //   |        |        |        |
   //   |[ICON]  |[ICON]  |[ICON]  |
   // If we shift both our offset and our divider points later by half an icon
   // plus one spacing unit, then it becomes very easy to calculate how many
   // divider points we've passed, because they're the multiples of "one icon
   // plus padding".
   int before_icon_unclamped = (offset_into_icon_area + (IconWidth(false) / 2) +
       kItemSpacing) / IconWidth(true);
 
+  // We need to figure out how many icons are visible on the relevant row.
+  // In the main container, this will just be the visible actions.
+  int visible_icons_on_row = VisibleBrowserActionsAfterAnimation();
+  if (in_overflow_mode()) {
+    // If this is the final row of the overflow, then this is the remainder of
+    // visible icons. Otherwise, it's a full row (kIconsPerRow).
+    visible_icons_on_row =
+        row_index ==
+            static_cast<size_t>(visible_icons_on_row / kIconsPerMenuRow) ?
+                visible_icons_on_row % kIconsPerMenuRow :
+                kIconsPerMenuRow;
+  }
+
   // Because the user can drag outside the container bounds, we need to clamp to
   // the valid range.  Note that the maximum allowable value is (num icons), not
   // (num icons - 1), because we represent the indicator being past the last
   // icon as being "before the (last + 1) icon".
-  int before_icon = std::min(std::max(before_icon_unclamped, 0),
-                             static_cast<int>(VisibleBrowserActions()));
+  size_t before_icon_in_row =
+      std::min(std::max(before_icon_unclamped, 0), visible_icons_on_row);
 
-  // Now we convert back to a pixel offset into the container.  We want to place
-  // the center of the drop indicator at the midpoint of the space before our
-  // chosen icon.
-  SetDropIndicator(width_before_icons + (before_icon * IconWidth(true)) -
-      (kItemSpacing / 2));
+  if (!drop_position_.get() ||
+      !(drop_position_->row == row_index &&
+        drop_position_->icon_in_row == before_icon_in_row)) {
+    drop_position_.reset(new DropPosition(row_index, before_icon_in_row));
+    SchedulePaint();
+  }
 
   return ui::DragDropTypes::DRAG_MOVE;
 }
 
 void BrowserActionsContainer::OnDragExited() {
   StopShowFolderDropMenuTimer();
-  drop_indicator_position_ = -1;
+  drop_position_.reset();
   SchedulePaint();
 }
 
 int BrowserActionsContainer::OnPerformDrop(
     const ui::DropTargetEvent& event) {
   BrowserActionDragData data;
   if (!data.Read(event.data()))
     return ui::DragDropTypes::DRAG_NONE;
 
   // Make sure we have the same view as we started with.
   DCHECK_EQ(browser_action_views_[data.index()]->button()->extension()->id(),
             data.id());
   DCHECK(model_);
 
-  size_t i = 0;
-  for (; i < browser_action_views_.size(); ++i) {
-    int view_x = browser_action_views_[i]->GetMirroredBounds().x();
-    if (!browser_action_views_[i]->visible() ||
-        (base::i18n::IsRTL() ? (view_x < drop_indicator_position_) :
-            (view_x >= drop_indicator_position_))) {
-      // We have reached the end of the visible icons or found one that has a
-      // higher x position than the drop point.
-      break;
-    }
-  }
-
+  size_t i =
+      drop_position_->row * kIconsPerMenuRow + drop_position_->icon_in_row;
+  if (in_overflow_mode())
+    i += GetFirstVisibleIconIndex();
   // |i| now points to the item to the right of the drop indicator*, which is
   // correct when dragging an icon to the left. When dragging to the right,
   // however, we want the icon being dragged to get the index of the item to
   // the left of the drop indicator, so we subtract one.
   // * Well, it can also point to the end, but not when dragging to the left. :)
   if (i > data.index())
     --i;
 
   if (profile_->IsOffTheRecord())
     i = model_->IncognitoIndexToOriginal(i);
@@ skipping 180 matching lines @@
 
   for (BrowserActionViews::iterator it = browser_action_views_.begin();
        it != browser_action_views_.end(); ++it) {
     BrowserActionButton* button = (*it)->button();
     if (button && button->extension() == extension)
       return ShowPopup(button, ExtensionPopup::SHOW, should_grant);
   }
   return false;
 }
 
+size_t BrowserActionsContainer::GetFirstVisibleIconIndex() const {
+  return in_overflow_mode() ? model_->GetVisibleIconCount() : 0;
+}
+
 void BrowserActionsContainer::HidePopup() {
   // Remove this as an observer and clear |popup_| and |popup_button_| here,
   // since we might change them before OnWidgetDestroying() gets called.
   if (popup_) {
     popup_->GetWidget()->RemoveObserver(this);
     popup_->GetWidget()->Close();
     popup_ = NULL;
   }
   if (popup_button_) {
     popup_button_->SetButtonNotPushed();
@@ skipping 17 matching lines @@
 void BrowserActionsContainer::OnPaint(gfx::Canvas* canvas) {
   // If the views haven't been initialized yet, wait for the next call to
   // paint (one will be triggered by entering highlight mode).
   if (model_->is_highlighting() && !browser_action_views_.empty()) {
     views::Painter::PaintPainterAt(
         canvas, highlight_painter_.get(), GetLocalBounds());
   }
 
   // TODO(sky/glen): Instead of using a drop indicator, animate the icons while
   // dragging (like we do for tab dragging).
-  if (drop_indicator_position_ > -1) {
+  if (drop_position_.get()) {
     // The two-pixel width drop indicator.
     static const int kDropIndicatorWidth = 2;
-    gfx::Rect indicator_bounds(
-        drop_indicator_position_ - (kDropIndicatorWidth / 2),
-        0,
-        kDropIndicatorWidth,
-        height());
+
+    // Convert back to a pixel offset into the container. We want to place
+    // the center of the drop indicator at the midpoint of the space before
+    // the drop position's referenced icon.
+    // This is:
+    //     The width before the first icon, plus
+    //     The width of all icons before the target icon in the row, minus
+    //     Half the spacing between the target icon and the previous icon,
+    //         plus 1 because it kItemSpacing is odd and the indicator looks

[Peter Kasting, 2014/07/22 21:10:00]

Nit: remove "it"

[Devlin, 2014/07/22 22:13:20]

Moot.

+    //         better closer to the icon than the address bar, minus
+    //     The width of the drop indicator.
+    int drop_indicator_x =
+        browser_action_views_[GetFirstVisibleIconIndex()]->x() +
+        drop_position_->icon_in_row * IconWidth(true) -
+        kItemSpacing / 2 + 1 -

[Peter Kasting, 2014/07/22 21:10:00]

It still doesn't look to me like this is doing what you're saying.

"... - kItemSpacing / 2 + 1 ..."
--> "... - 3 / 2 + 1 ..."
--> "... - 1 + 1 ..."
--> "... <does nothing> ..."

It sounds from the comment like you were trying to describe something more like
"(kItemSpacing + 1) / 2", which will always mean "half, rounding up" no matter
whether kItemSpacing is even or odd.

Here's what I think you really want.  This should do The Right Thing no matter
what the size of kItemSpacing and kDropIndicatorWidth are.

    // Convert back to a pixel offset into the container.  First find the X
    // coordinate of the drop icon.
    int drop_icon_x = browser_action_views_[GetFirstVisibleIconIndex()]->x() +
        (drop_position_->icon_in_row * IconWidth(true));
    // Now place the drop indicator halfway between this and the end of the
    // previous icon.  If there is an odd amount of available space between the
    // two icons after subtracting the drop indicator width, this calculation
    // puts the extra pixel on the left side of the indicator, since when the
    // indicator is between the address bar and the first icon, it looks better
    // closer to the icon.
    int drop_indicator_x = drop_icon_x -
        ((kItemSpacing + kDropIndicatorWidth) / 2);
    int row_height = IconHeight();
    int drop_indicator_y = row_height * drop_position_->row;
    gfx::Rect indicator_bounds(drop_indicator_x, drop_indicator_y,
                               kDropIndicatorWidth, row_height);
    indicator_bounds.set_x(GetMirroredXForRect(indicator_bounds));

Adding the item spacing and the drop indicator width together before dividing
handles any combination of even and odd widths correctly.  Note that the
GetMirroredXForRect() call at the end takes care of adjusting by the indicator
width for you in RTL.

[Devlin, 2014/07/22 22:13:20]

1. Wow, that's much more articulate. Let's use that. :)
2. If we're gonna do The Right Thing independent of actual values, we also need
to check if we should use ToolbarView::kStandardSpacing vs kItemSpacing (see
added step).
3. Thanks for the pointer to GetMirroredXForRect(). Still new to views and
learning where everything is...

+        kDropIndicatorWidth;
+
+    // Mirror the x if it's RTL. We also need to shift the drop indicator if in
+    // RTL because FillRect always fills LTR, and we need to compensate.
+    drop_indicator_x = GetMirroredXInView(drop_indicator_x);
+    if (base::i18n::IsRTL())
+      drop_indicator_x -= kDropIndicatorWidth;
+
+    // The y is simply the start of the row (which for the main container is
+    // always 0).
+    int drop_indicator_y = IconHeight() * drop_position_->row;
+
+    gfx::Rect indicator_bounds(drop_indicator_x,
+                               drop_indicator_y,
+                               kDropIndicatorWidth,
+                               IconHeight());
 
     // Color of the drop indicator.
     static const SkColor kDropIndicatorColor = SK_ColorBLACK;
     canvas->FillRect(indicator_bounds, kDropIndicatorColor);
   }
 }
 
 void BrowserActionsContainer::OnThemeChanged() {
   LoadImages();
 }
@@ skipping 190 matching lines @@
 void BrowserActionsContainer::StartShowFolderDropMenuTimer() {
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&BrowserActionsContainer::ShowDropFolder,
                  show_menu_task_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(views::GetMenuShowDelay()));
 }
 
 void BrowserActionsContainer::ShowDropFolder() {
   DCHECK(!overflow_menu_);
-  SetDropIndicator(-1);
+  drop_position_.reset();
   overflow_menu_ =
       new BrowserActionOverflowMenuController(this,
                                               browser_,
                                               chevron_,
                                               browser_action_views_,
                                               VisibleBrowserActions(),
                                               true);
   overflow_menu_->set_observer(this);
   overflow_menu_->RunMenu(GetWidget());
 }
 
-void BrowserActionsContainer::SetDropIndicator(int x_pos) {
-  if (drop_indicator_position_ != x_pos) {
-    drop_indicator_position_ = x_pos;
-    SchedulePaint();
-  }
-}
-
 int BrowserActionsContainer::IconCountToWidth(int icons,
                                               bool display_chevron) const {
   if (icons < 0)
     icons = browser_action_views_.size();
   if ((icons == 0) && !display_chevron)
     return ToolbarView::kStandardSpacing;
   int icons_size =
       (icons == 0) ? 0 : ((icons * IconWidth(true)) - kItemSpacing);
   int chevron_size = chevron_ && display_chevron ?
       (kChevronSpacing + chevron_->GetPreferredSize().width()) : 0;
@@ skipping 87 matching lines @@
                                      views::BubbleBorder::TOP_RIGHT,
                                      show_action);
   popup_->GetWidget()->AddObserver(this);
   popup_button_ = button;
 
   // Only set button as pushed if it was triggered by a user click.
   if (should_grant)
     popup_button_->SetButtonPushed();
   return true;
 }