Skip to content

Commit

Permalink
[bookmarks] Add conditions to showing the price-tracking filter
Browse files Browse the repository at this point in the history
- Only should be shown if the price-tracking feature is enabled.
- Only should be shown if there's at least one price-tracked bookmark.
- Shouldn't be shown while inside the reading list folder.

Bug: 1474621, 1474643
Change-Id: I8c27963aeed64b748fb226a57b705d4805c3af44
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4804129
Reviewed-by: Sky Malice <skym@chromium.org>
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1188625}
  • Loading branch information
Brandon Wylie authored and Chromium LUCI CQ committed Aug 26, 2023
1 parent d3a70a5 commit 5aa8cb5
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.chromium.chrome.browser.bookmarks.BookmarkUiPrefs.Observer;
import org.chromium.chrome.browser.bookmarks.BookmarkUiState.BookmarkUiMode;
import org.chromium.chrome.browser.bookmarks.ImprovedBookmarkRowProperties.ImageVisibility;
import org.chromium.chrome.browser.commerce.ShoppingFeatures;
import org.chromium.chrome.browser.feature_engagement.TrackerFactory;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.partnerbookmarks.PartnerBookmarksReader;
Expand Down Expand Up @@ -357,6 +358,8 @@ public void onBookmarkRowSortOrderChanged(@BookmarkRowSortOrder int sortOrder) {
private boolean mIsSelectionEnabled;
// Track if we're the source of bookmark model reordering so the event can be ignored.
private boolean mIsBookmarkModelReorderingInProgress;
// Whether the shopping feature is available and there are price-tracked bookmarks.
private boolean mShoppingFilterAvailable;

BookmarkManagerMediator(Context context, BookmarkModel bookmarkModel,
BookmarkOpener bookmarkOpener, SelectableListLayout<BookmarkId> selectableListLayout,
Expand Down Expand Up @@ -431,6 +434,9 @@ public void onScrolled(@NonNull RecyclerView recyclerView, int dx, int dy) {
void onBookmarkModelLoaded() {
mDragStateDelegate.onBookmarkDelegateInitialized(this);

if (BookmarkFeatures.isAndroidImprovedBookmarksEnabled()) {
updateShoppingFilterVisible();
}
// TODO(https://crbug.com/1413463): This logic is here to keep the same execution order
// from when it was in the original adapter. It doesn't conceptually make sense to be here,
// and should happen earlier.
Expand Down Expand Up @@ -905,6 +911,9 @@ private void setBookmarks(List<BookmarkListEntry> bookmarkListEntryList) {
// Don't replace if it already exists. The text box is stateful.
if (getCurrentSearchBoxIndex() < 0) {
updateOrAdd(index, buildSearchBoxRow());
} else {
// Update the filter visibility if the search box is already built.
updateSearchBoxShoppingFilterVisibility(getSearchBoxPropertyModel());
}
index++;
}
Expand Down Expand Up @@ -1121,8 +1130,6 @@ private ListItem buildPersonalizedPromoListItem(@ViewType int promoHeaderType) {
}

private ListItem buildSearchBoxRow() {
// TODO(https://crbug.com/1444122): Check if there are any bookmarks with shopping meta.
boolean hasAnyShopping = true;
PropertyModel propertyModel =
new PropertyModel.Builder(BookmarkSearchBoxRowProperties.ALL_KEYS)
.with(BookmarkSearchBoxRowProperties.SEARCH_TEXT_CHANGE_CALLBACK,
Expand All @@ -1131,15 +1138,14 @@ private ListItem buildSearchBoxRow() {
this::onClearSearchTextRunnable)
.with(BookmarkSearchBoxRowProperties.FOCUS_CHANGE_CALLBACK,
this::onSearchBoxFocusChange)
.with(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_VISIBILITY,
hasAnyShopping)
.with(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_START_ICON_RES,
R.drawable.notifications_active)
.with(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_TEXT_RES,
R.string.price_tracking_bookmarks_filter_title)
.with(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_TOGGLE_CALLBACK,
this::onShoppingFilterToggle)
.build();
updateSearchBoxShoppingFilterVisibility(propertyModel);
return new ListItem(ViewType.SEARCH_BOX, propertyModel);
}

Expand Down Expand Up @@ -1326,6 +1332,9 @@ void setPriceTrackingEnabled(PropertyModel model, boolean enabled) {
ShoppingAccessoryCoordinator shoppingAccessoryCoordinator =
model.get(ImprovedBookmarkRowProperties.SHOPPING_ACCESSORY_COORDINATOR);
shoppingAccessoryCoordinator.setPriceTrackingEnabled(enabled);
if (BookmarkFeatures.isAndroidImprovedBookmarksEnabled()) {
updateShoppingFilterVisible();
}
};

PowerBookmarkUtils.setPriceTrackingEnabledWithSnackbars(mBookmarkModel,
Expand Down Expand Up @@ -1391,6 +1400,8 @@ private void onShoppingFilterToggle(boolean isFiltering) {
final @NonNull Set<PowerBookmarkType> powerFilter = isFiltering
? Collections.singleton(PowerBookmarkType.SHOPPING)
: Collections.emptySet();
getSearchBoxPropertyModel().set(
BookmarkSearchBoxRowProperties.SHOPPING_CHIP_SELECTED, isFiltering);
onSearchChange(getCurrentSearchText(), powerFilter);
}

Expand Down Expand Up @@ -1440,6 +1451,43 @@ private void assertIsAndroidImprovedBookmarksEnabled() {
assert BookmarkFeatures.isAndroidImprovedBookmarksEnabled();
}

// The shopping filter should only be visible if the shopping feature is enabled and
// there's at least one price-tracked bookmark available.
// TODO(crbug.com/1476104): Make this method private when price-tracking utils are mocked
// properly.
@VisibleForTesting
void updateShoppingFilterVisible() {
boolean eligible = ShoppingFeatures.isShoppingListEligible();
if (!eligible) {
updateFilterAvailability(false);
return;
}

mShoppingService.getAllPriceTrackedBookmarks(
(bookmarks) -> { updateFilterAvailability(!bookmarks.isEmpty()); });
}

private void updateFilterAvailability(boolean shoppingFilterAvailable) {
mShoppingFilterAvailable = shoppingFilterAvailable;
PropertyModel searchBoxPropertyModel = getSearchBoxPropertyModel();
// If the search box has already been built the it needs updating.
if (searchBoxPropertyModel != null) {
updateSearchBoxShoppingFilterVisibility(searchBoxPropertyModel);
}
}

private void updateSearchBoxShoppingFilterVisibility(PropertyModel searchBoxPropertyModel) {
// We purposefully hide the shopping filter in reading list even though search is
// global to avoid confusing users.
boolean filterVisible = mShoppingFilterAvailable
&& !Objects.equals(mBookmarkModel.getReadingListFolder(), getCurrentFolderId());
searchBoxPropertyModel.set(
BookmarkSearchBoxRowProperties.SHOPPING_CHIP_VISIBILITY, filterVisible);
if (!filterVisible && getCurrentSearchPowerFilter().contains(PowerBookmarkType.SHOPPING)) {
onShoppingFilterToggle(false);
}
}

// Testing methods.

/** Whether to prevent the bookmark model from fully loading for testing. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class BookmarkSearchBoxRowProperties {

public static final ReadableObjectPropertyKey<Callback<Boolean>> SHOPPING_CHIP_TOGGLE_CALLBACK =
new ReadableObjectPropertyKey<>();
public static final WritableBooleanPropertyKey SHOPPING_CHIP_SELECTED =
new WritableBooleanPropertyKey();
public static final WritableBooleanPropertyKey SHOPPING_CHIP_VISIBILITY =
new WritableBooleanPropertyKey();
public static final ReadableObjectPropertyKey<Integer> SHOPPING_CHIP_START_ICON_RES =
Expand All @@ -36,6 +38,6 @@ class BookmarkSearchBoxRowProperties {
static final PropertyKey[] ALL_KEYS = {BookmarkManagerProperties.BOOKMARK_LIST_ENTRY,
SEARCH_TEXT_CHANGE_CALLBACK, SEARCH_TEXT, FOCUS_CHANGE_CALLBACK, HAS_FOCUS,
CLEAR_SEARCH_TEXT_RUNNABLE, CLEAR_SEARCH_TEXT_BUTTON_VISIBILITY,
SHOPPING_CHIP_TOGGLE_CALLBACK, SHOPPING_CHIP_VISIBILITY, SHOPPING_CHIP_START_ICON_RES,
SHOPPING_CHIP_TEXT_RES};
SHOPPING_CHIP_TOGGLE_CALLBACK, SHOPPING_CHIP_SELECTED, SHOPPING_CHIP_VISIBILITY,
SHOPPING_CHIP_START_ICON_RES, SHOPPING_CHIP_TEXT_RES};
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,12 @@ static void bind(PropertyModel model, View view, PropertyKey key) {
Callback<Boolean> onToggle =
model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_TOGGLE_CALLBACK);
shoppingChip.setOnClickListener((View v) -> {
boolean newState = !shoppingChip.isSelected();
shoppingChip.setSelected(newState);
onToggle.onResult(newState);
onToggle.onResult(
!model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_SELECTED));
});
} else if (key == BookmarkSearchBoxRowProperties.SHOPPING_CHIP_SELECTED) {
shoppingChip.setSelected(
model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_SELECTED));
} else if (key == BookmarkSearchBoxRowProperties.SHOPPING_CHIP_START_ICON_RES) {
final @DrawableRes int res =
model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_START_ICON_RES);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,12 @@ public void testShoppingChipVisibility() {

@Test
@MediumTest
public void testShoppingChipToggle() {
public void testShoppingChipToggleCallback() {
setProperty(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_SELECTED, false);
onView(withId(R.id.shopping_filter_chip)).perform(click());
verify(mToggleCallback).onResult(true);

setProperty(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_SELECTED, true);
onView(withId(R.id.shopping_filter_chip)).perform(click());
verify(mToggleCallback).onResult(false);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import org.chromium.chrome.browser.bookmarks.ImprovedBookmarkRowProperties.ImageVisibility;
import org.chromium.chrome.browser.commerce.PriceTrackingUtils;
import org.chromium.chrome.browser.commerce.PriceTrackingUtilsJni;
import org.chromium.chrome.browser.commerce.ShoppingFeatures;
import org.chromium.chrome.browser.feature_engagement.TrackerFactory;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
Expand Down Expand Up @@ -121,6 +122,7 @@

import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.function.Consumer;

/** Unit tests for {@link BookmarkManagerMediator}. */
Expand Down Expand Up @@ -219,6 +221,8 @@ public class BookmarkManagerMediatorTest {
private final BookmarkId mReadingListFolderId =
new BookmarkId(/*id=*/9, BookmarkType.READING_LIST);
private final BookmarkId mReadingListId = new BookmarkId(/*id=*/10, BookmarkType.READING_LIST);
private final BookmarkId mPriceTrackedBookmarkId =
new BookmarkId(/*id=*/11, BookmarkType.NORMAL);

private final BookmarkItem mDesktopFolderItem = new BookmarkItem(
mDesktopFolderId, "Bookmarks bar", null, true, mRootFolderId, true, false, 0, false, 0);
Expand All @@ -240,6 +244,10 @@ public class BookmarkManagerMediatorTest {
private final BookmarkItem mReadingListItem = new BookmarkItem(mReadingListId,
JUnitTestGURLs.EXAMPLE_URL, JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL), false,
mReadingListFolderId, true, false, 0, false, 0);
private final BookmarkItem mPriceTrackedBookmarkItem = new BookmarkItem(mPriceTrackedBookmarkId,
"Price tracked bookmark", JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL), false,
mMobileFolderId, true, false, 0, false, 0);

private final ModelList mModelList = new ModelList();
private final Bitmap mBitmap = Bitmap.createBitmap(1, 1, Bitmap.Config.ARGB_8888);
private BookmarkUiPrefs mBookmarkUiPrefs =
Expand Down Expand Up @@ -278,6 +286,9 @@ public void setUp() {
doReturn(mDesktopFolderItem).when(mBookmarkModel).getBookmarkById(mDesktopFolderId);
doReturn(mMobileFolderId).when(mBookmarkModel).getMobileFolderId();
doReturn(mMobileFolderItem).when(mBookmarkModel).getBookmarkById(mMobileFolderId);
doReturn(Arrays.asList(mPriceTrackedBookmarkItem))
.when(mBookmarkModel)
.getChildIds(mMobileFolderId);
doReturn(mOtherFolderId).when(mBookmarkModel).getOtherFolderId();
doReturn(mOtherFolderItem).when(mBookmarkModel).getBookmarkById(mOtherFolderId);
doReturn(mReadingListFolderId).when(mBookmarkModel).getReadingListFolder();
Expand Down Expand Up @@ -359,12 +370,16 @@ public void setUp() {
.when(mBookmarkImageFetcher)
.fetchFaviconForBookmark(any(), any());

// Setup price tracking utils JNI.
// Setup price tracking utils.
mJniMocker.mock(PriceTrackingUtilsJni.TEST_HOOKS, mPriceTrackingUtilsJniMock);
doCallback(3, (Callback<Boolean> callback) -> callback.onResult(true))
.when(mPriceTrackingUtilsJniMock)
.setPriceTrackingStateForBookmark(
any(), anyLong(), anyBoolean(), any(), anyBoolean());
doCallback(0, (Callback<List<BookmarkId>> callback) -> {
callback.onResult(Arrays.asList(mPriceTrackedBookmarkId));
}).when(mShoppingService).getAllPriceTrackedBookmarks(any());
ShoppingFeatures.setShoppingListEligibleForTesting(true);

mDragReorderableRecyclerViewAdapter =
spy(new DragReorderableRecyclerViewAdapter(mActivity, mModelList));
Expand Down Expand Up @@ -1433,4 +1448,73 @@ public void testBackNavigationDoesNotRestoreSearch() {
assertEquals(BookmarkUiMode.FOLDER, mMediator.getCurrentUiMode());
verifyCurrentBookmarkIds(null, mFolderId2, mFolderId3);
}

@Test
@EnableFeatures(ChromeFeatureList.ANDROID_IMPROVED_BOOKMARKS)
public void testSearchBox_priceTrackingFilterVisible() {
finishLoading();

mMediator.openFolder(mFolderId3);

assertEquals(1, mModelList.size());
assertEquals(ViewType.SEARCH_BOX, mModelList.get(0).type);

PropertyModel model = mModelList.get(0).model;
assertTrue(model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_VISIBILITY));
}

@Test
@EnableFeatures(ChromeFeatureList.ANDROID_IMPROVED_BOOKMARKS)
public void testSearchBox_priceTrackingFilterGoneInReadingList() {
finishLoading();

mMediator.openFolder(mReadingListFolderId);

assertEquals(4, mModelList.size());
assertEquals(ViewType.SEARCH_BOX, mModelList.get(0).type);

PropertyModel model = mModelList.get(0).model;
assertFalse(model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_VISIBILITY));
}

@Test
@EnableFeatures(ChromeFeatureList.ANDROID_IMPROVED_BOOKMARKS)
public void testSearchBox_priceTrackingFilterGoneWithoutBookmarks() {
finishLoading();

mMediator.openFolder(mFolderId3);

assertEquals(1, mModelList.size());
assertEquals(ViewType.SEARCH_BOX, mModelList.get(0).type);

PropertyModel model = mModelList.get(0).model;
assertTrue(model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_VISIBILITY));

// The price-tracked filter shouldn't be visible without any price-tracked bookmarks.
doCallback(0, (Callback<List<BookmarkId>> callback) -> {
callback.onResult(Arrays.asList());
}).when(mShoppingService).getAllPriceTrackedBookmarks(any());

mMediator.updateShoppingFilterVisible();
assertEquals(1, mModelList.size());
assertEquals(ViewType.SEARCH_BOX, mModelList.get(0).type);

model = mModelList.get(0).model;
assertFalse(model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_VISIBILITY));
}

@Test
@EnableFeatures(ChromeFeatureList.ANDROID_IMPROVED_BOOKMARKS)
public void testSearchBox_priceTrackingFilterGoneWithEligibility() {
ShoppingFeatures.setShoppingListEligibleForTesting(false);
finishLoading();

mMediator.openFolder(mFolderId3);

assertEquals(1, mModelList.size());
assertEquals(ViewType.SEARCH_BOX, mModelList.get(0).type);

PropertyModel model = mModelList.get(0).model;
assertFalse(model.get(BookmarkSearchBoxRowProperties.SHOPPING_CHIP_VISIBILITY));
}
}

0 comments on commit 5aa8cb5

Please sign in to comment.