[Download Home] Displaying offline page bundle per day

chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.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.widget;
 
 import android.os.AsyncTask;
 import android.support.v7.widget.RecyclerView;
 import android.support.v7.widget.RecyclerView.Adapter;
 import android.support.v7.widget.RecyclerView.ViewHolder;
 import android.text.format.DateUtils;
 import android.util.Pair;
 import android.view.LayoutInflater;
 import android.view.View;
 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;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.Date;
 import java.util.List;
 import java.util.SortedSet;
 import java.util.TreeSet;
 import java.util.concurrent.ExecutionException;
@@ skipping 16 matching lines @@
         /** Value indicating that a TimedItem is not currently being displayed. */
         public static final int INVALID_POSITION = -1;
 
         /** Position of the TimedItem in the list, or {@link #INVALID_POSITION} if not shown. */
         private int mPosition = INVALID_POSITION;
 
         private boolean mIsFirstInGroup;
         private boolean mIsLastInGroup;
 
         /** See {@link #mPosition}. */
-        private final void setPosition(int position) {
+        public final void setPosition(int position) {
             mPosition = position;
         }
 
         /** See {@link #mPosition}. */
         public final int getPosition() {
             return mPosition;
         }
 
         /**
          * @param isFirst Whether this item is the first in its group.
@@ skipping 78 matching lines @@
 
     protected static class BasicViewHolder extends RecyclerView.ViewHolder {
         public BasicViewHolder(View itemView) {
             super(itemView);
         }
     }
 
     /**
      * 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;
 
         private boolean mIsSorted;
         private boolean mIsListHeaderOrFooter;
 
         public ItemGroup(long timestamp) {
             mDate = new Date(timestamp);
             mIsSorted = true;
         }
 
         public void addItem(TimedItem item) {
             mItems.add(item);
             mIsSorted = mItems.size() == 1;
         }
 
         public void removeItem(TimedItem item) {
             mItems.remove(item);
         }
 
         /** Records the position of all the TimedItems in this group, relative to the full list. */
         public void setPosition(int index) {
             assert mIndex == 0 || mIndex == TimedItem.INVALID_POSITION;
             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;
             for (TimedItem item : mItems) item.setPosition(TimedItem.INVALID_POSITION);
         }
 
         /**
          * @return Whether the given date happens in the same day as the items in this group.
          */
         public boolean isSameDay(Date otherDate) {
             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.
          */
         public int size() {
             if (mIsListHeaderOrFooter) return 1;
 
             // Plus 1 to account for the date header.
             return mItems.size() + 1;
         }
 
         public TimedItem getItemAt(int index) {
             // 0 is allocated to the date header. The list header has no items.
             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);
         }
 
         /**
          * 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.
     private static final AsyncTask<Void, Void, Calendar> sCal1 = createCalendar();
     private static final AsyncTask<Void, Void, Calendar> sCal2 = createCalendar();
 
     public static final int TYPE_FOOTER = -2;
     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;
     private boolean mHasListFooter;
 
     private SortedSet<ItemGroup> mGroups = new TreeSet<>(new Comparator<ItemGroup>() {
         @Override
         public int compare(ItemGroup lhs, ItemGroup rhs) {
             return compareDate(lhs.mDate, rhs.mDate);
         }
@@ skipping 47 matching lines @@
      * {@link #clear(boolean)} to remove previous items.
      */
     public void loadItems(List<? extends TimedItem> timedItems) {
         for (TimedItem timedItem : timedItems) {
             Date date = new Date(timedItem.getTimestamp());
             boolean found = false;
             for (ItemGroup group : mGroups) {
                 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();
             group.setPosition(startIndex);
             startIndex += group.size();
         }
     }
 
     /**
      * Adds a header as the first group in this adapter.
@@ skipping 89 matching lines @@
         return new Pair<>(group.mDate, group.getItemAt(pair.second));
     }
 
     @Override
     public final int getItemViewType(int position) {
         Pair<ItemGroup, Integer> pair = getGroupAt(position);
         if (pair.second == TYPE_HEADER) {
             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) {
             return createViewHolder(parent);
         } else if (viewType == TYPE_HEADER) {
             return createHeader(parent);
         } else if (viewType == TYPE_FOOTER) {
             return createFooter(parent);
         }
         assert false;
         return null;
     }
 
     @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);
         } else if (!(holder instanceof BasicViewHolder)) {
             bindViewHolderForTimedItem(holder, pair.second);
         }
     }
 
     @Override
     public final int getItemCount() {
@@ skipping 24 matching lines @@
         assert false;
         return null;
     }
 
     /**
      * @param item The item to remove from the adapter.
      */
     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.
      * @return Long that has the day of the year (1-365) in the lowest 16 bits and the year in the
      *         next 16 bits over.
      */
     private static long getStableIdFromDate(Date date) {
         Pair<Calendar, Calendar> pair = getCachedCalendars();
         Calendar calendar = pair.first;
         calendar.setTime(date);
         long dayOfYear = calendar.get(Calendar.DAY_OF_YEAR);
         long year = calendar.get(Calendar.YEAR);
         return (year << 16) + dayOfYear;
     }
 
     /**
      * Compares two {@link Date}s. Note if you already have two {@link Calendar} objects, use
      * {@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);
         cal2.setTime(date2);
         return compareCalendar(cal1, cal2);
     }
 
     /**
      * @return 0 if cal1 and cal2 are in the same day; 1 if cal1 happens before cal2; -1 otherwise.
      */
@@ skipping 30 matching lines @@
      */
     private static AsyncTask<Void, Void, Calendar> createCalendar() {
         return new AsyncTask<Void, Void, Calendar>() {
             @Override
             protected Calendar doInBackground(Void... unused) {
                 return Calendar.getInstance();
             }
         }.execute();
     }
 }