[Download Home] Displaying offline page bundle per day

chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.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 org.chromium.chrome.browser.widget.DateDividedAdapter;
+import org.chromium.chrome.browser.widget.DateDividedAdapter.TimedItem;
+
+/**
+ * A bucket of downloaded items with the same date.
+ */
+public class DownloadItemGroup extends DateDividedAdapter.ItemGroup {

[gone, 2017/02/02 23:59:48]

This is confusing because the user can download offline pages manually that
aren't bucketed into your new group.  You need to be explicitly clear that these
are for automatically downloaded ones throughout the code.  Consider creating an
inner class that encapsulates all of your new logic, then making the
DownloadItemGroup own an instance.

[shaktisahu, 2017/02/04 18:57:42]

Okay. I renamed everything with *OfflinePage* here to *SuggestedOfflinePage*.

+    private int mNumOfflinePages;

[gone, 2017/02/02 23:59:48]

mNumSuggestedOfflinePages or mNumAutomaticallyDownloadedOfflinePages

[shaktisahu, 2017/02/04 18:57:42]

Done.

+    private boolean mOfflinePagesExpanded;

[gone, 2017/02/02 23:59:48]

mIsSuggestedOfflinePagesSectionExpanded / isSuggestedOfflinePagesSectionExpanded
/ setIsSuggestedOfflinePagesSectionExpanded

[shaktisahu, 2017/02/04 18:57:42]

Done.

+
+    // The header for the group containing offline pages.

[gone, 2017/02/02 23:59:48]

1) Don't say there's a "group" inside a "group".
2) Use single line javadoc notation instead of this for individual fields.

/** The header representing the Offline Pages that are automatically downloaded.
*/

[shaktisahu, 2017/02/04 18:57:42]

Done.

+    private TimedItem mOfflineHeader;
+
+    public DownloadItemGroup(long timestamp) {
+        super(timestamp);
+        mNumOfflinePages = 0;
+        mOfflinePagesExpanded = false;
+
+        mOfflineHeader = new TimedItem() {

[gone, 2017/02/02 23:59:47]

This should only be created when it's actually necessary.  The likelihood of
there being a bucket every single day is highly unlikely and wastes memory.

[shaktisahu, 2017/02/04 18:57:42]

Done.

+            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 = getTimestamp() & 0x0FFFFFFFF;
+                    mStableId = (mStableId << 32) + ~(getTimestamp() & 0x0FFFFFFFF);

[gone, 2017/02/02 23:59:47]

This should be more consistent with the stable IDs generated for everything
else. 

Date headers get a header based on their date:
(day of year << 16) + year

Download items get a header based on their timestamp and their ID:
(hash of download guid << 32) + (lower 32 bits of download timestamp)

Given that this header is meant to be in the list of download items, maybe this
would
make more sense:
(0xffffffff00000000) + (lower 32 bits of download timestamp)

[shaktisahu, 2017/02/04 18:57:42]

Done.

+                }
+                return mStableId;
+            }
+        };
+    }
+
+    public boolean offlinePageExpanded() {
+        return mOfflinePagesExpanded;
+    }
+
+    public void setOfflinePageExpanded(boolean expanded) {
+        mOfflinePagesExpanded = expanded;
+    }
+
+    public int getOfflinePageCount() {
+        return mNumOfflinePages;
+    }
+
+    @Override
+    public void addItem(TimedItem item) {
+        super.addItem(item);
+        if (item.isOfflinePage()) {
+            mNumOfflinePages++;
+        }
+    }
+
+    @Override
+    public void removeItem(TimedItem item) {
+        super.removeItem(item);
+        if (item.isOfflinePage()) {
+            mNumOfflinePages--;
+        }
+    }
+
+    @Override
+    protected void setPositionForItems(int startIndex) {
+        int index = startIndex;
+        boolean firstOfflineItem = false;
+        for (TimedItem item : mItems) {
+            if (item.isOfflinePage()) {
+                if (!firstOfflineItem) {
+                    index += 1;
+                    firstOfflineItem = true;
+                }
+                if (!offlinePageExpanded()) continue;
+            }
+            item.setPosition(index);
+            index += 1;
+        }
+    }
+
+    @Override
+    public int size() {
+        int size = super.size();
+
+        if (mNumOfflinePages > 0) {
+            size += 1;
+            if (!offlinePageExpanded()) {
+                size -= mNumOfflinePages;
+            }
+        }
+
+        return size;
+    }
+
+    public long getOfflinePagesFileSize() {
+        long size = 0;
+        for (TimedItem item : mItems) {
+            if (item.isOfflinePage()) {
+                size += ((DownloadHistoryItemWrapper) item).getFileSize();
+            }
+        }
+        return size;
+    }
+
+    public boolean isOfflineHeader(int index) {

[gone, 2017/02/02 23:59:47]

This is used only here.  Make it private.

[shaktisahu, 2017/02/04 18:57:42]

Done.

+        return index == (mItems.size() - mNumOfflinePages + 1) && mNumOfflinePages > 0;
+    }
+
+    @Override
+    public TimedItem getItemAt(int index) {
+        // 0 is allocated to the date header. The list header has no items.
+        if (index == 0) return null;
+
+        sortIfNeeded();
+
+        // If the offline pages would start at the beginning of the group
+        /*if (mNumOfflinePages > 0) {
+            if (index == 1) return mOfflineHeader;
+
+            if (offlinePageExpanded()) {
+                return mItems.get(index - 2);
+            } else {
+                int newIndex = index - 2 + mNumOfflinePages;
+                return mItems.get(newIndex);
+            }
+        } else {
+            return mItems.get(index - 1);
+        }*/
+
+        // Offline pages start at the end of the group.
+        if (index <= mItems.size() - mNumOfflinePages) {
+            return mItems.get(index - 1);
+        } else {
+            if (mNumOfflinePages > 0 && index == mItems.size() - mNumOfflinePages + 1) {
+                return mOfflineHeader;
+            } else if (offlinePageExpanded()) {
+                return mItems.get(index - 2);
+            } else {
+                System.out.println("shakti, index " + index + " total " + mItems.size()
+                        + " offline " + mNumOfflinePages + " expanded " + offlinePageExpanded());
+                assert false;
+                return null;
+            }
+        }
+    }
+
+    @Override
+    public int getItemViewType(int position) {
+        if (isOfflineHeader(position)) {
+            return DateDividedAdapter.TYPE_OFFLINE_HEADER;
+        } else {
+            return DateDividedAdapter.TYPE_NORMAL;
+        }
+    }
+
+    @Override
+    public int compareItem(TimedItem lhs, TimedItem rhs) {
+        if (lhs.isOfflinePage() && !rhs.isOfflinePage()) return 1;
+        if (!lhs.isOfflinePage() && rhs.isOfflinePage()) return -1;
+
+        return super.compareItem(lhs, rhs);
+    }
+}