Add a feature of dragging an unpinned open app to pin in shelf

Previously the only way to pin an unpinned open app in shelf is to use the context menu. In this update, we enable the feature that dragging an unpinned open app to the pinned app side is available to pin the app.

Bug: 1084856

Change-Id: I559a8967942a1c4d6ee7eb370cd81181b173c361
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2321065Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Commit-Queue: Wen-Chien Wang <wcwang@google.com>
Cr-Commit-Position: refs/heads/master@{#798198}

ash/public/cpp/ash_features.cc

@@ -148,6 +148,9 @@ const base::Feature kMaintainShelfStateWhenEnteringOverview{
 const base::Feature kTemporaryHoldingSpace{"TemporaryHoldingSpace",
                                            base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kDragUnpinnedAppToPin{"DragUnpinnedAppToPin",
+                                          base::FEATURE_DISABLED_BY_DEFAULT};
+
 bool IsAllowAmbientEQEnabled() {
   return base::FeatureList::IsEnabled(kAllowAmbientEQ);
 }
@@ -303,6 +306,10 @@ bool IsTemporaryHoldingSpaceEnabled() {
   return base::FeatureList::IsEnabled(kTemporaryHoldingSpace);
 }
 
+bool IsDragUnpinnedAppToPinEnabled() {
+  return base::FeatureList::IsEnabled(kDragUnpinnedAppToPin);
+}
+
 namespace {
 
 // The boolean flag indicating if "WebUITabStrip" feature is enabled in Chrome.

ash/public/cpp/ash_features.h

@@ -163,6 +163,9 @@ ASH_PUBLIC_EXPORT extern const base::Feature
 // later.
 ASH_PUBLIC_EXPORT extern const base::Feature kTemporaryHoldingSpace;
 
+// Enables dragging an unpinned open app to pinned app side to pin.
+ASH_PUBLIC_EXPORT extern const base::Feature kDragUnpinnedAppToPin;
+
 ASH_PUBLIC_EXPORT bool IsAllowAmbientEQEnabled();
 
 ASH_PUBLIC_EXPORT bool IsAltTabLimitedToActiveDesk();
@@ -227,6 +230,8 @@ ASH_PUBLIC_EXPORT bool IsMaintainShelfStateWhenEnteringOverviewEnabled();
 
 ASH_PUBLIC_EXPORT bool IsTemporaryHoldingSpaceEnabled();
 
+ASH_PUBLIC_EXPORT bool IsDragUnpinnedAppToPinEnabled();
+
 // These two functions are supposed to be temporary functions to set or get
 // whether "WebUITabStrip" feature is enabled from Chrome.
 ASH_PUBLIC_EXPORT void SetWebUITabStripEnabled(bool enabled);

ash/shelf/shelf_view.h

@@ -337,9 +337,8 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   // not available for app icons.
   int GetAvailableSpaceForAppIcons() const;
 
-  // Returns the index of the item after which the separator should be shown,
-  // or -1 if no separator is required.
-  int GetSeparatorIndex() const;
+  // Updates the index of the separator and save it to |separator_index_|.
+  void UpdateSeparatorIndex();
 
   // Sets the bounds of each view to its ideal bounds.
   void LayoutToIdealBounds();
@@ -406,6 +405,15 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   // dragged to.
   std::pair<int, int> GetDragRange(int index);
 
+  // Checks if the item at |dragged_item_index| should be pinned or unpinned on
+  // pointer release.
+  bool ShouldUpdateDraggedViewPinStatus(int dragged_item_index);
+
+  // Checks if |dragged_view| is allowed to be dragged across the separator to
+  // perform pinning and unpinning. Note that this function doesn't check if the
+  // separator exists.
+  bool CanDragAcrossSeparator(views::View* dragged_view) const;
+
   // If there is a drag operation in progress it's canceled. If |modified_index|
   // is valid, the new position of the corresponding item is returned.
   int CancelDrag(int modified_index);
@@ -553,6 +561,25 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   // items.
   views::Separator* separator_ = nullptr;
 
+  // Index of |separator_|. It is set to -1 if it is invisible.
+  int separator_index_ = -1;
+
+  // Used in |drag_view_relative_to_ideal_bounds_| to represent the relative
+  // position between |drag_view_| and its ideal bounds in shelf.
+  enum class RelativePosition {
+    // Set if |drag_view_| is not available or the relative position is not
+    // calculated yet.
+    kNotAvailable,
+    // Set if |drag_view_| is to the left of its ideal bounds.
+    kLeft,
+    // Set if |drag_view_| is to the right of its ideal bounds.
+    kRight
+  };
+
+  // The |drag_view_|'s current position relative to its ideal bounds.
+  RelativePosition drag_view_relative_to_ideal_bounds_ =
+      RelativePosition::kNotAvailable;
+
   // Position of the mouse down event in |drag_view_|'s coordinates.
   gfx::Point drag_origin_;
 

ash/shelf/shelf_view_test_api.cc

@@ -13,6 +13,7 @@
 #include "base/run_loop.h"
 #include "ui/views/animation/bounds_animator.h"
 #include "ui/views/controls/menu/menu_runner.h"
+#include "ui/views/controls/separator.h"
 #include "ui/views/view_model.h"
 
 namespace {
@@ -153,4 +154,12 @@ void ShelfViewTestAPI::SetShelfContextMenuCallback(
   shelf_view_->context_menu_shown_callback_ = std::move(closure);
 }
 
+int ShelfViewTestAPI::GetSeparatorIndex() const {
+  return shelf_view_->separator_index_;
+}
+
+bool ShelfViewTestAPI::IsSeparatorVisible() const {
+  return shelf_view_->separator_->GetVisible();
+}
+
 }  // namespace ash

ash/shelf/shelf_view_test_api.h

@@ -113,6 +113,12 @@ class ShelfViewTestAPI {
   // Set callback which will run after showing shelf context menu.
   void SetShelfContextMenuCallback(base::RepeatingClosure closure);
 
+  // Returns |separator_index_|.
+  int GetSeparatorIndex() const;
+
+  // Checks whether the separator is visible or not.
+  bool IsSeparatorVisible() const;
+
  private:
   ShelfView* shelf_view_;
   int id_ = 0;

ash/shelf/shelf_view_unittest.cc

@@ -402,7 +402,6 @@ class ShelfViewTest : public AshTestBase {
       ShelfItem item = model_->items()[model_index];
       ShelfID id = item.id;
       EXPECT_EQ(id_map[map_index].first, id);
-      EXPECT_EQ(id_map[map_index].second, GetButtonByID(id));
       ++map_index;
     }
     ASSERT_EQ(map_index, id_map.size());
@@ -622,6 +621,17 @@ class ShelfViewTextDirectionTest : public ShelfViewTest,
   DISALLOW_COPY_AND_ASSIGN(ShelfViewTextDirectionTest);
 };
 
+class ShelfViewDragToPinTest : public ShelfViewTest {
+ public:
+  ShelfViewDragToPinTest() {
+    feature_list_.InitAndEnableFeature(features::kDragUnpinnedAppToPin);
+  }
+  ~ShelfViewDragToPinTest() override = default;
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 TEST_F(ShelfViewTest, VisibleShelfItemsBounds) {
   // Add 3 pinned apps, and a normal app.
   AddAppShortcut();
@@ -734,6 +744,153 @@ TEST_F(ShelfViewTest, SimultaneousDrag) {
   ASSERT_NO_FATAL_FAILURE(CheckModelIDs(id_map));
 }
 
+// Ensure that the behavior of pinning and unpinning by dragging works as
+// expected.
+TEST_F(ShelfViewDragToPinTest, DragAppsToPinAndUnpin) {
+  std::vector<std::pair<ShelfID, views::View*>> id_map;
+  SetupForDragTest(&id_map);
+  int pinned_apps_size = id_map.size();
+
+  const ShelfID open_app_id = AddApp();
+  id_map.push_back(std::make_pair(open_app_id, GetButtonByID(open_app_id)));
+
+  // Run the pinned app at index 1.
+  ShelfItem item = model_->items()[1];
+  item.status = STATUS_RUNNING;
+  model_->Set(1, item);
+  id_map[1].second = GetButtonByID(item.id);
+
+  ASSERT_TRUE(static_cast<ShelfAppButton*>(id_map[1].second)->state() &
+              ShelfAppButton::STATE_RUNNING);
+  ASSERT_FALSE(static_cast<ShelfAppButton*>(id_map[2].second)->state() &
+               ShelfAppButton::STATE_RUNNING);
+  ASSERT_TRUE(GetButtonByID(open_app_id)->state() &
+              ShelfAppButton::STATE_RUNNING);
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 1);
+
+  // Drag the app at index 1 and move it to the end of the shelf. With separator
+  // available and the app is dragged to the unpinned app side, the dragged open
+  // app should be unpinned and moved to the released position.
+  views::View* dragged_button =
+      SimulateDrag(ShelfView::MOUSE, 1, id_map.size() - 1, false);
+  std::rotate(id_map.begin() + 1, id_map.begin() + 2, id_map.end());
+  ASSERT_NO_FATAL_FAILURE(CheckModelIDs(id_map));
+  shelf_view_->PointerReleasedOnButton(dragged_button, ShelfView::MOUSE, false);
+  EXPECT_FALSE(IsAppPinned(id_map.back().first));
+  --pinned_apps_size;
+
+  // Drag the app at index 2 and move it to the end of the shelf. With separator
+  // available and the app is dragged to the unpinned app side, the dragged app
+  // with no running instance should be unpinned and removed from shelf.
+  dragged_button = SimulateDrag(ShelfView::MOUSE, 2, id_map.size() - 1, false);
+  shelf_view_->PointerReleasedOnButton(dragged_button, ShelfView::MOUSE, false);
+  id_map.erase(id_map.begin() + 2);
+  EXPECT_EQ(id_map.size(), model_->items().size());
+  ASSERT_NO_FATAL_FAILURE(CheckModelIDs(id_map));
+  --pinned_apps_size;
+
+  // Drag an app in unpinned app side and move it to the beginning of the shelf.
+  // With separator available and the app is dragged to the pinned app side, the
+  // dragged app should be pinned and moved to the released position.
+  dragged_button = SimulateDrag(ShelfView::MOUSE, id_map.size() - 2, 0, false);
+  std::rotate(id_map.rbegin() + 1, id_map.rbegin() + 2, id_map.rend());
+  ASSERT_NO_FATAL_FAILURE(CheckModelIDs(id_map));
+  shelf_view_->PointerReleasedOnButton(dragged_button, ShelfView::MOUSE, false);
+  EXPECT_TRUE(IsAppPinned(id_map[0].first));
+  ++pinned_apps_size;
+
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 1);
+}
+
+// Check that the Browser Shortcut will not be dragged to the unpinned app side
+// or be unpinned by dragging.
+TEST_F(ShelfViewDragToPinTest, BlockBrowserShortcutFromUnpinningByDragging) {
+  std::vector<std::pair<ShelfID, views::View*>> id_map;
+  SetupForDragTest(&id_map);
+  const int pinned_apps_size = id_map.size();
+
+  const ShelfID open_app_id = AddApp();
+  id_map.push_back(std::make_pair(open_app_id, GetButtonByID(open_app_id)));
+
+  EXPECT_TRUE(model_->items()[0].type == TYPE_BROWSER_SHORTCUT);
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 1);
+
+  // Dragging the browser shortcut to the unpinned app side should not make it
+  // unpinned.
+  views::View* dragged_button =
+      SimulateDrag(ShelfView::MOUSE, 0, id_map.size() - 1, false);
+  std::rotate(id_map.begin(), id_map.begin() + 1,
+              id_map.begin() + pinned_apps_size);
+  ASSERT_NO_FATAL_FAILURE(CheckModelIDs(id_map));
+
+  // When drag pointer released, the browser shortcut should be the last app in
+  // the pinned app side.
+  shelf_view_->PointerReleasedOnButton(dragged_button, ShelfView::MOUSE, false);
+  EXPECT_EQ(model_->items()[pinned_apps_size - 1].type, TYPE_BROWSER_SHORTCUT);
+}
+
+// Check that separator index updates as expected when a drag view is dragged
+// over it.
+TEST_F(ShelfViewDragToPinTest, DragAppAroundSeparator) {
+  std::vector<std::pair<ShelfID, views::View*>> id_map;
+  SetupForDragTest(&id_map);
+  const int pinned_apps_size = id_map.size();
+
+  const ShelfID open_app_id = AddApp();
+  id_map.push_back(std::make_pair(open_app_id, GetButtonByID(open_app_id)));
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 1);
+  const int button_width =
+      GetButtonByID(open_app_id)->GetBoundsInScreen().width();
+
+  ui::test::EventGenerator* generator = GetEventGenerator();
+
+  // The test makes some assumptions that the shelf is bottom aligned.
+  ASSERT_EQ(shelf_view_->shelf()->alignment(), ShelfAlignment::kBottom);
+
+  // Drag an unpinned open app that is beside the separator around and check
+  // that the separator is correctly placed.
+  ASSERT_EQ(model_->ItemIndexByID(open_app_id),
+            test_api_->GetSeparatorIndex() + 1);
+  gfx::Point unpinned_app_location =
+      GetButtonCenter(GetButtonByID(open_app_id));
+  generator->set_current_screen_location(unpinned_app_location);
+  generator->PressLeftButton();
+  // Drag the mouse slightly to the left. The dragged app will stay at the same
+  // index but the separator will move to the right.
+  generator->MoveMouseBy(-button_width / 4, 0);
+  // In this case, the separator is moved to the end of the shelf so it is set
+  // invisible and the |separator_index_| will be updated to -1.
+  EXPECT_FALSE(test_api_->IsSeparatorVisible());
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), -1);
+  // Drag the mouse slightly to the right where the dragged app will stay at the
+  // same index.
+  generator->MoveMouseBy(button_width / 2, 0);
+  // In this case, because the dragged app is not released or pinned yet,
+  // dragging it back to its original place will show the separator again.
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 1);
+  generator->ReleaseLeftButton();
+
+  // Drag an pinned app that is beside the separator around and check that the
+  // separator is correctly placed. Check that the dragged app is not a browser
+  // shortcut, which can not be dragged across the separator.
+  ASSERT_NE(model_->items()[pinned_apps_size - 1].type, TYPE_BROWSER_SHORTCUT);
+  ASSERT_EQ(model_->ItemIndexByID(id_map[pinned_apps_size - 1].first),
+            test_api_->GetSeparatorIndex());
+  gfx::Point pinned_app_location =
+      GetButtonCenter(id_map[pinned_apps_size - 1].first);
+  generator->set_current_screen_location(pinned_app_location);
+  generator->PressLeftButton();
+  // Drag the mouse slightly to the right. The dragged app will stay at the same
+  // index but the separator will move to the left.
+  generator->MoveMouseBy(button_width / 4, 0);
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 2);
+  // Drag the mouse slightly to the left. The dragged app will stay at the same
+  // index but the separator will move to the right.
+  generator->MoveMouseBy(-button_width / 2, 0);
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 1);
+  generator->ReleaseLeftButton();
+}
+
 // Ensure that clicking on one item and then dragging another works as expected.
 TEST_F(ShelfViewTest, ClickOneDragAnother) {
   std::vector<std::pair<ShelfID, views::View*>> id_map;