Skip to content

GitLab

T
tangled
Commit a9efb918 authored by Cathy Li's avatar Cathy Li Committed by Commit Bot
Browse files
Options

[Explore sites]: Clean up code and remove unneeded header to ESP. 

Also fixes a number of issues:
1. Re-reinforce the max 2 rows of sites on categories
2. Update tests to account for the 4-sites-per-row rule

Change-Id: I27b232466314ce0398c6f5ba31ebdb340e6dbdfc
Reviewed-on: https://chromium-review.googlesource.com/c/1377141Reviewed-by: default avatarJustin DeWitt <dewittj@chromium.org>
Reviewed-by: default avatarTheresa <twellington@chromium.org>
Commit-Queue: Cathy Li <chili@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619598}
parent 2d3f8519
Hide whitespace changes
Inline Side-by-side
Showing with 55 additions and 116 deletions
chrome/android/java/res/layout/explore_sites_title_card.xml deleted 100644 → 0
View file @ 2d3f8519
<?xml version="1.0" encoding="utf-8"?>
<!-- Copyright 2018 The Chromium Authors. All rights reserved.
Use of this source code is governed by a BSD-style license that can be
found in the LICENSE file. -->
<TextView
xmlns:android="http://schemas.android.com/apk/res/android"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:gravity="center"
android:textAppearance="@style/BlackHeadline"
android:text="@string/explore_sites_title"
android:paddingBottom="@dimen/explore_sites_category_padding"
android:paddingTop="@dimen/explore_sites_page_padding"/>
chrome/android/java/src/org/chromium/chrome/browser/explore_sites/CategoryCardAdapter.java
View file @ a9efb918
......@@ -12,9 +12,8 @@ import android.text.SpannableString;
import android.text.SpannableStringBuilder;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.modelutil.ListObservable;
import org.chromium.chrome.browser.modelutil.ForwardingListObservable;
import org.chromium.chrome.browser.modelutil.ListObservable.ListObserver;
import org.chromium.chrome.browser.modelutil.ListObservableImpl;
import org.chromium.chrome.browser.modelutil.PropertyKey;
import org.chromium.chrome.browser.modelutil.PropertyModel;
import org.chromium.chrome.browser.modelutil.PropertyObservable;
......@@ -33,17 +32,16 @@ import java.lang.annotation.RetentionPolicy;
/**
* Recycler view adapter delegate for a model containing a list of category cards and an error code.
*/
class CategoryCardAdapter extends ListObservableImpl<Void>
class CategoryCardAdapter extends ForwardingListObservable<Void>
implements RecyclerViewAdapter
.Delegate<CategoryCardViewHolderFactory.CategoryCardViewHolder, Void>,
PropertyObservable.PropertyObserver<PropertyKey>, ListObserver<Void> {
@IntDef({ViewType.HEADER, ViewType.CATEGORY, ViewType.LOADING, ViewType.ERROR})
@IntDef({ViewType.CATEGORY, ViewType.LOADING, ViewType.ERROR})
@Retention(RetentionPolicy.SOURCE)
public @interface ViewType {
int HEADER = 0;
int CATEGORY = 1;
int LOADING = 2;
int ERROR = 3;
int CATEGORY = 0;
int LOADING = 1;
int ERROR = 2;
}
private final RoundedIconGenerator mIconGenerator;
......@@ -75,8 +73,7 @@ class CategoryCardAdapter extends ListObservableImpl<Void>
!= ExploreSitesPage.CatalogLoadingState.SUCCESS) {
return 1;
}
// Add 1 for title.
return mCategoryModel.get(ExploreSitesPage.CATEGORY_LIST_KEY).size() + 1;
return mCategoryModel.get(ExploreSitesPage.CATEGORY_LIST_KEY).size();
}
@Override
......@@ -89,7 +86,6 @@ class CategoryCardAdapter extends ListObservableImpl<Void>
case ExploreSitesPage.CatalogLoadingState.LOADING_NET:
return ViewType.LOADING;
case ExploreSitesPage.CatalogLoadingState.SUCCESS:
if (position == 0) return ViewType.HEADER;
return ViewType.CATEGORY;
default:
assert(false);
......@@ -102,10 +98,8 @@ class CategoryCardAdapter extends ListObservableImpl<Void>
int position, @Nullable Void payload) {
if (holder.getItemViewType() == ViewType.CATEGORY) {
ExploreSitesCategoryCardView view = (ExploreSitesCategoryCardView) holder.itemView;
// Position - 1 because there is always title.
view.setCategory(
mCategoryModel.get(ExploreSitesPage.CATEGORY_LIST_KEY).get(position - 1),
position - 1, mIconGenerator, mContextMenuManager, mNavDelegate, mProfile);
view.setCategory(mCategoryModel.get(ExploreSitesPage.CATEGORY_LIST_KEY).get(position),
position, mIconGenerator, mContextMenuManager, mNavDelegate, mProfile);
} else if (holder.getItemViewType() == ViewType.LOADING) {
// Start spinner.
LoadingView spinner = holder.itemView.findViewById(R.id.loading);
......@@ -139,28 +133,8 @@ class CategoryCardAdapter extends ListObservableImpl<Void>
}
}
if (key == ExploreSitesPage.SCROLL_TO_CATEGORY_KEY) {
int pos = mCategoryModel.get(ExploreSitesPage.SCROLL_TO_CATEGORY_KEY);
// Add 1 for title.
mLayoutManager.scrollToPosition(pos + 1);
mLayoutManager.scrollToPosition(
mCategoryModel.get(ExploreSitesPage.SCROLL_TO_CATEGORY_KEY));
}
}
@Override
public void onItemRangeInserted(ListObservable source, int index, int count) {
// Add 1 because of title.
notifyItemRangeInserted(index + 1, count);
}
@Override
public void onItemRangeRemoved(ListObservable source, int index, int count) {
// Add 1 because of title.
notifyItemRangeRemoved(index + 1, count);
}
@Override
public void onItemRangeChanged(
ListObservable<Void> source, int index, int count, @Nullable Void payload) {
// Add 1 because of title.
notifyItemRangeChanged(index + 1, count, payload);
}
}
chrome/android/java/src/org/chromium/chrome/browser/explore_sites/CategoryCardViewHolderFactory.java
View file @ a9efb918
......@@ -27,11 +27,6 @@ class CategoryCardViewHolderFactory implements RecyclerViewAdapter.ViewHolderFac
ViewGroup parent, @CategoryCardAdapter.ViewType int viewType) {
View view;
switch (viewType) {
case CategoryCardAdapter.ViewType.HEADER:
view = LayoutInflater.from(parent.getContext())
.inflate(R.layout.explore_sites_title_card, parent,
/* attachToRoot = */ false);
break;
case CategoryCardAdapter.ViewType.CATEGORY:
view = LayoutInflater.from(parent.getContext())
.inflate(R.layout.explore_sites_category_card_view, parent,
......
chrome/android/java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesCategory.java
View file @ a9efb918
......@@ -23,21 +23,24 @@ import java.util.List;
*/
public class ExploreSitesCategory {
private static final String TAG = "ExploreSitesCategory";
// The ID to use when creating the More button, that should not scroll the ESP when clicked.
public static final int MORE_BUTTON_ID = -1;
// The ID to use when creating placeholder icons on the NTP when there is no data or the More
// button.
private static final int PLACEHOLDER_ID = -1;
static final int MAX_COLUMNS = 4;
// This enum must match the numbering for ExploreSites.CategoryClick in histograms.xml. Do not
// reorder or remove items, only add new items before COUNT.
@Retention(RetentionPolicy.SOURCE)
@IntDef({CategoryType.DEFAULT, CategoryType.SOCIAL, CategoryType.ENTERTAINMENT,
CategoryType.SPORT, CategoryType.NEWS, CategoryType.SHOPPING, CategoryType.REFERENCE,
CategoryType.BANKING, CategoryType.GOVERNMENT, CategoryType.TRAVEL,
CategoryType.EDUCATION, CategoryType.JOBS, CategoryType.APPS_GAMES,
CategoryType.FAVORITE, CategoryType.GOOGLE, CategoryType.FOOD, CategoryType.HEALTH,
CategoryType.BOOKS, CategoryType.TECHNOLOGY, CategoryType.SCIENCE, CategoryType.COUNT})
@IntDef({CategoryType.MORE_BUTTON, CategoryType.DEFAULT, CategoryType.SOCIAL,
CategoryType.ENTERTAINMENT, CategoryType.SPORT, CategoryType.NEWS,
CategoryType.SHOPPING, CategoryType.REFERENCE, CategoryType.BANKING,
CategoryType.GOVERNMENT, CategoryType.TRAVEL, CategoryType.EDUCATION, CategoryType.JOBS,
CategoryType.APPS_GAMES, CategoryType.FAVORITE, CategoryType.GOOGLE, CategoryType.FOOD,
CategoryType.HEALTH, CategoryType.BOOKS, CategoryType.TECHNOLOGY, CategoryType.SCIENCE,
CategoryType.COUNT})
public @interface CategoryType {
int MORE_BUTTON = -1; // This is not included in histograms.xml.
int DEFAULT = 0;
int SOCIAL = 1;
int ENTERTAINMENT = 2;
......@@ -62,6 +65,11 @@ public class ExploreSitesCategory {
int COUNT = 20;
}
public static ExploreSitesCategory createPlaceholder(
@CategoryType int categoryType, String title) {
return new ExploreSitesCategory(PLACEHOLDER_ID, categoryType, title);
}
private int mCategoryId;
private @CategoryType int mCategoryType;
......@@ -95,7 +103,11 @@ public class ExploreSitesCategory {
}
public boolean isPlaceholder() {
return mCategoryId == -1;
return mCategoryId == PLACEHOLDER_ID;
}
public boolean isMoreButton() {
return mCategoryType == CategoryType.MORE_BUTTON;
}
public String getTitle() {
......
chrome/android/java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesCategoryCardView.java
View file @ a9efb918
......@@ -89,10 +89,9 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
// Remove from model (category).
mCategory.removeSite(mTileIndex);
// Update the view This may add any sites that we didn't have room for before. It
// should reset the tile indexeds for views we keep.
updateTileViews(
mCategory.getSites(), mCategory.getNumDisplayed(), mCategory.getMaxRows());
// Update the view. This may add sites that we didn't have room for before. It
// should reset the tile indexes for views we keep.
updateTileViews(mCategory);
}
@Override
public String getUrl() {
......@@ -160,14 +159,14 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
mCategory = category;
updateTitle(category.getTitle());
updateTileViews(category.getSites(), category.getNumDisplayed(), category.getMaxRows());
updateTileViews(category);
}
public void updateTitle(String categoryTitle) {
mTitleView.setText(categoryTitle);
}
public void updateTileViews(List<ExploreSitesSite> sites, int numSitesToShow, int maxRows) {
public void updateTileViews(ExploreSitesCategory category) {
// Clear observers.
for (PropertyModelChangeProcessor<PropertyModel, ExploreSitesTileView, PropertyKey>
observer : mModelChangeProcessors) {
......@@ -177,11 +176,12 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
// Only show rows that would be fully populated by original list of sites. This is
// calculated within the category.
mTileView.setMaxRows(maxRows);
mTileView.setMaxRows(Math.min(category.getMaxRows(), MAX_ROWS));
// Maximum number of sites that can be shown, defined as min of
// numSitesToShow and maxRows * maxCols.
int tileMax = Math.min(maxRows * ExploreSitesCategory.MAX_COLUMNS, numSitesToShow);
int tileMax = Math.min(category.getMaxRows() * ExploreSitesCategory.MAX_COLUMNS,
category.getNumDisplayed());
// Remove extra tiles if too many.
if (mTileView.getChildCount() > tileMax) {
......@@ -199,7 +199,7 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
// Initialize all the non-empty tiles again to update.
int tileIndex = 0;
for (ExploreSitesSite site : sites) {
for (ExploreSitesSite site : category.getSites()) {
if (tileIndex >= tileMax) break;
final PropertyModel siteModel = site.getModel();
// Skip blacklisted sites.
......
chrome/android/java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesPage.java
View file @ a9efb918
......@@ -33,7 +33,6 @@ import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.tab.EmptyTabObserver;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tab.TabObserver;
import org.chromium.chrome.browser.tabmodel.TabModelSelector;
import org.chromium.chrome.browser.widget.RoundedIconGenerator;
import org.chromium.content_public.browser.NavigationController;
import org.chromium.content_public.browser.NavigationEntry;
......@@ -70,7 +69,6 @@ public class ExploreSitesPage extends BasicNativePage {
int LOADING_NET = 4; // Retrieving catalog resources from internet.
}
private TabModelSelector mTabModelSelector;
private NativePageHost mHost;
private Tab mTab;
private TabObserver mTabObserver;
......@@ -97,7 +95,6 @@ public class ExploreSitesPage extends BasicNativePage {
mHost = host;
mTab = mHost.getActiveTab();
mTabModelSelector = activity.getTabModelSelector();
mTitle = activity.getString(R.string.explore_sites_title);
mView = (ViewGroup) activity.getLayoutInflater().inflate(
R.layout.explore_sites_page_layout, null);
......@@ -118,8 +115,9 @@ public class ExploreSitesPage extends BasicNativePage {
context.getResources(), R.color.default_favicon_background_color),
context.getResources().getDimensionPixelSize(R.dimen.tile_view_icon_text_size));
NativePageNavigationDelegateImpl navDelegate =
new NativePageNavigationDelegateImpl(activity, mProfile, host, mTabModelSelector);
NativePageNavigationDelegateImpl navDelegate = new NativePageNavigationDelegateImpl(
activity, mProfile, host, activity.getTabModelSelector());
// Don't direct reference activity because it might change if tab is reparented.
Runnable closeContextMenuCallback =
() -> host.getActiveTab().getActivity().closeContextMenu();
......@@ -139,8 +137,6 @@ public class ExploreSitesPage extends BasicNativePage {
ExploreSitesBridge.getEspCatalog(mProfile, this::translateToModel);
RecordUserAction.record("Android.ExploreSitesPage.Open");
// TODO(chili): Set layout to be an observer of list model
}
void translateToModel(@Nullable List<ExploreSitesCategory> categoryList) {
......
chrome/android/java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesPageLayout.java deleted 100644 → 0
View file @ 2d3f8519
// Copyright 2018 The Chromium Authors. All rights reserved.
// 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.explore_sites;
import android.view.View;
import org.chromium.chrome.browser.profiles.Profile;
/**
* Class to manage layout of explore sites page.
*/
public class ExploreSitesPageLayout {
private View mParent;
private Profile mProfile;
public ExploreSitesPageLayout(View parent, Profile profile) {
mParent = parent;
mProfile = profile;
}
}
chrome/android/java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesSection.java
View file @ a9efb918
......@@ -76,20 +76,19 @@ public class ExploreSitesSection {
List<ExploreSitesCategory> categoryList = new ArrayList<>();
// News category.
ExploreSitesCategory category =
new ExploreSitesCategory(-1 /* category_id */, CategoryType.NEWS,
getContext().getString(R.string.explore_sites_default_category_news));
ExploreSitesCategory category = ExploreSitesCategory.createPlaceholder(CategoryType.NEWS,
getContext().getString(R.string.explore_sites_default_category_news));
category.setDrawable(getVectorDrawable(R.drawable.ic_article_blue_24dp));
categoryList.add(category);
// Shopping category.
category = new ExploreSitesCategory(-1 /* category_id */, CategoryType.SHOPPING,
category = ExploreSitesCategory.createPlaceholder(CategoryType.SHOPPING,
getContext().getString(R.string.explore_sites_default_category_shopping));
category.setDrawable(getVectorDrawable(R.drawable.ic_shopping_basket_blue_24dp));
categoryList.add(category);
// Sport category.
category = new ExploreSitesCategory(-1 /* category_id */, CategoryType.SPORT,
category = ExploreSitesCategory.createPlaceholder(CategoryType.SPORT,
getContext().getString(R.string.explore_sites_default_category_sports));
category.setDrawable(getVectorDrawable(R.drawable.ic_directions_run_blue_24dp));
categoryList.add(category);
......@@ -98,8 +97,8 @@ public class ExploreSitesSection {
}
private ExploreSitesCategory createMoreTileCategory() {
ExploreSitesCategory category = new ExploreSitesCategory(-1 /* category_id */,
ExploreSitesCategory.MORE_BUTTON_ID, getContext().getString(R.string.more));
ExploreSitesCategory category = ExploreSitesCategory.createPlaceholder(
CategoryType.MORE_BUTTON, getContext().getString(R.string.more));
category.setDrawable(getVectorDrawable(R.drawable.ic_arrow_forward_blue_24dp));
return category;
}
......@@ -147,8 +146,7 @@ public class ExploreSitesSection {
ExploreSitesCategoryTileView v =
(ExploreSitesCategoryTileView) mExploreSection.getChildAt(i);
ExploreSitesCategory category = v.getCategory();
if (category == null || category.getType() == ExploreSitesCategory.MORE_BUTTON_ID)
continue;
if (category == null || category.isMoreButton()) continue;
viewTypes.put(category.getType(), v);
}
......
chrome/android/java_sources.gni
View file @ a9efb918
......@@ -648,7 +648,6 @@ chrome_java_sources = [
"java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesCategoryTileView.java",
"java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesEnums.java",
"java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesPage.java",
"java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesPageLayout.java",
"java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesSection.java",
"java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesSite.java",
"java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesTileView.java",
......
chrome/android/javatests/src/org/chromium/chrome/browser/explore_sites/ExploreSitesPageTest.java
View file @ a9efb918
......@@ -52,10 +52,12 @@ public class ExploreSitesPageTest {
ArrayList<ExploreSitesCategory> getTestingCatalog() {
final ArrayList<ExploreSitesCategory> categoryList = new ArrayList<>();
for (int i = 0; i < 4; i++) {
for (int i = 0; i < 5; i++) {
ExploreSitesCategory category =
new ExploreSitesCategory(i, i, "Category #" + Integer.toString(i));
for (int j = 0; j < 8; j++) {
// 0th category would be filtered out. Tests that row maximums are obeyed.
int numSites = 4 * i + 1;
for (int j = 0; j < numSites; j++) {
ExploreSitesSite site = new ExploreSitesSite(
i * 8 + j, "Site #" + Integer.toString(j), "https://example.com/", false);
category.addSite(site);
......
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