Rework app list item drag zones.

ui/app_list/views/apps_grid_view.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/app_list/views/apps_grid_view.h"
 
 #include <algorithm>
 #include <set>
 #include <string>
 
@@ skipping 171 matching lines @@
   virtual void AnimationCanceled(const gfx::Animation* animation) OVERRIDE {
     view_->SchedulePaint();
   }
 
  private:
   views::View* view_;
 
   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);
 }
 
 bool IsOEMFolderItem(AppListItem* item) {
   return IsFolderItem(item) &&
          (static_cast<AppListFolderItem*>(item))->folder_type() ==
              AppListFolderItem::FOLDER_TYPE_OEM;
 }
@@ skipping 273 matching lines @@
                                 Pointer pointer,
                                 const ui::LocatedEvent& event) {
   DCHECK(view);
   if (drag_view_ || pulsing_blocks_model_.view_size())
     return;
 
   drag_view_ = 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());
 }
 
 void AppsGridView::StartSettingUpSynchronousDrag() {
 #if defined(OS_WIN)
   if (!delegate_ || !use_synchronous_drag_)
     return;
 
   // Folders and downloading items can't be integrated with the OS.
@@ skipping 276 matching lines @@
   layer()->SetOpacity(show ? 1.0f : 0.0f);
 }
 
 void AppsGridView::InitiateDragFromReparentItemInRootLevelGridView(
     AppListItemView* original_drag_view,
     const gfx::Rect& drag_view_rect,
     const gfx::Point& drag_point) {
   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());
   AddChildView(view);
   drag_view_ = view;
   drag_view_->SetPaintToLayer(true);
   // Note: For testing purpose, SetFillsBoundsOpaquely can be set to true to
   // show the gray background.
   drag_view_->SetFillsBoundsOpaquely(false);
   drag_view_->SetBoundsRect(drag_view_rect);
@@ skipping 29 matching lines @@
 
 bool AppsGridView::IsDraggedView(const views::View* view) const {
   return drag_view_ == view;
 }
 
 void AppsGridView::ClearDragState() {
   drop_attempt_ = DROP_FOR_NONE;
   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();
 
   if (drag_view_) {
     drag_view_->OnDragEnded();
     if (IsDraggingForReparentInRootLevelGridView()) {
       const int drag_view_index = view_model_.GetIndexOfView(drag_view_);
       CHECK_EQ(view_model_.view_size() - 1, drag_view_index);
       DeleteItemViewAtIndex(drag_view_index);
@@ skipping 262 matching lines @@
 }
 
 views::View* AppsGridView::GetViewAtIndex(const Index& index) const {
   if (!IsValidIndex(index))
     return NULL;
 
   const int model_index = GetModelIndexFromIndex(index);
   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) {
   if (!selected_view_)
     return SetSelectedItemByIndex(Index(pagination_model_.selected_page(), 0));
 
   const Index& selected = GetIndexOfView(selected_view_);
   int target_slot = selected.slot + slot_x_delta + slot_y_delta * cols_;
 
   if (selected.slot % cols_ == 0 && slot_x_delta == -1) {
@@ skipping 251 matching lines @@
   // Limits to the last possible slot on last page.
   if (reorder_drop_target_.page == pagination_model_.total_pages() - 1) {
     reorder_drop_target_.slot =
         std::min((view_model_.view_size() - 1) % tiles_per_page(),
                  reorder_drop_target_.slot);
   }
 }
 
 
 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_ &&
       page_switcher_view_->bounds().Contains(point)) {
     gfx::Point page_switcher_point(point);
     views::View::ConvertPointToTarget(this, page_switcher_view_,
                                       &page_switcher_point);
     int page = page_switcher_view_->GetPageForPoint(page_switcher_point);
     if (pagination_model_.is_valid_page(page))
       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() {
   DCHECK(folder_delegate_);
   if (drag_out_of_folder_container_ && drag_view_) {
     folder_delegate_->ReparentItem(drag_view_, last_drag_point_);
 
     // Set the flag in the folder's grid view.
     dragging_for_reparent_item_ = true;
 
@@ skipping 609 matching lines @@
   view->layer()->SetOpacity(hide ? 0 : 1);
 }
 
 void AppsGridView::OnImplicitAnimationsCompleted() {
   if (layer()->opacity() == 0.0f)
     SetVisible(false);
 }
 
 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) {
   views::View* target_view = GetViewAtSlotOnCurrentPage(drop_target.slot);
   if (!target_view)
     return true;
 
   AppListItem* target_item =
       static_cast<AppListItemView*>(target_view)->item();
   // Items can be dropped into non-folders (which have no children) or folders
   // that have fewer than the max allowed items.
   // OEM folder does not allow to drag/drop other items in it.
   return target_item->ChildItemCount() < kMaxFolderItems &&
          !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 {
   gfx::Rect bounds = GetExpectedTileBounds(0, 0);
   bounds.Union(GetExpectedTileBounds(rows_per_page_ - 1, cols_ - 1));
   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);
   return gfx::Rect(gfx::Point(bounds.x() + col * tile_size.width(),
                               bounds.y() + row * tile_size.height()),
                    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;
 
   // Calculate the original bound of the tile at |index|.
   int row = slot / cols_;
   int col = slot % cols_;
   gfx::Rect tile_rect = GetExpectedTileBounds(row, col);
 
   for (int i = 0; i < view_model_.view_size(); ++i) {
     views::View* view = view_model_.view_at(i);
     if (view->bounds() == tile_rect && view != drag_view_)
       return view;
   }
   return NULL;
 }
 
 void AppsGridView::SetAsFolderDroppingTarget(const Index& target_index,
                                              bool is_target_folder) {
   AppListItemView* target_view =
       static_cast<AppListItemView*>(
           GetViewAtSlotOnCurrentPage(target_index.slot));
   if (target_view)
     target_view->SetAsAttemptedFolderTarget(is_target_folder);
 }
 
 }  // namespace app_list