Zine: support multiple sections in the ui

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
index 791ff025f3385521cf87bc6d040a86a0dab29f1f..83941effa5701751644aa31bb0382b0e02c3c0ea 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
@@ -17,6 +17,7 @@ import org.chromium.chrome.browser.ntp.NewTabPageLayout;
 import org.chromium.chrome.browser.ntp.NewTabPageUma;
 import org.chromium.chrome.browser.ntp.NewTabPageView.NewTabPageManager;
 import org.chromium.chrome.browser.ntp.UiConfig;
+import org.chromium.chrome.browser.ntp.snippets.ContentSuggestionsCategory;
 import org.chromium.chrome.browser.ntp.snippets.ContentSuggestionsCategoryStatus;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticleListItem;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticleViewHolder;
@@ -24,9 +25,14 @@ import org.chromium.chrome.browser.ntp.snippets.SnippetHeaderListItem;
 import org.chromium.chrome.browser.ntp.snippets.SnippetHeaderViewHolder;
 import org.chromium.chrome.browser.ntp.snippets.SnippetsBridge;
 import org.chromium.chrome.browser.ntp.snippets.SnippetsBridge.SnippetsObserver;
+import org.chromium.chrome.browser.ntp.snippets.SuggestionsCategory;
+import org.chromium.chrome.browser.ntp.snippets.SuggestionsCategoryStatus;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
 
 /**
  * A class that handles merging above the fold elements and below the fold cards into an adapter
@@ -39,17 +45,17 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
 
     private final NewTabPageManager mNewTabPageManager;
     private final NewTabPageLayout mNewTabPageLayout;
-    private final AboveTheFoldListItem mAboveTheFold;
-    private final SnippetHeaderListItem mHeader;
+    private final SnippetsBridge mSnippetsBridge;

[PEConn, 2016/08/01 16:26:51]

Just a heads up, when you merge this will be changed to SnippetsSource (if my CL
passes...)

[Bernhard Bauer, 2016/08/02 13:03:26]

Acknowledged.

     private final UiConfig mUiConfig;
-    private StatusListItem mStatusCard;
-    private final SpacingListItem mBottomSpacer;
-    private final List<NewTabPageListItem> mItems;
     private final ItemTouchCallbacks mItemTouchCallbacks;
     private NewTabPageRecyclerView mRecyclerView;
-    private int mProviderStatus;
 
-    private SnippetsBridge mSnippetsBridge;
+    private final List<NewTabPageItemGroup> mGroups;
+    private final AboveTheFoldListItem mAboveTheFold;
+    private final SpacingListItem mBottomSpacer;
+
+    // Maps suggestion categories to sections, with stable iteration ordering.
+    private final Map<Integer, SuggestionsSection> mSections;
 
     private class ItemTouchCallbacks extends ItemTouchHelper.Callback {
         @Override
@@ -112,17 +118,20 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
             SnippetsBridge snippetsBridge, UiConfig uiConfig) {
         mNewTabPageManager = manager;
         mNewTabPageLayout = newTabPageLayout;
-        mAboveTheFold = new AboveTheFoldListItem();
-        mHeader = new SnippetHeaderListItem();
-        mBottomSpacer = new SpacingListItem();
-        mItemTouchCallbacks = new ItemTouchCallbacks();
-        mItems = new ArrayList<>();
-        mProviderStatus = ContentSuggestionsCategoryStatus.INITIALIZING;
         mSnippetsBridge = snippetsBridge;
         mUiConfig = uiConfig;
-        mStatusCard = StatusListItem.create(snippetsBridge.getCategoryStatus(), this);
+        mItemTouchCallbacks = new ItemTouchCallbacks();
 
-        loadSnippets(new ArrayList<SnippetArticleListItem>());
+        mGroups = new ArrayList<>();

[PEConn, 2016/08/01 16:26:50]

Why not initialize these inline where they are declared?

[Bernhard Bauer, 2016/08/02 13:03:26]

Done.

+        mAboveTheFold = new AboveTheFoldListItem();
+        mBottomSpacer = new SpacingListItem();
+        mSections = new TreeMap<>();
+
+        // TODO(mvanouwerkerk): Do not hard code ARTICLES. Maybe do not initialize an empty
+        // section in the constructor.
+        setSuggestions(ContentSuggestionsCategory.ARTICLES,
+                Collections.<SnippetArticleListItem>emptyList(),
+                ContentSuggestionsCategoryStatus.INITIALIZING);
         mSnippetsBridge.setObserver(this);
     }
 
@@ -132,54 +141,70 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
     }
 
     @Override
-    public void onSnippetsReceived(List<SnippetArticleListItem> snippets) {
+    public void onSuggestionsReceived(
+            @SuggestionsCategory int category, List<SnippetArticleListItem> suggestions) {
         // We never want to refresh the suggestions if we already have some content.
-        if (hasSuggestions()) return;
+        if (mSections.containsKey(category) && mSections.get(category).hasSuggestions()) return;
 
-        if (!SnippetsBridge.isCategoryStatusInitOrAvailable(mProviderStatus)) {
+        // The status may have changed while the suggestions were loading, perhaps they should not
+        // be displayed any more.
+        if (!SnippetsBridge.isCategoryStatusInitOrAvailable(
+                    mSnippetsBridge.getCategoryStatus(category))) {
             return;
         }
 
-        Log.d(TAG, "Received %d new snippets.", snippets.size());
+        Log.d(TAG, "Received %d new suggestions for category %d.", suggestions.size(), category);
 
-        // At first, there might be no snippets available, we wait until they have been fetched.
-        if (snippets.isEmpty()) return;
+        // At first, there might be no suggestions available, we wait until they have been fetched.
+        if (suggestions.isEmpty()) return;
 
-        loadSnippets(snippets);
+        setSuggestions(category, suggestions, mSnippetsBridge.getCategoryStatus(category));
 
         NewTabPageUma.recordSnippetAction(NewTabPageUma.SNIPPETS_ACTION_SHOWN);
     }
 
     @Override
-    public void onCategoryStatusChanged(int categoryStatus) {
-        // Observers should not be registered for that state
-        assert categoryStatus
-                != ContentSuggestionsCategoryStatus.ALL_SUGGESTIONS_EXPLICITLY_DISABLED;
-
-        mProviderStatus = categoryStatus;
-        mStatusCard = StatusListItem.create(mProviderStatus, this);
-
-        // We had suggestions but we just got notified about the provider being enabled. Nothing to
-        // do then.
-        if (SnippetsBridge.isCategoryStatusAvailable(mProviderStatus) && hasSuggestions()) return;
-
-        if (hasSuggestions()) {
-            // We have suggestions, this implies that the service was previously enabled and just
-            // transitioned to a disabled state. Clear them.
-            loadSnippets(new ArrayList<SnippetArticleListItem>());
+    public void onCategoryStatusChanged(
+            @SuggestionsCategory int category, @SuggestionsCategoryStatus int status) {
+        // Observers should not be registered for this state.
+        assert status != ContentSuggestionsCategoryStatus.ALL_SUGGESTIONS_EXPLICITLY_DISABLED;
+
+        // If there is no section for this category there is nothing to do.
+        if (!mSections.containsKey(category)) return;
+
+        SuggestionsSection section = mSections.get(category);
+
+        // The section already has suggestions but we just got notified about the provider being
+        // enabled. Nothing to do.
+        if (SnippetsBridge.isCategoryStatusAvailable(status) && section.hasSuggestions()) return;
+
+        if (section.hasSuggestions()) {
+            // The section has suggestions, this implies that the service was previously enabled
+            // and just transitioned to a disabled state. Clear it.
+            setSuggestions(category, Collections.<SnippetArticleListItem>emptyList(), status);

[PEConn, 2016/08/01 16:26:51]

Do we want to pull the rug out from under the user? If they have suggestions
then something goes wrong in the back end, why not let them continue to see
their suggestions?

[Marc Treib, 2016/08/01 16:34:22]

Drive-by: In some cases, yes, namely when the suggestions actually become
unavailable. This can happen for offline pages, if an offline item actually gets
deleted. Then it's better to pull it out from under the user, rather than having
them click on something that will then fail to load.

Note that this will not happen for regular background updates, only when a
provider explicitly says "all my stuff must be removed right now".

         } else {
-            // If there are no suggestions there is an old status card that must be replaced.
-            int firstCardPosition = getFirstCardPosition();
-            mItems.set(firstCardPosition, mStatusCard);
-            // Update both the status card and the spacer after it.
-            notifyItemRangeChanged(firstCardPosition, 2);
+            // If the section has no suggestions, it has an old status card that must be replaced.
+            section.setSuggestions(Collections.<SnippetArticleListItem>emptyList(), status, this);
+
+            // Update both the status card, and the bottom spacer, which may get a different height
+            // if the status card height changed.
+            int statusItemPosition = getGroupPositionOffset(section);
+            List<NewTabPageListItem> sectionItems = section.getItems();
+            for (int i = 0; i < sectionItems.size(); i++) {
+                if (sectionItems.get(i) instanceof StatusListItem) {

[PEConn, 2016/08/01 16:26:51]

Won't the StatusListItem only ever be the first item in a section?

[Bernhard Bauer, 2016/08/02 13:03:26]

Done.

+                    statusItemPosition += i;
+                    break;
+                }
+            }
+            notifyItemChanged(statusItemPosition);
+            notifyItemChanged(getBottomSpacerPosition());
         }
     }
 
     @Override
     @NewTabPageListItem.ViewType
     public int getItemViewType(int position) {
-        return mItems.get(position).getType();
+        return getItems().get(position).getType();
     }
 
     @Override
@@ -212,24 +237,28 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
 
     @Override
     public void onBindViewHolder(NewTabPageViewHolder holder, final int position) {
-        holder.onBindViewHolder(mItems.get(position));
+        holder.onBindViewHolder(getItems().get(position));
     }
 
     @Override
     public int getItemCount() {
-        return mItems.size();
+        return getItems().size();
     }
 
     public int getAboveTheFoldPosition() {
-        return mItems.indexOf(mAboveTheFold);
+        return getItems().indexOf(mAboveTheFold);
     }
 
-    public int getHeaderPosition() {
-        return mItems.indexOf(mHeader);
+    public int getFirstHeaderPosition() {
+        List<NewTabPageListItem> items = getItems();
+        for (int i = 0; i < items.size(); i++) {
+            if (items.get(i) instanceof SnippetHeaderListItem) return i;
+        }
+        return RecyclerView.NO_POSITION;
     }
 
     public int getFirstCardPosition() {
-        return getHeaderPosition() + 1;
+        return getFirstHeaderPosition() + 1;
     }
 
     public int getLastCardPosition() {
@@ -237,7 +266,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
     }
 
     public int getBottomSpacerPosition() {
-        return mItems.indexOf(mBottomSpacer);
+        return getItems().indexOf(mBottomSpacer);
     }
 
     /** Start a request for new snippets. */
@@ -245,36 +274,43 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
         SnippetsBridge.fetchSnippets(/*forceRequest=*/true);
     }
 
-    private void loadSnippets(List<SnippetArticleListItem> snippets) {
-        // Copy thumbnails over
-        for (SnippetArticleListItem snippet : snippets) {
-            int existingSnippetIdx = mItems.indexOf(snippet);
-            if (existingSnippetIdx == -1) continue;
+    private void setSuggestions(@SuggestionsCategory int category,
+            List<SnippetArticleListItem> suggestions, @SuggestionsCategoryStatus int status) {
+        copyThumbnails(category, suggestions);
 
-            snippet.setThumbnailBitmap(
-                    ((SnippetArticleListItem) mItems.get(existingSnippetIdx)).getThumbnailBitmap());
-        }
+        mGroups.clear();
+        mGroups.add(mAboveTheFold);
 
-        boolean hasContentToShow = !snippets.isEmpty();
-
-        // TODO(mvanouwerkerk): Make it so that the header does not need to be manipulated
-        // separately from the cards to which it belongs - crbug.com/616090.
-        mHeader.setVisible(hasContentToShow);
-
-        mItems.clear();
-        mItems.add(mAboveTheFold);
-        mItems.add(mHeader);
-        if (hasContentToShow) {
-            mItems.addAll(snippets);
+        if (!mSections.containsKey(category)) {
+            mSections.put(category,
+                    new SuggestionsSection(suggestions, status, this));
         } else {
-            mItems.add(mStatusCard);
+            mSections.get(category).setSuggestions(suggestions, status, this);
         }
 
-        mItems.add(mBottomSpacer);
+        for (@SuggestionsCategory Integer key : mSections.keySet()) {

[PEConn, 2016/08/01 16:26:51]

Integer -> int ?

[Bernhard Bauer, 2016/08/02 13:03:26]

Hm, the key type for the Map is a boxed Integer, so it would have to be boxed
again when indexing the map on the next line. I was able to get around that
though by just directly getting a collection of the map values (which is going
to be ordered by the key).

One other thing I could do is use a SparseArray instead of TreeMap, which would
have unboxed ints as keys and is supposedly pretty efficient for small numbers
of items. WDYT?

[PEConn, 2016/08/02 15:45:14]

Given we're probably going to have a handful of categories, it may be better to
just stick to the simpler data structure (though I'm not sure which on that
is!).

+            mGroups.add(mSections.get(key));
+        }
+
+        mGroups.add(mBottomSpacer);
 
         notifyDataSetChanged();
     }
 
+    private void copyThumbnails(
+            @SuggestionsCategory int category, List<SnippetArticleListItem> suggestions) {
+        if (!mSections.containsKey(category)) return;
+
+        List<NewTabPageListItem> items = mSections.get(category).getItems();
+        for (SnippetArticleListItem suggestion : suggestions) {
+            int index = items.indexOf(suggestion);
+            if (index == -1) continue;
+
+            suggestion.setThumbnailBitmap(
+                    ((SnippetArticleListItem) items.get(index)).getThumbnailBitmap());
+        }
+    }
+
     @Override
     public void onAttachedToRecyclerView(RecyclerView recyclerView) {
         super.onAttachedToRecyclerView(recyclerView);
@@ -293,9 +329,9 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
         assert itemViewHolder.getItemViewType() == NewTabPageListItem.VIEW_TYPE_SNIPPET;
 
         int position = itemViewHolder.getAdapterPosition();
-        SnippetArticleListItem dismissedSnippet = (SnippetArticleListItem) mItems.get(position);
+        SnippetArticleListItem suggestion = (SnippetArticleListItem) getItems().get(position);
 
-        mSnippetsBridge.getSnippedVisited(dismissedSnippet, new Callback<Boolean>() {
+        mSnippetsBridge.getSnippedVisited(suggestion, new Callback<Boolean>() {
             @Override
             public void onResult(Boolean result) {
                 NewTabPageUma.recordSnippetAction(result
@@ -304,32 +340,48 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
             }
         });
 
-        mSnippetsBridge.discardSnippet(dismissedSnippet);
-        mItems.remove(position);
-        notifyItemRemoved(position);
+        mSnippetsBridge.discardSnippet(suggestion);
+        SuggestionsSection section = (SuggestionsSection) getGroup(position);
+        section.dismissSuggestion(suggestion);
 
-        addStatusCardIfNecessary();
+        if (section.hasSuggestions()) {
+            // If one of many suggestions was dismissed, it's a simple item removal, which can be
+            // animated smoothly by the RecyclerView.
+            notifyItemRemoved(position);
+        } else {
+            // If the last suggestion was dismissed, multiple items will have changed, so mark
+            // everything as changed.
+            notifyDataSetChanged();
+        }
     }
 
-    private void addStatusCardIfNecessary() {
-        if (!hasSuggestions() && !mItems.contains(mStatusCard)) {
-            mItems.add(getFirstCardPosition(), mStatusCard);
-
-            // We also want to refresh the header and the bottom padding.
-            mHeader.setVisible(false);
-            notifyDataSetChanged();
+    /**
+     * Returns an unmodifiable list containing all items in the adapter.
+     */
+    List<NewTabPageListItem> getItems() {

[Philipp Keck, 2016/08/01 15:28:57]

Not sure if this is relevant, but this method is called a lot because all the
methods for the Adapter interface depend on it. So performance on low-end
devices might be bad (not sure if that actually matters for the number of
suggestions we usually display, namely <= 10*#sections

[Bernhard Bauer, 2016/08/01 15:37:24]

Acknowledged. This is the area where I'm planning to do more refactorings :)

+        List<NewTabPageListItem> items = new ArrayList<>();
+        for (NewTabPageItemGroup group : mGroups) {
+            items.addAll(group.getItems());
         }
+        return Collections.unmodifiableList(items);
     }
 
-    /** Returns whether we have some suggested content to display. */
-    private boolean hasSuggestions() {
-        for (NewTabPageListItem item : mItems) {
-            if (item instanceof SnippetArticleListItem) return true;
+    private NewTabPageItemGroup getGroup(int itemPosition) {
+        int itemsSkipped = 0;
+        for (NewTabPageItemGroup group : mGroups) {
+            List<NewTabPageListItem> items = group.getItems();
+            itemsSkipped += items.size();
+            if (itemPosition < itemsSkipped) return group;
         }
-        return false;
+        return null;
     }
 
-    List<NewTabPageListItem> getItemsForTesting() {
-        return mItems;
+    private int getGroupPositionOffset(NewTabPageItemGroup group) {
+        int positionOffset = 0;
+        for (NewTabPageItemGroup candidateGroup : mGroups) {
+            if (candidateGroup == group) return positionOffset;
+            positionOffset += candidateGroup.getItems().size();
+        }
+        return RecyclerView.NO_POSITION;
     }
 }