[Download Home] Displaying offline page bundle per day

chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.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.download.ui;
 
 import android.content.ComponentName;
 import android.support.v7.widget.RecyclerView.ViewHolder;
 import android.text.TextUtils;
 import android.view.LayoutInflater;
@@ skipping 10 matching lines @@
 import org.chromium.chrome.browser.download.ui.DownloadHistoryItemWrapper.DownloadItemWrapper;
 import org.chromium.chrome.browser.download.ui.DownloadHistoryItemWrapper.OfflinePageItemWrapper;
 import org.chromium.chrome.browser.download.ui.DownloadManagerUi.DownloadUiObserver;
 import org.chromium.chrome.browser.offlinepages.downloads.OfflinePageDownloadBridge;
 import org.chromium.chrome.browser.offlinepages.downloads.OfflinePageDownloadItem;
 import org.chromium.chrome.browser.widget.DateDividedAdapter;
 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. */
 public class DownloadHistoryAdapter extends DateDividedAdapter
         implements DownloadUiObserver, DownloadSharedPreferenceHelper.Observer {
 
     /** Alerted about changes to internal state. */
     static interface TestObserver {
         abstract void onDownloadItemCreated(DownloadItem item);
         abstract void onDownloadItemUpdated(DownloadItem item);
     }
 
     private class BackendItemsImpl extends BackendItems {
         @Override
         public DownloadHistoryItemWrapper removeItem(String guid) {
             DownloadHistoryItemWrapper wrapper = super.removeItem(guid);
 
             if (wrapper != null) {
                 mFilePathsToItemsMap.removeItem(wrapper);
                 if (getSelectionDelegate().isItemSelected(wrapper)) {
                     getSelectionDelegate().toggleSelectionForItem(wrapper);
                 }
             }
 
             return wrapper;
         }
     }
 
+    /** 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.
      */
     private static final DeletedFileTracker sDeletedFileTracker = new DeletedFileTracker();
 
     private static final String EMPTY_QUERY = null;
 
     private final BackendItems mRegularDownloadItems = new BackendItemsImpl();
     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();
 
     private final ComponentName mParentComponent;
     private final boolean mShowOffTheRecord;
     private final LoadingStateDelegate mLoadingDelegate;
     private final ObserverList<TestObserver> mObservers = new ObserverList<>();
     private final List<DownloadItemView> mViews = new ArrayList<>();
 
@@ skipping 119 matching lines @@
         totalSize += mOfflinePageItems.getTotalBytes();
         return totalSize;
     }
 
     @Override
     protected int getTimedItemViewResId() {
         return R.layout.date_view;
     }
 
     @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);
         v.setSelectionDelegate(getSelectionDelegate());
         mViews.add(v);
         return new DownloadHistoryItemViewHolder(v);
     }
 
     @Override
     public void bindViewHolderForTimedItem(ViewHolder current, TimedItem timedItem) {
         final DownloadHistoryItemWrapper item = (DownloadHistoryItemWrapper) timedItem;
 
         DownloadHistoryItemViewHolder holder = (DownloadHistoryItemViewHolder) current;
         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();
         if (isOffTheRecord && !mShowOffTheRecord) return;
 
         BackendItems list = getDownloadItemList(isOffTheRecord);
         assert list.findItemIndex(item.getId()) == BackendItems.INVALID_INDEX;
 
         DownloadItemWrapper wrapper = createDownloadItemWrapper(item);
         boolean wasAdded = addDownloadHistoryItemWrapper(wrapper);
@@ skipping 151 matching lines @@
     private SelectionDelegate<DownloadHistoryItemWrapper> getSelectionDelegate() {
         return mBackendProvider.getSelectionDelegate();
     }
 
     /** Filters the list of downloads to show only files of a specific type. */
     private void filter(int filterType) {
         mFilter = filterType;
         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(mFilteredItems);
+        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);
+        filter(mFilter);
+    }
+
+    @Override
+    protected boolean isSubsectionHeader(TimedItem timedItem) {
+        return timedItem instanceof SubsectionHeader;
     }
 
     private void initializeOfflinePageBridge() {
         mOfflinePageObserver = new OfflinePageDownloadBridge.Observer() {
             @Override
             public void onItemsLoaded() {
                 onAllOfflinePagesRetrieved(getOfflinePageBridge().getAllItems());
             }
 
             @Override
@@ skipping 59 matching lines @@
         RecordHistogram.recordCountHistogram("Android.DownloadManager.InitialCount.Video",
                 itemCounts[DownloadFilter.FILTER_VIDEO]);
     }
 
     private void recordTotalDownloadCountHistogram() {
         // The total count intentionally leaves out incognito downloads. This should be revisited
         // if/when incognito downloads are persistently available in downloads home.
         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();
+    }
 }