[Download Home] Displaying offline page bundle per day

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

Index: chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java b/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java
new file mode 100644
index 0000000000000000000000000000000000000000..007d9c135ea5dccb187d8b658aa3c6fb10092075
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java
@@ -0,0 +1,199 @@
+// Copyright 2017 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 org.chromium.chrome.browser.download.ui.DownloadHistoryItemWrapper.OfflinePageItemWrapper;
+import org.chromium.chrome.browser.widget.DateDividedAdapter;
+import org.chromium.chrome.browser.widget.DateDividedAdapter.TimedItem;
+
+/**
+ * A bucket of downloaded items with the same date. It also holds the suggested offline items which
+ * are shown at the end of the list for that date.
+ */
+public class DownloadItemGroup extends DateDividedAdapter.ItemGroup {
+    private int mNumSuggestedOfflinePages;
+    private boolean mIsSuggestedOfflinePagesSectionExpanded;
+
+    /** The header representing the offline pages that are automatically downloaded */
+    private TimedItem mSuggestedOfflinePagesHeader;
+
+    public DownloadItemGroup(long timestamp) {
+        super(timestamp);
+        mNumSuggestedOfflinePages = 0;

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

ints are automatically initialized to 0.

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

Done.

+        mIsSuggestedOfflinePagesSectionExpanded = false;

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

booleans are automatically initialized to false.

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

Done.

+    }
+
+    /** @return Whether the suggested pages are expanded. */
+    public boolean isSuggestedOfflinePagesSectionExpanded() {
+        return mIsSuggestedOfflinePagesSectionExpanded;
+    }
+
+    /**
+     * Expands or collapses the suggested offline pages.
+     * @param expanded Whether the suggested pages should be shown.
+     */
+    public void setIsSuggestedOfflinePagesSectionExpanded(boolean expanded) {
+        mIsSuggestedOfflinePagesSectionExpanded = expanded;
+    }
+
+    /** @return The total number of suggested pages for this date. */
+    public int getSuggestedOfflinePageCount() {

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

Make this accessor match the variable name.

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

Done.

+        return mNumSuggestedOfflinePages;
+    }
+
+    private TimedItem getSuggestedOfflinePagesHeader() {
+        if (mSuggestedOfflinePagesHeader == null) {
+            mSuggestedOfflinePagesHeader = new TimedItem() {
+                private Long mStableId;
+
+                @Override
+                public long getTimestamp() {
+                    return getDate().getTime();
+                }
+
+                @Override
+                public long getStableId() {
+                    if (mStableId == null) {
+                        // Generate a stable ID based on timestamp.
+                        mStableId = 0xFFFFFFFF00000000L + getTimestamp() & 0x0FFFFFFFF;

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

Add parentheses around the & to make the order of ops more obvious

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

Done.

+                    }
+                    return mStableId;
+                }
+            };
+        }
+
+        return mSuggestedOfflinePagesHeader;
+    }
+
+    @Override
+    public void addItem(TimedItem item) {
+        super.addItem(item);
+        if (isSuggestedOfflinePage(item)) {
+            mNumSuggestedOfflinePages++;
+        }
+    }
+
+    @Override
+    public void removeItem(TimedItem item) {
+        super.removeItem(item);
+        if (isSuggestedOfflinePage(item)) {
+            mNumSuggestedOfflinePages--;
+        }
+    }
+
+    /**
+     * The visible items for the group are set to their respective positions in the list view.
+     * Invisible (collpased) items are skipped.
+     */
+    @Override
+    protected void setPositionForItems(int startIndex) {
+        int index = startIndex;
+        boolean firstSuggestedItem = true;

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

use "isFirstSuggestedItem"; "firstSuggestedItem" makes it sound like it's the
actual object

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

Done.

+        for (TimedItem item : mItems) {
+            if (isSuggestedOfflinePage(item)) {
+                // Skip one position for the header at the beginning of the suggested items section.
+                if (firstSuggestedItem) {
+                    index += 1;
+                    firstSuggestedItem = false;
+                }
+                if (!isSuggestedOfflinePagesSectionExpanded()) break;

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

This check only needs to be done for the first suggested item, doesn't it?  You
should be able to combine this as

if (isSuggestedOfflinePage(item) && isFirstSuggestedItem)

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

Done.

+            }
+            item.setPosition(index);
+            index += 1;
+        }
+    }
+
+    @Override
+    protected void setPositionForFirstAndLastInGroup() {
+        TimedItem first = mItems.get(0);
+        first.setIsFirstInGroup(true);
+
+        TimedItem last = mItems.get(mItems.size() - 1);
+        if (!isSuggestedOfflinePagesSectionExpanded() && mNumSuggestedOfflinePages > 0) {
+            last = getSuggestedOfflinePagesHeader();
+        }
+
+        last.setIsLastInGroup(true);
+    }
+
+    @Override
+    public int size() {
+        int size = super.size();
+
+        // The size should match the number of rows in the list view. So, adjust the size depending
+        // on whether the suggested items section is expanded or collapsed.

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

Have you considered just not adding the suggested offline pages to the
DateDividedAdapter until the group is expanded?  That would likely remove a
bunch of this special casing.  We do the same thing when we filter items, where
DownloadHistoryAdapter#filter calls DateDividedAdapter#loadItems.

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

That would still require adding the suggested page section header to the
adapter.
The header would need to be treated in a different way from the other items as
it is a different view and it needs to keep the page count too.
I am not sure if it will make it less complex.

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

It'd be consistent with how we handle filters, though.  The adapter already
stores a bunch of items that are not displayed on the list until they're needed.
 If you add items only when the groups are expanded or collapsed, then the
size() value will always be accurate, you wouldn't have to special case
setPositionForItems, you wouldn't have to track how many suggested offline pages
are in this group here, and getItemAtInternal wouldn't have to exist because the
list would be correctly numbered.

[Theresa, 2017/02/13 19:15:00]

+1 -- I'd also be interested in seeing if this can be done with filtering. We
may be able to hook into DownloadHistoryItemWrapper#isVisibleToUser() and || in
whether the offline page header associated with the suggested offline page is
expanded or not. There may be some pitfalls here I'm not considering, so it'd
probably be good to start with a light-weight (~1 hour) investigation of the
feasibility.

+        if (mNumSuggestedOfflinePages > 0) {
+            // First item of the suggested pages section is the section header.
+            size += 1;
+            if (!isSuggestedOfflinePagesSectionExpanded()) {
+                size -= mNumSuggestedOfflinePages;
+            }
+        }
+
+        return size;
+    }
+
+    /** @return The total file size of the suggested pages for this date. */
+    public long getSuggestedOfflinePagesFileSize() {
+        long size = 0;
+        for (TimedItem item : mItems) {
+            if (isSuggestedOfflinePage(item)) {
+                size += ((DownloadHistoryItemWrapper) item).getFileSize();
+            }
+        }
+        return size;
+    }
+
+    private int getSuggestedOfflinePagesSectionHeaderPosition() {
+        return mItems.size() - mNumSuggestedOfflinePages + 1;
+    }
+
+    private boolean isSuggestedOfflinePagesSectionHeader(int index) {
+        return index == getSuggestedOfflinePagesSectionHeaderPosition()
+                && mNumSuggestedOfflinePages > 0;
+    }
+
+    @Override
+    protected TimedItem getItemAtInner(int index) {
+        // The suggested pages start at the end of the group.
+        // So, the ordering of the items in this list is :
+        // date header, regular items, suggested pages header, suggested pages (if expanded).
+        int suggestedSectionHeaderIndex = getSuggestedOfflinePagesSectionHeaderPosition();
+        if (index < suggestedSectionHeaderIndex) {
+            return mItems.get(index - 1);

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

nit: would it make sense for this to be return super.getItemInternal(); ?

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

hmm... I think, it seems more readable to keep this here with the above
comments.

+        } else if (index == suggestedSectionHeaderIndex) {
+            assert mNumSuggestedOfflinePages > 0;
+            return getSuggestedOfflinePagesHeader();
+        } else {
+            assert isSuggestedOfflinePagesSectionExpanded();
+            return mItems.get(index - 2);
+        }
+    }
+
+    @Override
+    public int getItemViewType(int position) {
+        if (isSuggestedOfflinePagesSectionHeader(position)) {
+            return DateDividedAdapter.TYPE_SUBSECTION_HEADER;
+        } else {
+            return super.getItemViewType(position);
+        }
+    }
+
+    @Override
+    public int compareItem(TimedItem lhs, TimedItem rhs) {
+        if (isSuggestedOfflinePage(lhs) && !isSuggestedOfflinePage(rhs)) return 1;
+        if (!isSuggestedOfflinePage(lhs) && isSuggestedOfflinePage(rhs)) return -1;
+
+        return super.compareItem(lhs, rhs);
+    }
+
+    private boolean isSuggestedOfflinePage(TimedItem timedItem) {
+        if (timedItem instanceof OfflinePageItemWrapper) {
+            return ((OfflinePageItemWrapper) timedItem).isSuggested();
+        }
+
+        return false;
+    }
+}