Rework app list item drag zones.

ui/app_list/views/apps_grid_view.cc

Index: ui/app_list/views/apps_grid_view.cc
diff --git a/ui/app_list/views/apps_grid_view.cc b/ui/app_list/views/apps_grid_view.cc
index dda664fd8f615211aaf04379244f6b41b0bdfb44..e7c50bb038bc6abd9452ad5f54a785df8cb1913f 100644
--- a/ui/app_list/views/apps_grid_view.cc
+++ b/ui/app_list/views/apps_grid_view.cc
@@ -189,12 +189,6 @@ class ItemMoveAnimationDelegate : public gfx::AnimationDelegate {
   DISALLOW_COPY_AND_ASSIGN(ItemMoveAnimationDelegate);
 };
 
-// Gets the distance between the centers of the |rect_1| and |rect_2|.
-int GetDistanceBetweenRects(gfx::Rect rect_1,
-                            gfx::Rect rect_2) {
-  return (rect_1.CenterPoint() - rect_2.CenterPoint()).Length();
-}
-
 // Returns true if the |item| is an folder item.
 bool IsFolderItem(AppListItem* item) {
   return (item->GetItemType() == AppListFolderItem::kItemType);
@@ -488,6 +482,7 @@ void AppsGridView::InitiateDrag(AppListItemView* view,
   drag_view_init_index_ = GetIndexOfView(drag_view_);
   drag_view_offset_ = event.location();
   drag_start_page_ = pagination_model_.selected_page();
+  reorder_placeholder_ = GetIndexOfView(drag_view_);

[tapted, 2014/09/11 08:08:54]

nit: GetIndexOfView(drag_view_); -> drag_view_init_index_

[calamity, 2014/09/15 03:26:10]

Done.

[tapted, 2014/09/15 06:12:53]

missed?

[calamity, 2014/09/15 06:36:15]

Done for real.

   ExtractDragLocation(event, &drag_start_grid_view_);
   drag_view_start_ = gfx::Point(drag_view_->x(), drag_view_->y());
 }
@@ -784,6 +779,10 @@ void AppsGridView::InitiateDragFromReparentItemInRootLevelGridView(
   DCHECK(original_drag_view && !drag_view_);
   DCHECK(!dragging_for_reparent_item_);
 
+  // Since the item is new, it's placeholder is conceptually at the back of the

[tapted, 2014/09/11 08:08:54]

nit: `it's` -> `its`

[calamity, 2014/09/15 03:26:09]

Done.

+  // entire apps grid.
+  reorder_placeholder_ = GetLastViewIndex();
+
   // Create a new AppListItemView to duplicate the original_drag_view in the
   // folder's grid view.
   AppListItemView* view = new AppListItemView(this, original_drag_view->item());
@@ -833,6 +832,7 @@ void AppsGridView::ClearDragState() {
   drag_pointer_ = NONE;
   reorder_drop_target_ = Index();
   folder_drop_target_ = Index();
+  reorder_placeholder_ = Index();
   drag_start_grid_view_ = gfx::Point();
   drag_start_page_ = -1;
   drag_view_offset_ = gfx::Point();
@@ -1115,6 +1115,11 @@ views::View* AppsGridView::GetViewAtIndex(const Index& index) const {
   return view_model_.view_at(model_index);
 }
 
+AppsGridView::Index AppsGridView::GetLastViewIndex() const {
+  int view_index = view_model_.view_size() - 1;

[tapted, 2014/09/11 08:08:53]

DCHECK(!view_model_.empty());  .. assuming we can guarantee that

[calamity, 2014/09/15 03:26:10]

It'll be a shoddy guarantee.. (A component app should always exist)

[tapted, 2014/09/15 06:12:52]

Yep - that's all i'm after really. Since if it is empty and things go negative
it's more thinking here.

+  return Index(view_index / tiles_per_page(), view_index % tiles_per_page());
+}
+
 void AppsGridView::MoveSelected(int page_delta,
                                 int slot_x_delta,
                                 int slot_y_delta) {
@@ -1388,9 +1393,15 @@ void AppsGridView::CalculateDropTarget(const gfx::Point& drag_point,
 void AppsGridView::CalculateDropTargetWithFolderEnabled(
     const gfx::Point& drag_point,

[tapted, 2014/09/11 08:08:53]

|drag_point| looks unused now -> remove?

[calamity, 2014/09/15 03:26:10]

Done.

     bool use_page_button_hovering) {
-  gfx::Point point(drag_point);
-  if (!IsPointWithinDragBuffer(drag_point)) {
-    point = drag_start_grid_view_;
+  gfx::Point point = drag_view_->icon()->bounds().CenterPoint();
+  views::View::ConvertPointToTarget(drag_view_, this, &point);
+  if (!IsPointWithinDragBuffer(point)) {
+    // Reset the reorder target to the original position if the cursor is
+    // outside the drag buffer.
+    if (!IsDraggingForReparentInRootLevelGridView())
+      reorder_drop_target_ = GetIndexOfView(drag_view_);

[tapted, 2014/09/11 08:08:53]

maybe.. drag_view_init_index_

[calamity, 2014/09/15 03:26:11]

Done.

+    drop_attempt_ = DROP_FOR_NONE;
+    return;
   }
 
   if (use_page_button_hovering && page_switcher_view_ &&
@@ -1403,14 +1414,62 @@ void AppsGridView::CalculateDropTargetWithFolderEnabled(
       drop_attempt_ = DROP_FOR_NONE;
   } else {

[tapted, 2014/09/11 08:08:54]

early return before this. no else.

[calamity, 2014/09/15 03:26:10]

Done.

     DCHECK(drag_view_);

[tapted, 2014/09/11 08:08:54]

move this to the start of the function

[calamity, 2014/09/15 03:26:09]

Done.

-    // Try to find the nearest target for folder dropping or re-ordering.
-    CalculateNearestTileForDragView();
+    const int d_folder_dropping =

[tapted, 2014/09/11 08:08:55]

d looks like an abbreviation - maybe `folder_threshold`. But this probably needs
a comment too about why it's calculated this way (e.g. is it coincidence that
half the icon dimension gives a good result, or does it need to be that way)

[calamity, 2014/09/15 03:26:09]

I think this is saying the radius is the distance to the edge of the icon +
15...

Maybe kFolderDroppingCircleRadius should just be the folder dropping circle
radius..

[tapted, 2014/09/15 06:12:52]

yep - much nicer

+        kFolderDroppingCircleRadius + kGridIconDimension / 2;
+
+    Index nearest_tile_index(pagination_model_.selected_page(),
+                             GetNearestTileSlotForPoint(point));
+
+    int distance_to_tile_center =
+        (point - GetExpectedTileBounds(nearest_tile_index.slot).CenterPoint())
+            .Length();
+    if (nearest_tile_index != reorder_placeholder_ &&
+        distance_to_tile_center < d_folder_dropping &&
+        !IsFolderItem(drag_view_->item()) &&
+        CanDropIntoTarget(nearest_tile_index)) {

[tapted, 2014/09/11 08:08:54]

CanDropIntoTarget returns true if the drop_target is past the end (I think) - is
that what we want here?

[calamity, 2014/09/15 03:26:09]

Nope. Fixed.

+      drop_attempt_ = DROP_FOR_FOLDER;
+      folder_drop_target_ = nearest_tile_index;
+      return;

[tapted, 2014/09/11 08:08:53]

there's a lot of state above that isn't used for the rest of the function. I
think this needs some decomposition. Something like

  if (IsFolderDrop(point, &folder_drop_target_)) {
    drop_attempt_ = DROP_FOR_FOLDER;
    return;
  }

bool AppsGridView::IsFolderDrop(const gfx::Point& point,
                                Index* drop_target) const {
  const int folder_threshold =
      kFolderDroppingCircleRadius + kGridIconDimension / 2;
  Index nearest_tile_index(pagination_model_.selected_page(),
                           GetNearestTileSlotForPoint(point));
  int distance_to_tile_center =
      (point - GetExpectedTileBounds(nearest_tile_index.slot).CenterPoint())
          .Length();
  if (nearest_tile_index != reorder_placeholder_ &&
      distance_to_tile_center < d_folder_dropping &&
      !IsFolderItem(drag_view_->item()) &&
      CanDropIntoTarget(nearest_tile_index)) {
    *drop_target = nearest_tile_index;
    return true;
  }
  return false;
}

[calamity, 2014/09/15 03:26:11]

Done.

+    }
+
+    gfx::Rect bounds = GetContentsBounds();
+    // Get coordinates in tile grid space.
+    int x = point.x() - bounds.x();
+    int y = point.y() - bounds.y();
+    int row = y / kPreferredTileHeight;
+    int grid_col = x / kPreferredTileWidth;
+    Index grid_index(pagination_model_.selected_page(),
+                     std::min(row * cols_ + grid_col, view_model_.view_size()));

[tapted, 2014/09/11 08:08:54]

This is pretty fiddly... but I don't see an obvious improvement. Maybe you can
re-use GetNearestTileSlotForPoint(point)) here and ignore the clamping until the
end of the function.

But the big question here is: what about folders-disabled? I think
AppsGridView::CalculateDropTarget() needs an update too. Probably this last bit
needs to be a function like AppsGridView::CalculateReorderDropTarget(), which is
also called by CalculateDropTarget

[calamity, 2014/09/15 03:26:11]

Done.
Done. This removes some functionality. Updated the CL description.

+    gfx::Point reorder_placeholder_origin =
+        GetExpectedTileBounds(reorder_placeholder_.slot).CenterPoint();
+
+    int dir = 0;

[tapted, 2014/09/11 08:08:54]

dir isn't a good name - maybe column_offset?

[calamity, 2014/09/15 03:26:09]

Done.

+    if (grid_index == reorder_placeholder_) {
+      dir = reorder_placeholder_origin.x() < point.x() ? 1 : -1;
+    } else {
+      dir = reorder_placeholder_ < grid_index ? 1 : -1;
+    }
+
+    // Offset the target column base on the direction of the target. This will

[tapted, 2014/09/11 08:08:54]

nit: base->based

[calamity, 2014/09/15 03:26:09]

Done.

+    // result in earlier targets getting their reorder zone shifted backwards
+    // and later targets getting their reorder zones shifted forwards.
+    //
+    // This causes the animation to trigger when slotting items into the spaces
+    // between apps.
+    int col = (x - dir * kPreferredTileWidth / 3) / kPreferredTileWidth;
+    col = std::min(std::max(col, 0), cols_ - 1);
+    drop_attempt_ = DROP_FOR_REORDER;
+    reorder_drop_target_ =
+        std::min(Index(pagination_model_.selected_page(), row * cols_ + col),
+                 GetLastViewIndex());
   }
 }
 
 void AppsGridView::OnReorderTimer() {
-  if (drop_attempt_ == DROP_FOR_REORDER)
+  if (drop_attempt_ == DROP_FOR_REORDER) {
+    reorder_placeholder_ = reorder_drop_target_;
     AnimateToIdealBounds();
+  }
 }
 
 void AppsGridView::OnFolderItemReparentTimer() {
@@ -2040,9 +2099,8 @@ void AppsGridView::OnImplicitAnimationsCompleted() {
 
 bool AppsGridView::EnableFolderDragDropUI() {
   // Enable drag and drop folder UI only if it is at the app list root level
-  // and the switch is on and the target folder can still accept new items.
-  return model_->folders_enabled() && !folder_delegate_ &&
-         CanDropIntoTarget(folder_drop_target_);

[tapted, 2014/09/11 08:08:54]

I don't think this belongs here. I'd always assumed `EnableFolderDragDropUI()`
doesn't change its return value after the AppsGridView is constructed. What
breaks without this?

[calamity, 2014/09/15 03:26:09]

Nothing afaict. It was obviously needed for the "can this be dropped?" case
which is at line 1429 but that now uses CanDropIntoTarget directly.

+  // and the switch is on.
+  return model_->folders_enabled() && !folder_delegate_;
 }
 
 bool AppsGridView::CanDropIntoTarget(const Index& drop_target) {
@@ -2059,124 +2117,11 @@ bool AppsGridView::CanDropIntoTarget(const Index& drop_target) {
          !IsOEMFolderItem(target_item);
 }
 
-// TODO(jennyz): Optimize the calculation for finding nearest tile.
-void AppsGridView::CalculateNearestTileForDragView() {
-  Index nearest_tile;
-  nearest_tile.page = -1;
-  nearest_tile.slot = -1;
-  int d_min = -1;
-
-  // Calculate the top left tile |drag_view| intersects.
-  gfx::Point pt = drag_view_->bounds().origin();
-  CalculateNearestTileForVertex(pt, &nearest_tile, &d_min);
-
-  // Calculate the top right tile |drag_view| intersects.
-  pt = drag_view_->bounds().top_right();
-  CalculateNearestTileForVertex(pt, &nearest_tile, &d_min);
-
-  // Calculate the bottom left tile |drag_view| intersects.
-  pt = drag_view_->bounds().bottom_left();
-  CalculateNearestTileForVertex(pt, &nearest_tile, &d_min);
-
-  // Calculate the bottom right tile |drag_view| intersects.
-  pt = drag_view_->bounds().bottom_right();
-  CalculateNearestTileForVertex(pt, &nearest_tile, &d_min);
-
-  const int d_folder_dropping =
-      kFolderDroppingCircleRadius + kGridIconDimension / 2;
-  const int d_reorder = kReorderDroppingCircleRadius + kGridIconDimension / 2;
-
-  // If user drags an item across pages to the last page, and targets it
-  // to the last empty slot on it, push the last item for re-ordering.
-  if (IsLastPossibleDropTarget(nearest_tile) && d_min < d_reorder) {
-    drop_attempt_ = DROP_FOR_REORDER;
-    nearest_tile.slot = nearest_tile.slot - 1;
-    reorder_drop_target_ = nearest_tile;
-    return;
-  }
-
-  if (IsValidIndex(nearest_tile)) {
-    if (d_min < d_folder_dropping) {
-      views::View* target_view = GetViewAtSlotOnCurrentPage(nearest_tile.slot);
-      if (target_view &&
-          !IsFolderItem(static_cast<AppListItemView*>(drag_view_)->item())) {
-        // If a non-folder item is dragged to the target slot with an item
-        // sitting on it, attempt to drop the dragged item into the folder
-        // containing the item on nearest_tile.
-        drop_attempt_ = DROP_FOR_FOLDER;
-        folder_drop_target_ = nearest_tile;
-        return;
-      } else {
-        // If the target slot is blank, or the dragged item is a folder, attempt
-        // to re-order.
-        drop_attempt_ = DROP_FOR_REORDER;
-        reorder_drop_target_ = nearest_tile;
-        return;
-      }
-    } else if (d_min < d_reorder) {
-      // Entering the re-order circle of the slot.
-      drop_attempt_ = DROP_FOR_REORDER;
-      reorder_drop_target_ = nearest_tile;
-      return;
-    }
-  }
-
-  // If |drag_view| is not entering the re-order or fold dropping region of
-  // any items, cancel any previous re-order or folder dropping timer.
-  drop_attempt_ = DROP_FOR_NONE;
-  reorder_timer_.Stop();
-  folder_dropping_timer_.Stop();
-
-  // When dragging for reparent a folder item, it should go back to its parent
-  // folder item if there is no drop target.
-  if (IsDraggingForReparentInRootLevelGridView()) {
-    DCHECK(activated_folder_item_view_);
-    folder_drop_target_ = GetIndexOfView(activated_folder_item_view_);
-  }
-}
-
-void AppsGridView::CalculateNearestTileForVertex(const gfx::Point& vertex,
-                                                 Index* nearest_tile,
-                                                 int* d_min) {
-  Index target_index;
-  gfx::Rect target_bounds = GetTileBoundsForPoint(vertex, &target_index);
-
-  if (target_bounds.IsEmpty() || target_index == *nearest_tile)
-    return;
-
-  // Do not count the tile, where drag_view_ used to sit on and is still moving
-  // on top of it, in calculating nearest tile for drag_view_.
-  views::View* target_view = GetViewAtSlotOnCurrentPage(target_index.slot);
-  if (target_index == drag_view_init_index_ && !target_view &&
-      !IsDraggingForReparentInRootLevelGridView()) {
-    return;
-  }
-
-  int d_center = GetDistanceBetweenRects(drag_view_->bounds(), target_bounds);
-  if (*d_min < 0 || d_center < *d_min) {
-    *d_min = d_center;
-    *nearest_tile = target_index;
-  }
-}
-
-gfx::Rect AppsGridView::GetTileBoundsForPoint(const gfx::Point& point,
-                                              Index *tile_index) {
-  // Check if |point| is outside of contents bounds.
-  gfx::Rect bounds(GetContentsBounds());
-  if (!bounds.Contains(point))
-    return gfx::Rect();
-
-  // Calculate which tile |point| is enclosed in.
-  int x = point.x();
-  int y = point.y();
-  int col = (x - bounds.x()) / kPreferredTileWidth;
-  int row = (y - bounds.y()) / kPreferredTileHeight;
-  gfx::Rect tile_rect = GetExpectedTileBounds(row, col);
-
-  // Check if |point| is outside a valid item's tile.
-  Index index(pagination_model_.selected_page(), row * cols_ + col);
-  *tile_index = index;
-  return tile_rect;
+int AppsGridView::GetNearestTileSlotForPoint(const gfx::Point& point) {
+  gfx::Rect bounds = GetContentsBounds();

[tapted, 2014/09/11 08:08:54]

maybe DCHECK(IsPointWithinDragBuffer(point)); before this? Otherwise, I have to
think harder about whether it makes sense

[calamity, 2014/09/15 03:26:09]

Hmm. Unfortunately IsPointWithinDragBuffer includes a 15px buffer around the
drag view which threatens to ruin this.

How about this?

[tapted, 2014/09/15 06:12:52]

sweet

+  int col = (point.x() - bounds.x()) / kPreferredTileWidth;
+  int row = (point.y() - bounds.y()) / kPreferredTileHeight;
+  return row * cols_ + col;
 }
 
 gfx::Size AppsGridView::GetTileGridSize() const {
@@ -2185,6 +2130,10 @@ gfx::Size AppsGridView::GetTileGridSize() const {
   return bounds.size();
 }
 
+gfx::Rect AppsGridView::GetExpectedTileBounds(int slot) const {
+  return GetExpectedTileBounds(slot / cols_, slot % cols_);
+}
+
 gfx::Rect AppsGridView::GetExpectedTileBounds(int row, int col) const {
   gfx::Rect bounds(GetContentsBounds());
   gfx::Size tile_size(kPreferredTileWidth, kPreferredTileHeight);
@@ -2193,12 +2142,6 @@ gfx::Rect AppsGridView::GetExpectedTileBounds(int row, int col) const {
                    tile_size);
 }
 
-bool AppsGridView::IsLastPossibleDropTarget(const Index& index) const {
-  int last_possible_slot = view_model_.view_size() % tiles_per_page();
-  return (index.page == pagination_model_.total_pages() - 1 &&
-          index.slot == last_possible_slot + 1);
-}
-
 views::View* AppsGridView::GetViewAtSlotOnCurrentPage(int slot) {
   if (slot < 0)
     return NULL;