Refactor app list max folder logic.

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 194 matching lines @@
   virtual void AnimationCanceled(const gfx::Animation* animation) OVERRIDE {
     view_->SchedulePaint();
   }
 
  private:
   views::View* view_;
 
   DISALLOW_COPY_AND_ASSIGN(ItemMoveAnimationDelegate);
 };
 
-// Returns true if the |item| is an folder item.
+// Returns true if the |item| is a 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 1160 matching lines @@
     drop_attempt_ = DROP_FOR_FOLDER;
     return;
   }
 
   drop_attempt_ = DROP_FOR_REORDER;
   CalculateReorderDropTarget(point, &reorder_drop_target_);
 }
 
 bool AppsGridView::CalculateFolderDropTarget(const gfx::Point& point,
                                              Index* drop_target) const {
+  // Folders can't be dropped into other folders.
+  if (IsFolderItem(drag_view_->item()))
+    return false;
+
+  // A folder drop shouldn't happen on the reorder placeholder since that would
+  // be merging an item with itself.
   Index nearest_tile_index(GetNearestTileIndexForPoint(point));
+  if (GetLastViewIndex() < nearest_tile_index ||

[Matt Giuca, 2014/10/02 06:08:59]

I don't understand where this condition (GetLastViewIndex() <
nearest_tile_index) came from or what it means.

[calamity, 2014/10/03 03:37:36]

Wups. Roundabout way of checking index validity. Fixed.

+      nearest_tile_index == reorder_placeholder_) {
+    return false;
+  }
+
   int distance_to_tile_center =
       (point - GetExpectedTileBounds(nearest_tile_index.slot).CenterPoint())
           .Length();
-  if (nearest_tile_index != reorder_placeholder_ &&
-      distance_to_tile_center < kFolderDroppingCircleRadius &&
-      !IsFolderItem(drag_view_->item()) &&
-      CanDropIntoTarget(nearest_tile_index)) {
-    *drop_target = nearest_tile_index;
-    DCHECK(IsValidIndex(*drop_target));
-    return true;
+  if (distance_to_tile_center > kFolderDroppingCircleRadius)

[Matt Giuca, 2014/10/02 06:08:59]

FYI, you are slightly changing the behaviour here. (Previously, if
dist_to_tile_center == kFDCR, it would be false, but now it is true.)

Perhaps this is an improvement, but let's be sure we want this.

[calamity, 2014/10/03 03:37:36]

I think radii of drop zones should be inclusive of their borders. The old code
provided 2 * kFDCR - 2 units of drop diameter which is weird.

[Matt Giuca, 2014/10/09 17:25:18]

Acknowledged.

+    return false;
+
+  AppListItemView* target_view =

[Matt Giuca, 2014/10/02 06:08:59]

I'm not sure I agree with inlining CanDropIntoTarget here. What was the
rationale?

[calamity, 2014/10/03 03:37:37]

It's only used in one place and the logic should be display based. So that means
we can have a method which is current-page-based (which kinda kills the
reusability of the method and makes it uncomfortably situational) or we can pass
the entire index and expect callers to figure out that the model doesn't match
the view (which is the kind of pitfall that led to the bug in the first place).

Keeping the view grabbing logic here is the safest way to avoid those problems.
Maybe it's worth having an CanAcceptFolderDrop() on AppListItemView if we want a
more reusable place for this code.

[Matt Giuca, 2014/10/09 17:25:18]

Acknowledged.

+      GetViewDisplayedAtSlotOnCurrentPage(nearest_tile_index.slot);

[Matt Giuca, 2014/10/02 06:08:59]

You removed the NULL-check here?

[calamity, 2014/10/03 03:37:37]

Hmmmmm. I _think_ this can't be NULL. But maybe it can...

I'll put it back.

+  AppListItem* target_item = target_view->item();
+
+  // Items can only be dropped into non-folders (which have no children) or
+  // folders that have fewer than the max allowed items.
+  // The OEM folder does not allow drag/drop of other items into it.
+  if (target_item->ChildItemCount() >= kMaxFolderItems ||
+      IsOEMFolderItem(target_item)) {
+    return false;
   }
 
-  return false;
+  *drop_target = nearest_tile_index;
+  DCHECK(IsValidIndex(*drop_target));
+  return true;
 }
 
 void AppsGridView::CalculateReorderDropTarget(const gfx::Point& point,
                                               Index* drop_target) const {
   gfx::Rect bounds = GetContentsBounds();
   Index grid_index = GetNearestTileIndexForPoint(point);
   gfx::Point reorder_placeholder_center =
       GetExpectedTileBounds(reorder_placeholder_.slot).CenterPoint();
 
   int x_offset_direction = 0;
@@ skipping 657 matching lines @@
   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.
   return model_->folders_enabled() && !folder_delegate_;
 }
 
-bool AppsGridView::CanDropIntoTarget(const Index& drop_target) const {
-  AppListItemView* target_view =
-      GetViewDisplayedAtSlotOnCurrentPage(drop_target.slot);
-  if (!target_view)
-    return false;
-
-  AppListItem* target_item = 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);
-}
-
 AppsGridView::Index AppsGridView::GetNearestTileIndexForPoint(
     const gfx::Point& point) const {
   gfx::Rect bounds = GetContentsBounds();
   gfx::Size total_tile_size = GetTotalTileSize();
   int col = ClampToRange(
       (point.x() - bounds.x()) / total_tile_size.width(), 0, cols_ - 1);
   int row = ClampToRange((point.y() - bounds.y()) / total_tile_size.height(),
                          0,
                          rows_per_page_ - 1);
   return Index(pagination_model_.selected_page(), row * cols_ + col);
@@ skipping 42 matching lines @@
 
 void AppsGridView::SetAsFolderDroppingTarget(const Index& target_index,
                                              bool is_target_folder) {
   AppListItemView* target_view =
       GetViewDisplayedAtSlotOnCurrentPage(target_index.slot);
   if (target_view)
     target_view->SetAsAttemptedFolderTarget(is_target_folder);
 }
 
 }  // namespace app_list