Skip to content

Commit

Permalink
Extract Selection manager from BaseCarouselSuggestion component.
Browse files Browse the repository at this point in the history 
We have at least three components with partially- or fully overlapping
functionality:
- BaseCarouselSuggestionSelectionManager
- ActionChipsAdapter
- OmniboxSuggestionsDropdownAdapter.

The common denominator of all the three components appears to be what
is supported by the BaseCarouselSuggestionSelectionManager.

No functional changes were made to the relocated class.

Bug: 1490333
Change-Id: I75180d057053c497edba5277717519be6f1f63ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4980803
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Fred Mello <fredmello@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216411}
  • Loading branch information
@tomasz-wiszkowski
tomasz-wiszkowski committed Oct 27, 2023
1 parent 279ebb3 commit 4f06b56
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 63 deletions.
4 changes: 2 additions & 2 deletions chrome/browser/ui/android/omnibox/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ android_library("java") {
"java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdownEmbedder.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdownScrollListener.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/PreWarmingRecycledViewPool.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/RecyclerViewSelectionController.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/SuggestionCommonProperties.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/SuggestionHorizontalDivider.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/SuggestionHost.java",
Expand Down Expand Up @@ -114,7 +115,6 @@ android_library("java") {
"java/src/org/chromium/chrome/browser/omnibox/suggestions/basic/SuggestionViewViewBinder.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionItemViewBuilder.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionProcessor.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionSelectionManager.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionView.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionViewBinder.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionViewProperties.java",
Expand Down Expand Up @@ -443,6 +443,7 @@ robolectric_library("junit") {
"java/src/org/chromium/chrome/browser/omnibox/suggestions/DropdownItemViewInfoListManagerUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdownUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/PreWarmingRecycledViewPoolTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/RecyclerViewSelectionControllerUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/SuggestionHorizontalDividerTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/action/HistoryClustersActionUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/action/OmniboxActionDelegateImplUnitTest.java",
Expand All @@ -464,7 +465,6 @@ robolectric_library("junit") {
"java/src/org/chromium/chrome/browser/omnibox/suggestions/base/SpacingRecyclerViewItemDecorationUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/basic/BasicSuggestionProcessorUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionProcessorUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionSelectionManagerUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionViewBinderUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionViewUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/clipboard/ClipboardSuggestionProcessorUnitTest.java",
Expand Down
20 changes: 11 additions & 9 deletions ...seCarouselSuggestionSelectionManager.java → ...ions/RecyclerViewSelectionController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,22 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package org.chromium.chrome.browser.omnibox.suggestions.carousel;
package org.chromium.chrome.browser.omnibox.suggestions;

import android.view.View;

import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import androidx.recyclerview.widget.RecyclerView;
import androidx.recyclerview.widget.RecyclerView.LayoutManager;

/** Selection manager for BaseCarouselSuggestion. */
public class BaseCarouselSuggestionSelectionManager
/** Selection manager for RecyclerViews. */
public class RecyclerViewSelectionController
implements RecyclerView.OnChildAttachStateChangeListener {
private int mSelectedItem = RecyclerView.NO_POSITION;
private LayoutManager mLayoutManager;

BaseCarouselSuggestionSelectionManager(LayoutManager layoutManager) {
public RecyclerViewSelectionController(LayoutManager layoutManager) {
mLayoutManager = layoutManager;
}

Expand All @@ -37,12 +38,12 @@ public void onChildViewDetachedFromWindow(View view) {
}

/** Reset the active selection. */
void resetSelection() {
public void resetSelection() {
setSelectedItem(RecyclerView.NO_POSITION, false);
}

/** Move selection to the next element on the list. */
void selectNextItem() {
public void selectNextItem() {
if (mLayoutManager == null) return;

int newSelectedItem;
Expand All @@ -58,7 +59,7 @@ void selectNextItem() {
}

/** Move selection to the previous element on the list. */
void selectPreviousItem() {
public void selectPreviousItem() {
if (mLayoutManager == null) return;

int newSelectedItem;
Expand All @@ -75,7 +76,7 @@ void selectPreviousItem() {

/** Retrieve currently selected element. */
@Nullable
View getSelectedView() {
public View getSelectedView() {
return mLayoutManager.findViewByPosition(mSelectedItem);
}

Expand All @@ -86,7 +87,7 @@ View getSelectedView() {
* @param force Whether to apply the selection even if the current selected item has not
* changed.
*/
void setSelectedItem(int index, boolean force) {
public void setSelectedItem(int index, boolean force) {
if (mLayoutManager == null) return;
if (index != RecyclerView.NO_POSITION
&& (index < 0 || index >= mLayoutManager.getItemCount())) {
Expand All @@ -109,6 +110,7 @@ void setSelectedItem(int index, boolean force) {
}

/** Returns the selected item index. */
@VisibleForTesting
int getSelectedItemForTest() {
return mSelectedItem;
}
Expand Down
31 changes: 14 additions & 17 deletions ...elSuggestionSelectionManagerUnitTest.java → ...yclerViewSelectionControllerUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package org.chromium.chrome.browser.omnibox.suggestions.carousel;
package org.chromium.chrome.browser.omnibox.suggestions;

import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.Mockito.reset;
Expand All @@ -18,40 +18,37 @@

import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.robolectric.annotation.Config;
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;

import org.chromium.base.test.BaseRobolectricTestRunner;

/** Tests for {@link BaseCarouselSuggestionRecyclerViewAdapter}. */
/** Tests for {@link RecyclerViewSelectionController}. */
@RunWith(BaseRobolectricTestRunner.class)
@Config(manifest = Config.NONE)
public class BaseCarouselSuggestionSelectionManagerUnitTest {
BaseCarouselSuggestionSelectionManager mSelectionManager;
public class RecyclerViewSelectionControllerUnitTest {
public @Rule MockitoRule mMockitoRule = MockitoJUnit.rule();

@Mock RecyclerView mRecyclerView;

@Mock LayoutManager mLayoutManager;

@Mock View mChildView1;

@Mock View mChildView2;

@Mock View mChildView3;
private @Mock RecyclerView mRecyclerView;
private @Mock LayoutManager mLayoutManager;
private @Mock View mChildView1;
private @Mock View mChildView2;
private @Mock View mChildView3;
RecyclerViewSelectionController mSelectionManager;

@Before
public void setUp() {
MockitoAnnotations.initMocks(this);

when(mLayoutManager.getItemCount()).thenReturn(3);
when(mLayoutManager.findViewByPosition(0)).thenReturn(mChildView1);
when(mLayoutManager.findViewByPosition(1)).thenReturn(mChildView2);
when(mLayoutManager.findViewByPosition(2)).thenReturn(mChildView3);

mSelectionManager = new BaseCarouselSuggestionSelectionManager(mLayoutManager);
mSelectionManager = new RecyclerViewSelectionController(mLayoutManager);
}

@Test
Expand Down
27 changes: 14 additions & 13 deletions .../org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@
import org.chromium.build.annotations.MockedInTests;
import org.chromium.chrome.browser.omnibox.R;
import org.chromium.chrome.browser.omnibox.styles.OmniboxResourceProvider;
import org.chromium.chrome.browser.omnibox.suggestions.RecyclerViewSelectionController;
import org.chromium.chrome.browser.util.KeyNavigationUtil;
import org.chromium.ui.modelutil.SimpleRecyclerViewAdapter;

/** View for Carousel Suggestions. */
@MockedInTests
public class BaseCarouselSuggestionView extends RecyclerView {
private BaseCarouselSuggestionSelectionManager mSelectionManager;
private RecyclerViewSelectionController mSelectionController;

/**
* Constructs a new carousel suggestion view.
Expand All @@ -42,8 +43,8 @@ public BaseCarouselSuggestionView(Context context, SimpleRecyclerViewAdapter ada
getResources().getDimensionPixelSize(R.dimen.omnibox_carousel_suggestion_padding);
setPaddingRelative(0, topPadding, getPaddingEnd(), bottomPadding);

mSelectionManager = new BaseCarouselSuggestionSelectionManager(getLayoutManager());
addOnChildAttachStateChangeListener(mSelectionManager);
mSelectionController = new RecyclerViewSelectionController(getLayoutManager());
addOnChildAttachStateChangeListener(mSelectionController);

setAdapter(adapter);
}
Expand All @@ -53,14 +54,14 @@ public boolean onKeyDown(int keyCode, KeyEvent event) {
boolean isRtl = getLayoutDirection() == LAYOUT_DIRECTION_RTL;
if ((!isRtl && KeyNavigationUtil.isGoRight(event))
|| (isRtl && KeyNavigationUtil.isGoLeft(event))) {
mSelectionManager.selectNextItem();
mSelectionController.selectNextItem();
return true;
} else if ((isRtl && KeyNavigationUtil.isGoRight(event))
|| (!isRtl && KeyNavigationUtil.isGoLeft(event))) {
mSelectionManager.selectPreviousItem();
mSelectionController.selectPreviousItem();
return true;
} else if (KeyNavigationUtil.isEnter(event)) {
var tile = mSelectionManager.getSelectedView();
var tile = mSelectionController.getSelectedView();
if (tile != null) return tile.performClick();
}
return superOnKeyDown(keyCode, event);
Expand All @@ -79,17 +80,17 @@ public boolean superOnKeyDown(int keyCode, KeyEvent event) {
@Override
public void setSelected(boolean isSelected) {
if (isSelected) {
mSelectionManager.setSelectedItem(0, true);
mSelectionController.setSelectedItem(0, true);
} else {
mSelectionManager.setSelectedItem(RecyclerView.NO_POSITION, false);
mSelectionController.setSelectedItem(RecyclerView.NO_POSITION, false);
}
}

@VisibleForTesting(otherwise = VisibleForTesting.NONE)
/* package */ void setSelectionManagerForTesting(
BaseCarouselSuggestionSelectionManager manager) {
removeOnChildAttachStateChangeListener(mSelectionManager);
mSelectionManager = manager;
addOnChildAttachStateChangeListener(mSelectionManager);
/* package */ void setSelectionControllerForTesting(
RecyclerViewSelectionController controller) {
removeOnChildAttachStateChangeListener(mSelectionController);
mSelectionController = controller;
addOnChildAttachStateChangeListener(mSelectionController);
}
}
45 changes: 23 additions & 22 deletions ...omium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionViewUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,23 @@

import org.chromium.base.ContextUtils;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.chrome.browser.omnibox.suggestions.RecyclerViewSelectionController;
import org.chromium.ui.modelutil.SimpleRecyclerViewAdapter;

/** Tests for {@link BaseCarouselSuggestionView}. */
@RunWith(BaseRobolectricTestRunner.class)
public class BaseCarouselSuggestionViewUnitTest {
public @Rule MockitoRule mMockitoRule = MockitoJUnit.rule();
private @Mock SimpleRecyclerViewAdapter mAdapter;
private @Mock BaseCarouselSuggestionSelectionManager mManager;
private @Mock RecyclerViewSelectionController mController;
private @Mock View mChild;
private @Spy BaseCarouselSuggestionView mView =
new BaseCarouselSuggestionView(ContextUtils.getApplicationContext(), mAdapter);

@Before
public void setUp() {
mView.setSelectionManagerForTesting(mManager);
clearInvocations(mView, mAdapter, mManager, mChild);
mView.setSelectionControllerForTesting(mController);
clearInvocations(mView, mAdapter, mController, mChild);
}

@Test
Expand All @@ -54,9 +55,9 @@ public void onKeyDown_selectNextItem_ltr() {
assertTrue(event.dispatch(mView));

verify(mView).onKeyDown(event.getKeyCode(), event);
verify(mManager).selectNextItem();
verify(mController).selectNextItem();

verifyNoMoreInteractions(mManager);
verifyNoMoreInteractions(mController);
}

@Test
Expand All @@ -67,9 +68,9 @@ public void onKeyDown_selectNextItem_rtl() {
assertTrue(event.dispatch(mView));

verify(mView).onKeyDown(event.getKeyCode(), event);
verify(mManager).selectNextItem();
verify(mController).selectNextItem();

verifyNoMoreInteractions(mManager);
verifyNoMoreInteractions(mController);
}

@Test
Expand All @@ -80,9 +81,9 @@ public void onKeyDown_selectPrevItem_ltr() {
assertTrue(event.dispatch(mView));

verify(mView).onKeyDown(event.getKeyCode(), event);
verify(mManager).selectPreviousItem();
verify(mController).selectPreviousItem();

verifyNoMoreInteractions(mManager);
verifyNoMoreInteractions(mController);
}

@Test
Expand All @@ -93,38 +94,38 @@ public void onKeyDown_selectPrevItem_rtl() {
assertTrue(event.dispatch(mView));

verify(mView).onKeyDown(event.getKeyCode(), event);
verify(mManager).selectPreviousItem();
verify(mController).selectPreviousItem();

verifyNoMoreInteractions(mManager);
verifyNoMoreInteractions(mController);
}

@Test
public void onKeyDown_enterKeyPassedThroughWhenNoItemSelected() {
doReturn(null).when(mManager).getSelectedView();
doReturn(null).when(mController).getSelectedView();

var event = new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_ENTER);
assertFalse(event.dispatch(mView));

verify(mView).onKeyDown(event.getKeyCode(), event);
verify(mManager).getSelectedView();
verify(mController).getSelectedView();
verify(mView).superOnKeyDown(event.getKeyCode(), event);

verifyNoMoreInteractions(mChild, mManager);
verifyNoMoreInteractions(mChild, mController);
}

@Test
public void onKeyDown_enterKeyAcceptsSelectedItem() {
doReturn(mChild).when(mManager).getSelectedView();
doReturn(mChild).when(mController).getSelectedView();
doReturn(true).when(mChild).performClick();

var event = new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_ENTER);
assertTrue(event.dispatch(mView));

verify(mView).onKeyDown(event.getKeyCode(), event);
verify(mManager).getSelectedView();
verify(mController).getSelectedView();
verify(mChild).performClick();

verifyNoMoreInteractions(mChild, mManager);
verifyNoMoreInteractions(mChild, mController);
}

@Test
Expand All @@ -135,20 +136,20 @@ public void onKeyDown_unhandledKeysArePassedToSuper() {
verify(mView).onKeyDown(KeyEvent.KEYCODE_T, event);
verify(mView).superOnKeyDown(KeyEvent.KEYCODE_T, event);

verifyNoMoreInteractions(mManager);
verifyNoMoreInteractions(mController);
}

@Test
public void setSelected_resetsCarouselSelectionWhenSelected() {
mView.setSelected(true);
verify(mManager, times(1)).setSelectedItem(0, /* force= */ true);
verifyNoMoreInteractions(mManager);
verify(mController, times(1)).setSelectedItem(0, /* force= */ true);
verifyNoMoreInteractions(mController);
}

@Test
public void setSelected_resetsCarouselSelectionWhenDeselected() {
mView.setSelected(false);
verify(mManager, times(1)).setSelectedItem(RecyclerView.NO_POSITION, /* force= */ false);
verifyNoMoreInteractions(mManager);
verify(mController, times(1)).setSelectedItem(RecyclerView.NO_POSITION, /* force= */ false);
verifyNoMoreInteractions(mController);
}
}

0 comments on commit 4f06b56

Please sign in to comment.