shelf: Allow dragging items from main shelf to overflow shelf.

ash/shelf/shelf_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 "ash/shelf/shelf_view.h"
 
 #include <algorithm>
 #include <memory>
 
 #include "ash/ash_constants.h"
@@ skipping 248 matching lines @@
       cancelling_drag_model_changed_(false),
       last_hidden_index_(0),
       closing_event_time_(base::TimeTicks()),
       drag_and_drop_item_pinned_(false),
       drag_and_drop_shelf_id_(0),
       drag_replaced_view_(nullptr),
       dragged_off_shelf_(false),
       snap_back_from_rip_off_view_(nullptr),
       overflow_mode_(false),
       main_shelf_(nullptr),
-      dragged_off_from_overflow_to_shelf_(false),
+      dragged_off_from_shelf_to_other_shelf_(false),
       is_repost_event_on_same_item_(false),
       last_pressed_index_(-1),
       weak_factory_(this) {
   DCHECK(model_);
   DCHECK(delegate_);
   DCHECK(wm_shelf_);
   DCHECK(shelf_widget_);
   bounds_animator_.reset(new views::BoundsAnimator(this));
   bounds_animator_->AddObserver(this);
   set_context_menu_controller(this);
@@ skipping 467 matching lines @@
 }
 
 void ShelfView::UpdateAllButtonsVisibilityInOverflowMode() {
   // The overflow button is not shown in overflow mode.
   overflow_button_->SetVisible(false);
   DCHECK_LT(last_visible_index_, view_model_->view_size());
   for (int i = 0; i < view_model_->view_size(); ++i) {
     bool visible = i >= first_visible_index_ && i <= last_visible_index_;
     // To track the dragging of |drag_view_| continuously, its visibility
     // should be always true regardless of its position.
-    if (dragged_off_from_overflow_to_shelf_ &&
+    if (dragged_off_from_shelf_to_other_shelf_ &&
         view_model_->view_at(i) == drag_view_)
       view_model_->view_at(i)->SetVisible(true);
     else
       view_model_->view_at(i)->SetVisible(visible);
   }
 }
 
 void ShelfView::CalculateIdealBounds(gfx::Rect* overflow_bounds) const {
   DCHECK(model_->item_count() == view_model_->view_size());
 
@@ skipping 302 matching lines @@
       WmWindow::Get(GetWidget()->GetNativeWindow())
           ->GetRootWindow()
           ->ConvertPointToScreen(event.root_location());
 
   // To avoid ugly forwards and backwards flipping we use different constants
   // for ripping off / re-inserting the items.
   if (dragged_off_shelf_) {
     // If the shelf/overflow bubble bounds contains |screen_location| we insert
     // the item back into the shelf.
     if (GetBoundsForDragInsertInScreen().Contains(screen_location)) {
-      if (dragged_off_from_overflow_to_shelf_) {
+      if (dragged_off_from_shelf_to_other_shelf_) {
         // During the dragging an item from Shelf to Overflow, it can enter here
-        // directly because both are located very closly.
-        main_shelf_->EndDrag(true);
+        // directly because both are located very closely.
+        if (is_overflow_mode())
+          main_shelf_->EndDrag(true);
+        else if (overflow_bubble_ && overflow_bubble_->IsShowing())

[msw, 2017/04/17 19:29:09]

q: shouldn't this definitely be true if we are dragging between shelves, and
|is_overflow_mode()| is false? (should this be a DCHECK instead?)

[sammiequon, 2017/04/17 23:42:28]

Done.

+          overflow_bubble_->shelf_view()->EndDrag(true);
+
         // Stops the animation of |drag_view_| and sets its bounds explicitly
-        // becase ContinueDrag() stops its animation. Without this, unexpected
+        // because ContinueDrag() stops its animation. Without this, unexpected
         // bounds will be set.
         bounds_animator_->StopAnimatingView(drag_view_);
         int drag_view_index = view_model_->GetIndexOfView(drag_view_);
         drag_view_->SetBoundsRect(view_model_->ideal_bounds(drag_view_index));
-        dragged_off_from_overflow_to_shelf_ = false;
+        dragged_off_from_shelf_to_other_shelf_ = false;
       }
       // Destroy our proxy view item.
       DestroyDragIconProxy();
       // Re-insert the item and return simply false since the caller will handle
       // the move as in any normal case.
       dragged_off_shelf_ = false;
       drag_view_->layer()->SetOpacity(1.0f);
       // The size of Overflow bubble should be updated immediately when an item
       // is re-inserted.
       if (is_overflow_mode())
         PreferredSizeChanged();
       return false;
     } else if (is_overflow_mode() &&
                main_shelf_->GetBoundsForDragInsertInScreen().Contains(
                    screen_location)) {
-      if (!dragged_off_from_overflow_to_shelf_) {
-        dragged_off_from_overflow_to_shelf_ = true;
+      // Item is dragged from overflow shelf to shelf.

[msw, 2017/04/17 19:29:09]

nit: "The item was dragged from the overflow shelf to the main shelf."

[sammiequon, 2017/04/17 23:42:29]

Done.

+      if (!dragged_off_from_shelf_to_other_shelf_) {
+        dragged_off_from_shelf_to_other_shelf_ = true;
         drag_image_->SetOpacity(1.0f);
         main_shelf_->StartDrag(dragged_app_id, screen_location);
       } else {
         main_shelf_->Drag(screen_location);
       }
-    } else if (dragged_off_from_overflow_to_shelf_) {
+    } else if (!is_overflow_mode() && overflow_bubble_ &&
+               overflow_bubble_->IsShowing() &&
+               overflow_bubble_->shelf_view()
+                   ->GetBoundsForDragInsertInScreen()
+                   .Contains(screen_location)) {
+      // Item is dragged from shelf to overflow shelf.

[msw, 2017/04/17 19:29:09]

The item was dragged from the main shelf to the overflow shelf.

[sammiequon, 2017/04/17 23:42:28]

Done.

+      if (!dragged_off_from_shelf_to_other_shelf_) {
+        dragged_off_from_shelf_to_other_shelf_ = true;
+        drag_image_->SetOpacity(1.0f);
+        overflow_bubble_->shelf_view()->StartDrag(dragged_app_id,
+                                                  screen_location);
+      } else {
+        overflow_bubble_->shelf_view()->Drag(screen_location);
+      }
+    } else if (dragged_off_from_shelf_to_other_shelf_) {
       // Makes the |drag_image_| partially disappear again.
-      dragged_off_from_overflow_to_shelf_ = false;
+      dragged_off_from_shelf_to_other_shelf_ = false;
       drag_image_->SetOpacity(kDraggedImageOpacity);
-      main_shelf_->EndDrag(true);
+
+      if (is_overflow_mode()) {
+        main_shelf_->EndDrag(true);
+      } else if (overflow_bubble_ && overflow_bubble_->IsShowing()) {

[msw, 2017/04/17 19:29:09]

ditto q: shouldn't this definitely be true if we are dragging between shelves,
and |is_overflow_mode()| is false? (should this be a DCHECK instead?)

+        overflow_bubble_->shelf_view()->EndDrag(true);
+        // After entering the overflow shelf the last item becomes visible

[msw, 2017/04/17 19:29:09]

I don't understand what is happening here (I also don't know this dragging code
terribly well), you should ping skuhne@ or someone else more familiar with this
code. Why does hiding the last item in the overflow shelf help? Are you saying
that item always matches the dragged item here (what happens when you're
dragging a non-last-item?)? Where do we reset the last_visible_index_ to the
correct value? This doesn't make sense to me...

[sammiequon, 2017/04/17 23:42:29]

If we do not hide the last item, when an item is ripped off, you'll see a copy
of it under the cursor, as well as the same item in the last spot of the
overflow shelf (because the overflow shelf does not disappear when the main
shelf is clicked anymore).
The last item should also match the dragged item because we swap it to the back
before starting our drag phase.
last_visible_index_ gets updated when shelf items get rearranged again (ie when
the dragged item enters a shelf etc.).
So if our model has 13 items (and lets say 6 should be overflow), both shelfs
will have all 13 items except the main shelf will have first_visible_index=0 and
last_visible_index=6, while the overflow shelf will have first_visible_index=7
and last_visible_index=12. So when rip an item off the main shelf below (lets
say item at index 3), we swap it will the last index in the model. The items on
the shelf then get redraw according to the model. The chosen item is now index
12 and will not be displayed on the main shelf, but it will be displayed on the
overflow shelf as the last item. This is actually a bug introduced in my last CL
I believe because previously the overflow shelf would disappear when an item on
the main shelf was chosen so it didn't matter). To compensate for this we set
the last_visible_index of the overflow to 11, so that the shelf will get redrawn
without the duplicate (because the ripped of item is also shown where the cursor
is).

Let me know if that clears things up :-), before I attempt to put it in comment
form.

[msw, 2017/04/18 00:11:03]

Ah, I didn't realize that when we started dragging an item, it's moved to the
end of the list; that clears up most of my confusion. I don't see exactly where
the |last_visible_index_| members are update after the drag completes, but there
are plenty of places that seem to recalculate it, so I'll trust you on that...
Please do try to clarify the comments here and on line 1175, and it would still
be good to loop in a more knowledgeable reviewer.

+        // again. Hide the last item when we exit the overflow shelf, otherwise
+        // a copy of the item we are dragging will appear on the overflow shelf.
+        overflow_bubble_->shelf_view()->last_visible_index_ =

[msw, 2017/04/17 19:29:09]

nit: overflow_bubble_->shelf_view()->last_visible_index_--;

[sammiequon, 2017/04/17 23:42:28]

Done.

+            overflow_bubble_->shelf_view()->last_visible_index_ - 1;
+      }
+
       bounds_animator_->StopAnimatingView(drag_view_);
       int drag_view_index = view_model_->GetIndexOfView(drag_view_);
       drag_view_->SetBoundsRect(view_model_->ideal_bounds(drag_view_index));
     }
     // Move our proxy view item.
     UpdateDragIconProxy(screen_location);
     return true;
   }
   // Check if we are too far away from the shelf to enter the ripped off state.
   // Determine the distance to the shelf.
   int delta = CalculateShelfDistance(screen_location);
   if (delta > kRipOffDistance) {
     // Create a proxy view item which can be moved anywhere.
     CreateDragIconProxy(event.root_location(), drag_view_->GetImage(),
                         drag_view_, gfx::Vector2d(0, 0),
                         kDragAndDropProxyScale);
     drag_view_->layer()->SetOpacity(0.0f);
     dragged_off_shelf_ = true;
     if (RemovableByRipOff(current_index) == REMOVABLE) {
       // Move the item to the front of the first panel item and hide it.
       // ShelfItemMoved() callback will handle the |view_model_| update and
       // call AnimateToIdealBounds().
       if (current_index != model_->FirstPanelIndex() - 1) {
         model_->Move(current_index, model_->FirstPanelIndex() - 1);
         StartFadeInLastVisibleItem();
+
+        // If the overflow bubble is showing, the current item will swap to the

[msw, 2017/04/17 19:29:09]

Ditto, I'm a bit confused by this... current/last/dragged, can you try to
explain this a bit more/differently?

+        // back of the overflow shelf as well, so we tell the overflow shelf to
+        // not show their last item, which would be the current item.
+        if (overflow_bubble_ && overflow_bubble_->IsShowing()) {
+          overflow_bubble_->shelf_view()->last_visible_index_ =

[msw, 2017/04/17 19:29:09]

nit: overflow_bubble_->shelf_view()->last_visible_index_--; (and remove {})

[sammiequon, 2017/04/17 23:42:28]

Done.

+              overflow_bubble_->shelf_view()->last_visible_index_ - 1;
+        }
       } else if (is_overflow_mode()) {
         // Overflow bubble should be shrunk when an item is ripped off.
         PreferredSizeChanged();
       }
       // Make the item partially disappear to show that it will get removed if
       // dropped.
       drag_image_->SetOpacity(kDraggedImageOpacity);
     }
     return true;
   }
@@ skipping 14 matching lines @@
   // and only delete the proxy image.
   if (current_index == -1) {
     DestroyDragIconProxy();
     return;
   }
 
   // Set to true when the animation should snap back to where it was before.
   bool snap_back = false;
   // Items which cannot be dragged off will be handled as a cancel.
   if (!cancel) {
-    if (dragged_off_from_overflow_to_shelf_) {
-      dragged_off_from_overflow_to_shelf_ = false;
-      main_shelf_->EndDrag(false);
+    if (dragged_off_from_shelf_to_other_shelf_) {
+      dragged_off_from_shelf_to_other_shelf_ = false;
+      if (is_overflow_mode())
+        main_shelf_->EndDrag(false);
+      else if (overflow_bubble_ && overflow_bubble_->IsShowing())

[msw, 2017/04/17 19:29:09]

ditto q: shouldn't this definitely be true if we are dragging between shelves,
and |is_overflow_mode()| is false? (should this be a DCHECK instead?)

[sammiequon, 2017/04/17 23:42:28]

Done.

+        overflow_bubble_->shelf_view()->EndDrag(false);
       drag_view_->layer()->SetOpacity(1.0f);
     } else if (RemovableByRipOff(current_index) != REMOVABLE) {
       // Make sure we do not try to remove un-removable items like items which
       // were not pinned or have to be always there.
       cancel = true;
       snap_back = true;
     } else {
       // Make sure the item stays invisible upon removal.
       drag_view_->SetVisible(false);
       std::string app_id =
           delegate_->GetAppIDForShelfID(model_->items()[current_index].id);
       delegate_->UnpinAppWithID(app_id);
     }
   }
   if (cancel || snap_back) {
-    if (dragged_off_from_overflow_to_shelf_) {
-      dragged_off_from_overflow_to_shelf_ = false;
-      // Main shelf handles revert of dragged item.
-      main_shelf_->EndDrag(true);
+    if (dragged_off_from_shelf_to_other_shelf_) {
+      dragged_off_from_shelf_to_other_shelf_ = false;
+      // Other shelf handles revert of dragged item.
+      if (is_overflow_mode())
+        main_shelf_->EndDrag(true);
+      else if (overflow_bubble_ && overflow_bubble_->IsShowing())

[msw, 2017/04/17 19:29:09]

ditto q: shouldn't this definitely be true if we are dragging between shelves,
and |is_overflow_mode()| is false? (should this be a DCHECK instead?)

[sammiequon, 2017/04/17 23:42:29]

Done.

+        overflow_bubble_->shelf_view()->EndDrag(true);
       drag_view_->layer()->SetOpacity(1.0f);
     } else if (!cancelling_drag_model_changed_) {
       // Only do something if the change did not come through a model change.
       gfx::Rect drag_bounds = drag_image_->GetBoundsInScreen();
       gfx::Point relative_to = GetBoundsInScreen().origin();
       gfx::Rect target(
           gfx::PointAtOffsetFromOrigin(drag_bounds.origin() - relative_to),
           drag_bounds.size());
       drag_view_->SetBoundsRect(target);
       // Hide the status from the active item since we snap it back now. Upon
@@ skipping 193 matching lines @@
   if (!is_overflow_mode()) {
     if (last_hidden_index_ < view_model_->view_size() - 1)
       last_button_index = view_model_->view_size() - 1;
     else if (overflow_button_ && overflow_button_->visible())
       last_button_index++;
   }
 
   // When an item is dragged off from the overflow bubble, it is moved to last
   // position and and changed to invisible. Overflow bubble size should be
   // shrunk to fit only for visible items.
-  // If |dragged_off_from_overflow_to_shelf_| is set, there will be no invisible
-  // items in the shelf.
+  // If |dragged_off_from_shelf_to_other_shelf_| is set, there will be no
+  // invisible items in the shelf.
   if (is_overflow_mode() && dragged_off_shelf_ &&
-      !dragged_off_from_overflow_to_shelf_ &&
+      !dragged_off_from_shelf_to_other_shelf_ &&
       RemovableByRipOff(view_model_->GetIndexOfView(drag_view_)) == REMOVABLE)
     last_button_index--;
 
   const gfx::Rect last_button_bounds =
       last_button_index >= first_visible_index_
           ? view_model_->ideal_bounds(last_button_index)
           : gfx::Rect(gfx::Size(shelf_size, shelf_size));
 
   if (wm_shelf_->IsHorizontalAlignment())
     return gfx::Size(last_button_bounds.right(), shelf_size);
@@ skipping 336 matching lines @@
 
 int ShelfView::CalculateShelfDistance(const gfx::Point& coordinate) const {
   const gfx::Rect bounds = GetBoundsInScreen();
   int distance = wm_shelf_->SelectValueForShelfAlignment(
       bounds.y() - coordinate.y(), coordinate.x() - bounds.right(),
       bounds.x() - coordinate.x());
   return distance > 0 ? distance : 0;
 }
 
 }  // namespace ash