Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1884883004:
Implement snap scrolling for Zine NTP. (Closed)
DescriptionImplement snap scrolling for Zine NTP.
Use absolute coordinates to smooth scroll to either the top of the NTP or to the top of the articles view.
Additionally, removed NewTabPageRecyclerView.OnScrollListener and related functionality, since NewTabPageRecyclerView inherits that behaviour from RecyclerView.
BUG=596416
Committed: https://crrev.com/e4a2b825f7517623ae026660f81ce96eb965d46f
Cr-Commit-Position: refs/heads/master@{#387880}
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 2
Messages
Total messages: 21 (7 generated)
Description was changed from ========== Implement snap scrolling for Zine NTP. Use absolute coordinates to smooth scroll to either the top of the NTP or to the top of the articles view. BUG=596416 ========== to ========== Implement snap scrolling for Zine NTP. Use absolute coordinates to smooth scroll to either the top of the NTP or to the top of the articles view. Additionally, removed NewTabPageRecyclerView.OnScrollListener and related functionality, since NewTabPageRecyclerView inherits that behaviour from RecyclerView. BUG=596416 ==========
peconn@chromium.org changed reviewers: + bauerb@chromium.org, maybelle@chromium.org
PTAL The actual behaviour of the scroll (what causes it to snap to which points) is still in discussion.
https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:418: // getVerticalScroll is valid only for the recyclerview if the first item is visible. Does this logic work as expected in extreme screen configurations such as landscape on phone, or portrait on tablet?
https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:68: private static final String TAG = "NewTabPageView"; That is not required to be the class name, debug logs are prefixed with a class name and line numbers already... "Ntp" would make it easier to grep/filter logs IMO.
https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:418: // getVerticalScroll is valid only for the recyclerview if the first item is visible. Nit: RecyclerView https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:440: if (!mPendingSnapScroll) return; This shouldn't really happen, no? We only set the flag to false in this method, or in onTouch() where we remove all callbacks beforehand (and conversely, we only schedule this when the flag is true). Can you assert mPendingSnapScroll instead? https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:482: if (mRecyclerView.getHandler() == null) return false; When would this happen? https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:91: LinearSmoothScroller scroller = new LinearSmoothScroller(mContext){ Space before the opening brace.
https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:68: private static final String TAG = "NewTabPageView"; On 2016/04/13 15:51:21, dgn wrote: > That is not required to be the class name, debug logs are prefixed with a class > name and line numbers already... "Ntp" would make it easier to grep/filter logs > IMO. Done. https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:418: // getVerticalScroll is valid only for the recyclerview if the first item is visible. On 2016/04/13 13:40:15, May wrote: > Does this logic work as expected in extreme screen configurations such as > landscape on phone, or portrait on tablet? Not tested on tablet, however landscape on phone essentially means you are unable to access part of Most Visited Items since this ends up always being scrolled off one side of the screen. We'll need to figure out what behaviour we want there. https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:418: // getVerticalScroll is valid only for the recyclerview if the first item is visible. On 2016/04/13 16:15:51, Bernhard Bauer wrote: > Nit: RecyclerView Done. https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:440: if (!mPendingSnapScroll) return; On 2016/04/13 16:15:51, Bernhard Bauer wrote: > This shouldn't really happen, no? We only set the flag to false in this method, > or in onTouch() where we remove all callbacks beforehand (and conversely, we > only schedule this when the flag is true). Can you assert mPendingSnapScroll > instead? Done. https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:482: if (mRecyclerView.getHandler() == null) return false; On 2016/04/13 16:15:51, Bernhard Bauer wrote: > When would this happen? I'm not sure - this body of code was largely lifted from 'initializeSearchBoxScrollHandling' below that sets up scroll handling for the old ScrollView. I'm assuming they came across a case where it was needed. However, I'll remove it and see if something breaks. https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1884883004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:91: LinearSmoothScroller scroller = new LinearSmoothScroller(mContext){ On 2016/04/13 16:15:51, Bernhard Bauer wrote: > Space before the opening brace. Done.
https://codereview.chromium.org/1884883004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1884883004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:451: int targetScroll = mContentView.getHeight() - mContentView.getPaddingTop(); Why do we use pixels to indicate where to scroll to, instead of the adapter position of the item? Wouldn't the latter be less prone to bugs?
https://codereview.chromium.org/1884883004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1884883004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:451: int targetScroll = mContentView.getHeight() - mContentView.getPaddingTop(); On 2016/04/15 12:33:55, May wrote: > Why do we use pixels to indicate where to scroll to, instead of the adapter > position of the item? Wouldn't the latter be less prone to bugs? I did originally try this. Part of the problem is that we don't want to scroll to position 1 (the articles), but a bit before it (so that position 1 appears underneath the toolbar). I ended up implementing smoothScrollToPositionAndOffset, but it was buggy (only scrolling about 80% of the time) and had the logic spread around. I understand using pixel values doesn't seem very clean but I think this approach is quite simple and straight forward and keeps the logic all in one place.
lgtm
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884883004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884883004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by peconn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884883004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884883004/20001
Message was sent while issue was closed.
Description was changed from ========== Implement snap scrolling for Zine NTP. Use absolute coordinates to smooth scroll to either the top of the NTP or to the top of the articles view. Additionally, removed NewTabPageRecyclerView.OnScrollListener and related functionality, since NewTabPageRecyclerView inherits that behaviour from RecyclerView. BUG=596416 ========== to ========== Implement snap scrolling for Zine NTP. Use absolute coordinates to smooth scroll to either the top of the NTP or to the top of the articles view. Additionally, removed NewTabPageRecyclerView.OnScrollListener and related functionality, since NewTabPageRecyclerView inherits that behaviour from RecyclerView. BUG=596416 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Implement snap scrolling for Zine NTP. Use absolute coordinates to smooth scroll to either the top of the NTP or to the top of the articles view. Additionally, removed NewTabPageRecyclerView.OnScrollListener and related functionality, since NewTabPageRecyclerView inherits that behaviour from RecyclerView. BUG=596416 ========== to ========== Implement snap scrolling for Zine NTP. Use absolute coordinates to smooth scroll to either the top of the NTP or to the top of the articles view. Additionally, removed NewTabPageRecyclerView.OnScrollListener and related functionality, since NewTabPageRecyclerView inherits that behaviour from RecyclerView. BUG=596416 Committed: https://crrev.com/e4a2b825f7517623ae026660f81ce96eb965d46f Cr-Commit-Position: refs/heads/master@{#387880} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e4a2b825f7517623ae026660f81ce96eb965d46f Cr-Commit-Position: refs/heads/master@{#387880} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
