[Tab Groups] Removing from group moves the tab to the closest position

Previously, tabs would always get removed from the group and moved to
the end of the tab group. This change results in the tab moving the
minimum distance when removing from the group.

Bug: 1038362
Change-Id: I513b4e6d97ec847d200cdec0c54f64745cab4191
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1986922
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728377}

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -1021,6 +1021,14 @@ void TabStripModel::AddToExistingGroup(const std::vector<int>& indices,
   AddToExistingGroupImpl(indices, group);
 }
 
+void TabStripModel::MoveTabsAndSetGroup(const std::vector<int>& indices,
+                                        int destination_index,
+                                        tab_groups::TabGroupId group) {
+  ReentrancyCheck reentrancy_check(&reentrancy_guard_);
+
+  MoveTabsAndSetGroupImpl(indices, destination_index, group);
+}
+
 void TabStripModel::AddToGroupForRestore(const std::vector<int>& indices,
                                          tab_groups::TabGroupId group) {
   ReentrancyCheck reentrancy_check(&reentrancy_guard_);
@@ -1050,20 +1058,33 @@ void TabStripModel::UpdateGroupForDragRevert(
 void TabStripModel::RemoveFromGroup(const std::vector<int>& indices) {
   ReentrancyCheck reentrancy_check(&reentrancy_guard_);
 
-  // Remove each tab from the group it's in, if any. Go from right to left
-  // since tabs may move to the right.
-  for (int i = indices.size() - 1; i >= 0; i--) {
-    const int index = indices[i];
+  std::map<tab_groups::TabGroupId, std::vector<int>> indices_per_tab_group;
+
+  for (int index : indices) {
     base::Optional<tab_groups::TabGroupId> old_group = GetTabGroupForTab(index);
-    if (!old_group.has_value())
-      continue;
+    if (old_group.has_value())
+      indices_per_tab_group[old_group.value()].push_back(index);
+  }
 
-    // Move the tab until it's the rightmost tab in its group
-    int new_index = index;
-    while (ContainsIndex(new_index + 1) &&
-           GetTabGroupForTab(new_index + 1) == old_group)
-      new_index++;
-    MoveAndSetGroup(index, new_index, base::nullopt);
+  for (const auto& kv : indices_per_tab_group) {
+    const std::vector<int>& tabs_in_group =
+        group_model_->GetTabGroup(kv.first)->ListTabs();
+
+    // Split group into |left_of_group| and |right_of_group| depending on
+    // whether the index is closest to the left or right edge.
+    std::vector<int> left_of_group;
+    std::vector<int> right_of_group;
+    for (int index : kv.second) {
+      if (index < tabs_in_group[tabs_in_group.size() / 2]) {
+        left_of_group.push_back(index);
+      } else {
+        right_of_group.push_back(index);
+      }
+    }
+    MoveTabsAndSetGroupImpl(left_of_group, tabs_in_group.front(),
+                            base::nullopt);
+    MoveTabsAndSetGroupImpl(right_of_group, tabs_in_group.back() + 1,
+                            base::nullopt);
   }
 }
 
@@ -1751,7 +1772,7 @@ void TabStripModel::AddToNewGroupImpl(const std::vector<int>& indices,
   std::vector<int> new_indices = indices;
   new_indices = SetTabsPinned(new_indices, false);
 
-  MoveTabsIntoGroupImpl(new_indices, destination_index, new_group);
+  MoveTabsAndSetGroupImpl(new_indices, destination_index, new_group);
 }
 
 void TabStripModel::AddToExistingGroupImpl(const std::vector<int>& indices,
@@ -1790,13 +1811,14 @@ void TabStripModel::AddToExistingGroupImpl(const std::vector<int>& indices,
     }
   }
 
-  MoveTabsIntoGroupImpl(tabs_left_of_group, tabs_in_group.front(), group);
-  MoveTabsIntoGroupImpl(tabs_right_of_group, tabs_in_group.back() + 1, group);
+  MoveTabsAndSetGroupImpl(tabs_left_of_group, tabs_in_group.front(), group);
+  MoveTabsAndSetGroupImpl(tabs_right_of_group, tabs_in_group.back() + 1, group);
 }
 
-void TabStripModel::MoveTabsIntoGroupImpl(const std::vector<int>& indices,
-                                          int destination_index,
-                                          tab_groups::TabGroupId group) {
+void TabStripModel::MoveTabsAndSetGroupImpl(
+    const std::vector<int>& indices,
+    int destination_index,
+    base::Optional<tab_groups::TabGroupId> group) {
   // Some tabs will need to be moved to the right, some to the left. We need to
   // handle those separately. First, move tabs to the right, starting with the
   // rightmost tab so we don't cause other tabs we are about to move to shift.

chrome/browser/ui/tabs/tab_strip_model.h

@@ -419,6 +419,13 @@ class TabStripModel : public TabGroupController {
   void AddToExistingGroup(const std::vector<int>& indices,
                           tab_groups::TabGroupId group);
 
+  // Moves the set of tabs indicated by |indices| to precede the tab at index
+  // |destination_index|, maintaining their order and the order of tabs not
+  // being moved, and adds them to the tab group |group|.
+  void MoveTabsAndSetGroup(const std::vector<int>& indices,
+                           int destination_index,
+                           tab_groups::TabGroupId group);
+
   // Similar to AddToExistingGroup(), but creates a group with id |group| if it
   // doesn't exist. This is only intended to be called from session restore
   // code.
@@ -649,12 +656,12 @@ class TabStripModel : public TabGroupController {
   void AddToExistingGroupImpl(const std::vector<int>& indices,
                               tab_groups::TabGroupId group);
 
-  // Moves the set of tabs indicated by |indices| to precede the tab at index
-  // |destination_index|, maintaining their order and the order of tabs not
-  // being moved, and adds them to the tab group |group|.
-  void MoveTabsIntoGroupImpl(const std::vector<int>& indices,
-                             int destination_index,
-                             tab_groups::TabGroupId group);
+  // Implementation of MoveTabsAndSetGroupImpl. Moves the set of tabs in
+  // |indices| to the |destination_index| and updates the tabs to the
+  // appropriate |group|.
+  void MoveTabsAndSetGroupImpl(const std::vector<int>& indices,
+                               int destination_index,
+                               base::Optional<tab_groups::TabGroupId> group);
 
   // Moves the tab at |index| to |new_index| and sets its group to |new_group|.
   // Notifies any observers that group affiliation has changed for the tab.

chrome/browser/ui/tabs/tab_strip_model_unittest.cc

@@ -3155,7 +3155,7 @@ TEST_F(TabStripModelTest, RemoveTabFromGroupUpdatesObservers) {
   strip.CloseAllTabs();
 }
 
-TEST_F(TabStripModelTest, RemoveTabFromGroupReorders) {
+TEST_F(TabStripModelTest, RemoveTabFromGroupMaintainsOrder) {
   TestTabStripModelDelegate delegate;
   TabStripModel strip(&delegate, profile());
   PrepareTabs(&strip, 2);
@@ -3163,9 +3163,9 @@ TEST_F(TabStripModelTest, RemoveTabFromGroupReorders) {
 
   strip.RemoveFromGroup({0});
 
-  EXPECT_FALSE(strip.GetTabGroupForTab(1).has_value());
-  EXPECT_TRUE(strip.GetTabGroupForTab(0).has_value());
-  EXPECT_EQ("1 0", GetTabStripStateString(strip));
+  EXPECT_TRUE(strip.GetTabGroupForTab(1).has_value());
+  EXPECT_FALSE(strip.GetTabGroupForTab(0).has_value());
+  EXPECT_EQ("0 1", GetTabStripStateString(strip));
 
   strip.CloseAllTabs();
 }
@@ -3182,7 +3182,8 @@ TEST_F(TabStripModelTest, RemoveTabFromGroupDoesntReorderIfNoGroup) {
   strip.CloseAllTabs();
 }
 
-TEST_F(TabStripModelTest, RemoveTabFromGroupMaintainsOrderOfSelectedTabs) {
+TEST_F(TabStripModelTest,
+       RemoveTabFromGroupMaintainsRelativeOrderOfSelectedTabs) {
   TestTabStripModelDelegate delegate;
   TabStripModel strip(&delegate, profile());
   PrepareTabs(&strip, 4);
@@ -3190,11 +3191,11 @@ TEST_F(TabStripModelTest, RemoveTabFromGroupMaintainsOrderOfSelectedTabs) {
 
   strip.RemoveFromGroup({0, 2});
 
-  EXPECT_TRUE(strip.GetTabGroupForTab(0).has_value());
+  EXPECT_FALSE(strip.GetTabGroupForTab(0).has_value());
   EXPECT_TRUE(strip.GetTabGroupForTab(1).has_value());
-  EXPECT_FALSE(strip.GetTabGroupForTab(2).has_value());
+  EXPECT_TRUE(strip.GetTabGroupForTab(2).has_value());
   EXPECT_FALSE(strip.GetTabGroupForTab(3).has_value());
-  EXPECT_EQ("1 3 0 2", GetTabStripStateString(strip));
+  EXPECT_EQ("0 1 3 2", GetTabStripStateString(strip));
 
   strip.CloseAllTabs();
 }
@@ -3208,12 +3209,12 @@ TEST_F(TabStripModelTest, RemoveTabFromGroupMixtureOfGroups) {
 
   strip.RemoveFromGroup({0, 3, 4});
 
-  EXPECT_TRUE(strip.GetTabGroupForTab(0).has_value());
-  EXPECT_FALSE(strip.GetTabGroupForTab(1).has_value());
+  EXPECT_FALSE(strip.GetTabGroupForTab(0).has_value());
+  EXPECT_TRUE(strip.GetTabGroupForTab(1).has_value());
   EXPECT_TRUE(strip.GetTabGroupForTab(2).has_value());
   EXPECT_FALSE(strip.GetTabGroupForTab(3).has_value());
   EXPECT_FALSE(strip.GetTabGroupForTab(4).has_value());
-  EXPECT_EQ("1 0 2 3 4", GetTabStripStateString(strip));
+  EXPECT_EQ("0 1 2 3 4", GetTabStripStateString(strip));
 
   strip.CloseAllTabs();
 }

chrome/browser/ui/views/tabs/tab_drag_controller.cc

@@ -2142,7 +2142,8 @@ void TabDragController::UpdateGroupForDraggedTabs() {
   }
 
   if (updated_group.has_value()) {
-    attached_model->AddToExistingGroup(selected, updated_group.value());
+    attached_model->MoveTabsAndSetGroup(selected, selected[0],
+                                        updated_group.value());
   } else {
     attached_model->RemoveFromGroup(selected);
     attached_model->MoveSelectedTabsTo(to_index);