Skip to content

GitLab

T
tangled
Commit 85ea958a authored by Mei Liang's avatar Mei Liang Committed by Commit Bot
Browse files
Options

Refactor TabListMediator onMovedTab 

This CL refactors some tab group specific logic in
TabLiseMediator#onMovedTab() to TabGroupModelFilter. And TabListMediator
should listen to additional the TabGroupModelFilter.Observer to update
the UI model.

Change-Id: Ieb7bf00b4811956b11dbb0a44fcc3f6415cd4340
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1628272
Commit-Queue: Mei Liang <meiliang@chromium.org>
Reviewed-by: default avatarYusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: default avatarWei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663981}
parent 1f96bac0
Hide whitespace changes
Inline Side-by-side
Showing with 167 additions and 67 deletions
chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_groups/TabGroupUtils.java
View file @ 85ea958a
......@@ -12,7 +12,6 @@ import org.chromium.chrome.browser.ChromeFeatureList;
import org.chromium.chrome.browser.feature_engagement.TrackerFactory;
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tabmodel.TabModel;
import org.chromium.chrome.browser.tabmodel.TabModelSelector;
import org.chromium.chrome.browser.tabmodel.TabModelSelectorTabObserver;
import org.chromium.chrome.browser.tasks.tabgroup.TabGroupModelFilter;
......@@ -132,20 +131,4 @@ public class TabGroupUtils {
return selector.getCurrentModel().indexOf(tabs.get(tabs.size() - 1));
}
/**
* This method judges whether the current move from {@code curIndex} to {@code newIndex} in
* {@code tabModel} is a move within one TabGroup or not.
* @param tabModel The TabModel that owns the moving tab.
* @param curIndex The current index of the moving tab.
* @param newIndex The new index of the moving tab.
* @return Whether is move happens within one TabGroup or not.
*/
public static boolean isMoveInSameGroup(TabModel tabModel, int curIndex, int newIndex) {
int rootId = tabModel.getTabAt(curIndex).getRootId();
for (int i = Math.min(newIndex, curIndex); i <= Math.max(newIndex, curIndex); i++) {
if (tabModel.getTabAt(i).getRootId() != rootId) return false;
}
return true;
}
}
chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java
View file @ 85ea958a
......@@ -233,6 +233,8 @@ class TabListMediator {
private final TabModelObserver mTabModelObserver;
private TabGroupModelFilter.Observer mTabGroupObserver;
/**
* Interface for implementing a {@link Runnable} that takes a tabId for a generic action.
*/
......@@ -324,12 +326,69 @@ class TabListMediator {
@Override
public void didMoveTab(Tab tab, int newIndex, int curIndex) {
if (mTabModelSelector.getTabModelFilterProvider().getCurrentTabModelFilter()
instanceof TabGroupModelFilter) {
return;
}
onTabMoved(tab, newIndex, curIndex);
}
};
mTabModelSelector.getTabModelFilterProvider().addTabModelFilterObserver(mTabModelObserver);
if (mTabModelSelector.getTabModelFilterProvider().getCurrentTabModelFilter()
instanceof TabGroupModelFilter) {
mTabGroupObserver = new TabGroupModelFilter.Observer() {
@Override
public void didMoveWithinGroup(
Tab movedTab, int tabModelOldIndex, int tabModelNewIndex) {
int curPosition = mModel.indexFromId(movedTab.getId());
TabModel tabModel = mTabModelSelector.getCurrentModel();
if (!isValidMovePosition(curPosition)) return;
Tab destinationTab = tabModel.getTabAt(tabModelNewIndex > tabModelOldIndex
? tabModelNewIndex - 1
: tabModelNewIndex + 1);
int newPosition = mModel.indexFromId(destinationTab.getId());
if (!isValidMovePosition(newPosition)) return;
mModel.move(curPosition, newPosition);
}
@Override
public void didMoveTabGroup(
Tab movedTab, int tabModelOldIndex, int tabModelNewIndex) {
List<Tab> relatedTabs = getRelatedTabsForId(movedTab.getId());
Tab currentGroupSelectedTab =
TabGroupUtils.getSelectedTabInGroupForTab(mTabModelSelector, movedTab);
TabModel tabModel = mTabModelSelector.getCurrentModel();
int curPosition = mModel.indexFromId(currentGroupSelectedTab.getId());
if (!isValidMovePosition(curPosition)) return;
// Find the tab which was in the destination index before this move. Use that
// tab to figure out the new position.
int destinationTabIndex = tabModelNewIndex > tabModelOldIndex
? tabModelNewIndex - relatedTabs.size()
: tabModelNewIndex + 1;
Tab destinationTab = tabModel.getTabAt(destinationTabIndex);
Tab destinationGroupSelectedTab = TabGroupUtils.getSelectedTabInGroupForTab(
mTabModelSelector, destinationTab);
int newPosition = mModel.indexFromId(destinationGroupSelectedTab.getId());
if (!isValidMovePosition(newPosition)) return;
mModel.move(curPosition, newPosition);
}
};
((TabGroupModelFilter) mTabModelSelector.getTabModelFilterProvider().getTabModelFilter(
false))
.addTabGroupObserver(mTabGroupObserver);
((TabGroupModelFilter) mTabModelSelector.getTabModelFilterProvider().getTabModelFilter(
true))
.addTabGroupObserver(mTabGroupObserver);
}
// TODO(meiliang): follow up with unit tests to test the close signal is sent correctly with
// the recommendedNextTab.
mTabClosedListener = new TabActionListener() {
......@@ -424,54 +483,12 @@ class TabListMediator {
}
private void onTabMoved(Tab tab, int newIndex, int curIndex) {
List<Tab> relatedTabs = getRelatedTabsForId(tab.getId());
TabModel tabModel = mTabModelSelector.getCurrentModel();
// Handle move without groups enabled.
if (mTabModelSelector.getTabModelFilterProvider().getCurrentTabModelFilter()
instanceof EmptyTabModelFilter) {
if (!isValidMovePosition(curIndex) || !isValidMovePosition(newIndex)) return;
mModel.move(curIndex, newIndex);
return;
}
// Handle move with groups enabled.
// When move within one group.
if (TabGroupUtils.isMoveInSameGroup(tabModel, curIndex, newIndex)) {
int curPosition = mModel.indexFromId(tab.getId());
if (!isValidMovePosition(curPosition)) return;
Tab destinationTab =
tabModel.getTabAt(newIndex > curIndex ? newIndex - 1 : newIndex + 1);
int newPosition = mModel.indexFromId(destinationTab.getId());
if (!isValidMovePosition(newPosition)) return;
mModel.move(curPosition, newPosition);
return;
}
// When move between groups.
// Only proceed when all members of the group are moved to target index in TabModel.
// Regardless of the size of tab group, one tab(group) only proceeds once here per
// move.
int lastTabInGroupIndex = newIndex - relatedTabs.size() + 1;
if (!relatedTabs.contains(tabModel.getTabAt(lastTabInGroupIndex))) return;
// Locate current position of the moving tab in TabListModel.
Tab currentGroupSelectedTab =
TabGroupUtils.getSelectedTabInGroupForTab(mTabModelSelector, tab);
int curPosition = mModel.indexFromId(currentGroupSelectedTab.getId());
if (!isValidMovePosition(curPosition)) return;
// Find the tab which was in the destination index before this move. Use that tab to
// figure out the new position.
int destinationTabIndex =
newIndex > curIndex ? newIndex - relatedTabs.size() : newIndex + 1;
Tab destinationTab = tabModel.getTabAt(destinationTabIndex);
Tab destinationGroupSelectedTab =
TabGroupUtils.getSelectedTabInGroupForTab(mTabModelSelector, destinationTab);
int newPosition = mModel.indexFromId(destinationGroupSelectedTab.getId());
if (!isValidMovePosition(newPosition)) return;
mModel.move(curPosition, newPosition);
}
private boolean isValidMovePosition(int position) {
......@@ -613,6 +630,14 @@ class TabListMediator {
mTabModelSelector.getTabModelFilterProvider().removeTabModelFilterObserver(
mTabModelObserver);
}
if (mTabGroupObserver != null) {
((TabGroupModelFilter) mTabModelSelector.getTabModelFilterProvider().getTabModelFilter(
false))
.removeTabGroupObserver(mTabGroupObserver);
((TabGroupModelFilter) mTabModelSelector.getTabModelFilterProvider().getTabModelFilter(
true))
.removeTabGroupObserver(mTabGroupObserver);
}
if (mComponentCallbacks != null) {
ContextUtils.getApplicationContext().unregisterComponentCallbacks(mComponentCallbacks);
}
......
chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java
View file @ 85ea958a
......@@ -100,6 +100,8 @@ public class TabListMediatorUnitTest {
ArgumentCaptor<TabObserver> mTabObserverCaptor;
@Captor
ArgumentCaptor<Callback<Drawable>> mCallbackCaptor;
@Captor
ArgumentCaptor<TabGroupModelFilter.Observer> mTabGroupModelFilterObserverCaptor;
private Tab mTab1;
private Tab mTab2;
......@@ -432,7 +434,7 @@ public class TabListMediatorUnitTest {
@Test
public void tabMovementWithGroup_Forward() {
initAndAssertAllProperties();
setUpForTabGroupOperation();
// Assume that moveTab in TabModel is finished.
doReturn(mTab1).when(mTabModel).getTabAt(POSITION2);
......@@ -443,7 +445,7 @@ public class TabListMediatorUnitTest {
assertThat(mModel.get(1).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
assertThat(mModel.get(1).get(TabProperties.TITLE), equalTo(TAB2_TITLE));
mTabModelObserverCaptor.getValue().didMoveTab(mTab2, POSITION1, POSITION2);
mTabGroupModelFilterObserverCaptor.getValue().didMoveTabGroup(mTab2, POSITION2, POSITION1);
assertThat(mModel.size(), equalTo(2));
assertThat(mModel.get(0).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
......@@ -452,7 +454,7 @@ public class TabListMediatorUnitTest {
@Test
public void tabMovementWithGroup_Backward() {
initAndAssertAllProperties();
setUpForTabGroupOperation();
// Assume that moveTab in TabModel is finished.
doReturn(mTab1).when(mTabModel).getTabAt(POSITION2);
......@@ -463,7 +465,7 @@ public class TabListMediatorUnitTest {
assertThat(mModel.get(1).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
assertThat(mModel.get(1).get(TabProperties.TITLE), equalTo(TAB2_TITLE));
mTabModelObserverCaptor.getValue().didMoveTab(mTab1, POSITION2, POSITION1);
mTabGroupModelFilterObserverCaptor.getValue().didMoveTabGroup(mTab1, POSITION1, POSITION2);
assertThat(mModel.size(), equalTo(2));
assertThat(mModel.get(0).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
......@@ -472,7 +474,7 @@ public class TabListMediatorUnitTest {
@Test
public void tabMovementWithinGroup_Forward() {
initAndAssertAllProperties();
setUpForTabGroupOperation();
// Assume that moveTab in TabModel is finished.
doReturn(mTab1).when(mTabModel).getTabAt(POSITION2);
......@@ -485,7 +487,8 @@ public class TabListMediatorUnitTest {
assertThat(mModel.get(1).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
assertThat(mModel.get(1).get(TabProperties.TITLE), equalTo(TAB2_TITLE));
mTabModelObserverCaptor.getValue().didMoveTab(mTab2, POSITION1, POSITION2);
mTabGroupModelFilterObserverCaptor.getValue().didMoveWithinGroup(
mTab2, POSITION2, POSITION1);
assertThat(mModel.size(), equalTo(2));
assertThat(mModel.get(0).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
......@@ -494,7 +497,7 @@ public class TabListMediatorUnitTest {
@Test
public void tabMovementWithinGroup_Backward() {
initAndAssertAllProperties();
setUpForTabGroupOperation();
// Assume that moveTab in TabModel is finished.
doReturn(mTab1).when(mTabModel).getTabAt(POSITION2);
......@@ -507,7 +510,8 @@ public class TabListMediatorUnitTest {
assertThat(mModel.get(1).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
assertThat(mModel.get(1).get(TabProperties.TITLE), equalTo(TAB2_TITLE));
mTabModelObserverCaptor.getValue().didMoveTab(mTab1, POSITION2, POSITION1);
mTabGroupModelFilterObserverCaptor.getValue().didMoveWithinGroup(
mTab1, POSITION1, POSITION2);
assertThat(mModel.size(), equalTo(2));
assertThat(mModel.get(0).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
......@@ -570,4 +574,19 @@ public class TabListMediatorUnitTest {
doReturn(position).when(viewHolder).getAdapterPosition();
return viewHolder;
}
private void setUpForTabGroupOperation() {
doReturn(mTabGroupModelFilter).when(mTabModelFilterProvider).getCurrentTabModelFilter();
doReturn(mTabGroupModelFilter).when(mTabModelFilterProvider).getTabModelFilter(true);
doReturn(mTabGroupModelFilter).when(mTabModelFilterProvider).getTabModelFilter(false);
doNothing()
.when(mTabGroupModelFilter)
.addTabGroupObserver(mTabGroupModelFilterObserverCaptor.capture());
mMediator = new TabListMediator(mModel, mTabModelSelector,
mTabContentManager::getTabThumbnailWithCallback, null, mTabListFaviconProvider,
false, null, null, getClass().getSimpleName());
initAndAssertAllProperties();
}
}
chrome/android/java/src/org/chromium/chrome/browser/tasks/tabgroup/TabGroupModelFilter.java
View file @ 85ea958a
......@@ -9,6 +9,7 @@ import android.content.SharedPreferences;
import android.support.annotation.NonNull;
import org.chromium.base.ContextUtils;
import org.chromium.base.ObserverList;
import org.chromium.base.ThreadUtils;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.metrics.RecordUserAction;
......@@ -42,6 +43,29 @@ public class TabGroupModelFilter extends TabModelFilter {
private static final String SESSIONS_COUNT_FOR_GROUP = "SessionsCountForGroup-";
private static SharedPreferences sPref;
/**
* An interface to be notified about changes to a {@link TabGroupModelFilter}.
*/
public interface Observer {
/**
* This method is called after a group is moved.
*
* @param movedTab The tab which has been moved. This is the last tab within the group.
* @param tabModelOldIndex The old index of the {@code movedTab} in the {@link TabModel}.
* @param tabModelNewIndex The new index of the {@code movedTab} in the {@link TabModel}.
*/
void didMoveTabGroup(Tab movedTab, int tabModelOldIndex, int tabModelNewIndex);
/**
* This method is called after a tab within a group is moved.
*
* @param movedTab The tab which has been moved.
* @param tabModelOldIndex The old index of the {@code movedTab} in the {@link TabModel}.
* @param tabModelNewIndex The new index of the {@code movedTab} in the {@link TabModel}.
*/
void didMoveWithinGroup(Tab movedTab, int tabModelOldIndex, int tabModelNewIndex);
}
/**
* This class is a representation of a group of tabs. It knows the last selected tab within the
* group.
......@@ -108,6 +132,7 @@ public class TabGroupModelFilter extends TabModelFilter {
return ids.get(position - 1);
}
}
private ObserverList<Observer> mGroupFilterObserver = new ObserverList<>();
private Map<Integer, Integer> mGroupIdToGroupIndexMap = new HashMap<>();
private Map<Integer, TabGroup> mGroupIdToGroupMap = new HashMap<>();
private int mCurrentGroupIndex = TabList.INVALID_TAB_INDEX;
......@@ -131,6 +156,22 @@ public class TabGroupModelFilter extends TabModelFilter {
});
}
/**
* This method adds a {@link Observer} to be notified on {@link TabGroupModelFilter} changes.
* @param observer The {@link Observer} to add.
*/
public void addTabGroupObserver(Observer observer) {
mGroupFilterObserver.addObserver(observer);
}
/**
* This method removes a {@link Observer}.
* @param observer The {@link Observer} to remove.
*/
public void removeTabGroupObserver(Observer observer) {
mGroupFilterObserver.removeObserver(observer);
}
/**
* @return Number of {@link TabGroup}s that has at least two tabs.
*/
......@@ -340,6 +381,38 @@ public class TabGroupModelFilter extends TabModelFilter {
return mAbsentSelectedTab == null;
}
@Override
public void didMoveTab(Tab tab, int newIndex, int curIndex) {
super.didMoveTab(tab, newIndex, curIndex);
if (isMoveWithinGroup(tab, curIndex, newIndex)) {
for (Observer observer : mGroupFilterObserver) {
observer.didMoveWithinGroup(tab, curIndex, newIndex);
}
} else {
if (!hasFinishedMovingGroup(tab, newIndex)) return;
for (Observer observer : mGroupFilterObserver) {
observer.didMoveTabGroup(tab, curIndex, newIndex);
}
}
}
private boolean isMoveWithinGroup(
Tab movedTab, int oldIndexInTabModel, int newIndexInTabModel) {
int startIndex = Math.min(oldIndexInTabModel, newIndexInTabModel);
int endIndex = Math.max(oldIndexInTabModel, newIndexInTabModel);
for (int i = startIndex; i <= endIndex; i++) {
if (getTabModel().getTabAt(i).getRootId() != movedTab.getRootId()) return false;
}
return true;
}
private boolean hasFinishedMovingGroup(Tab movedTab, int newIndexInTabModel) {
TabGroup tabGroup = mGroupIdToGroupMap.get(movedTab.getRootId());
int offsetIndex = Math.abs(newIndexInTabModel - tabGroup.size() + 1);
return tabGroup.contains(getTabModel().getTabAt(offsetIndex).getId());
}
// TabList implementation.
@Override
public boolean isIncognito() {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment