Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1849173002:
Add a snippet item decoration to recycler view for cards and make the card full bleed with a divide… (Closed)
|
Created:
4 years, 8 months ago by mcwilliams Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@always-show-snippets Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a snippet item decoration to recycler view for cards and make the card full bleed with a divider and padding
BUG=584351
Sreenshots:
https://screenshot.googleplex.com/1D4fGYLxmk0
https://screenshot.googleplex.com/m6rp2Cv5TKJ
Committed: https://crrev.com/ab37a038eafc9bbcbe9ce3fed24472ed4736a0e2
Cr-Commit-Position: refs/heads/master@{#385146}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update comment to adhear to camel case of class name #
Total comments: 8
Patch Set 3 : Changes from review #
Total comments: 4
Patch Set 4 : Update outRect to empty #
Messages
Total messages: 20 (6 generated)
mcwilliams@chromium.org changed reviewers: + bauerb@chromium.org, newt@chromium.org
bauerb@chromium.org: Please review changes in newt@chromium.org: Please review changes in
Description was changed from ========== Add a snippet item decoration to recycler view for cards and make the card full bleed with a divider and padding BUG=584351 ========== to ========== Add a snippet item decoration to recycler view for cards and make the card full bleed with a divider and padding BUG=584351 Sreenshots: https://screenshot.googleplex.com/1D4fGYLxmk0 https://screenshot.googleplex.com/m6rp2Cv5TKJ ==========
lgtm https://codereview.chromium.org/1849173002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1849173002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:12: * A class that decorates the recyclerView elements. Nit: capitalize RecyclerView.
On 2016/04/01 12:42:31, Bernhard Bauer wrote: > lgtm > > https://codereview.chromium.org/1849173002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java > (right): > > https://codereview.chromium.org/1849173002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:12: > * A class that decorates the recyclerView elements. > Nit: capitalize RecyclerView. done
Thanks for attaching screenshots :) Comments inline. https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:389: mRecyclerView.addItemDecoration(new SnippetItemDecoration(4)); Don't hard-code pixel values (or in general, any value). Android devices vary drastically in their resolutions, and if we used pixels to determine layout sizes, users with high-res phones would need a magnifying glass to see anything. Instead, Android uses "density-independent pixels" or "dp" to ensure that visual elements are approximately the same size across devices. Please do read through these two pages for a fuller understanding: - http://developer.android.com/guide/topics/resources/more-resources.html#Dimen... - http://developer.android.com/guide/practices/screens_support.html In this case, you'll want to load a "2dp" dimension resource and let Android will handle the conversion from dp to px: int verticalSpacingPx = context.getResouces().getDimensionPixelSize(R.dimen.snippet_padding); where snippet_padding is a 2dp dimen resource. Also, instead of passing the pixel value into SnippetItemDecoration, it would probably make sense to pass a Context to SnippetItemDecoration's constructor and let SnippetItemDecoration load the snippet_padding resource. https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:18: this.mVerticalSpaceHeight = mVerticalSpaceHeight; remove "m" from the parameter name, and remove "this." https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:22: public void getItemOffsets(Rect outRect, View view, RecyclerView parent, It looks like this method needs to set all fields in outRect. So you should call outRect.setEmpty() as the first line in this method. https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:27: view.setPadding(32, 32, 32, 32); Why did you remove the padding from the XML files and set it here instead? IIUC, this isn't the right place to modify the view; this method should only be modifying outRect. I'd recommend setting the padding in XML unless there's a good reason not to. Second, if you were going to set the padding here, don't hard-code 32. (see discussion about dp and px in NewTabPageView).
On 2016/04/01 16:58:02, newt wrote: > Thanks for attaching screenshots :) > > Comments inline. > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java > (right): > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:389: > mRecyclerView.addItemDecoration(new SnippetItemDecoration(4)); > Don't hard-code pixel values (or in general, any value). > > Android devices vary drastically in their resolutions, and if we used pixels to > determine layout sizes, users with high-res phones would need a magnifying glass > to see anything. > > Instead, Android uses "density-independent pixels" or "dp" to ensure that visual > elements are approximately the same size across devices. > > Please do read through these two pages for a fuller understanding: > - > http://developer.android.com/guide/topics/resources/more-resources.html#Dimen... > - http://developer.android.com/guide/practices/screens_support.html > > In this case, you'll want to load a "2dp" dimension resource and let Android > will handle the conversion from dp to px: > > int verticalSpacingPx = > context.getResouces().getDimensionPixelSize(R.dimen.snippet_padding); > > where snippet_padding is a 2dp dimen resource. > > Also, instead of passing the pixel value into SnippetItemDecoration, it would > probably make sense to pass a Context to SnippetItemDecoration's constructor and > let SnippetItemDecoration load the snippet_padding resource. > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java > (right): > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:18: > this.mVerticalSpaceHeight = mVerticalSpaceHeight; > remove "m" from the parameter name, and remove "this." > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:22: > public void getItemOffsets(Rect outRect, View view, RecyclerView parent, > It looks like this method needs to set all fields in outRect. So you should call > outRect.setEmpty() as the first line in this method. > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:27: > view.setPadding(32, 32, 32, 32); > Why did you remove the padding from the XML files and set it here instead? IIUC, > this isn't the right place to modify the view; this method should only be > modifying outRect. I'd recommend setting the padding in XML unless there's a > good reason not to. > > Second, if you were going to set the padding here, don't hard-code 32. (see > discussion about dp and px in NewTabPageView). PTAL
On 2016/04/04 10:47:44, mcwilliams wrote: > On 2016/04/01 16:58:02, newt wrote: > > Thanks for attaching screenshots :) > > > > Comments inline. > > > > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java > > (right): > > > > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:389: > > mRecyclerView.addItemDecoration(new SnippetItemDecoration(4)); > > Don't hard-code pixel values (or in general, any value). > > > > Android devices vary drastically in their resolutions, and if we used pixels > to > > determine layout sizes, users with high-res phones would need a magnifying > glass > > to see anything. > > > > Instead, Android uses "density-independent pixels" or "dp" to ensure that > visual > > elements are approximately the same size across devices. > > > > Please do read through these two pages for a fuller understanding: > > - > > > http://developer.android.com/guide/topics/resources/more-resources.html#Dimen... > > - http://developer.android.com/guide/practices/screens_support.html > > > > In this case, you'll want to load a "2dp" dimension resource and let Android > > will handle the conversion from dp to px: > > > > int verticalSpacingPx = > > context.getResouces().getDimensionPixelSize(R.dimen.snippet_padding); > > > > where snippet_padding is a 2dp dimen resource. > > > > Also, instead of passing the pixel value into SnippetItemDecoration, it would > > probably make sense to pass a Context to SnippetItemDecoration's constructor > and > > let SnippetItemDecoration load the snippet_padding resource. > > > > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java > > (right): > > > > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:18: > > this.mVerticalSpaceHeight = mVerticalSpaceHeight; > > remove "m" from the parameter name, and remove "this." > > > > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:22: > > public void getItemOffsets(Rect outRect, View view, RecyclerView parent, > > It looks like this method needs to set all fields in outRect. So you should > call > > outRect.setEmpty() as the first line in this method. > > > > > https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:27: > > view.setPadding(32, 32, 32, 32); > > Why did you remove the padding from the XML files and set it here instead? > IIUC, > > this isn't the right place to modify the view; this method should only be > > modifying outRect. I'd recommend setting the padding in XML unless there's a > > good reason not to. > > > > Second, if you were going to set the padding here, don't hard-code 32. (see > > discussion about dp and px in NewTabPageView). > > PTAL
PTAL https://codereview.chromium.org/1849173002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1849173002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:12: * A class that decorates the recyclerView elements. On 2016/04/01 12:42:31, Bernhard Bauer wrote: > Nit: capitalize RecyclerView. Done. https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:389: mRecyclerView.addItemDecoration(new SnippetItemDecoration(4)); On 2016/04/01 16:58:02, newt wrote: > Don't hard-code pixel values (or in general, any value). > > Android devices vary drastically in their resolutions, and if we used pixels to > determine layout sizes, users with high-res phones would need a magnifying glass > to see anything. > > Instead, Android uses "density-independent pixels" or "dp" to ensure that visual > elements are approximately the same size across devices. > > Please do read through these two pages for a fuller understanding: > - > http://developer.android.com/guide/topics/resources/more-resources.html#Dimen... > - http://developer.android.com/guide/practices/screens_support.html > > In this case, you'll want to load a "2dp" dimension resource and let Android > will handle the conversion from dp to px: > > int verticalSpacingPx = > context.getResouces().getDimensionPixelSize(R.dimen.snippet_padding); > > where snippet_padding is a 2dp dimen resource. > > Also, instead of passing the pixel value into SnippetItemDecoration, it would > probably make sense to pass a Context to SnippetItemDecoration's constructor and > let SnippetItemDecoration load the snippet_padding resource. Thanks, new to android dev so apologies for things like this. https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:18: this.mVerticalSpaceHeight = mVerticalSpaceHeight; On 2016/04/01 16:58:02, newt wrote: > remove "m" from the parameter name, and remove "this." Done. https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:22: public void getItemOffsets(Rect outRect, View view, RecyclerView parent, On 2016/04/01 16:58:02, newt wrote: > It looks like this method needs to set all fields in outRect. So you should call > outRect.setEmpty() as the first line in this method. There are a number of other methods on outRect, would prefer not to reset unless you feel strongly about this as there could be defaults set? https://codereview.chromium.org/1849173002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:27: view.setPadding(32, 32, 32, 32); On 2016/04/01 16:58:02, newt wrote: > Why did you remove the padding from the XML files and set it here instead? IIUC, > this isn't the right place to modify the view; this method should only be > modifying outRect. I'd recommend setting the padding in XML unless there's a > good reason not to. > > Second, if you were going to set the padding here, don't hard-code 32. (see > discussion about dp and px in NewTabPageView). Reason for putting this here was to keep the 'decorations' in one place. I have moved this back to the xml
lgtm after comment https://codereview.chromium.org/1849173002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1849173002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:26: public void getItemOffsets(Rect outRect, View view, RecyclerView parent, The javadoc for getItemOffsets() says "If this ItemDecoration does not affect the positioning of item views, it should set all four fields of <code>outRect</code> (left, top, right, bottom) to zero before returning." That's why I think you should call outRect.clear() as the first step in this method.
Thanks for the review https://codereview.chromium.org/1849173002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1849173002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:26: public void getItemOffsets(Rect outRect, View view, RecyclerView parent, On 2016/04/04 20:57:34, newt (no new reviews. mostly) wrote: > The javadoc for getItemOffsets() says "If this ItemDecoration does not affect > the positioning of item views, it should set all four fields of > <code>outRect</code> (left, top, right, bottom) to zero before returning." > That's why I think you should call outRect.clear() as the first step in this > method. Done.
The CQ bit was checked by mcwilliams@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1849173002/#ps60001 (title: "Update outRect to empty")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849173002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849173002/60001
Message was sent while issue was closed.
Description was changed from ========== Add a snippet item decoration to recycler view for cards and make the card full bleed with a divider and padding BUG=584351 Sreenshots: https://screenshot.googleplex.com/1D4fGYLxmk0 https://screenshot.googleplex.com/m6rp2Cv5TKJ ========== to ========== Add a snippet item decoration to recycler view for cards and make the card full bleed with a divider and padding BUG=584351 Sreenshots: https://screenshot.googleplex.com/1D4fGYLxmk0 https://screenshot.googleplex.com/m6rp2Cv5TKJ ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a snippet item decoration to recycler view for cards and make the card full bleed with a divider and padding BUG=584351 Sreenshots: https://screenshot.googleplex.com/1D4fGYLxmk0 https://screenshot.googleplex.com/m6rp2Cv5TKJ ========== to ========== Add a snippet item decoration to recycler view for cards and make the card full bleed with a divider and padding BUG=584351 Sreenshots: https://screenshot.googleplex.com/1D4fGYLxmk0 https://screenshot.googleplex.com/m6rp2Cv5TKJ Committed: https://crrev.com/ab37a038eafc9bbcbe9ce3fed24472ed4736a0e2 Cr-Commit-Position: refs/heads/master@{#385146} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ab37a038eafc9bbcbe9ce3fed24472ed4736a0e2 Cr-Commit-Position: refs/heads/master@{#385146}
Message was sent while issue was closed.
Hey Nicole, one follow-up comment. Welcome to Android UI :) https://codereview.chromium.org/1849173002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1849173002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:26: public void getItemOffsets(Rect outRect, View view, RecyclerView parent, On 2016/04/05 09:58:37, mcwilliams wrote: > On 2016/04/04 20:57:34, newt (no new reviews. mostly) wrote: > > The javadoc for getItemOffsets() says "If this ItemDecoration does not affect > > the positioning of item views, it should set all four fields of > > <code>outRect</code> (left, top, right, bottom) to zero before returning." > > That's why I think you should call outRect.clear() as the first step in this > > method. > > Done. Actually, you need to call outRect.setEmpty() outside the "if" statement. According to the getItemOffsets() API, you always need to set all four fields on outRect, not just modify some fields in some cases. (I imagine the resulting bug would manifest itself as the bottom element having the wrong offsets in some cases.) If it's not clear *why* you need to call outRect.clear() in all cases, it's probably because outRect is reused many times, and Android doesn't reset the Rect object before passing it to another method. This is an optimization to reduce garbage collection, but requires some extra care on the programmer's part. Could you fix this in a tiny follow-up CL?
Message was sent while issue was closed.
https://codereview.chromium.org/1849173002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1849173002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:26: public void getItemOffsets(Rect outRect, View view, RecyclerView parent, On 2016/04/05 16:43:36, newt (no new reviews. mostly) wrote: > On 2016/04/05 09:58:37, mcwilliams wrote: > > On 2016/04/04 20:57:34, newt (no new reviews. mostly) wrote: > > > The javadoc for getItemOffsets() says "If this ItemDecoration does not > affect > > > the positioning of item views, it should set all four fields of > > > <code>outRect</code> (left, top, right, bottom) to zero before returning." > > > That's why I think you should call outRect.clear() as the first step in this > > > method. > > > > Done. > > Actually, you need to call outRect.setEmpty() outside the "if" statement. > According to the getItemOffsets() API, you always need to set all four fields on > outRect, not just modify some fields in some cases. (I imagine the resulting bug > would manifest itself as the bottom element having the wrong offsets in some > cases.) > > If it's not clear *why* you need to call outRect.clear() in all cases, it's > probably because outRect is reused many times, and Android doesn't reset the > Rect object before passing it to another method. This is an optimization to > reduce garbage collection, but requires some extra care on the programmer's > part. > > Could you fix this in a tiny follow-up CL? That makes sense :) https://codereview.chromium.org/1857313002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
