[Android NTP] Move suggestions in SuggestionSection into a separate TreeNode.

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

 // Copyright 2016 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.ntp.cards;
 
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.ntp.snippets.CategoryInt;
 import org.chromium.chrome.browser.ntp.snippets.CategoryStatus.CategoryStatusEnum;
 import org.chromium.chrome.browser.ntp.snippets.SectionHeader;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
+import org.chromium.chrome.browser.ntp.snippets.SnippetArticleViewHolder;
 import org.chromium.chrome.browser.ntp.snippets.SnippetsBridge;
 
 import java.util.ArrayList;
 import java.util.List;
 
 /**
  * A group of suggestions, with a header, a status card, and a progress indicator.
  */
 public class SuggestionsSection extends InnerNode {
     private final List<TreeNode> mChildren = new ArrayList<>();
     private final List<SnippetArticle> mSuggestions = new ArrayList<>();
     private final SectionHeader mHeader;
+    private final TreeNode mSuggestionsList = new SuggestionsList(this, mSuggestions);
     private final StatusItem mStatus;
     private final ProgressItem mProgressIndicator = new ProgressItem();
     private final ActionItem mMoreButton;
     private final SuggestionsCategoryInfo mCategoryInfo;
 
     public SuggestionsSection(NodeParent parent, SuggestionsCategoryInfo info) {
         super(parent);
         mHeader = new SectionHeader(info.getTitle());
         mCategoryInfo = info;
         mMoreButton = new ActionItem(info);
         mStatus = StatusItem.createNoSuggestionsItem(info);
         resetChildren();
     }
 
+    private static class SuggestionsList extends ChildNode {
+        private final List<SnippetArticle> mSuggestions;

[dgn, 2016/10/21 17:13:30]

I find it a bit weird that we have another reference to the suggestion list
stored here. It's fine in the current case because mSuggestions is final in both
objects, but if the class was accessed from outside, we would have no guarantee
that the list if used/modified properly, right?

Maybe make the inner class non static and use directly the mSuggestions list?

Actually, what is the target end state/goal of the refactor? Do we have a design
doc or quick description for that somewhere?

[Bernhard Bauer, 2016/10/22 16:43:47]

Yeah, I considered that as well. I'm not sure what would be preferable here, as
inner classes are somewhat discouraged, but then again in this case there is not
much risk of a memory leak.

So, uh, TL;DR: Done.
Hm, I suppose I should update that design Doc Nicole started at some point…

I don't have an exact end state in mind, but there are a couple of things I
would like to achieve:
1) Decoupling the various pieces of logic, by moving them from the adapter into
the tree structure. For example, dismissal is something that could be handled by
a TreeNode instead of doing the big switch statement we have right now in the
Adapter.
2) Stabilizing the structure of the tree. Right now we tend to throw away
everything and rebuild, which means we have to manually figure out what changes
exactly to notify about (cf. removeSuggestion). If we keep the tree structure
stable and just adjust the number of children below each node as necessary (like
SigninPromo already does), we get correct notifications for free, and it will
allow for some optimizations as well (we could cache the number of children for
each node and update it incrementally when a subtree changes. Right now we'd
have to contort ourselves a bit to make that work with things like
resetChildren()).

+
+        public SuggestionsList(NodeParent parent, List<SnippetArticle> suggestions) {
+            super(parent);
+            mSuggestions = suggestions;
+        }
+
+        @Override
+        public int getItemCount() {
+            return mSuggestions.size();
+        }
+
+        @Override
+        @ItemViewType
+        public int getItemViewType(int position) {
+            return ItemViewType.SNIPPET;
+        }
+
+        @Override
+        public void onBindViewHolder(NewTabPageViewHolder holder, int position) {
+            assert holder instanceof SnippetArticleViewHolder;
+            ((SnippetArticleViewHolder) holder).onBindViewHolder(getSuggestionAt(position));
+        }
+
+        @Override
+        public SnippetArticle getSuggestionAt(int position) {
+            return mSuggestions.get(position);
+        }
+
+        @Override
+        public int getDismissSiblingPosDelta(int position) {
+            return 0;
+        }
+    }
+
     @Override
     protected List<TreeNode> getChildren() {
         return mChildren;
     }
 
     private void resetChildren() {
         mChildren.clear();
         mChildren.add(mHeader);
-        mChildren.addAll(mSuggestions);
+        mChildren.add(mSuggestionsList);
 
         if (mSuggestions.isEmpty()) mChildren.add(mStatus);
         if (mCategoryInfo.hasMoreButton() || mSuggestions.isEmpty()) mChildren.add(mMoreButton);
         if (mSuggestions.isEmpty()) mChildren.add(mProgressIndicator);
     }
 
     public void removeSuggestion(SnippetArticle suggestion) {
         int removedIndex = mSuggestions.indexOf(suggestion);
         if (removedIndex == -1) return;
 
@@ skipping 125 matching lines @@
     @VisibleForTesting
     ActionItem getActionItem() {
         return mMoreButton;
     }
 
     @VisibleForTesting
     StatusItem getStatusItem() {
         return mStatus;
     }
 }