[Download Home] Displaying offline page bundle per day

chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java

Index: chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
index da379763aac45652d550a98a73658cef2557e42d..7e792048826c1c8d57fe3a54358f07702817f040 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
@@ -16,7 +16,6 @@ import android.view.ViewGroup;
 import android.widget.TextView;
 
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.widget.DateDividedAdapter.ItemGroup;
 
 import java.util.ArrayList;
 import java.util.Calendar;
@@ -53,7 +52,7 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
         private boolean mIsLastInGroup;
 
         /** See {@link #mPosition}. */
-        private final void setPosition(int position) {
+        public final void setPosition(int position) {
             mPosition = position;
         }
 
@@ -152,9 +151,9 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
     /**
      * A bucket of items with the same date.
      */
-    protected static class ItemGroup {
+    public static class ItemGroup {
         private final Date mDate;
-        private final List<TimedItem> mItems = new ArrayList<>();
+        protected final List<TimedItem> mItems = new ArrayList<>();
 
         /** Index of the header, relative to the full list.  Must be set only once.*/
         private int mIndex;
@@ -182,15 +181,26 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
             mIndex = index;
 
             sortIfNeeded();
+            setPositionForItems(index + 1);
+            setPositionForFirstAndLastInGroup();
+        }
+
+        protected void setPositionForItems(int startIndex) {
+            int index = startIndex;
             for (int i = 0; i < mItems.size(); i++) {
-                index += 1;
                 TimedItem item = mItems.get(i);
                 item.setPosition(index);
-                item.setIsFirstInGroup(i == 0);
-                item.setIsLastInGroup(i == mItems.size() - 1);
+                index += 1;
             }
         }
 
+        protected void setPositionForFirstAndLastInGroup() {

[gone, 2017/02/10 02:31:45]

This isn't setting positions; call it "identifyFirstAndLastItems" or something.

[shaktisahu, 2017/02/10 21:30:25]

Done.

+            TimedItem first = mItems.get(0);
+            TimedItem last = mItems.get(mItems.size() - 1);
+            first.setIsFirstInGroup(true);
+            last.setIsLastInGroup(true);
+        }
+
         /** Unsets the position of all TimedItems in this group. */
         public void resetPosition() {
             mIndex = TimedItem.INVALID_POSITION;
@@ -204,6 +214,10 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
             return compareDate(mDate, otherDate) == 0;
         }
 
+        protected Date getDate() {
+            return mDate;

[Theresa, 2017/02/10 00:55:12]

For consistency, we should either have a getter that returns mItems and leave
that as private or make mDate protected and remove this getter. 

[shaktisahu, 2017/02/10 21:30:25]

Done. I would make mDate as protected and remove this getter.

+        }
+
         /**
          * @return The size of this group.
          */
@@ -219,6 +233,10 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
             if (index == 0 || mIsListHeaderOrFooter) return null;
 
             sortIfNeeded();
+            return getItemAtInner(index);
+        }
+
+        protected TimedItem getItemAtInner(int index) {

[Theresa, 2017/02/10 00:55:12]

nit: getItemInternal() is more consistent with naming for protected/private
methods associated with a public method.

[shaktisahu, 2017/02/10 21:30:25]

Done.
Renamed it to getItemAtInternal

             return mItems.get(index - 1);
         }
 
@@ -226,26 +244,34 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
          * Rather than sorting the list each time a new item is added, the list is sorted when
          * something requires a correct ordering of the items.
          */
-        private void sortIfNeeded() {
+        protected void sortIfNeeded() {
             if (mIsSorted) return;
             mIsSorted = true;
 
             Collections.sort(mItems, new Comparator<TimedItem>() {
                 @Override
                 public int compare(TimedItem lhs, TimedItem rhs) {
-                    // More recent items are listed first.  Ideally we'd use Long.compare, but that
-                    // is an API level 19 call for some inexplicable reason.
-                    long timeDelta = lhs.getTimestamp() - rhs.getTimestamp();
-                    if (timeDelta > 0) {
-                        return -1;
-                    } else if (timeDelta == 0) {
-                        return 0;
-                    } else {
-                        return 1;
-                    }
+                    return compareItem(lhs, rhs);
                 }
             });
         }
+
+        protected int compareItem(TimedItem lhs, TimedItem rhs) {
+            // More recent items are listed first.  Ideally we'd use Long.compare, but that
+            // is an API level 19 call for some inexplicable reason.
+            long timeDelta = lhs.getTimestamp() - rhs.getTimestamp();
+            if (timeDelta > 0) {
+                return -1;
+            } else if (timeDelta == 0) {
+                return 0;
+            } else {
+                return 1;
+            }
+        }
+
+        public int getItemViewType(int position) {
+            return position == 0 ? TYPE_DATE : TYPE_NORMAL;
+        }
     }
 
     // Cached async tasks to get the two Calendar objects, which are used when comparing dates.
@@ -256,6 +282,7 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
     public static final int TYPE_HEADER = -1;
     public static final int TYPE_DATE = 0;
     public static final int TYPE_NORMAL = 1;
+    public static final int TYPE_SUBSECTION_HEADER = 2;
 
     private int mSize;
     private boolean mHasListHeader;
@@ -323,28 +350,32 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
                 if (group.isSameDay(date)) {
                     found = true;
                     group.addItem(timedItem);
-                    mSize++;
                     break;
                 }
             }
             if (!found) {
                 // Create a new ItemGroup with the date for the new item. This increases the
                 // size by two because we add new views for the date and the item itself.
-                ItemGroup newGroup = new ItemGroup(timedItem.getTimestamp());
+                ItemGroup newGroup = createGroup(timedItem.getTimestamp());
                 newGroup.addItem(timedItem);
                 mGroups.add(newGroup);
-                mSize += 2;
             }
         }
 
+        computeItemCount();
         setGroupPositions();
         notifyDataSetChanged();
     }
 
+    protected ItemGroup createGroup(long timeStamp) {

[Theresa, 2017/02/10 00:55:12]

nit: s/timeStamp/timestamp

[shaktisahu, 2017/02/10 21:30:25]

Done.

+        ItemGroup group = new ItemGroup(timeStamp);
+        return group;

[gone, 2017/02/10 02:31:45]

return new ItemGroup(timestamp);

[shaktisahu, 2017/02/10 21:30:25]

Done.

+    }
+
     /**
      * Tells each group where they start in the list.
      */
-    private void setGroupPositions() {
+    protected void setGroupPositions() {
         int startIndex = 0;
         for (ItemGroup group : mGroups) {
             group.resetPosition();
@@ -454,15 +485,14 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
             return TYPE_HEADER;
         } else if (pair.second == TYPE_FOOTER) {
             return TYPE_FOOTER;
-        } else if (pair.second == 0) {
-            return TYPE_DATE;
         } else {
-            return TYPE_NORMAL;
+            ItemGroup group = pair.first;
+            return group.getItemViewType(pair.second);

[Theresa, 2017/02/10 00:55:12]

nit: This could be condensed to one line:
pair.first.getItemViewType(pair.second);

[shaktisahu, 2017/02/10 21:30:25]

hmm.. I think this is more readable.

         }
     }
 
     @Override
-    public final RecyclerView.ViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
+    public RecyclerView.ViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
         if (viewType == TYPE_DATE) {
             return createDateViewHolder(parent);
         } else if (viewType == TYPE_NORMAL) {
@@ -477,7 +507,7 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
     }
 
     @Override
-    public final void onBindViewHolder(RecyclerView.ViewHolder holder, int position) {
+    public void onBindViewHolder(RecyclerView.ViewHolder holder, int position) {
         Pair<Date, TimedItem> pair = getItemAt(position);
         if (holder instanceof DateViewHolder) {
             ((DateViewHolder) holder).setDate(pair.first);
@@ -522,18 +552,24 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
     protected void removeItem(TimedItem item) {
         ItemGroup group = getGroupAt(item.getPosition()).first;
         group.removeItem(item);
-        mSize--;
 
         // Remove the group if only the date header is left.
         if (group.size() == 1) {
             mGroups.remove(group);
-            mSize--;
         }
 
+        computeItemCount();

[Theresa, 2017/02/10 00:55:12]

If there are a lot of item groups, this is less efficient than decrementing
mSize.

[gone, 2017/02/10 02:31:45]

+1 again.  I asked about this in Patch Set 1.

[shaktisahu, 2017/02/10 21:30:25]

I thought this makes it cleaner (also used when adding items in loadItems). 

Actually, on the next line when we do setGroupPositions, we iterate through the
group anyway and set each element to its correct position after the delete,
which is a bigger operation than this one (O(itemcount)). So I think this should
be okay since this is O(groups) operation.

Otherwise, I would have to find out how many rows need to be deleted (it can be
1 or 2 or 3).

Something like : 

int rowsToBeDeleted = group.getNumRowsToBeDeletedForItem(item);
group.removeItem(item);
mSize -= rowsToBeDeleted;

And then implement getNumRowsToBeDeletedForItem for ItemGroup and
DownloadItemGroup subclass. But I feel the current code is cleaner than that.

[gone, 2017/02/10 22:49:52]

Cleanliness < efficiency sometimes, and depending on how many downloads a user
has, that could be a big difference on the UI thread–additional lag contributes
to ANRs.  How complicated is the logic for determining how many rows need to be
deleted? 

[shaktisahu, 2017/02/11 00:39:06]

Done.

         setGroupPositions();
         notifyDataSetChanged();
     }
 
+    protected void computeItemCount() {
+        mSize = 0;
+        for (ItemGroup group : mGroups) {
+            mSize += group.size();
+        }
+    }
+
     /**
      * Creates a long ID that identifies a particular day in history.
      * @param date Date to process.
@@ -554,7 +590,7 @@ public abstract class DateDividedAdapter extends Adapter<RecyclerView.ViewHolder
      * {@link #compareCalendar(Calendar, Calendar)} instead.
      * @return 0 if date1 and date2 are in the same day; 1 if date1 is before date2; -1 otherwise.
      */
-    private static int compareDate(Date date1, Date date2) {
+    protected static int compareDate(Date date1, Date date2) {
         Pair<Calendar, Calendar> pair = getCachedCalendars();
         Calendar cal1 = pair.first, cal2 = pair.second;
         cal1.setTime(date1);