Drag and drop creation of shortcuts for running applications fixed

The problem was that applications which are already running, are not (yet) pinned. As such they were limited in movement while dragging within the launcher. This problem was addressed by checking the pinned status and pinning the item when it is not (and reverting that state if the operation gets cancelled).
While fixing that I was running into a problem which predates my involvement: Unpinning running applications was able to scramble the item order.

BUG=248362, 248769
TEST=mostly unit tested, partial visual tests

Review URL: https://chromiumcodereview.appspot.com/15662010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@205886 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 69 insertions, 10 deletions

diff --git a/ash/launcher/launcher_delegate.h b/ash/launcher/launcher_delegate.h
index cb80588..20064ea 100644
--- a/ash/launcher/launcher_delegate.h
+++ b/ash/launcher/launcher_delegate.h
@@ -101,6 +101,9 @@ class ASH_EXPORT LauncherDelegate {
   // pinned.
   virtual void PinAppWithID(const std::string& app_id) = 0;
 
+  // Check if the app with |app_id_| is pinned to the launcher.
+  virtual bool IsAppPinned(const std::string& app_id) = 0;
+
   // Unpins any app item(s) whose id is |app_id|. The new launcher will collect
   // all items under one item, the old launcher might have multiple items.
   virtual void UnpinAppsWithID(const std::string& app_id) = 0;
diff --git a/ash/launcher/launcher_model.cc b/ash/launcher/launcher_model.cc
index cac37c0..68e57e0 100644
--- a/ash/launcher/launcher_model.cc
+++ b/ash/launcher/launcher_model.cc
@@ -94,8 +94,18 @@ void LauncherModel::Set(int index, const LauncherItem& item) {
                     LauncherItemChanged(index, old_item));
 
   // If the type changes confirm that the item is still in the right order.
-  if (new_index != index)
+  if (new_index != index) {
+    // The move function works by removing one item and then inserting it at the
+    // new location. However - by removing the item first the order will change
+    // so that our target index needs to be corrected.
+    // TODO(skuhne): Moving this into the Move function breaks lots of unit
+    // tests. So several functions were already using this incorrectly.
+    // That needs to be cleaned up.
+    if (index < new_index)
+      new_index--;
+
     Move(index, new_index);
+  }
 }
 
 int LauncherModel::ItemIndexByID(LauncherID id) const {
diff --git a/ash/launcher/launcher_model_unittest.cc b/ash/launcher/launcher_model_unittest.cc
index 382f224..c91a040 100644
--- a/ash/launcher/launcher_model_unittest.cc
+++ b/ash/launcher/launcher_model_unittest.cc
@@ -241,4 +241,38 @@ TEST(LauncherModel, LauncherIDTests) {
   EXPECT_NE(model.next_id(), id2);
 }
 
+// This verifies that converting an existing item into a lower weight category
+// (e.g. shortcut to running but not pinned app) will move it to the proper
+// location. See crbug.com/248769.
+TEST(LauncherModel, CorrectMoveItemsWhenStateChange) {
+  LauncherModel model;
+
+  // The app list should be the last item in the list.
+  EXPECT_EQ(1, model.item_count());
+
+  // The first item is the browser.
+  LauncherItem browser_shortcut;
+  browser_shortcut.type = TYPE_BROWSER_SHORTCUT;
+  int browser_shortcut_index = model.Add(browser_shortcut);
+  EXPECT_EQ(0, browser_shortcut_index);
+
+  // Add three shortcuts. They should all be moved between the two.
+  LauncherItem item;
+  item.type = TYPE_APP_SHORTCUT;
+  int app1_index = model.Add(item);
+  EXPECT_EQ(1, app1_index);
+  int app2_index = model.Add(item);
+  EXPECT_EQ(2, app2_index);
+  int app3_index = model.Add(item);
+  EXPECT_EQ(3, app3_index);
+
+  // Now change the type of the second item and make sure that it is moving
+  // behind the shortcuts.
+  item.type = TYPE_PLATFORM_APP;
+  model.Set(app2_index, item);
+
+  // The item should have moved in front of the app launcher.
+  EXPECT_EQ(TYPE_PLATFORM_APP, model.items()[3].type);
+}
+
 }  // namespace ash
diff --git a/ash/launcher/launcher_view.cc b/ash/launcher/launcher_view.cc
index 8585e49..8e8281b 100644
--- a/ash/launcher/launcher_view.cc
+++ b/ash/launcher/launcher_view.cc
@@ -394,7 +394,7 @@ LauncherView::LauncherView(LauncherModel* model,
       last_hidden_index_(0),
       closing_event_time_(base::TimeDelta()),
       got_deleted_(NULL),
-      drag_and_drop_item_created_(false),
+      drag_and_drop_item_pinned_(false),
       drag_and_drop_launcher_id_(0) {
   DCHECK(model_);
   bounds_animator_.reset(new views::BoundsAnimator(this));
@@ -589,18 +589,20 @@ bool LauncherView::StartDrag(const std::string& app_id,
   // If the AppsGridView (which was dispatching this event) was opened by our
   // button, LauncherView dragging operations are locked and we have to unlock.
   CancelDrag(-1);
-  drag_and_drop_item_created_ = false;
+  drag_and_drop_item_pinned_ = false;
   drag_and_drop_app_id_ = app_id;
   drag_and_drop_launcher_id_ =
       delegate_->GetLauncherIDForAppID(drag_and_drop_app_id_);
-
-  if (!drag_and_drop_launcher_id_) {
+  // Check if the application is known and pinned - if not, we have to pin it so
+  // that we can re-arrange the launcher order accordingly. Note that items have
+  // to be pinned to give them the same (order) possibilities as a shortcut.
+  if (!drag_and_drop_launcher_id_ || !delegate_->IsAppPinned(app_id)) {
     delegate_->PinAppWithID(app_id);
     drag_and_drop_launcher_id_ =
         delegate_->GetLauncherIDForAppID(drag_and_drop_app_id_);
     if (!drag_and_drop_launcher_id_)
       return false;
-    drag_and_drop_item_created_ = true;
+    drag_and_drop_item_pinned_ = true;
   }
   views::View* drag_and_drop_view = view_model_->view_at(
       model_->ItemIndexByID(drag_and_drop_launcher_id_));
@@ -651,7 +653,7 @@ void LauncherView::EndDrag(bool cancel) {
       drag_and_drop_view, LauncherButtonHost::DRAG_AND_DROP, cancel);
 
   // Either destroy the temporarily created item - or - make the item visible.
-  if (drag_and_drop_item_created_ && cancel)
+  if (drag_and_drop_item_pinned_ && cancel)
     delegate_->UnpinAppsWithID(drag_and_drop_app_id_);
   else if (drag_and_drop_view)
     drag_and_drop_view->SetSize(pre_drag_and_drop_size_);
diff --git a/ash/launcher/launcher_view.h b/ash/launcher/launcher_view.h
index a431d12..0bee1c9 100644
--- a/ash/launcher/launcher_view.h
+++ b/ash/launcher/launcher_view.h
@@ -345,9 +345,9 @@ class ASH_EXPORT LauncherView : public views::View,
   // element will be set to false.
   bool* got_deleted_;
 
-  // True if a drag and drop operation created the item in the launcher and it
-  // needs to be deleted again if the operation gets cancelled.
-  bool drag_and_drop_item_created_;
+  // True if a drag and drop operation created/pinned the item in the launcher
+  // and it needs to be deleted/unpinned again if the operation gets cancelled.
+  bool drag_and_drop_item_pinned_;
 
   // The launcher item which is currently used for a drag and a drop operation
   // or 0 otherwise.
diff --git a/ash/shell/launcher_delegate_impl.cc b/ash/shell/launcher_delegate_impl.cc
index f933119..51f6171 100644
--- a/ash/shell/launcher_delegate_impl.cc
+++ b/ash/shell/launcher_delegate_impl.cc
@@ -76,6 +76,10 @@ LauncherID LauncherDelegateImpl::GetLauncherIDForAppID(
 void LauncherDelegateImpl::PinAppWithID(const std::string& app_id) {
 }
 
+bool LauncherDelegateImpl::IsAppPinned(const std::string& app_id) {
+  return false;
+}
+
 void LauncherDelegateImpl::UnpinAppsWithID(const std::string& app_id) {
 }
 
diff --git a/ash/shell/launcher_delegate_impl.h b/ash/shell/launcher_delegate_impl.h
index 754ee69..4788029 100644
--- a/ash/shell/launcher_delegate_impl.h
+++ b/ash/shell/launcher_delegate_impl.h
@@ -42,6 +42,7 @@ class LauncherDelegateImpl : public ash::LauncherDelegate {
   virtual bool IsPerAppLauncher() OVERRIDE;
   virtual LauncherID GetLauncherIDForAppID(const std::string& app_id) OVERRIDE;
   virtual void PinAppWithID(const std::string& app_id) OVERRIDE;
+  virtual bool IsAppPinned(const std::string& app_id) OVERRIDE;
   virtual void UnpinAppsWithID(const std::string& app_id) OVERRIDE;
 
  private:
diff --git a/ash/test/test_launcher_delegate.cc b/ash/test/test_launcher_delegate.cc
index a22ccb7..444a79c 100644
--- a/ash/test/test_launcher_delegate.cc
+++ b/ash/test/test_launcher_delegate.cc
@@ -132,6 +132,10 @@ LauncherID TestLauncherDelegate::GetLauncherIDForAppID(
 void TestLauncherDelegate::PinAppWithID(const std::string& app_id) {
 }
 
+bool TestLauncherDelegate::IsAppPinned(const std::string& app_id) {
+  return false;
+}
+
 void TestLauncherDelegate::UnpinAppsWithID(const std::string& app_id) {
 }
 
diff --git a/ash/test/test_launcher_delegate.h b/ash/test/test_launcher_delegate.h
index 60d7411..3add6545 100644
--- a/ash/test/test_launcher_delegate.h
+++ b/ash/test/test_launcher_delegate.h
@@ -51,6 +51,7 @@ class TestLauncherDelegate : public LauncherDelegate,
   virtual bool IsPerAppLauncher() OVERRIDE;
   virtual LauncherID GetLauncherIDForAppID(const std::string& app_id) OVERRIDE;
   virtual void PinAppWithID(const std::string& app_id) OVERRIDE;
+  virtual bool IsAppPinned(const std::string& app_id) OVERRIDE;
   virtual void UnpinAppsWithID(const std::string& app_id) OVERRIDE;
 
  private: