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

ash/shelf/shelf_view.cc

Index: ash/shelf/shelf_view.cc
diff --git a/ash/shelf/shelf_view.cc b/ash/shelf/shelf_view.cc
index 434c5bfc76f6b15fee7922e1cf3c0695322809db..71b120abbce3ece9957a78859cd50bb7b8ef1ae3 100644
--- a/ash/shelf/shelf_view.cc
+++ b/ash/shelf/shelf_view.cc
@@ -266,7 +266,7 @@ ShelfView::ShelfView(ShelfModel* model,
       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) {
@@ -754,7 +754,7 @@ void ShelfView::UpdateAllButtonsVisibilityInOverflowMode() {
     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
@@ -1077,17 +1077,21 @@ bool ShelfView::HandleRipOffDrag(const ui::LocatedEvent& event) {
     // 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();
@@ -1103,18 +1107,44 @@ bool ShelfView::HandleRipOffDrag(const ui::LocatedEvent& event) {
     } 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));
@@ -1140,6 +1170,14 @@ bool ShelfView::HandleRipOffDrag(const ui::LocatedEvent& event) {
       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();
@@ -1174,9 +1212,12 @@ void ShelfView::FinalizeRipOffDrag(bool cancel) {
   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
@@ -1192,10 +1233,13 @@ void ShelfView::FinalizeRipOffDrag(bool cancel) {
     }
   }
   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.
@@ -1409,10 +1453,10 @@ gfx::Size ShelfView::GetPreferredSize() const {
   // 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--;