Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1928063002:
[NTP Snippets] Fill space below the last snippet if necessary (Closed)
Description[NTP Snippets] Fill space below the last snippet if necessary
Add a filler item at the end of the recycler view to push the
snippet items to the top of the view when there are not enough
of them.
Preview: https://goo.gl/photos/BDHFexhW8Cvo9Y4v5
BUG=603308
Committed: https://crrev.com/e047ff07f0e05ec8f9ba0556eeda413e314599e3
Cr-Commit-Position: refs/heads/master@{#394147}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : rebase #
Total comments: 18
Patch Set 4 : remove the bottom padding #Patch Set 5 : Address comments, fix tests #
Total comments: 13
Patch Set 6 : address comments, refactor SpacingListItem #Patch Set 7 : Move SnippetItemDecoration to another CL #Patch Set 8 : Take into account the expanding header #
Total comments: 13
Patch Set 9 : actually upload CL #
Total comments: 34
Patch Set 10 : address comments #Patch Set 11 : #
Total comments: 17
Patch Set 12 : Address comments #Patch Set 13 : #
Total comments: 3
Patch Set 14 : suppress warning, back to int instead of viewlist, remove unnecessary checks #
Total comments: 9
Patch Set 15 : unsuppress warning, fix doc #
Total comments: 2
Patch Set 16 : rebase, address comments #Messages
Total messages: 49 (12 generated)
dgn@chromium.org changed reviewers: + maybelle@chromium.org, peconn@chromium.org
https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java (right): https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:53: public void updateDimensions() { Have you tried using 'requestLayout'? I think this forces a view to call onMeasure(), in which case you can get rid of updateDimensions() and just call requestLayout(). https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:59: private void calculateDimensions() { Can this just be 'calculateHeight'? And return the height instead of setting a 'mHeight' (which you could get rid of).
Sounds good, feel free to take over the CL and land it 😀 On Tue, 3 May 2016, 10:48 , <peconn@chromium.org> wrote: > > > https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... > File > > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java > (right): > > > https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:53: > public void updateDimensions() { > Have you tried using 'requestLayout'? I think this forces a view to call > onMeasure(), in which case you can get rid of updateDimensions() and > just call requestLayout(). > > > https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:59: > private void calculateDimensions() { > Can this just be 'calculateHeight'? And return the height instead of > setting a 'mHeight' (which you could get rid of). > > https://codereview.chromium.org/1928063002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java (right): https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:53: public void updateDimensions() { On 2016/05/03 at 08:48:48, PEConn1 wrote: > Have you tried using 'requestLayout'? I think this forces a view to call onMeasure(), in which case you can get rid of updateDimensions() and just call requestLayout(). Thanks! Done. https://codereview.chromium.org/1928063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:59: private void calculateDimensions() { On 2016/05/03 at 08:48:48, PEConn1 wrote: > Can this just be 'calculateHeight'? And return the height instead of setting a 'mHeight' (which you could get rid of). Renamed. I'd prefer keeping mHeight to avoid recalculating it when it's not needed (e.g. when the number of elements has no changed).
Description was changed from ========== [NTP Snippets] Fill space below the last snippet if necessary WIP CL, free to a good home. BUG=603308 ========== to ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Also adds a padding of 40dp at the bottom of the list. BUG=603308 ==========
Description was changed from ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Also adds a padding of 40dp at the bottom of the list. BUG=603308 ========== to ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Also adds a padding of 40dp at the bottom of the list. Preview: https://goo.gl/photos/tfTVzHBcNMqZivT2A BUG=603308 ==========
PTAL. Added a link to a preview video in the CL description
https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:182: // TODO(https://crbug.com/608918): Remove this for now as we need to come up with a better This doesn't seem to be the right bug number. Also, Nicole ran into this problem with the headers. Apparently in a ListView (also a RecyclerView), the view still takes up space even when GONE. The only way to do this is to remove/add the item dynamically https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java (right): https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:38: Log.d(TAG, "onMeasure"); Remove https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:54: int firstPos = lm.findFirstVisibleItemPosition(); As discussed, please clarify what's being done here with comments https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:62: if (rv.getAdapter().getItemViewType(1) == NewTabPageListItem.VIEW_TYPE_PADDING) { Ditto.
bauerb@chromium.org changed reviewers: + mcwilliams@chromium.org
+Nicole
https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:268: <dimen name="snippets_bottom_padding">40dp</dimen> We should remove this for now as discussed https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java (right): https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:21: public class PaddingListItem implements NewTabPageListItem { Possibly rename this to SpacingListItem. You might change to margins later on for some reason https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:31: mMinHeight = res.getDimensionPixelSize(R.dimen.snippets_bottom_padding); Remove this https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:52: NewTabPageRecyclerView rv = (NewTabPageRecyclerView) getParent(); avoid using abbreviations like rv and lm - it makes the code really hard to read https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:54: int firstPos = lm.findFirstVisibleItemPosition(); What is firstPos?
PTAL https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:268: <dimen name="snippets_bottom_padding">40dp</dimen> On 2016/05/10 at 12:42:52, mcwilliams wrote: > We should remove this for now as discussed Done. https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:182: // TODO(https://crbug.com/608918): Remove this for now as we need to come up with a better On 2016/05/10 at 10:31:51, May wrote: > This doesn't seem to be the right bug number. > > Also, Nicole ran into this problem with the headers. Apparently in a ListView (also a RecyclerView), the view still takes up space even when GONE. The only way to do this is to remove/add the item dynamically It should have been commented out at the same time a line 127, so I just did it and copied the explanation comment. Nicole, which bug did you intend to link there? https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java (right): https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:21: public class PaddingListItem implements NewTabPageListItem { On 2016/05/10 12:42:52, mcwilliams wrote: > Possibly rename this to SpacingListItem. You might change to margins later on > for some reason Done. https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:31: mMinHeight = res.getDimensionPixelSize(R.dimen.snippets_bottom_padding); On 2016/05/10 12:42:52, mcwilliams wrote: > Remove this Done. https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:38: Log.d(TAG, "onMeasure"); On 2016/05/10 at 10:31:51, May wrote: > Remove Done. https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:52: NewTabPageRecyclerView rv = (NewTabPageRecyclerView) getParent(); On 2016/05/10 12:42:52, mcwilliams wrote: > avoid using abbreviations like rv and lm - it makes the code really hard to read Done. https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:54: int firstPos = lm.findFirstVisibleItemPosition(); On 2016/05/10 at 10:31:51, May wrote: > As discussed, please clarify what's being done here with comments Done. https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:54: int firstPos = lm.findFirstVisibleItemPosition(); On 2016/05/10 12:42:52, mcwilliams wrote: > What is firstPos? Is the current state enough or should I comment some more? https://codereview.chromium.org/1928063002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/PaddingListItem.java:62: if (rv.getAdapter().getItemViewType(1) == NewTabPageListItem.VIEW_TYPE_PADDING) { On 2016/05/10 at 10:31:51, May wrote: > Ditto. Done.
https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:426: public void updateSearchBoxOnScroll() { Revert this? https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:159: mPaddingViewHolder = new NewTabPageViewHolder(SpacingListItem.createView(parent)); Should we lazily initialize this? https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:182: // TODO(https://crbug.com/608918): Remove this for now as we need to come up with a better This will be fixed once https://codereview.chromium.org/1961073002/ lands, right? https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:67: if (childPos == adapter.getItemCount() - 1 || childPos == RecyclerView.NO_POSITION) { When does this happen? It might warrant a comment.
https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:44: private NewTabPageViewHolder mPaddingViewHolder; rename SpacingViewHolder Actually should only be used for snippets so mSnippetsSpacingViewHolder Also change the SpacingListItem to SnippetsSpacingListItem https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageListItem.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageListItem.java:46: public static final int VIEW_TYPE_PADDING = 4; VIEW_TYPE_SPACING/VIEW_TYPE_SNIPPET_SPACING https://codereview.chromium.org/1928063002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:93: // If we don't get anything, we should still have the above-the-fold item and the padding padding - spacing
Description was changed from ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Also adds a padding of 40dp at the bottom of the list. Preview: https://goo.gl/photos/tfTVzHBcNMqZivT2A BUG=603308 ========== to ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Preview: https://goo.gl/photos/tfTVzHBcNMqZivT2A BUG=603308 ==========
Description was changed from ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Preview: https://goo.gl/photos/tfTVzHBcNMqZivT2A BUG=603308 ========== to ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Preview: https://goo.gl/photos/et8561k5v4KKntuq5 BUG=603308 ==========
PTAL Also updated the preview video. https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:426: public void updateSearchBoxOnScroll() { On 2016/05/10 at 14:37:33, Bernhard Bauer wrote: > Revert this? I need this to make sure the state of the above-the-fold item is right. I forgot to include the file that uses it after the rename. https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:44: private NewTabPageViewHolder mPaddingViewHolder; On 2016/05/10 at 14:38:20, mcwilliams wrote: > rename SpacingViewHolder > > Actually should only be used for snippets so mSnippetsSpacingViewHolder > > Also change the SpacingListItem to SnippetsSpacingListItem As discussed, I left it as SpacingListItem. https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:159: mPaddingViewHolder = new NewTabPageViewHolder(SpacingListItem.createView(parent)); On 2016/05/10 at 14:37:33, Bernhard Bauer wrote: > Should we lazily initialize this? Changed to follow the same pattern as the other items, where a ListItem is created at the same time as the Adapter. https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:182: // TODO(https://crbug.com/608918): Remove this for now as we need to come up with a better On 2016/05/10 at 14:37:33, Bernhard Bauer wrote: > This will be fixed once https://codereview.chromium.org/1961073002/ lands, right? Yup, uncommented. https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:67: if (childPos == adapter.getItemCount() - 1 || childPos == RecyclerView.NO_POSITION) { On 2016/05/10 at 14:37:33, Bernhard Bauer wrote: > When does this happen? It might warrant a comment. That's the fix for https://bugs.chromium.org/p/chromium/issues/detail?id=610665. I'll move it to another CL. https://codereview.chromium.org/1928063002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/1928063002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:93: // If we don't get anything, we should still have the above-the-fold item and the padding On 2016/05/10 at 14:38:21, mcwilliams wrote: > padding - spacing Done.
https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:180: && getItemViewType(2) == NewTabPageListItem.VIEW_TYPE_SPACING) { This would happen when the snippet at position 2 is removed, right? Should we generalize and future-proof this for the case of multiple sections with a header each? https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:188: mSpacingListItem.refreshSpacing(); If all you want to do here is call through to the view to request it to layout, couldn't you just get it from the RecyclerView? https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java (right): https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:41: if (!(item instanceof SpacingListItem)) return; Would this ever be called with a different type of item? https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:47: private final int mMinHeight; Wait, this is final and set to 0? It could be just a constant. https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:65: // for now. Now that I see this, it does look rather tangled right now. Could we move this calculation somewhere else and then just set a single height value on this item?
dgn@chromium.org changed reviewers: + dfalcantara@chromium.org
PTAL https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:180: && getItemViewType(2) == NewTabPageListItem.VIEW_TYPE_SPACING) { On 2016/05/11 at 16:07:13, Bernhard Bauer wrote: > This would happen when the snippet at position 2 is removed, right? Should we generalize and future-proof this for the case of multiple sections with a header each? The previous version was more specific and broke when we removed the header. I think it's better to wait when we have multiple sections before we generalize this. Hopefully we'll have means to know exactly how many items are in each sections, and things like that, that will make the code clean here. https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:188: mSpacingListItem.refreshSpacing(); On 2016/05/11 at 16:07:13, Bernhard Bauer wrote: > If all you want to do here is call through to the view to request it to layout, couldn't you just get it from the RecyclerView? Done. It's a bit awkward do to so from the adapter, since in theory it could be backing multiple recyclerviews. But anyway, that works. https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java (right): https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:41: if (!(item instanceof SpacingListItem)) return; On 2016/05/11 at 16:07:13, Bernhard Bauer wrote: > Would this ever be called with a different type of item? No, replaced by an assert. https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:47: private final int mMinHeight; On 2016/05/11 at 16:07:13, Bernhard Bauer wrote: > Wait, this is final and set to 0? It could be just a constant. Done. https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:65: // for now. On 2016/05/11 at 16:07:13, Bernhard Bauer wrote: > Now that I see this, it does look rather tangled right now. Could we move this calculation somewhere else and then just set a single height value on this item? Moved it to NewTabPageRecyclerView. WDYT?
Did you forget to upload a new patch set? https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:180: && getItemViewType(2) == NewTabPageListItem.VIEW_TYPE_SPACING) { On 2016/05/11 22:48:31, dgn wrote: > On 2016/05/11 at 16:07:13, Bernhard Bauer wrote: > > This would happen when the snippet at position 2 is removed, right? Should we > generalize and future-proof this for the case of multiple sections with a header > each? > > The previous version was more specific and broke when we removed the header. I > think it's better to wait when we have multiple sections before we generalize > this. Hopefully we'll have means to know exactly how many items are in each > sections, and things like that, that will make the code clean here. In that case, I would be more explicit about our assumptions. For example, assert that |position| is 2? (Maybe even with something stronger than `assert`, because it's unlikely that our asserts will become effective before this is rewritten anyway.)
Oops, thanks. Uploaded for real now. https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:180: && getItemViewType(2) == NewTabPageListItem.VIEW_TYPE_SPACING) { On 2016/05/12 at 09:31:04, Bernhard Bauer wrote: > On 2016/05/11 22:48:31, dgn wrote: > > On 2016/05/11 at 16:07:13, Bernhard Bauer wrote: > > > This would happen when the snippet at position 2 is removed, right? Should we > > generalize and future-proof this for the case of multiple sections with a header > > each? > > > > The previous version was more specific and broke when we removed the header. I > > think it's better to wait when we have multiple sections before we generalize > > this. Hopefully we'll have means to know exactly how many items are in each > > sections, and things like that, that will make the code clean here. > > In that case, I would be more explicit about our assumptions. For example, assert that |position| is 2? (Maybe even with something stronger than `assert`, because it's unlikely that our asserts will become effective before this is rewritten anyway.) Made it a bit more general in the end. But we're going to start keeping the header soon anyway...
https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:180: && getItemViewType(2) == NewTabPageListItem.VIEW_TYPE_SPACING) { On 2016/05/12 10:28:49, dgn wrote: > On 2016/05/12 at 09:31:04, Bernhard Bauer wrote: > > On 2016/05/11 22:48:31, dgn wrote: > > > On 2016/05/11 at 16:07:13, Bernhard Bauer wrote: > > > > This would happen when the snippet at position 2 is removed, right? Should > we > > > generalize and future-proof this for the case of multiple sections with a > header > > > each? > > > > > > The previous version was more specific and broke when we removed the header. > I > > > think it's better to wait when we have multiple sections before we > generalize > > > this. Hopefully we'll have means to know exactly how many items are in each > > > sections, and things like that, that will make the code clean here. > > > > In that case, I would be more explicit about our assumptions. For example, > assert that |position| is 2? (Maybe even with something stronger than `assert`, > because it's unlikely that our asserts will become effective before this is > rewritten anyway.) > > Made it a bit more general in the end. But we're going to start keeping the > header soon anyway... Are we? I thought we'd just show the placeholder without a header? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:49: mRecyclerView.refreshBottomSpacing(); You could move this to dismissItem()? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:177: // getting the reference as we are doing here is going to be broken if that changes. What would change? Could the same adapter in principle actually be used in multiple views at the same time? Or could the adapter be attached to different views sequentially? The latter we could deal with, I think. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:194: position -= 1; // Position of the header position--? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:196: ++numRemovedItems; Also, use postincrement here. The compiler should be able to know the value of the expression isn't used and optimize correspondingly :) https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:38: private int mToolbarHeight; This can be final. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:103: * {@link #getChildAt}, which takes indices in terms of visible items. Couldn't you just use `findViewHolderForAdapterPosition().itemView`? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:24: assert getParent() instanceof NewTabPageRecyclerView; You'll get an exception anyway if this isn't the case. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:26: 0, ((NewTabPageRecyclerView) getParent()).calculateBottomSpacing()); So, stupid question: If all we are doing here is setting a height value that we get from somewhere else anyway... couldn't we just set this as a minimum height in advance?
AFAICT from https://docs.google.com/presentation/d/10RoYrYkofS4CG1AO7Hr5oIPDnZK67ccT7wDX1... you're not supposed to hide the header, and instead make it say "All read". Any reason you're hiding it here? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:43: nit: private static finals above private finals? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:177: // getting the reference as we are doing here is going to be broken if that changes. On 2016/05/12 11:20:59, Bernhard Bauer wrote: > What would change? Could the same adapter in principle actually be used in > multiple views at the same time? Or could the adapter be attached to different > views sequentially? The latter we could deal with, I think. I'd be worried about cases where the same adapter were being used to display data for one RecyclerView, then fail to update the other one. Out of scope for the current CL, but can Zine handle two different ChromeTabbedActivities showing NTPs side-by-side in Android N? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:193: && getItemViewType(position) != NewTabPageListItem.VIEW_TYPE_SNIPPET) { Should you explicitly be checking for a SPACING here? If you add another type later, this code might break. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:112: * below the fold. Isn't the point of the bottom spacing item that there isn't enough content below the fold? Should this say that the spacing is large enough to push the header to the top or something? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:123: // No snippets, the spacing item should occupy the full height. ... full height because the header isn't being displayed? (At least from the video) https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:124: return fullHeight; Were there any concerns from UX about users accidentally swiping down and not being able to tell how to get out of that state? I saw that UX mocked a box for "Check again soon"; is that coming? https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:127: int lastContentItemPosition = getAdapter().getItemCount() - 2; nit: Put a comment here about the spacing item being the last?
PTAL https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:43: On 2016/05/12 at 18:04:38, dfalcantara wrote: > nit: private static finals above private finals? Done. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:49: mRecyclerView.refreshBottomSpacing(); On 2016/05/12 at 11:21:00, Bernhard Bauer wrote: > You could move this to dismissItem()? I added a clearView callback, in which I have a call paired with calls here. I think having them here is clearer with the recent changes. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:177: // getting the reference as we are doing here is going to be broken if that changes. On 2016/05/12 at 11:20:59, Bernhard Bauer wrote: > What would change? Could the same adapter in principle actually be used in multiple views at the same time? Or could the adapter be attached to different views sequentially? The latter we could deal with, I think. It supports being watched by multiple views at the same time. I don't think we'll ever go this route, but I preferred writing that down here https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:177: // getting the reference as we are doing here is going to be broken if that changes. On 2016/05/12 at 18:04:38, dfalcantara wrote: > On 2016/05/12 11:20:59, Bernhard Bauer wrote: > > What would change? Could the same adapter in principle actually be used in > > multiple views at the same time? Or could the adapter be attached to different > > views sequentially? The latter we could deal with, I think. > > I'd be worried about cases where the same adapter were being used to display data for one RecyclerView, then fail to update the other one. > > Out of scope for the current CL, but can Zine handle two different ChromeTabbedActivities showing NTPs side-by-side in Android N? That should work, as the objects here are scoped to a specific NewTabPage, and they are different instances for each new tab. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:193: && getItemViewType(position) != NewTabPageListItem.VIEW_TYPE_SNIPPET) { On 2016/05/12 at 18:04:39, dfalcantara wrote: > Should you explicitly be checking for a SPACING here? If you add another type later, this code might break. Removed that code. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:194: position -= 1; // Position of the header On 2016/05/12 at 11:21:00, Bernhard Bauer wrote: > position--? Removed that code. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:196: ++numRemovedItems; On 2016/05/12 at 11:21:00, Bernhard Bauer wrote: > Also, use postincrement here. The compiler should be able to know the value of the expression isn't used and optimize correspondingly :) Removed that code. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:38: private int mToolbarHeight; On 2016/05/12 at 11:21:00, Bernhard Bauer wrote: > This can be final. Done. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:103: * {@link #getChildAt}, which takes indices in terms of visible items. On 2016/05/12 at 11:21:00, Bernhard Bauer wrote: > Couldn't you just use `findViewHolderForAdapterPosition().itemView`? Ah thanks, I missed it. I was searching under "get*". Done. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:112: * below the fold. On 2016/05/12 at 18:04:39, dfalcantara wrote: > Isn't the point of the bottom spacing item that there isn't enough content below the fold? Should this say that the spacing is large enough to push the header to the top or something? Hum, yes, there isn't enough content so we have this item grow to balance it and make so that there is enough content. I updated to doc, please let me know if it still needs to be clarified. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:123: // No snippets, the spacing item should occupy the full height. On 2016/05/12 at 18:04:39, dfalcantara wrote: > ... full height because the header isn't being displayed? > > (At least from the video) Removed that code. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:124: return fullHeight; On 2016/05/12 at 18:04:39, dfalcantara wrote: > Were there any concerns from UX about users accidentally swiping down and not being able to tell how to get out of that state? I saw that UX mocked a box for "Check again soon"; is that coming? Yes, it's going to be added for the MVP. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:127: int lastContentItemPosition = getAdapter().getItemCount() - 2; On 2016/05/12 at 18:04:39, dfalcantara wrote: > nit: Put a comment here about the spacing item being the last? Done. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:24: assert getParent() instanceof NewTabPageRecyclerView; On 2016/05/12 at 11:21:00, Bernhard Bauer wrote: > You'll get an exception anyway if this isn't the case. Removed. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingListItem.java:26: 0, ((NewTabPageRecyclerView) getParent()).calculateBottomSpacing()); On 2016/05/12 at 11:21:00, Bernhard Bauer wrote: > So, stupid question: If all we are doing here is setting a height value that we get from somewhere else anyway... couldn't we just set this as a minimum height in advance? The places where we manually request a recalculation (header being resized, snippet being removed) are not enough to make sure it's always going to be the right size. For example, when the padding is attached while the cards are being pulled up following a dismissal. It's a null reference before that so the value can't be set on it, it has to pull it itself instead.
https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:187: // getting the reference as we are doing here is going to be broken if that changes. Nit: Capitalize "getting". https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:189: assert recyclerView instanceof NewTabPageRecyclerView; This is also unnecessary, because of the cast on the next line. https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:150: /** Notify that an item is in the process of being removed from the view. */ Nit: "Called when". https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:156: void itemDismissDone(View itemView) { Nit: The opposite of "started" would be "finished".
Got an updated video? https://chromiumcodereview.appspot.com/1928063002/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://chromiumcodereview.appspot.com/1928063002/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:112: * below the fold. On 2016/05/13 13:49:58, dgn wrote: > On 2016/05/12 at 18:04:39, dfalcantara wrote: > > Isn't the point of the bottom spacing item that there isn't enough content > below the fold? Should this say that the spacing is large enough to push the > header to the top or something? > > Hum, yes, there isn't enough content so we have this item grow to balance it and > make so that there is enough content. I updated to doc, please let me know if it > still needs to be clarified. Acknowledged. https://chromiumcodereview.appspot.com/1928063002/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:124: return fullHeight; On 2016/05/13 13:49:58, dgn wrote: > On 2016/05/12 at 18:04:39, dfalcantara wrote: > > Were there any concerns from UX about users accidentally swiping down and not > being able to tell how to get out of that state? I saw that UX mocked a box for > "Check again soon"; is that coming? > > Yes, it's going to be added for the MVP. Er, guessing that's a Most Visited Page or something. Either way, acknowledged. https://chromiumcodereview.appspot.com/1928063002/diff/200001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://chromiumcodereview.appspot.com/1928063002/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:136: // The spacing item is the last item. nit: // The spacing item is the last item; the last content item is directly above that. https://chromiumcodereview.appspot.com/1928063002/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:139: findViewHolderForAdapterPosition(lastContentItemPosition).itemView.getBottom() findViewForAdapterPosition can return null. Is that something you need to account for? https://chromiumcodereview.appspot.com/1928063002/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:151: void itemDismissStarted(View itemView) { nit: onItemDismissStarted?
PTAL The behaviour and looks are exactly the same, so I didn't upload the video. https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:124: return fullHeight; On 2016/05/13 at 17:08:09, dfalcantara wrote: > On 2016/05/13 13:49:58, dgn wrote: > > On 2016/05/12 at 18:04:39, dfalcantara wrote: > > > Were there any concerns from UX about users accidentally swiping down and not > > being able to tell how to get out of that state? I saw that UX mocked a box for > > "Check again soon"; is that coming? > > > > Yes, it's going to be added for the MVP. > > Er, guessing that's a Most Visited Page or something. Either way, acknowledged. oh sorry, MVP here stands for Minimum Viable Product https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:189: assert recyclerView instanceof NewTabPageRecyclerView; On 2016/05/13 at 16:26:56, Bernhard Bauer wrote: > This is also unnecessary, because of the cast on the next line. I need to have some sort of check for the type of the instance to pass compilation, the clang android bot doesn't like it otherwise. https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:136: // The spacing item is the last item. On 2016/05/13 at 17:08:09, dfalcantara wrote: > nit: // The spacing item is the last item; the last content item is directly above that. Done. https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:139: findViewHolderForAdapterPosition(lastContentItemPosition).itemView.getBottom() On 2016/05/13 at 17:08:09, dfalcantara wrote: > findViewForAdapterPosition can return null. Is that something you need to account for? All the concerned items should be visible so the method would not return null. For SNAP_ITEM_ADAPTER_POSITION, we have a check above to verify that the item is visible, and for lastContentItemPosition, it will be visible if the spacing item is visible. Added the checks for safety anyway. https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:150: /** Notify that an item is in the process of being removed from the view. */ On 2016/05/13 at 16:26:57, Bernhard Bauer wrote: > Nit: "Called when". Done. https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:151: void itemDismissStarted(View itemView) { On 2016/05/13 at 17:08:09, dfalcantara wrote: > nit: onItemDismissStarted? Done. https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:156: void itemDismissDone(View itemView) { On 2016/05/13 at 16:26:57, Bernhard Bauer wrote: > Nit: The opposite of "started" would be "finished". Done.
nvm, note quite done yet, I found more glitches.
Description was changed from ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Preview: https://goo.gl/photos/et8561k5v4KKntuq5 BUG=603308 ========== to ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Preview: https://goo.gl/photos/BDHFexhW8Cvo9Y4v5 BUG=603308 ==========
PTAL. Updated the preview video. https://codereview.chromium.org/1928063002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:46: private final List<View> mInProgressDismissItems; Reverted to using a list of views instead of a simple int with the combined height to compensate for. The clearView callback is called even when the dismiss gesture is not completed, so it was possible to get to negative value, and I didn't find a more satisfying way to distinguish the aborted gestures from the completed ones at that point.
https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:189: assert recyclerView instanceof NewTabPageRecyclerView; On 2016/05/16 09:44:09, dgn wrote: > On 2016/05/13 at 16:26:56, Bernhard Bauer wrote: > > This is also unnecessary, because of the cast on the next line. > > I need to have some sort of check for the type of the instance to pass > compilation, the clang android bot doesn't like it otherwise. But the cast should provide that check, no? We do that all over the place, e.g. when getting a view by its ID. https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:139: findViewHolderForAdapterPosition(lastContentItemPosition).itemView.getBottom() On 2016/05/16 09:44:09, dgn wrote: > On 2016/05/13 at 17:08:09, dfalcantara wrote: > > findViewForAdapterPosition can return null. Is that something you need to > account for? > > All the concerned items should be visible so the method would not return null. > For SNAP_ITEM_ADAPTER_POSITION, we have a check above to verify that the item is > visible, and for lastContentItemPosition, it will be visible if the spacing item > is visible. > > Added the checks for safety anyway. Sorry, I'm going to have to disagree here. If this is not supposed to happen, you should not have code in a Release build that will handle it. If this should indeed happen, crashing is the best way for you to learn about it, and if it doesn't happen, you're good anyway. There is an explicit rule for C++ about it at http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-, but it equally applies to Java. https://codereview.chromium.org/1928063002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:46: private final List<View> mInProgressDismissItems; On 2016/05/16 12:49:11, dgn wrote: > Reverted to using a list of views instead of a simple int with the combined > height to compensate for. The clearView callback is called even when the dismiss > gesture is not completed, so it was possible to get to negative value, and I > didn't find a more satisfying way to distinguish the aborted gestures from the > completed ones at that point. So that means that clearView() can sometimes be called for an item for which onSwiped() hasn't been called? It might almost be worth adding a comment to that effect, so that someone debugging this in the future would not think that's a bug.
PTAL https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:189: assert recyclerView instanceof NewTabPageRecyclerView; On 2016/05/16 at 13:46:49, Bernhard Bauer wrote: > On 2016/05/16 09:44:09, dgn wrote: > > On 2016/05/13 at 16:26:56, Bernhard Bauer wrote: > > > This is also unnecessary, because of the cast on the next line. > > > > I need to have some sort of check for the type of the instance to pass > > compilation, the clang android bot doesn't like it otherwise. > > But the cast should provide that check, no? We do that all over the place, e.g. when getting a view by its ID. ¯\_(ツ)_/¯ I can see bug reports about false positives in the FB detection, and IDK why in other cases it doesn't get reported. Replaced with @Suppress. https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:139: findViewHolderForAdapterPosition(lastContentItemPosition).itemView.getBottom() On 2016/05/16 at 13:46:49, Bernhard Bauer wrote: > On 2016/05/16 09:44:09, dgn wrote: > > On 2016/05/13 at 17:08:09, dfalcantara wrote: > > > findViewForAdapterPosition can return null. Is that something you need to > > account for? > > > > All the concerned items should be visible so the method would not return null. > > For SNAP_ITEM_ADAPTER_POSITION, we have a check above to verify that the item is > > visible, and for lastContentItemPosition, it will be visible if the spacing item > > is visible. > > > > Added the checks for safety anyway. > > Sorry, I'm going to have to disagree here. If this is not supposed to happen, you should not have code in a Release build that will handle it. If this should indeed happen, crashing is the best way for you to learn about it, and if it doesn't happen, you're good anyway. There is an explicit rule for C++ about it at http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-, but it equally applies to Java. Understood. Reverted. https://codereview.chromium.org/1928063002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:46: private final List<View> mInProgressDismissItems; On 2016/05/16 at 13:46:49, Bernhard Bauer wrote: > On 2016/05/16 12:49:11, dgn wrote: > > Reverted to using a list of views instead of a simple int with the combined > > height to compensate for. The clearView callback is called even when the dismiss > > gesture is not completed, so it was possible to get to negative value, and I > > didn't find a more satisfying way to distinguish the aborted gestures from the > > completed ones at that point. > > So that means that clearView() can sometimes be called for an item for which onSwiped() hasn't been called? It might almost be worth adding a comment to that effect, so that someone debugging this in the future would not think that's a bug. Actually, I found a satisfying way to do it. itemViews are set to the NO_POSITION position before they are sent to clearView() in case of dismissals. Added a comment too.
More or less lgtm to me, but I really don't want that supression to land. https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:191: @SuppressFBWarnings("BC_UNCONFIRMED_CAST") Adding a type of FindBugs suppression is uglier than any adjusting the code to account for the cast. This will be one of only three places in the entire codebase that checks for this instead of using instanceof, and I don't want to have another. https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:42: * compensate for their removal animation, and avoid the scroll position to shift around. nit: "Total height of the items being dismissed. Tracked to allow the bottom space to compensate for their removal animation and avoid moving the scroll position."? https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:125: * below the fold to push the header to align with the top of the screen. nit: "below the fold to push the header up to the top of the screen"? The double "to" is a little bit awkward. https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:148: /** Notify that an item has been removed from the view. */ nit: This doesn't do any notifying. "Called when an item has finished being removed from the view."
PTAL https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:191: @SuppressFBWarnings("BC_UNCONFIRMED_CAST") On 2016/05/16 23:47:29, dfalcantara wrote: > Adding a type of FindBugs suppression is uglier than any adjusting the code to > account for the cast. This will be one of only three places in the entire > codebase that checks for this instead of using instanceof, and I don't want to > have another. Done. Do you know who FindBugs doesn't complain for the many other cases where we wildly cast things? (findViewById for example) https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:42: * compensate for their removal animation, and avoid the scroll position to shift around. On 2016/05/16 23:47:29, dfalcantara wrote: > nit: "Total height of the items being dismissed. Tracked to allow the bottom > space to compensate for their removal animation and avoid moving the scroll > position."? Done. https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:125: * below the fold to push the header to align with the top of the screen. On 2016/05/16 23:47:29, dfalcantara wrote: > nit: "below the fold to push the header up to the top of the screen"? The > double "to" is a little bit awkward. Done. https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:148: /** Notify that an item has been removed from the view. */ On 2016/05/16 23:47:29, dfalcantara wrote: > nit: This doesn't do any notifying. > "Called when an item has finished being removed from the view." Done.
lgtm https://codereview.chromium.org/1928063002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:52: mCompensationHeight = 0; 0 is the default value for int members in Java (and they can't be uninitialized).
lgtm https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:27: */ I believe you can change these from /** to just //
lgtm
Thanks! https://codereview.chromium.org/1928063002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1928063002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:52: mCompensationHeight = 0; On 2016/05/17 09:25:53, Bernhard Bauer wrote: > 0 is the default value for int members in Java (and they can't be > uninitialized). Done.
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, bauerb@chromium.org, mcwilliams@chromium.org Link to the patchset: https://codereview.chromium.org/1928063002/#ps300001 (title: "rebase, address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928063002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928063002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Preview: https://goo.gl/photos/BDHFexhW8Cvo9Y4v5 BUG=603308 ========== to ========== [NTP Snippets] Fill space below the last snippet if necessary Add a filler item at the end of the recycler view to push the snippet items to the top of the view when there are not enough of them. Preview: https://goo.gl/photos/BDHFexhW8Cvo9Y4v5 BUG=603308 Committed: https://crrev.com/e047ff07f0e05ec8f9ba0556eeda413e314599e3 Cr-Commit-Position: refs/heads/master@{#394147} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/e047ff07f0e05ec8f9ba0556eeda413e314599e3 Cr-Commit-Position: refs/heads/master@{#394147}
Message was sent while issue was closed.
https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:191: @SuppressFBWarnings("BC_UNCONFIRMED_CAST") On 2016/05/17 09:16:53, dgn wrote: > On 2016/05/16 23:47:29, dfalcantara wrote: > > Adding a type of FindBugs suppression is uglier than any adjusting the code to > > account for the cast. This will be one of only three places in the entire > > codebase that checks for this instead of using instanceof, and I don't want to > > have another. > > Done. Do you know who FindBugs doesn't complain for the many other cases where > we wildly cast things? (findViewById for example) Honestly don't have a clue :/ I normally just do whatever is necessary to appease FindBugs because it's *generally* correct. Maybe it's configured to ignore certain Android casts? We'd have walls of warnings, otherwise.
Message was sent while issue was closed.
https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1928063002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:191: @SuppressFBWarnings("BC_UNCONFIRMED_CAST") On 2016/05/17 09:16:53, dgn wrote: > On 2016/05/16 23:47:29, dfalcantara wrote: > > Adding a type of FindBugs suppression is uglier than any adjusting the code to > > account for the cast. This will be one of only three places in the entire > > codebase that checks for this instead of using instanceof, and I don't want to > > have another. > > Done. Do you know who FindBugs doesn't complain for the many other cases where > we wildly cast things? (findViewById for example) Honestly don't have a clue :/ I normally just do whatever is necessary to appease FindBugs because it's *generally* correct. Maybe it's configured to ignore certain Android casts? We'd have walls of warnings, otherwise. |
