Commit be825729 authored by dgn's avatar dgn Committed by Commit bot

[NTP Client] Implement offline badge refresh via partial bind

The RecyclerView library has a mechanism to perform partial refresh
of views from the adapter via partial binds[1]. Added support for it
in the NewTabPageAdapter and related classes, and rewrote the refresh
of the offline badge using that.

This patch also fixes an issue where the badge was not removed when an
offline page was deleted

Preview: https://goo.gl/photos/MKD2WXTiNqbQ17xD8

BUG=616090

Review-Url: https://codereview.chromium.org/2622793003
Cr-Commit-Position: refs/heads/master@{#443236}
parent 48c86fea
...@@ -22,8 +22,12 @@ public abstract class ChildNode implements TreeNode { ...@@ -22,8 +22,12 @@ public abstract class ChildNode implements TreeNode {
mParent = parent; mParent = parent;
} }
protected void notifyItemRangeChanged(int index, int count, Object payload) {
if (mParent != null) mParent.onItemRangeChanged(this, index, count, payload);
}
protected void notifyItemRangeChanged(int index, int count) { protected void notifyItemRangeChanged(int index, int count) {
if (mParent != null) mParent.onItemRangeChanged(this, index, count); notifyItemRangeChanged(index, count, null);
} }
protected void notifyItemRangeInserted(int index, int count) { protected void notifyItemRangeInserted(int index, int count) {
...@@ -34,6 +38,10 @@ public abstract class ChildNode implements TreeNode { ...@@ -34,6 +38,10 @@ public abstract class ChildNode implements TreeNode {
if (mParent != null) mParent.onItemRangeRemoved(this, index, count); if (mParent != null) mParent.onItemRangeRemoved(this, index, count);
} }
protected void notifyItemChanged(int index, Object payload) {
notifyItemRangeChanged(index, 1, payload);
}
protected void notifyItemChanged(int index) { protected void notifyItemChanged(int index) {
notifyItemRangeChanged(index, 1); notifyItemRangeChanged(index, 1);
} }
......
...@@ -71,10 +71,10 @@ public class InnerNode extends ChildNode implements NodeParent { ...@@ -71,10 +71,10 @@ public class InnerNode extends ChildNode implements NodeParent {
} }
@Override @Override
public void onBindViewHolder(NewTabPageViewHolder holder, int position) { public void onBindViewHolder(NewTabPageViewHolder holder, int position, List<Object> payloads) {
int index = getChildIndexForPosition(position); int index = getChildIndexForPosition(position);
mChildren.get(index).onBindViewHolder( mChildren.get(index).onBindViewHolder(
holder, position - getStartingOffsetForChildIndex(index)); holder, position - getStartingOffsetForChildIndex(index), payloads);
} }
@Override @Override
...@@ -99,8 +99,8 @@ public class InnerNode extends ChildNode implements NodeParent { ...@@ -99,8 +99,8 @@ public class InnerNode extends ChildNode implements NodeParent {
} }
@Override @Override
public void onItemRangeChanged(TreeNode child, int index, int count) { public void onItemRangeChanged(TreeNode child, int index, int count, Object payload) {
notifyItemRangeChanged(getStartingOffsetForChild(child) + index, count); notifyItemRangeChanged(getStartingOffsetForChild(child) + index, count, payload);
} }
@Override @Override
......
...@@ -7,6 +7,8 @@ package org.chromium.chrome.browser.ntp.cards; ...@@ -7,6 +7,8 @@ package org.chromium.chrome.browser.ntp.cards;
import org.chromium.base.Callback; import org.chromium.base.Callback;
import org.chromium.chrome.browser.ntp.snippets.SnippetArticle; import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
import java.util.List;
/** /**
* A permanent leaf in the tree, i.e. a single item. * A permanent leaf in the tree, i.e. a single item.
* If the leaf is not to be a permanent member of the tree, see {@link OptionalLeaf} for an * If the leaf is not to be a permanent member of the tree, see {@link OptionalLeaf} for an
...@@ -26,7 +28,7 @@ public abstract class Leaf extends ChildNode { ...@@ -26,7 +28,7 @@ public abstract class Leaf extends ChildNode {
} }
@Override @Override
public void onBindViewHolder(NewTabPageViewHolder holder, int position) { public void onBindViewHolder(NewTabPageViewHolder holder, int position, List<Object> payload) {
if (position != 0) throw new IndexOutOfBoundsException(); if (position != 0) throw new IndexOutOfBoundsException();
onBindViewHolder(holder); onBindViewHolder(holder);
} }
...@@ -51,7 +53,7 @@ public abstract class Leaf extends ChildNode { ...@@ -51,7 +53,7 @@ public abstract class Leaf extends ChildNode {
/** /**
* Display the data for this item. * Display the data for this item.
* @param holder The view holder that should be updated. * @param holder The view holder that should be updated.
* @see #onBindViewHolder(NewTabPageViewHolder, int) * @see #onBindViewHolder(NewTabPageViewHolder, int, List)
* @see android.support.v7.widget.RecyclerView.Adapter#onBindViewHolder * @see android.support.v7.widget.RecyclerView.Adapter#onBindViewHolder
*/ */
protected abstract void onBindViewHolder(NewTabPageViewHolder holder); protected abstract void onBindViewHolder(NewTabPageViewHolder holder);
......
...@@ -20,6 +20,9 @@ import org.chromium.chrome.browser.ntp.snippets.SnippetArticle; ...@@ -20,6 +20,9 @@ import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
import org.chromium.chrome.browser.ntp.snippets.SnippetArticleViewHolder; import org.chromium.chrome.browser.ntp.snippets.SnippetArticleViewHolder;
import org.chromium.chrome.browser.offlinepages.OfflinePageBridge; import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
import java.util.Collections;
import java.util.List;
/** /**
* A class that handles merging above the fold elements and below the fold cards into an adapter * A class that handles merging above the fold elements and below the fold cards into an adapter
* that will be used to back the NTP RecyclerView. The first element in the adapter should always be * that will be used to back the NTP RecyclerView. The first element in the adapter should always be
...@@ -130,9 +133,14 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements ...@@ -130,9 +133,14 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
return null; return null;
} }
@Override
public void onBindViewHolder(NewTabPageViewHolder holder, int position, List<Object> payloads) {
mRoot.onBindViewHolder(holder, position, payloads);
}
@Override @Override
public void onBindViewHolder(NewTabPageViewHolder holder, final int position) { public void onBindViewHolder(NewTabPageViewHolder holder, final int position) {
mRoot.onBindViewHolder(holder, position); mRoot.onBindViewHolder(holder, position, Collections.emptyList());
} }
@Override @Override
...@@ -174,9 +182,10 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements ...@@ -174,9 +182,10 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
} }
@Override @Override
public void onItemRangeChanged(TreeNode child, int itemPosition, int itemCount) { public void onItemRangeChanged(
TreeNode child, int itemPosition, int itemCount, Object payload) {
assert child == mRoot; assert child == mRoot;
notifyItemRangeChanged(itemPosition, itemCount); notifyItemRangeChanged(itemPosition, itemCount, payload);
} }
@Override @Override
......
...@@ -10,14 +10,15 @@ package org.chromium.chrome.browser.ntp.cards; ...@@ -10,14 +10,15 @@ package org.chromium.chrome.browser.ntp.cards;
public interface NodeParent { public interface NodeParent {
/** /**
* Notifies that {@code count} items starting at position {@code index} under the {@code child} * Notifies that {@code count} items starting at position {@code index} under the {@code child}
* have changed. * have changed with an optional payload object.
* @param child The child whose items have changed. * @param child The child whose items have changed.
* @param index The starting position of the range of changed items, relative to the * @param index The starting position of the range of changed items, relative to the
* {@code child}. * {@code child}.
* @param count The number of changed items. * @param count The number of changed items.
* @see android.support.v7.widget.RecyclerView.Adapter#notifyItemRangeChanged(int, int) * @param payload Optional parameter, use {@code null} to identify a "full" update.
* @see android.support.v7.widget.RecyclerView.Adapter#notifyItemRangeChanged(int, int, Object)
*/ */
void onItemRangeChanged(TreeNode child, int index, int count); void onItemRangeChanged(TreeNode child, int index, int count, Object payload);
/** /**
* Notifies that {@code count} items starting at position {@code index} under the {@code child} * Notifies that {@code count} items starting at position {@code index} under the {@code child}
......
...@@ -9,6 +9,8 @@ import android.support.annotation.CallSuper; ...@@ -9,6 +9,8 @@ import android.support.annotation.CallSuper;
import org.chromium.base.Callback; import org.chromium.base.Callback;
import org.chromium.chrome.browser.ntp.snippets.SnippetArticle; import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
import java.util.List;
/** /**
* An optional leaf (i.e. single item) in the tree. Depending on its internal state (see * An optional leaf (i.e. single item) in the tree. Depending on its internal state (see
* {@link #isVisible()}), the item will be present or absent from the tree, by manipulating the * {@link #isVisible()}), the item will be present or absent from the tree, by manipulating the
...@@ -32,7 +34,7 @@ public abstract class OptionalLeaf extends ChildNode { ...@@ -32,7 +34,7 @@ public abstract class OptionalLeaf extends ChildNode {
} }
@Override @Override
public void onBindViewHolder(NewTabPageViewHolder holder, int position) { public void onBindViewHolder(NewTabPageViewHolder holder, int position, List<Object> payload) {
checkIndex(position); checkIndex(position);
onBindViewHolder(holder); onBindViewHolder(holder);
} }
...@@ -80,7 +82,7 @@ public abstract class OptionalLeaf extends ChildNode { ...@@ -80,7 +82,7 @@ public abstract class OptionalLeaf extends ChildNode {
/** /**
* Display the data for this item. * Display the data for this item.
* @param holder The view holder that should be updated. * @param holder The view holder that should be updated.
* @see #onBindViewHolder(NewTabPageViewHolder, int) * @see #onBindViewHolder(NewTabPageViewHolder, int, List)
* @see android.support.v7.widget.RecyclerView.Adapter#onBindViewHolder * @see android.support.v7.widget.RecyclerView.Adapter#onBindViewHolder
*/ */
protected abstract void onBindViewHolder(NewTabPageViewHolder holder); protected abstract void onBindViewHolder(NewTabPageViewHolder holder);
......
...@@ -93,11 +93,12 @@ public class SuggestionsSection extends InnerNode { ...@@ -93,11 +93,12 @@ public class SuggestionsSection extends InnerNode {
} }
@Override @Override
public void onBindViewHolder(NewTabPageViewHolder holder, int position) { public void onBindViewHolder(
NewTabPageViewHolder holder, int position, List<Object> payloads) {
checkIndex(position); checkIndex(position);
assert holder instanceof SnippetArticleViewHolder; assert holder instanceof SnippetArticleViewHolder;
((SnippetArticleViewHolder) holder) ((SnippetArticleViewHolder) holder)
.onBindViewHolder(getSuggestionAt(position), mCategoryInfo); .onBindViewHolder(getSuggestionAt(position), mCategoryInfo, payloads);
} }
@Override @Override
...@@ -155,6 +156,15 @@ public class SuggestionsSection extends InnerNode { ...@@ -155,6 +156,15 @@ public class SuggestionsSection extends InnerNode {
suggestionsSource.dismissSuggestion(suggestion); suggestionsSource.dismissSuggestion(suggestion);
itemRemovedCallback.onResult(suggestion.mTitle); itemRemovedCallback.onResult(suggestion.mTitle);
} }
public void updateSuggestionOfflineId(SnippetArticle article, Long newId) {
Long oldId = article.getOfflinePageOfflineId();
article.setOfflinePageOfflineId(newId);
if ((oldId == null) == (newId == null)) return;
notifyItemChanged(mSuggestions.indexOf(article),
SnippetArticleViewHolder.PARTIAL_UPDATE_OFFLINE_ID);
}
} }
private void setupOfflinePageBridgeObserver(NewTabPageManager manager) { private void setupOfflinePageBridgeObserver(NewTabPageManager manager) {
...@@ -286,11 +296,8 @@ public class SuggestionsSection extends InnerNode { ...@@ -286,11 +296,8 @@ public class SuggestionsSection extends InnerNode {
@Override @Override
public void onResult(OfflinePageItem item) { public void onResult(OfflinePageItem item) {
if (mIsNtpDestroyed) return; if (mIsNtpDestroyed) return;
if (item == null) { mSuggestionsList.updateSuggestionOfflineId(
article.setOfflinePageOfflineId(null); article, item == null ? null : item.getOfflineId());
return;
}
article.setOfflinePageOfflineId(item.getOfflineId());
} }
}); });
} }
......
...@@ -7,6 +7,8 @@ package org.chromium.chrome.browser.ntp.cards; ...@@ -7,6 +7,8 @@ package org.chromium.chrome.browser.ntp.cards;
import org.chromium.base.Callback; import org.chromium.base.Callback;
import org.chromium.chrome.browser.ntp.snippets.SnippetArticle; import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
import java.util.List;
/** /**
* A tree interface to allow the New Tab Page RecyclerView to delegate to other components. * A tree interface to allow the New Tab Page RecyclerView to delegate to other components.
*/ */
...@@ -36,12 +38,13 @@ interface TreeNode { ...@@ -36,12 +38,13 @@ interface TreeNode {
int getItemViewType(int position); int getItemViewType(int position);
/** /**
* Display the data at {@code position} under this subtree. * Display the data at {@code position} under this subtree, making a partial update based on
* the {@code payload} data.
* @param holder The view holder that should be updated. * @param holder The view holder that should be updated.
* @param position The position of the item under this subtree. * @param position The position of the item under this subtree.
* @see android.support.v7.widget.RecyclerView.Adapter#onBindViewHolder * @see android.support.v7.widget.RecyclerView.Adapter#onBindViewHolder(ViewHolder, int, List)
*/ */
void onBindViewHolder(NewTabPageViewHolder holder, final int position); void onBindViewHolder(NewTabPageViewHolder holder, int position, List<Object> payloads);
/** /**
* @param position The position to query. * @param position The position to query.
......
...@@ -48,9 +48,6 @@ public class SnippetArticle { ...@@ -48,9 +48,6 @@ public class SnippetArticle {
/** Stores whether impression of this article has been tracked already. */ /** Stores whether impression of this article has been tracked already. */
private boolean mImpressionTracked; private boolean mImpressionTracked;
/** To be run when the offline status of the article changes. */
private Runnable mOfflineStatusChangeRunnable;
/** Whether the linked article represents an asset download. */ /** Whether the linked article represents an asset download. */
public boolean mIsAssetDownload; public boolean mIsAssetDownload;
...@@ -115,14 +112,6 @@ public class SnippetArticle { ...@@ -115,14 +112,6 @@ public class SnippetArticle {
return true; return true;
} }
/**
* Sets the {@link Runnable} to be run when the article's offline status changes.
* Pass null to wipe.
*/
public void setOfflineStatusChangeRunnable(Runnable runnable) {
mOfflineStatusChangeRunnable = runnable;
}
/** @return whether a snippet is either offline page or asset download. */ /** @return whether a snippet is either offline page or asset download. */
public boolean isDownload() { public boolean isDownload() {
return mCategory == KnownCategories.DOWNLOADS; return mCategory == KnownCategories.DOWNLOADS;
...@@ -202,14 +191,7 @@ public class SnippetArticle { ...@@ -202,14 +191,7 @@ public class SnippetArticle {
/** Sets offline id of the corresponding to the snippet offline page. Null to clear.*/ /** Sets offline id of the corresponding to the snippet offline page. Null to clear.*/
public void setOfflinePageOfflineId(@Nullable Long offlineId) { public void setOfflinePageOfflineId(@Nullable Long offlineId) {
Long previous = mOfflinePageOfflineId;
mOfflinePageOfflineId = offlineId; mOfflinePageOfflineId = offlineId;
if (mOfflineStatusChangeRunnable == null) return;
if ((previous == null) ? (mOfflinePageOfflineId != null)
: !previous.equals(mOfflinePageOfflineId)) {
mOfflineStatusChangeRunnable.run();
}
} }
/** /**
......
...@@ -43,6 +43,7 @@ import org.chromium.ui.mojom.WindowOpenDisposition; ...@@ -43,6 +43,7 @@ import org.chromium.ui.mojom.WindowOpenDisposition;
import java.net.URI; import java.net.URI;
import java.net.URISyntaxException; import java.net.URISyntaxException;
import java.util.List;
import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeUnit;
/** /**
...@@ -56,6 +57,8 @@ public class SnippetArticleViewHolder ...@@ -56,6 +57,8 @@ public class SnippetArticleViewHolder
private static final String FAVICON_SERVICE_FORMAT = private static final String FAVICON_SERVICE_FORMAT =
"https://s2.googleusercontent.com/s2/favicons?domain=%s&src=chrome_newtab_mobile&sz=%d&alt=404"; "https://s2.googleusercontent.com/s2/favicons?domain=%s&src=chrome_newtab_mobile&sz=%d&alt=404";
public static final int PARTIAL_UPDATE_OFFLINE_ID = 1;
private final NewTabPageManager mNewTabPageManager; private final NewTabPageManager mNewTabPageManager;
private final TextView mHeadlineTextView; private final TextView mHeadlineTextView;
private final TextView mPublisherTextView; private final TextView mPublisherTextView;
...@@ -220,11 +223,14 @@ public class SnippetArticleViewHolder ...@@ -220,11 +223,14 @@ public class SnippetArticleViewHolder
BidiFormatter.getInstance().unicodeWrap(article.mPublisher), relativeTimeSpan); BidiFormatter.getInstance().unicodeWrap(article.mPublisher), relativeTimeSpan);
} }
public void onBindViewHolder(SnippetArticle article, SuggestionsCategoryInfo categoryInfo) { public void onBindViewHolder(
super.onBindViewHolder(); SnippetArticle article, SuggestionsCategoryInfo categoryInfo, List<Object> payloads) {
if (!payloads.isEmpty() && article.equals(mArticle)) {
performPartialBind(payloads);
return;
}
// No longer listen for offline status changes to the old article. super.onBindViewHolder();
if (mArticle != null) mArticle.setOfflineStatusChangeRunnable(null);
mArticle = article; mArticle = article;
mCategoryInfo = categoryInfo; mCategoryInfo = categoryInfo;
...@@ -265,22 +271,24 @@ public class SnippetArticleViewHolder ...@@ -265,22 +271,24 @@ public class SnippetArticleViewHolder
} }
mOfflineBadge.setVisibility(View.GONE); mOfflineBadge.setVisibility(View.GONE);
if (SnippetsConfig.isOfflineBadgeEnabled()) { refreshOfflineBadgeVisibility();
Runnable offlineChecker = new Runnable() {
@Override
public void run() {
if (mArticle.getOfflinePageOfflineId() != null || mArticle.mIsAssetDownload) {
mOfflineBadge.setVisibility(View.VISIBLE);
}
}
};
mArticle.setOfflineStatusChangeRunnable(offlineChecker);
offlineChecker.run();
}
mRecyclerView.onSnippetBound(itemView); mRecyclerView.onSnippetBound(itemView);
} }
private void refreshOfflineBadgeVisibility() {
if (!SnippetsConfig.isOfflineBadgeEnabled()) return;
boolean visible = mArticle.getOfflinePageOfflineId() != null || mArticle.mIsAssetDownload;
if (visible == (mOfflineBadge.getVisibility() == View.VISIBLE)) return;
mOfflineBadge.setVisibility(visible ? View.VISIBLE : View.GONE);
}
private void performPartialBind(List<Object> payload) {
if (payload.contains(PARTIAL_UPDATE_OFFLINE_ID)) {
refreshOfflineBadgeVisibility();
}
}
private static class FetchImageCallback extends Callback<Bitmap> { private static class FetchImageCallback extends Callback<Bitmap> {
private SnippetArticleViewHolder mViewHolder; private SnippetArticleViewHolder mViewHolder;
private final SnippetArticle mSnippet; private final SnippetArticle mSnippet;
......
...@@ -30,6 +30,8 @@ import org.chromium.chrome.browser.ntp.snippets.SnippetArticle; ...@@ -30,6 +30,8 @@ import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
import org.chromium.testing.local.LocalRobolectricTestRunner; import org.chromium.testing.local.LocalRobolectricTestRunner;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List; import java.util.List;
/** /**
...@@ -79,15 +81,17 @@ public class InnerNodeTest { ...@@ -79,15 +81,17 @@ public class InnerNodeTest {
@Test @Test
public void testBindViewHolder() { public void testBindViewHolder() {
NewTabPageViewHolder holder = mock(NewTabPageViewHolder.class); NewTabPageViewHolder holder = mock(NewTabPageViewHolder.class);
mInnerNode.onBindViewHolder(holder, 0); List<Object> payload1 = Collections.emptyList();
mInnerNode.onBindViewHolder(holder, 5); List<Object> payload2 = Arrays.<Object>asList("some data", "some other data");
mInnerNode.onBindViewHolder(holder, 6); mInnerNode.onBindViewHolder(holder, 0, payload1);
mInnerNode.onBindViewHolder(holder, 11); mInnerNode.onBindViewHolder(holder, 5, payload1);
mInnerNode.onBindViewHolder(holder, 6, payload2);
verify(mChildren.get(0)).onBindViewHolder(holder, 0); mInnerNode.onBindViewHolder(holder, 11, payload1);
verify(mChildren.get(2)).onBindViewHolder(holder, 2);
verify(mChildren.get(4)).onBindViewHolder(holder, 0); verify(mChildren.get(0)).onBindViewHolder(holder, 0, payload1);
verify(mChildren.get(6)).onBindViewHolder(holder, 0); verify(mChildren.get(2)).onBindViewHolder(holder, 2, payload1);
verify(mChildren.get(4)).onBindViewHolder(holder, 0, payload2);
verify(mChildren.get(6)).onBindViewHolder(holder, 0, payload1);
} }
@Test @Test
...@@ -147,13 +151,13 @@ public class InnerNodeTest { ...@@ -147,13 +151,13 @@ public class InnerNodeTest {
@Test @Test
public void testNotifications() { public void testNotifications() {
mInnerNode.onItemRangeInserted(mChildren.get(0), 0, 23); mInnerNode.onItemRangeInserted(mChildren.get(0), 0, 23);
mInnerNode.onItemRangeChanged(mChildren.get(2), 2, 9000); mInnerNode.onItemRangeChanged(mChildren.get(2), 2, 9000, null);
mInnerNode.onItemRangeChanged(mChildren.get(4), 0, 6502); mInnerNode.onItemRangeChanged(mChildren.get(4), 0, 6502, null);
mInnerNode.onItemRangeRemoved(mChildren.get(6), 0, 8086); mInnerNode.onItemRangeRemoved(mChildren.get(6), 0, 8086);
verify(mParent).onItemRangeInserted(mInnerNode, 0, 23); verify(mParent).onItemRangeInserted(mInnerNode, 0, 23);
verify(mParent).onItemRangeChanged(mInnerNode, 5, 9000); verify(mParent).onItemRangeChanged(mInnerNode, 5, 9000, null);
verify(mParent).onItemRangeChanged(mInnerNode, 6, 6502); verify(mParent).onItemRangeChanged(mInnerNode, 6, 6502, null);
verify(mParent).onItemRangeRemoved(mInnerNode, 11, 8086); verify(mParent).onItemRangeRemoved(mInnerNode, 11, 8086);
} }
...@@ -203,7 +207,7 @@ public class InnerNodeTest { ...@@ -203,7 +207,7 @@ public class InnerNodeTest {
} }
@Override @Override
public void onItemRangeChanged(TreeNode child, int index, int count) { public void onItemRangeChanged(TreeNode child, int index, int count, Object payload) {
checkCount(child); checkCount(child);
} }
......
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