Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701253006:
Download Home : Suggested pages header selection (Closed)
|
Created:
3 years, 10 months ago by shaktisahu Modified:
3 years, 9 months ago CC:
chromium-reviews, asanka, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDownload Home : Suggested pages header selection
This CL enables user to select the suggested page section header.
Details:
- Long press enables selection mode. Tap during selection will toggle the
selection, but doesn't expand/collapse the items.
- Selecting the header internally selects all the related items.
- If all the items are selected manually, the header gets selected automatically.
- Deselecting any item will deselect the corresponding header if previously selected.
- The badging at the top reflects the number of items currently selected excluding the headers.
- When selected, the chrome logo on the header turns into tick mark similar to rest of the items.
- In presence of search text, headers are not displayed (no grouping),
but once the text is cleared out, the headers will be back in.
TODO: Hide the date headers when there is an active search.
BUG=689801
Review-Url: https://codereview.chromium.org/2701253006
Cr-Commit-Position: refs/heads/master@{#456001}
Committed: https://chromium.googlesource.com/chromium/src/+/ba3f171d0f3e4300341ba8a719f4f98f61af0165
Patch Set 1 #Patch Set 2 : talo@ feedback #
Total comments: 11
Patch Set 3 : More refactor #
Total comments: 26
Patch Set 4 : nits and fixed background color #
Total comments: 7
Patch Set 5 : nits #
Total comments: 3
Patch Set 6 : Introduced SubsectionHeaderSelectionObserver #
Total comments: 3
Patch Set 7 : nits #
Total comments: 1
Patch Set 8 : rebase #Patch Set 9 : Fixed tests #Messages
Total messages: 44 (20 generated)
Description was changed from ========== Download Home : Working version of header selection Used DownloadSelectionDelegate, TODO: Find a nice way to set it on BackendProvider (private class) Find a nice way to get the items associated with the header without going through all Find a nice way to add the observer? Find a nice way to resolve the issue of SelectionDelegate<DownloadHistoryItemWrapper> vs DownloadSelectionDelegate subclass Change to mSubsectionHeaders from boolean map BUG= ========== to ========== Download Home : Suggested pages header selection This CL enables user to select the suggested page section header. Details: - Long click enables selection mode. - Selecting the header internally selects all the related items. - If all the items are selected manually, the header gets selected automatically and vice versa. - The badging at the top reflects the number of items currently selected excluding the headers. - In presence of search text, headers are not displayed (no grouping). BUG=689801 ==========
Description was changed from ========== Download Home : Suggested pages header selection This CL enables user to select the suggested page section header. Details: - Long click enables selection mode. - Selecting the header internally selects all the related items. - If all the items are selected manually, the header gets selected automatically and vice versa. - The badging at the top reflects the number of items currently selected excluding the headers. - In presence of search text, headers are not displayed (no grouping). BUG=689801 ========== to ========== Download Home : Suggested pages header selection This CL enables user to select the suggested page section header. Details: - Long click enables selection mode. - Selecting the header internally selects all the related items. - If all the items are selected manually, the header gets selected automatically and vice versa. - The badging at the top reflects the number of items currently selected excluding the headers. - In presence of search text, headers are not displayed (no grouping). BUG=689801 ==========
Description was changed from ========== Download Home : Suggested pages header selection This CL enables user to select the suggested page section header. Details: - Long click enables selection mode. - Selecting the header internally selects all the related items. - If all the items are selected manually, the header gets selected automatically and vice versa. - The badging at the top reflects the number of items currently selected excluding the headers. - In presence of search text, headers are not displayed (no grouping). BUG=689801 ========== to ========== Download Home : Suggested pages header selection This CL enables user to select the suggested page section header. Details: - Long press enables selection mode. Tap during selection will toggle the selection, but doesn't expand/collapse the items. - Selecting the header internally selects all the related items. - If all the items are selected manually, the header gets selected automatically. - Deselecting any item will deselect the corresponding header if previously selected. - The badging at the top reflects the number of items currently selected excluding the headers. - When selected, the chrome logo on the header turns into tick mark similar to rest of the items. - In presence of search text, headers are not displayed (no grouping), but once the text is cleared out, the headers will be back in. TODO: Hide the date headers when there is an active search. BUG=689801 ==========
shaktisahu@chromium.org changed reviewers: + dfalcantara@chromium.org, twellington@chromium.org
PTAL. The CL is almost ready. Would like some feedback on the SelectionDelegate subclassing. (see my comments below). https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:158: public void setSelectionDelegate(DownloadItemSelectionDelegate delegate) { I had to replicate/override some of the functions of the SelectableItemView and SelectionDelegate since almost every class in the current code assumes of type <DownloadHistoryItemWrapper> while headers have to deal with <TimedItem> and conversions are not possible. Let me know if you think of a better way.
Would a setup like this work: 1. OfflineGroupHeaderView extends SelectableItemView so that it can re-use logic for showing highlight view. It would need some of the logic you implemented w/ setSelectionDelegate() so that it can correctly determine how to respond to a click. 2. When the header view's selection is toggled, it can rely on the new #getSuggestedItemsForDate() method and toggle selection for all of its sub-items. 3. When a sub-item's selection is toggled, the header view checks if any of its sub-items are selected to determine whether its selected state. This would hopefully make the additional complexity of selecting sub-sections or sub-section items more isolated in OfflineGroupHeaderView. https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:110: this.mIsExpanded = isExpanded; nit: "this." isn't necessary. https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:132: return !mSelectedHeaders.isEmpty() || super.isSelectionEnabled(); If any header is selected, doesn't that mean it's sub-items will be in the list of selected items the super class is maintaining? If so, this override doesn't seem necessary. https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:87: mSelectionDelegate = new SelectionDelegate<DownloadHistoryItemWrapper>(); Could the new DownloadItemSelectionDelegate be created here? https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:81: return true; This should still be returning false, right? https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:411: return group; nit: inline this: if (group.isSameDay(date)) return group;
On 2017/03/03 18:35:50, Theresa wrote: > Would a setup like this work: > > 1. OfflineGroupHeaderView extends SelectableItemView so that it can re-use logic > for showing highlight view. It would need some of the logic you implemented w/ > setSelectionDelegate() so that it can correctly determine how to respond to a > click. > 2. When the header view's selection is toggled, it can rely on the new > #getSuggestedItemsForDate() method and toggle selection for all of its > sub-items. > 3. When a sub-item's selection is toggled, the header view checks if any of its > sub-items are selected to determine whether its selected state. > > This would hopefully make the additional complexity of selecting sub-sections or > sub-section items more isolated in OfflineGroupHeaderView. > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java > (right): > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:110: > this.mIsExpanded = isExpanded; > nit: "this." isn't necessary. > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:132: > return !mSelectedHeaders.isEmpty() || super.isSelectionEnabled(); > If any header is selected, doesn't that mean it's sub-items will be in the list > of selected items the super class is maintaining? If so, this override doesn't > seem necessary. > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java > (right): > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:87: > mSelectionDelegate = new SelectionDelegate<DownloadHistoryItemWrapper>(); > Could the new DownloadItemSelectionDelegate be created here? > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java > (right): > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:81: > return true; > This should still be returning false, right? > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java > (right): > > https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:411: > return group; > nit: inline this: > > if (group.isSameDay(date)) return group; OfflineGroupHeaderView extends SelectableItemView<TimedItem> But I am not invoking the setters for the |mItem| or |mSelectionDelegate| for SelectableItemView at all, so these values are left to null. The reason is that SelectableItemView<DownloadHistoryItemWrapper> and SelectableItemView<TimedItem> are not inter-convertible and so is the case in SelectionDelegate<E>. So I had even though OfflineGroupHeaderView is a subclass, it still has to reimplement some of the methods of SelectableItemView, which makes it quite confusing. I was looking for if we could change all the <DownloadHistoryItemWrapper> to <TimedItem> in the UI code, but that will cause more issues than this.
Patchset #5 (id:80001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
I moved DownloadItemSelectionDelegate to a separate file which would be created during the construction of BackendProvider in DownloadManagerUi. Also I add a member to the SubsectionHeader class to keep the list of suggested items for that date. I think that looks better. PTAL. https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:110: this.mIsExpanded = isExpanded; On 2017/03/03 18:35:50, Theresa wrote: > nit: "this." isn't necessary. Done. https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:132: return !mSelectedHeaders.isEmpty() || super.isSelectionEnabled(); On 2017/03/03 18:35:50, Theresa wrote: > If any header is selected, doesn't that mean it's sub-items will be in the list > of selected items the super class is maintaining? If so, this override doesn't > seem necessary. Actually, this might be redundant in the current mode of operation, however since this class is responsible for keeping the selected state of headers in addition to other items (which is done in the superclass), I am trying to keep it consistent in clearSelection(), isItemSelected() methods. I feel this is more preferable, but we will see how the final code looks like. https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:87: mSelectionDelegate = new SelectionDelegate<DownloadHistoryItemWrapper>(); On 2017/03/03 18:35:50, Theresa wrote: > Could the new DownloadItemSelectionDelegate be created here? Done. https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:81: return true; On 2017/03/03 18:35:50, Theresa wrote: > This should still be returning false, right? Yes. I left it just for testing :) I won't check in this file. https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2701253006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:411: return group; On 2017/03/03 18:35:50, Theresa wrote: > nit: inline this: > > if (group.isSameDay(date)) return group; Method removed.
Thank you for splitting the DonwloadItemSelectionDelegate into a separate class -- this feels much better! Either in this CL or a follow up, can we add some tests to DownloadHistoryAdapterTest and DownloadActivityTest to check some of the basic functionality surrounding offline page headers (e.g. toggling selection, deleting selected items, expanding/collapsing, sections get correctly updated when new items are added, searching). There is some pretty significant functionality built up around the subsection headers now. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:114: * Helper method to set the properties of this class. nit: This is only setting the list of suggested pages, so this should be phrased: Helper method to set the suggested pages for this subsection. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:14: * Separately maintains the selected state of the headers in addition to the other selected items. nit: s/headers/{@link SubsectionHeader}s https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:40: * Toggles selection for a given subsection and sets the associated items to correct selection nit: to the correct selection state. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:57: * Sets the selection state for a given header. Doesn't affect the associated items. nit: I would note when this method is supposed to be called as opposed to toggleSelectionForSubsection. If I understand correctly, toggleSelectionForSubsection is called when the user long-presses on the subsection header itself and setSelectionForHeader is called to update the selection state for a header when one if its items changes selection state. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:105: * @param header The associated header. nit: s/header/{@link SubsectionHeader} https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:157: public void setSelectionDelegate(DownloadItemSelectionDelegate delegate) { Please add some comments about SelectionDelegates expecting all items to be of the same type, DownloadItemSelectionDelegate dealing with DownloadHistoryItemWrappers and this being a SelectableItemView<TimedItem>, etc. so that it's more obvious without tracing through the code why this overrides some of the super class methods.
Can you upload a movie of this in action? I'd normally ask for screenshots for layout, but this is a behavioral change. Run "adb shell screenrecord /sdcard/demo.mp4" before starting to play with Download Home. Once you're done, hit Ctrl+C and then "adb pull /sdcard/demo.mp4" to get the video off your device.
On 2017/03/07 21:10:29, dfalcantara (load balance plz) wrote: > Can you upload a movie of this in action? I'd normally ask for screenshots for > layout, but this is a behavioral change. > > Run "adb shell screenrecord /sdcard/demo.mp4" before starting to play with > Download Home. Once you're done, hit Ctrl+C and then "adb pull > /sdcard/demo.mp4" to get the video off your device. Video uploaded in the bug. Thanks!
On 2017/03/07 21:48:14, shaktisahu wrote: > On 2017/03/07 21:10:29, dfalcantara (load balance plz) wrote: > > Can you upload a movie of this in action? I'd normally ask for screenshots > for > > layout, but this is a behavioral change. > > > > Run "adb shell screenrecord /sdcard/demo.mp4" before starting to play with > > Download Home. Once you're done, hit Ctrl+C and then "adb pull > > /sdcard/demo.mp4" to get the video off your device. > > Video uploaded in the bug. Thanks! The background color doesn't match the rest of the items in the list; can you fix that?
https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:67: public class SubsectionHeader extends TimedItem { Does this class rely on anything inside the parent class? Should it be a private static class? https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:69: private List<DownloadHistoryItemWrapper> mSuggestedPages; You should be more generic in case this pattern expands. mSubsectionItems/subsectionItems? https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:491: } else { Add a comment here about how this shows suggested offline pages directly, outside of their subsections. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:17: private Set<SubsectionHeader> mSelectedHeaders = new HashSet<>(); private final https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:20: public boolean isSelectionEnabled() { This function should probably be renamed to isSelectionModeActive() or something. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:42: * @param header The given header. "The given header" doesn't add any useful info. "Header for the subsection being toggled"? https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:40: private final ColorStateList mIconForegroundColorList; private finals above the other ones
Fixed the background color issue. PTAL https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:67: public class SubsectionHeader extends TimedItem { On 2017/03/07 23:55:16, dfalcantara (load balance plz) wrote: > Does this class rely on anything inside the parent class? Should it be a > private static class? No, I can make it protected static, it is accessed from OfflineGroupHeaderView class too. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:69: private List<DownloadHistoryItemWrapper> mSuggestedPages; On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > You should be more generic in case this pattern expands. > mSubsectionItems/subsectionItems? Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:114: * Helper method to set the properties of this class. On 2017/03/06 17:10:25, Theresa wrote: > nit: This is only setting the list of suggested pages, so this should be > phrased: > > Helper method to set the suggested pages for this subsection. Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:491: } else { On 2017/03/07 23:55:16, dfalcantara (load balance plz) wrote: > Add a comment here about how this shows suggested offline pages directly, > outside of their subsections. Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:14: * Separately maintains the selected state of the headers in addition to the other selected items. On 2017/03/06 17:10:25, Theresa wrote: > nit: s/headers/{@link SubsectionHeader}s Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:17: private Set<SubsectionHeader> mSelectedHeaders = new HashSet<>(); On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > private final Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:20: public boolean isSelectionEnabled() { On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > This function should probably be renamed to isSelectionModeActive() or > something. Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:40: * Toggles selection for a given subsection and sets the associated items to correct selection On 2017/03/06 17:10:25, Theresa wrote: > nit: to the correct selection state. Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:42: * @param header The given header. On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > "The given header" doesn't add any useful info. "Header for the subsection > being toggled"? Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:57: * Sets the selection state for a given header. Doesn't affect the associated items. On 2017/03/06 17:10:25, Theresa wrote: > nit: I would note when this method is supposed to be called as opposed to > toggleSelectionForSubsection. If I understand correctly, > toggleSelectionForSubsection is called when the user long-presses on the > subsection header itself and setSelectionForHeader is called to update the > selection state for a header when one if its items changes selection state. Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:40: private final ColorStateList mIconForegroundColorList; On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > private finals above the other ones Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:105: * @param header The associated header. On 2017/03/06 17:10:25, Theresa wrote: > nit: s/header/{@link SubsectionHeader} Done. https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:157: public void setSelectionDelegate(DownloadItemSelectionDelegate delegate) { On 2017/03/06 17:10:25, Theresa wrote: > Please add some comments about SelectionDelegates expecting all items to be of > the same type, DownloadItemSelectionDelegate dealing with > DownloadHistoryItemWrappers and this being a SelectableItemView<TimedItem>, etc. > so that it's more obvious without tracing through the code why this overrides > some of the super class methods. Done.
On 2017/03/08 02:35:31, shaktisahu wrote: > Fixed the background color issue. PTAL > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java > (right): > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:67: > public class SubsectionHeader extends TimedItem { > On 2017/03/07 23:55:16, dfalcantara (load balance plz) wrote: > > Does this class rely on anything inside the parent class? Should it be a > > private static class? > > No, I can make it protected static, it is accessed from OfflineGroupHeaderView > class too. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:69: > private List<DownloadHistoryItemWrapper> mSuggestedPages; > On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > > You should be more generic in case this pattern expands. > > mSubsectionItems/subsectionItems? > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:114: > * Helper method to set the properties of this class. > On 2017/03/06 17:10:25, Theresa wrote: > > nit: This is only setting the list of suggested pages, so this should be > > phrased: > > > > Helper method to set the suggested pages for this subsection. > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:491: > } else { > On 2017/03/07 23:55:16, dfalcantara (load balance plz) wrote: > > Add a comment here about how this shows suggested offline pages directly, > > outside of their subsections. > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java > (right): > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:14: > * Separately maintains the selected state of the headers in addition to the > other selected items. > On 2017/03/06 17:10:25, Theresa wrote: > > nit: s/headers/{@link SubsectionHeader}s > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:17: > private Set<SubsectionHeader> mSelectedHeaders = new HashSet<>(); > On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > > private final > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:20: > public boolean isSelectionEnabled() { > On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > > This function should probably be renamed to isSelectionModeActive() or > > something. > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:40: > * Toggles selection for a given subsection and sets the associated items to > correct selection > On 2017/03/06 17:10:25, Theresa wrote: > > nit: to the correct selection state. > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:42: > * @param header The given header. > On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > > "The given header" doesn't add any useful info. "Header for the subsection > > being toggled"? > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:57: > * Sets the selection state for a given header. Doesn't affect the associated > items. > On 2017/03/06 17:10:25, Theresa wrote: > > nit: I would note when this method is supposed to be called as opposed to > > toggleSelectionForSubsection. If I understand correctly, > > toggleSelectionForSubsection is called when the user long-presses on the > > subsection header itself and setSelectionForHeader is called to update the > > selection state for a header when one if its items changes selection state. > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java > (right): > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:40: > private final ColorStateList mIconForegroundColorList; > On 2017/03/07 23:55:17, dfalcantara (load balance plz) wrote: > > private finals above the other ones > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:105: > * @param header The associated header. > On 2017/03/06 17:10:25, Theresa wrote: > > nit: s/header/{@link SubsectionHeader} > > Done. > > https://codereview.chromium.org/2701253006/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:157: > public void setSelectionDelegate(DownloadItemSelectionDelegate delegate) { > On 2017/03/06 17:10:25, Theresa wrote: > > Please add some comments about SelectionDelegates expecting all items to be of > > the same type, DownloadItemSelectionDelegate dealing with > > DownloadHistoryItemWrappers and this being a SelectableItemView<TimedItem>, > etc. > > so that it's more obvious without tracing through the code why this overrides > > some of the super class methods. > > Done. I will add some tests for Subsection header click/long click behavior in a subsequent CL shortly.
Just a few nits -- this is really close https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java (right): https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:61: * this method, call toggleSelectionForSubsection instead directly. nit: the end of this should be "call {@link toggleSelectionForSubsection()} instead" https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:64: * @return Whether the header was successfully selected. nit: This returns whether the header is selected. If this is called with selected = false and it returns false, then I would consider that "successful". https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:114: setBackgroundResourceForGroupPosition(mHeader.isFirstInGroup(), mHeader.isLastInGroup()); The background also needs to be changed when the header is expanded/collapsed since that affects whether it is the last in the group.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/2701253006/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2701253006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:497: // into subsections and shown directly. the suggested offline pages are shown directly instead of being grouped into subsections https://codereview.chromium.org/2701253006/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2701253006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:42: private SelectionObserver<DownloadHistoryItemWrapper> mDownloadSelectionObserver = private final
Thanks for all the comments. There was an issue with the header not getting selected after the selection of its sub-items if the header is off the screen (since the view get reused for a different header :)) I fixed this by moving the individual SelectionObservers' from OfflineGroupHeaderView to DownloadItemSelectionDelegate which now keeps track of all the header selection related logic. It notifies the OfflineGroupHeaderView about the selection state change through another observer interface. PTAL one more time. https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java (right): https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:21: public boolean isSelectionEnabled() { Removing this as suggested in Theresa's prior comment. We shouldn't be counting the headers towards selected state as they are implicitly selected/deselected based on the state of the associated items. https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:61: * this method, call toggleSelectionForSubsection instead directly. On 2017/03/08 16:08:17, Theresa wrote: > nit: the end of this should be "call {@link toggleSelectionForSubsection()} > instead" Done. https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:64: * @return Whether the header was successfully selected. On 2017/03/08 16:08:17, Theresa wrote: > nit: This returns whether the header is selected. If this is called with > selected = false and it returns false, then I would consider that "successful". Done. https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2701253006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:114: setBackgroundResourceForGroupPosition(mHeader.isFirstInGroup(), mHeader.isLastInGroup()); On 2017/03/08 16:08:17, Theresa wrote: > The background also needs to be changed when the header is expanded/collapsed > since that affects whether it is the last in the group. Actually displayHeader is called everytime the adapter items are changed including after onclick(). So this takes care of setting the background. https://codereview.chromium.org/2701253006/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2701253006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:497: // into subsections and shown directly. On 2017/03/08 22:20:32, dfalcantara (load balance plz) wrote: > the suggested offline pages are shown directly instead of being grouped into > subsections Done.
lgtm % nits https://codereview.chromium.org/2701253006/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2701253006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:279: public Map<Date, SubsectionHeader> getSubsectionHeaders() { nit: JavaDocs. Does it make sense to return mSubsectionHeaders.values() instead of exposing the map? https://codereview.chromium.org/2701253006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:504: // In presence of an active search text, the suggested offline pages are show directly nit:s/show/shown https://codereview.chromium.org/2701253006/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java (right): https://codereview.chromium.org/2701253006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:52: addObserver(new SelectionObserver<DownloadHistoryItemWrapper>() { I like that this moved here because now we have one observer doing the work of identifying which subsections are selected vs an observer for each OfflineGroupHeaderView view.
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2701253006/#ps220001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/09 19:06:07, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... You're going to hit a bit of a wall trying to commit this without getting an l-g-t-m from an OWNER.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/03/09 19:10:30, dfalcantara (load balance plz) wrote: > On 2017/03/09 19:06:07, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > You're going to hit a bit of a wall trying to commit this without getting an > l-g-t-m from an OWNER. Yes, I was trying to see if the wall really exists :). And it does. A - The tool is broken, it is doesn't show the green, light green etc. for the individual files any more :( B - I have gotten l-g-t-m for all the Java files from an OWNER (except DownloadUtils.java apparently)
On 2017/03/09 19:26:22, shaktisahu wrote: > On 2017/03/09 19:10:30, dfalcantara (load balance plz) wrote: > > On 2017/03/09 19:06:07, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > You're going to hit a bit of a wall trying to commit this without getting an > > l-g-t-m from an OWNER. > > Yes, I was trying to see if the wall really exists :). And it does. > A - The tool is broken, it is doesn't show the green, light green etc. for the > individual files any more :( > B - I have gotten l-g-t-m for all the Java files from an OWNER (except > DownloadUtils.java apparently) A- Yeah, we noticed it earlier. B- It's fairly rude to try to submit a CL without someone's L-G-T-M after they've already left comments on it.
On 2017/03/09 19:26:22, shaktisahu wrote: > On 2017/03/09 19:10:30, dfalcantara (load balance plz) wrote: > > On 2017/03/09 19:06:07, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > You're going to hit a bit of a wall trying to commit this without getting an > > l-g-t-m from an OWNER. > > Yes, I was trying to see if the wall really exists :). And it does. > A - The tool is broken, it is doesn't show the green, light green etc. for the > individual files any more :( > B - I have gotten l-g-t-m for all the Java files from an OWNER (except > DownloadUtils.java apparently) A- Yeah, we noticed it earlier. B- It's fairly rude to try to submit a CL without someone's L-G-T-M after they've already left comments on it.
lgtm, but the tests are broken https://codereview.chromium.org/2701253006/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java (right): https://codereview.chromium.org/2701253006/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemSelectionDelegate.java:50: public void initialize(DownloadHistoryAdapter adapter) { nit:javadoc
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2701253006/#ps260001 (title: "Fixed tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489122962376040,
"parent_rev": "ca5b55100d024fb5b6ed95e9ad3fcb1d1ae24462", "commit_rev":
"ba3f171d0f3e4300341ba8a719f4f98f61af0165"}
Message was sent while issue was closed.
Description was changed from ========== Download Home : Suggested pages header selection This CL enables user to select the suggested page section header. Details: - Long press enables selection mode. Tap during selection will toggle the selection, but doesn't expand/collapse the items. - Selecting the header internally selects all the related items. - If all the items are selected manually, the header gets selected automatically. - Deselecting any item will deselect the corresponding header if previously selected. - The badging at the top reflects the number of items currently selected excluding the headers. - When selected, the chrome logo on the header turns into tick mark similar to rest of the items. - In presence of search text, headers are not displayed (no grouping), but once the text is cleared out, the headers will be back in. TODO: Hide the date headers when there is an active search. BUG=689801 ========== to ========== Download Home : Suggested pages header selection This CL enables user to select the suggested page section header. Details: - Long press enables selection mode. Tap during selection will toggle the selection, but doesn't expand/collapse the items. - Selecting the header internally selects all the related items. - If all the items are selected manually, the header gets selected automatically. - Deselecting any item will deselect the corresponding header if previously selected. - The badging at the top reflects the number of items currently selected excluding the headers. - When selected, the chrome logo on the header turns into tick mark similar to rest of the items. - In presence of search text, headers are not displayed (no grouping), but once the text is cleared out, the headers will be back in. TODO: Hide the date headers when there is an active search. BUG=689801 Review-Url: https://codereview.chromium.org/2701253006 Cr-Commit-Position: refs/heads/master@{#456001} Committed: https://chromium.googlesource.com/chromium/src/+/ba3f171d0f3e4300341ba8a719f4... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://chromium.googlesource.com/chromium/src/+/ba3f171d0f3e4300341ba8a719f4... |
