[Download Home] Displaying offline page bundle per day

chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java

Index: chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
index d0229505dd0382bcff8dc3a7609d4723119487bd..845d5ecbd89004363a7eb657e4c71014ccde51fb 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
@@ -28,7 +28,11 @@ import org.chromium.chrome.browser.widget.selection.SelectionDelegate;
 import org.chromium.content_public.browser.DownloadState;
 
 import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 /** Bridges the user's download history and the UI used to display it. */
@@ -57,6 +61,42 @@ public class DownloadHistoryAdapter extends DateDividedAdapter
         }
     }
 
+    /** Represents the subsection header of the suggested pages for a given date */

[gone, 2017/02/15 23:58:09]

end with a period

[shaktisahu, 2017/02/16 06:07:06]

Done.

+    public class SubsectionHeader extends TimedItem {
+        private Long mStableId;
+        private Date mDate;
+        private int mItemCount;
+        private long mTotalFileSize;

[gone, 2017/02/15 23:58:09]

Everything here should be final.  Stable ID can be calculated in the constructor
instead of on the fly.

[shaktisahu, 2017/02/16 06:07:06]

Done.

+
+        public SubsectionHeader(Date date, int itemCount, long totalFileSize) {
+            mDate = date;
+            mItemCount = itemCount;
+            mTotalFileSize = totalFileSize;
+        }
+
+        @Override
+        public long getTimestamp() {
+            return mDate.getTime();
+        }
+
+        public int getItemCount() {
+            return mItemCount;
+        }
+
+        public long getTotalFileSize() {
+            return mTotalFileSize;
+        }
+
+        @Override
+        public long getStableId() {
+            if (mStableId == null) {
+                // Generate a stable ID based on timestamp.
+                mStableId = 0xFFFFFFFF00000000L + (getTimestamp() & 0x0FFFFFFFF);
+            }
+            return mStableId;
+        }
+    }
+
     /**
      * Tracks externally deleted items that have been removed from downloads history.
      * Shared across instances.
@@ -69,6 +109,8 @@ public class DownloadHistoryAdapter extends DateDividedAdapter
     private final BackendItems mIncognitoDownloadItems = new BackendItemsImpl();
     private final BackendItems mOfflinePageItems = new BackendItemsImpl();
 
+    private final Map<Date, Boolean> mSuggestedSectionExpanded = new HashMap<>();

[gone, 2017/02/15 23:58:09]

Don't arbitrarily separate the BackendItemsImpls().  Move it above
mParentComponent.

[shaktisahu, 2017/02/16 06:07:06]

Done.

+
     private final BackendItems mFilteredItems = new BackendItemsImpl();
     private final FilePathsToDownloadItemsMap mFilePathsToItemsMap =
             new FilePathsToDownloadItemsMap();
@@ -208,6 +250,25 @@ public class DownloadHistoryAdapter extends DateDividedAdapter
     }
 
     @Override
+    protected SubsectionHeaderViewHolder createSubsectionHeader(ViewGroup parent) {
+        OfflineGroupHeaderView offlineHeader =
+                (OfflineGroupHeaderView) LayoutInflater.from(parent.getContext())
+                        .inflate(R.layout.offline_download_header, parent, false);

[gone, 2017/02/15 23:58:09]

Can you clarify what you're expecting here?  inflate() returns the root of the
view hierarchy, which is |parent| because you're passing it in.  Is |parent|
actually a OfflineGroupHeaderView?  If it is, can this be written more clearly?

[shaktisahu, 2017/02/16 06:07:06]

The third param attachToRoot is false here. The return value of inflate() is the
root of the inflated XML file. So it wouldn't matter if we pass |parent| or null
as the second param to inflate(). But as per documentation, if provided, it
provides a set of LayoutParams for the root of the returned hierarchy. 

[gone, 2017/02/17 19:21:16]

Acknowledged.  Misread the params list :/

+        offlineHeader.setAdapter(this);
+        return new SubsectionHeaderViewHolder(offlineHeader);
+    }
+
+    @Override
+    protected void bindViewHolderForSubsectionHeader(
+            SubsectionHeaderViewHolder holder, TimedItem timedItem) {
+        SubsectionHeader headerItem = (SubsectionHeader) timedItem;
+        Date date = new Date(headerItem.getTimestamp());
+        OfflineGroupHeaderView headerView = (OfflineGroupHeaderView) holder.getView();
+        headerView.updateView(date, isSubsectionExpanded(date), headerItem.getItemCount(),
+                headerItem.getTotalFileSize());
+    }
+
+    @Override
     public ViewHolder createViewHolder(ViewGroup parent) {
         DownloadItemView v = (DownloadItemView) LayoutInflater.from(parent.getContext()).inflate(
                 R.layout.download_item_view, parent, false);
@@ -224,6 +285,11 @@ public class DownloadHistoryAdapter extends DateDividedAdapter
         holder.getItemView().displayItem(mBackendProvider, item);
     }
 
+    @Override
+    protected ItemGroup createGroup(long timeStamp) {
+        return new DownloadItemGroup(timeStamp);
+    }
+
     /** Called when a new DownloadItem has been created by the native DownloadManager. */
     public void onDownloadItemCreated(DownloadItem item) {
         boolean isOffTheRecord = item.getDownloadInfo().isOffTheRecord();
@@ -395,9 +461,74 @@ public class DownloadHistoryAdapter extends DateDividedAdapter
         mFilteredItems.clear();
         mRegularDownloadItems.filter(mFilter, mSearchQuery, mFilteredItems);
         mIncognitoDownloadItems.filter(mFilter, mSearchQuery, mFilteredItems);
-        mOfflinePageItems.filter(mFilter, mSearchQuery, mFilteredItems);
+
+        Map<Date, Integer> suggestedPageCount = new HashMap<>();

[gone, 2017/02/15 23:58:09]

1) Pull this whole new block out to some private function.  Putting this inline
makes the basic flow of the filter() function painful to follow.

2) "Count" makes it sound like this is a single integer.  Call these maps
"suggestedPageCountMaps" e.g.

[shaktisahu, 2017/02/16 06:07:06]

Done.

+        Map<Date, Long> suggestedPageTotalSize = new HashMap<>();
+
+        BackendItems offlinePageItems = new BackendItemsImpl();

[gone, 2017/02/15 23:58:09]

filteredOfflinePageItems

[shaktisahu, 2017/02/16 06:07:06]

Done.

+        mOfflinePageItems.filter(mFilter, mSearchQuery, offlinePageItems);
+
+        // Add the suggested pages to the adapter only if the section is expanded for that date.

[gone, 2017/02/15 23:58:09]

This comment only applies to the inner if statement.  What you're really doing
is filtering all the offline pages with the for loop.  Update the comments.

[shaktisahu, 2017/02/16 06:07:06]

Done.

+        for (DownloadHistoryItemWrapper item : offlinePageItems) {
+            OfflinePageItemWrapper offlineItem = (OfflinePageItemWrapper) item;
+            if (offlineItem.isSuggested()) {
+                incrementSuggestedPageCount(
+                        offlineItem, suggestedPageCount, suggestedPageTotalSize);
+                if (!isSubsectionExpanded(getDateWithoutTime(offlineItem.getTimestamp()))) continue;
+            }
+            mFilteredItems.add(offlineItem);
+        }
+
+        List<TimedItem> itemsToLoad = new ArrayList<>();
+        itemsToLoad.addAll(mFilteredItems);
+
+        // Add a header suggested pages for each date

[gone, 2017/02/15 23:58:09]

// Add a TimedItem for each subsection.

[shaktisahu, 2017/02/16 06:07:06]

Done.

+        for (Date date : suggestedPageCount.keySet()) {
+            itemsToLoad.add(new SubsectionHeader(
+                    date, suggestedPageCount.get(date), suggestedPageTotalSize.get(date)));
+        }
+
+        clear(false);
+        loadItems(itemsToLoad);
+    }
+
+    // Updates the total number of suggested pages and file size grouped by date.
+    private void incrementSuggestedPageCount(OfflinePageItemWrapper offlineItem,
+            Map<Date, Integer> suggestedPageCount, Map<Date, Long> suggestedPageTotalSize) {
+        Date date = getDateWithoutTime(offlineItem.getTimestamp());
+
+        Integer count = suggestedPageCount.get(date);
+        if (count == null) count = 0;

[gone, 2017/02/15 23:58:09]

Avoid Integers if you don't need to use them.  They're another annoying vector
for null pointer exceptions.

Try:
int count = suggestedPageCount.containsKey(date) ? suggestedPageCount.get(date)
: 0;

[shaktisahu, 2017/02/16 06:07:06]

Done.

+        count++;
+        suggestedPageCount.put(date, count);

[gone, 2017/02/15 23:58:09]

suggestedPageCount.put(date, count + 1);

[shaktisahu, 2017/02/16 06:07:06]

Done.

+
+        Long fileSize = suggestedPageTotalSize.get(date);
+        if (fileSize == null) fileSize = 0L;
+        fileSize += offlineItem.getFileSize();
+        suggestedPageTotalSize.put(date, fileSize);

[gone, 2017/02/15 23:58:09]

Same comments as above.

[shaktisahu, 2017/02/16 06:07:06]

Done.

+    }
+
+    /**
+     * Whether the suggested pages section is expanded for a given date.
+     * @param date The download date.
+     * @return Whether the suggested pages section is expanded.
+     */
+    public boolean isSubsectionExpanded(Date date) {
+        // Default state is collpased.
+        if (mSuggestedSectionExpanded.get(date) == null) return false;
+
+        return mSuggestedSectionExpanded.get(date);
+    }
+
+    public void setSubsectionExpanded(Date date, boolean expanded) {

[gone, 2017/02/15 23:58:09]

Javadoc.

[shaktisahu, 2017/02/16 06:07:06]

Done.

+        mSuggestedSectionExpanded.put(date, expanded);

[gone, 2017/02/15 23:58:09]

Be consistent about what you're calling this thing.  It flips between "suggested
section" and "subsection" between fields and comments.

[shaktisahu, 2017/02/16 06:07:06]

Done.

         clear(false);
-        loadItems(mFilteredItems);
+        filter(mFilter);
+    }
+
+    @Override
+    protected boolean isSubsectionHeader(TimedItem timedItem) {
+        return timedItem instanceof SubsectionHeader;
     }
 
     private void initializeOfflinePageBridge() {
@@ -477,4 +608,14 @@ public class DownloadHistoryAdapter extends DateDividedAdapter
         RecordHistogram.recordCountHistogram("Android.DownloadManager.InitialCount.Total",
                 mRegularDownloadItems.size() + mOfflinePageItems.size());
     }
+
+    private Date getDateWithoutTime(long timestamp) {

[gone, 2017/02/15 23:58:09]

/** Calculates the {@link Date} for midnight of the date represented by the
timestamp. */

[shaktisahu, 2017/02/16 06:07:06]

Done.

+        Calendar cal = Calendar.getInstance();
+        cal.setTimeInMillis(timestamp);
+        cal.set(Calendar.HOUR_OF_DAY, 0);
+        cal.set(Calendar.MINUTE, 0);
+        cal.set(Calendar.SECOND, 0);
+        cal.set(Calendar.MILLISECOND, 0);
+        return cal.getTime();
+    }
 }