Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2789363003:
Implement single-selection mode for SelectionDelegate. (Closed)
DescriptionImplement single-selection mode for SelectionDelegate.
(This change was separated off my larger Photo Picker CL in an effort to shrink it).
BUG=656015
Review-Url: https://codereview.chromium.org/2789363003
Cr-Commit-Position: refs/heads/master@{#461866}
Committed: https://chromium.googlesource.com/chromium/src/+/8c317281b9cdbfc4e790aefca397b44662305df0
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 19
Patch Set 3 : Address feedback from Michael #
Total comments: 1
Patch Set 4 : Address feedback from Theresa #
Messages
Total messages: 27 (16 generated)
The CQ bit was checked by finnur@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...
finnur@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Mind doing the first pass here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java (right): https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:25: public interface SelectionObserver<E> { To reduce verbosity, I'd recommend renaming this to Observer. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:31: public void onSelectionStateChange(List<E> selectedItems); nit: methods of interfaces are public by default, omit it https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:31: public void onSelectionStateChange(List<E> selectedItems); Why does this receive a List and not a Set? It is populated from a HashSet, so the order implied by using a List is not actually stable. If you wanted both reliable ordering and guarantee there are no dupes, maybe a SortedSet is the best solution. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:34: private Set<E> mSelectedItems = new HashSet<>(); Should we document that E must have a meaningful equals() as SelectionDelegate relies on that by putting it in a HashSet? https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:37: /* nit: /** for JavaDoc https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java (right): https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:18: * Tests for the SelectionDelegate class. nit: {@link SelectionDelegate} https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:23: private SelectionDelegate<Object> mDelegate; It seems cleaner to track this state on a per-test-method level, and avoid making SelectionDelegateTest implement the observer interface. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:29: private List<Object> mSelectedItems; Same for this member, I'd avoid changing member state. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:46: assertEquals(false, mDelegate.isItemSelected(mData1)); Use assertFalse and assertTrue.
finnur@chromium.org changed reviewers: + twellington@chromium.org
Theresa, can you take a look? https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java (right): https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:25: public interface SelectionObserver<E> { This class is Theresa's domain, so I'll let her decide if I should go ahead with this. Happy to do the work. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:31: public void onSelectionStateChange(List<E> selectedItems); Removed public. I'll let Theresa chime in on the other part. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:34: private Set<E> mSelectedItems = new HashSet<>(); Ditto. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:37: /* On 2017/04/04 13:54:31, Michael van Ouwerkerk wrote: > nit: /** for JavaDoc Done. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java (right): https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/04 13:54:31, Michael van Ouwerkerk wrote: > nit: 2017 Done. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:18: * Tests for the SelectionDelegate class. On 2017/04/04 13:54:31, Michael van Ouwerkerk wrote: > nit: {@link SelectionDelegate} Done. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:23: private SelectionDelegate<Object> mDelegate; On 2017/04/04 13:54:31, Michael van Ouwerkerk wrote: > It seems cleaner to track this state on a per-test-method level, and avoid > making SelectionDelegateTest implement the observer interface. Done. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:29: private List<Object> mSelectedItems; On 2017/04/04 13:54:31, Michael van Ouwerkerk wrote: > Same for this member, I'd avoid changing member state. Done. https://codereview.chromium.org/2789363003/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/widget/selection/SelectionDelegateTest.java:46: assertEquals(false, mDelegate.isItemSelected(mData1)); On 2017/04/04 13:54:31, Michael van Ouwerkerk wrote: > Use assertFalse and assertTrue. Done.
The CQ bit was checked by finnur@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...
lgtm https://codereview.chromium.org/2789363003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java (right): https://codereview.chromium.org/2789363003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:20: private boolean mSingleSelection; nit: mIsSingleSelection
lgtm
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2789363003/#ps60001 (title: "Address feedback from Theresa")
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
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...)
Description was changed from ========== Implement single-selection mode for SelectionDelegate. (This change was separated off my larger Photo Picker CL in an effort to shrink it). BUG=656015 ========== to ========== Implement single-selection mode for SelectionDelegate. (This change was separated off my larger Photo Picker CL in an effort to shrink it). BUG=656015 ==========
finnur@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, do you mind giving OWNERS on the test and checking the CQ bit if it looks ok? Thanks!
lgtm
The CQ bit was checked by tedchoc@chromium.org
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": 60001, "attempt_start_ts": 1491343766328810,
"parent_rev": "e51fced5944c4d9c29f1fe70c1f31ebb7690d6c3", "commit_rev":
"8c317281b9cdbfc4e790aefca397b44662305df0"}
Message was sent while issue was closed.
Description was changed from ========== Implement single-selection mode for SelectionDelegate. (This change was separated off my larger Photo Picker CL in an effort to shrink it). BUG=656015 ========== to ========== Implement single-selection mode for SelectionDelegate. (This change was separated off my larger Photo Picker CL in an effort to shrink it). BUG=656015 Review-Url: https://codereview.chromium.org/2789363003 Cr-Commit-Position: refs/heads/master@{#461866} Committed: https://chromium.googlesource.com/chromium/src/+/8c317281b9cdbfc4e790aefca397... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8c317281b9cdbfc4e790aefca397... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
