[Tab Group Sync] Added separate method for move
Moving a tab results in a conflict in the tab position between the tab
being moved and the position to which it is being moved. This CL
corrects that by invoking SavedTabGroupModel::MoveTabInGroupTo that
auto-corrects position of all tabs.
Bug: b/337668132
Change-Id: I0f3e38e8ca2616bb2b9594d766c6b844478f159e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5495839
Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293828}
diff --git a/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/RemoteTabGroupMutationHelper.java b/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/RemoteTabGroupMutationHelper.java
index 4ecf742..18259df1 100644
--- a/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/RemoteTabGroupMutationHelper.java
+++ b/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/RemoteTabGroupMutationHelper.java
@@ -94,6 +94,10 @@
tabGroupId, tab.getId(), tab.getTitle(), tab.getUrl(), position);
}
+ public void moveTab(LocalTabGroupId tabGroupId, int tabId, int newPosition) {
+ mTabGroupSyncService.moveTab(tabGroupId, tabId, newPosition);
+ }
+
public void removeTab(LocalTabGroupId tabGroupId, int tabId) {
mTabGroupSyncService.removeTab(tabGroupId, tabId);
}
diff --git a/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TabGroupSyncLocalObserver.java b/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TabGroupSyncLocalObserver.java
index c8a12cc0..a8c7b66 100644
--- a/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TabGroupSyncLocalObserver.java
+++ b/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TabGroupSyncLocalObserver.java
@@ -161,7 +161,8 @@
LocalTabGroupId tabGroupId =
TabGroupSyncUtils.getLocalTabGroupId(
mTabGroupModelFilter, movedTab.getRootId());
- mRemoteTabGroupMutationHelper.updateTab(tabGroupId, movedTab, positionInGroup);
+ mRemoteTabGroupMutationHelper.moveTab(
+ tabGroupId, movedTab.getId(), positionInGroup);
}
@Override
diff --git a/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TabGroupSyncLocalObserverUnitTest.java b/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TabGroupSyncLocalObserverUnitTest.java
index 01d853c..d52ae91f 100644
--- a/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TabGroupSyncLocalObserverUnitTest.java
+++ b/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TabGroupSyncLocalObserverUnitTest.java
@@ -172,7 +172,7 @@
when(mTabGroupModelFilter.getIndexOfTabInGroup(mTab1)).thenReturn(0);
mTabGroupModelFilterObserverCaptor.getValue().didMoveWithinGroup(mTab1, 0, 1);
verify(mTabGroupSyncService, times(1))
- .updateTab(eq(LOCAL_TAB_GROUP_ID_1), eq(TAB_ID_1), any(), any(), anyInt());
+ .moveTab(eq(LOCAL_TAB_GROUP_ID_1), eq(TAB_ID_1), anyInt());
}
@Test
diff --git a/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TestTabGroupSyncService.java b/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TestTabGroupSyncService.java
index e6895b1..15c74a2 100644
--- a/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TestTabGroupSyncService.java
+++ b/chrome/browser/tab_group_sync/android/java/src/org/chromium/chrome/browser/tab_group_sync/TestTabGroupSyncService.java
@@ -53,6 +53,9 @@
public void removeTab(LocalTabGroupId tabGroupId, int tabId) {}
@Override
+ public void moveTab(LocalTabGroupId tabGroupId, int tabId, int newIndexInGroup) {}
+
+ @Override
public String[] getAllGroupIds() {
return new String[0];
}
diff --git a/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncService.java b/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncService.java
index e8bdac8f..07c5cf1 100644
--- a/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncService.java
+++ b/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncService.java
@@ -135,6 +135,15 @@
void removeTab(LocalTabGroupId tabGroupId, int tabId);
/**
+ * Moves a tab within a group.
+ *
+ * @param tabGroupId The local group ID of the corresponding tab group.
+ * @param tabId The local tab ID of the tab being removed.
+ * @param newIndexInGroup The new index of the tab in the group.
+ */
+ void moveTab(LocalTabGroupId tabGroupId, int tabId, int newIndexInGroup);
+
+ /**
* Called to return all the remote tab group IDs currently existing in the system.
*
* @return An array of IDs of the currently known tab groups.
diff --git a/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncServiceAndroidUnitTest.java b/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncServiceAndroidUnitTest.java
index 19f3f956..f84d437 100644
--- a/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncServiceAndroidUnitTest.java
+++ b/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncServiceAndroidUnitTest.java
@@ -39,6 +39,9 @@
private static final String TEST_TAB_TITLE = "Test Tab";
private static final String TEST_URL = "https://google.com";
private static final LocalTabGroupId LOCAL_TAB_GROUP_ID_1 = new LocalTabGroupId(TOKEN_1);
+ private static final int LOCAL_TAB_ID_1 = 2;
+ private static final int LOCAL_TAB_ID_2 = 4;
+ private static final int TAB_POSITION = 3;
private TabGroupSyncService mService;
private TabGroupSyncService.Observer mObserver;
@@ -129,19 +132,35 @@
@CalledByNative
public void testAddTab() {
- mService.addTab(LOCAL_TAB_GROUP_ID_1, 2, TEST_TAB_TITLE, new GURL(TEST_URL), 3);
- mService.addTab(LOCAL_TAB_GROUP_ID_1, 4, TEST_TAB_TITLE, new GURL(TEST_URL), -1);
+ mService.addTab(
+ LOCAL_TAB_GROUP_ID_1,
+ LOCAL_TAB_ID_1,
+ TEST_TAB_TITLE,
+ new GURL(TEST_URL),
+ TAB_POSITION);
+ mService.addTab(
+ LOCAL_TAB_GROUP_ID_1, LOCAL_TAB_ID_2, TEST_TAB_TITLE, new GURL(TEST_URL), -1);
}
@CalledByNative
public void testUpdateTab() {
- mService.updateTab(LOCAL_TAB_GROUP_ID_1, 2, TEST_TAB_TITLE, new GURL(TEST_URL), 3);
+ mService.updateTab(
+ LOCAL_TAB_GROUP_ID_1,
+ LOCAL_TAB_ID_1,
+ TEST_TAB_TITLE,
+ new GURL(TEST_URL),
+ TAB_POSITION);
mService.updateTab(LOCAL_TAB_GROUP_ID_1, 4, TEST_TAB_TITLE, new GURL(TEST_URL), -1);
}
@CalledByNative
public void testRemoveTab() {
- mService.removeTab(LOCAL_TAB_GROUP_ID_1, 2);
+ mService.removeTab(LOCAL_TAB_GROUP_ID_1, LOCAL_TAB_ID_1);
+ }
+
+ @CalledByNative
+ public void testMoveTab() {
+ mService.moveTab(LOCAL_TAB_GROUP_ID_1, LOCAL_TAB_ID_1, TAB_POSITION);
}
@CalledByNative
diff --git a/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncServiceImpl.java b/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncServiceImpl.java
index ad501d3..d0bdd8b 100644
--- a/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncServiceImpl.java
+++ b/components/saved_tab_groups/android/java/src/org/chromium/components/tab_group_sync/TabGroupSyncServiceImpl.java
@@ -65,11 +65,10 @@
}
@Override
- public void updateVisualData(LocalTabGroupId tabGroupId, String title, int color) {
+ public void updateVisualData(LocalTabGroupId groupId, String title, int color) {
if (mNativePtr == 0) return;
- assert tabGroupId != null;
- TabGroupSyncServiceImplJni.get()
- .updateVisualData(mNativePtr, this, tabGroupId, title, color);
+ assert groupId != null;
+ TabGroupSyncServiceImplJni.get().updateVisualData(mNativePtr, this, groupId, title, color);
}
@Override
@@ -97,6 +96,13 @@
}
@Override
+ public void moveTab(LocalTabGroupId groupId, int tabId, int newIndexInGroup) {
+ if (mNativePtr == 0) return;
+ assert groupId != null;
+ TabGroupSyncServiceImplJni.get().moveTab(mNativePtr, this, groupId, tabId, newIndexInGroup);
+ }
+
+ @Override
public String[] getAllGroupIds() {
if (mNativePtr == 0) return new String[0];
return TabGroupSyncServiceImplJni.get().getAllGroupIds(mNativePtr, this);
@@ -111,7 +117,7 @@
@Override
public SavedTabGroup getGroup(LocalTabGroupId localGroupId) {
- if (mNativePtr == 0) return null;
+ assert localGroupId != null;
return TabGroupSyncServiceImplJni.get()
.getGroupByLocalGroupId(mNativePtr, this, localGroupId);
}
@@ -235,6 +241,13 @@
LocalTabGroupId groupId,
int tabId);
+ void moveTab(
+ long nativeTabGroupSyncServiceAndroid,
+ TabGroupSyncServiceImpl caller,
+ LocalTabGroupId groupId,
+ int tabId,
+ int newIndexInGroup);
+
String[] getAllGroupIds(
long nativeTabGroupSyncServiceAndroid, TabGroupSyncServiceImpl caller);
diff --git a/components/saved_tab_groups/android/tab_group_sync_service_android.cc b/components/saved_tab_groups/android/tab_group_sync_service_android.cc
index ab738b9..b9e3647 100644
--- a/components/saved_tab_groups/android/tab_group_sync_service_android.cc
+++ b/components/saved_tab_groups/android/tab_group_sync_service_android.cc
@@ -182,6 +182,18 @@
tab_group_sync_service_->RemoveTab(group_id, tab_id);
}
+void TabGroupSyncServiceAndroid::MoveTab(
+ JNIEnv* env,
+ const JavaParamRef<jobject>& j_caller,
+ const JavaParamRef<jobject>& j_group_id,
+ jint j_tab_id,
+ int j_new_index_in_group) {
+ auto group_id =
+ TabGroupSyncConversionsBridge::FromJavaTabGroupId(env, j_group_id);
+ auto tab_id = FromJavaTabId(j_tab_id);
+ tab_group_sync_service_->MoveTab(group_id, tab_id, j_new_index_in_group);
+}
+
ScopedJavaLocalRef<jobjectArray> TabGroupSyncServiceAndroid::GetAllGroupIds(
JNIEnv* env,
const JavaParamRef<jobject>& j_caller) {
diff --git a/components/saved_tab_groups/android/tab_group_sync_service_android.h b/components/saved_tab_groups/android/tab_group_sync_service_android.h
index 8d8c4f9..82343c5 100644
--- a/components/saved_tab_groups/android/tab_group_sync_service_android.h
+++ b/components/saved_tab_groups/android/tab_group_sync_service_android.h
@@ -75,6 +75,12 @@
const JavaParamRef<jobject>& j_group_id,
jint j_tab_id);
+ void MoveTab(JNIEnv* env,
+ const JavaParamRef<jobject>& j_caller,
+ const JavaParamRef<jobject>& j_group_id,
+ jint j_tab_id,
+ int j_new_index_in_group);
+
// Accessor methods.
ScopedJavaLocalRef<jobjectArray> GetAllGroupIds(
JNIEnv* env,
diff --git a/components/saved_tab_groups/android/tab_group_sync_service_android_unittest.cc b/components/saved_tab_groups/android/tab_group_sync_service_android_unittest.cc
index f7d7569..26b4f30 100644
--- a/components/saved_tab_groups/android/tab_group_sync_service_android_unittest.cc
+++ b/components/saved_tab_groups/android/tab_group_sync_service_android_unittest.cc
@@ -32,6 +32,9 @@
const char16_t kTestGroupTitle[] = u"Test Group";
const char kTestUrl[] = "https://google.com";
const char16_t kTestTabTitle[] = u"Test Tab";
+const int kTabId1 = 2;
+const int kTabId2 = 4;
+const int kPosition = 3;
class MockTabGroupSyncService : public TabGroupSyncService {
public:
@@ -58,6 +61,7 @@
GURL,
std::optional<size_t>));
MOCK_METHOD(void, RemoveTab, (const LocalTabGroupID&, const LocalTabID&));
+ MOCK_METHOD(void, MoveTab, (const LocalTabGroupID&, const LocalTabID&, int));
MOCK_METHOD(std::vector<SavedTabGroup>, GetAllGroups, ());
MOCK_METHOD(std::optional<SavedTabGroup>, GetGroup, (const base::Uuid&));
@@ -229,15 +233,13 @@
auto* env = AttachCurrentThread();
GURL url(kTestUrl);
- std::optional<int> position = 3;
+ EXPECT_CALL(tab_group_sync_service_,
+ AddTab(Eq(test_tab_group_id_), Eq(kTabId1), Eq(kTestTabTitle),
+ Eq(url), Eq(kPosition)));
EXPECT_CALL(tab_group_sync_service_,
- AddTab(Eq(test_tab_group_id_), Eq(2), Eq(kTestTabTitle), Eq(url),
- Eq(position)));
-
- EXPECT_CALL(tab_group_sync_service_,
- AddTab(Eq(test_tab_group_id_), Eq(4), Eq(kTestTabTitle), Eq(url),
- Eq(std::nullopt)));
+ AddTab(Eq(test_tab_group_id_), Eq(kTabId2), Eq(kTestTabTitle),
+ Eq(url), Eq(std::nullopt)));
Java_TabGroupSyncServiceAndroidUnitTest_testAddTab(env, j_test_);
}
@@ -245,13 +247,11 @@
auto* env = AttachCurrentThread();
GURL url(kTestUrl);
- std::optional<int> position = 3;
-
EXPECT_CALL(tab_group_sync_service_,
- UpdateTab(Eq(test_tab_group_id_), Eq(2), Eq(kTestTabTitle),
- Eq(url), Eq(position)));
+ UpdateTab(Eq(test_tab_group_id_), Eq(kTabId1), Eq(kTestTabTitle),
+ Eq(url), Eq(kPosition)));
EXPECT_CALL(tab_group_sync_service_,
- UpdateTab(Eq(test_tab_group_id_), Eq(4), Eq(kTestTabTitle),
+ UpdateTab(Eq(test_tab_group_id_), Eq(kTabId2), Eq(kTestTabTitle),
Eq(url), Eq(std::nullopt)));
Java_TabGroupSyncServiceAndroidUnitTest_testUpdateTab(env, j_test_);
}
@@ -260,10 +260,18 @@
auto* env = AttachCurrentThread();
EXPECT_CALL(tab_group_sync_service_,
- RemoveTab(Eq(test_tab_group_id_), Eq(2)));
+ RemoveTab(Eq(test_tab_group_id_), Eq(kTabId1)));
Java_TabGroupSyncServiceAndroidUnitTest_testRemoveTab(env, j_test_);
}
+TEST_F(TabGroupSyncServiceAndroidTest, MoveTab) {
+ auto* env = AttachCurrentThread();
+
+ EXPECT_CALL(tab_group_sync_service_,
+ MoveTab(Eq(test_tab_group_id_), Eq(kTabId1), Eq(kPosition)));
+ Java_TabGroupSyncServiceAndroidUnitTest_testMoveTab(env, j_test_);
+}
+
TEST_F(TabGroupSyncServiceAndroidTest, GetAllGroups) {
auto group = test::CreateTestSavedTabGroup();
std::vector<SavedTabGroup> expectedGroups = {group};
diff --git a/components/saved_tab_groups/tab_group_sync_service.h b/components/saved_tab_groups/tab_group_sync_service.h
index f915660..0f242656 100644
--- a/components/saved_tab_groups/tab_group_sync_service.h
+++ b/components/saved_tab_groups/tab_group_sync_service.h
@@ -107,6 +107,9 @@
std::optional<size_t> position) = 0;
virtual void RemoveTab(const LocalTabGroupID& group_id,
const LocalTabID& tab_id) = 0;
+ virtual void MoveTab(const LocalTabGroupID& group_id,
+ const LocalTabID& tab_id,
+ int new_group_index) = 0;
// Accessor methods.
virtual std::vector<SavedTabGroup> GetAllGroups() = 0;
diff --git a/components/saved_tab_groups/tab_group_sync_service_impl.cc b/components/saved_tab_groups/tab_group_sync_service_impl.cc
index 756a146..89282ae 100644
--- a/components/saved_tab_groups/tab_group_sync_service_impl.cc
+++ b/components/saved_tab_groups/tab_group_sync_service_impl.cc
@@ -167,6 +167,23 @@
model_->RemoveTabFromGroupLocally(group->saved_guid(), tab->saved_tab_guid());
}
+void TabGroupSyncServiceImpl::MoveTab(const LocalTabGroupID& group_id,
+ const LocalTabID& tab_id,
+ int new_group_index) {
+ auto* group = model_->Get(group_id);
+ if (!group) {
+ return;
+ }
+
+ auto* tab = group->GetTab(tab_id);
+ if (!tab) {
+ return;
+ }
+
+ model_->MoveTabInGroupTo(group->saved_guid(), tab->saved_tab_guid(),
+ new_group_index);
+}
+
std::vector<SavedTabGroup> TabGroupSyncServiceImpl::GetAllGroups() {
VLOG(2) << __func__;
return model_->saved_tab_groups();
diff --git a/components/saved_tab_groups/tab_group_sync_service_impl.h b/components/saved_tab_groups/tab_group_sync_service_impl.h
index fe3d1bd2..1eee8764 100644
--- a/components/saved_tab_groups/tab_group_sync_service_impl.h
+++ b/components/saved_tab_groups/tab_group_sync_service_impl.h
@@ -69,6 +69,10 @@
std::optional<size_t> position) override;
void RemoveTab(const LocalTabGroupID& group_id,
const LocalTabID& tab_id) override;
+ void MoveTab(const LocalTabGroupID& group_id,
+ const LocalTabID& tab_id,
+ int new_group_index) override;
+
std::vector<SavedTabGroup> GetAllGroups() override;
std::optional<SavedTabGroup> GetGroup(const base::Uuid& guid) override;
std::optional<SavedTabGroup> GetGroup(LocalTabGroupID& local_id) override;
