Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2273713002:
📰Use the scroll offset to stop the card from peeking (Closed)
Description[NTP Client] Use the scroll offset to stop the card from peeking
Don't use properties of the above-the-fold view holder to
calculate how the peeking card should be, as it is sometimes
not already attached to the recycler view and thus not
accessible.
BUG=633511
Committed: https://crrev.com/8d28a70fc5513f85c16ea02abbf2e371cab83e97
Cr-Commit-Position: refs/heads/master@{#414715}
Patch Set 1 #
Total comments: 5
Patch Set 2 : rewrite and rebase #
Total comments: 17
Patch Set 3 : address comments #Patch Set 4 : #
Total comments: 2
Patch Set 5 : address comments #Patch Set 6 : rebase and fix render test #
Total comments: 4
Patch Set 7 : fix test #
Messages
Total messages: 42 (24 generated)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [NTP Client] Use the scroll offset to stop the card from peeking Don't use properties of the above-the-fold view holder to calculate how the peeking card should be, as it is sometimes not already attached to the recycler view and thus not accessible. BUG=633511 ========== to ========== [NTP Client] Use the scroll offset to stop the card from peeking Don't use properties of the above-the-fold view holder to calculate how the peeking card should be, as it is sometimes not already attached to the recycler view and thus not accessible. BUG=633511 ==========
dgn@chromium.org changed reviewers: + bauerb@chromium.org, mvanouwerkerk@chromium.org
PTAL
https://codereview.chromium.org/2273713002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/2273713002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:126: if (getMeasuredHeight() > mParentViewportHeight - mUiConfig.getMaxPeekHeight()) { This does not really seem correct, the code below is used for both the old ntp (without peeking card) and the new one. Maybe it can be conditionally taken into account by considering mCardsUiEnabled? https://codereview.chromium.org/2273713002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:126: if (getMeasuredHeight() > mParentViewportHeight - mUiConfig.getMaxPeekHeight()) { There are multiple places below where mParentViewportHeight is compared to getMeasuredHeight(). Don't those all need to be updated now?
https://codereview.chromium.org/2273713002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:130: NewTabPageLayout aboveTheFold = mRecyclerView.findAboveTheFoldView(); This is not necessary anymore :) https://codereview.chromium.org/2273713002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:197: peekingPercentage = MathUtils.clamp(2f - (float) visible / mMaxPeekPadding, 0f, 1f); Instead of duplicating the logic here about where to stop peeking, you could get the maxPeekHeight from UiConfig and use that to linearly interpolate between |maxPeekHeight| and |mMaxPeekPadding| (which really is |minPeekHeight| here). Actually, if we were just to use the vertical scroll offset to calculate the peeking percentage, would that work? https://codereview.chromium.org/2273713002/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/2273713002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:208: firstCard.setCanPeek(true); Oooh... if we don't actually set this flag from the outside, we can get rid of it completely and just directly check whether we're the first card :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
PTAL. I rewrote the calculation to leave more things in the recyclerview, since it's more aware of what happens and they depend a lot on the state of other views anyway. It should be simpler now.
https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:122: int maxAtfHeight = mParentViewportHeight - mPeekingCardHeight - mTabStripHeight; It took me way too long to figure out that ATF stands for above the fold here :) Maybe just write it out? https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:165: * screen. The name of the parameter describes the intent better here than the comment, I think -- it's the space that is available for the card, we just happen to calculate it at the call site the way it's described in the comment. https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:315: private boolean tryUpdatePeek() { We are duplicating a lot of logic between here and the RecyclerView. It would be nice to have a single method (either here or in the RecyclerView) that would update the state and that we would call when binding a new ViewHolder or when scrolling existing ones. https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:324: // The header is not attached to the recycler view yet. It must mean we area at the top Nit: s/area/are/ https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SectionHeaderViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SectionHeaderViewHolder.java:73: public void updateDisplay(int amountScrolled, boolean canTransition) { Add javadoc for the parameters?
peconn@chromium.org changed reviewers: + peconn@chromium.org
https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:126: // We don't have enough, we will push the peeking card it completely below the fold Nit: remove 'it' https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:140: // We leave more or just enough space than needed for the peeking card. Redistribute Nit: we leave more than or just enough space needed https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SectionHeaderViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SectionHeaderViewHolder.java:83: itemView.requestLayout(); Alternatively you can call itemView.setLayoutParams() which calls requestLayout() itself. I'm not sure which one is a bit more clear.
PTAL https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:122: int maxAtfHeight = mParentViewportHeight - mPeekingCardHeight - mTabStripHeight; On 2016/08/24 16:27:17, Bernhard Bauer wrote: > It took me way too long to figure out that ATF stands for above the fold here :) > Maybe just write it out? Done. https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:126: // We don't have enough, we will push the peeking card it completely below the fold On 2016/08/24 16:49:39, PEConn wrote: > Nit: remove 'it' Done. https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:140: // We leave more or just enough space than needed for the peeking card. Redistribute On 2016/08/24 16:49:39, PEConn wrote: > Nit: we leave more than or just enough space needed Done. https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:165: * screen. On 2016/08/24 16:27:18, Bernhard Bauer wrote: > The name of the parameter describes the intent better here than the comment, I > think -- it's the space that is available for the card, we just happen to > calculate it at the call site the way it's described in the comment. All right, removing. https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:315: private boolean tryUpdatePeek() { On 2016/08/24 16:27:18, Bernhard Bauer wrote: > We are duplicating a lot of logic between here and the RecyclerView. It would be > nice to have a single method (either here or in the RecyclerView) that would > update the state and that we would call when binding a new ViewHolder or when > scrolling existing ones. Done. https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:324: // The header is not attached to the recycler view yet. It must mean we area at the top On 2016/08/24 16:27:18, Bernhard Bauer wrote: > Nit: s/area/are/ Done. https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SectionHeaderViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SectionHeaderViewHolder.java:83: itemView.requestLayout(); On 2016/08/24 16:49:39, PEConn wrote: > Alternatively you can call itemView.setLayoutParams() which calls > requestLayout() itself. I'm not sure which one is a bit more clear. I prefer requestLayout(), I just wanted to keep track of what happens when we don't do this one, since I was trying to remove these calls to get rid of logcat warnings.
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
lgtm https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:165: * screen. On 2016/08/24 22:18:18, dgn wrote: > On 2016/08/24 16:27:18, Bernhard Bauer wrote: > > The name of the parameter describes the intent better here than the comment, I > > think -- it's the space that is available for the card, we just happen to > > calculate it at the call site the way it's described in the comment. > > All right, removing. Sorry, I meant keep the parameter, but change the comment to describe what the value is supposed to be (rather than how it's calculated) :) https://codereview.chromium.org/2273713002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2273713002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:227: peekingCard.updatePeek(0, /*shouldAnimate*/ false); Nit: space inside comment.
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks! https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:165: * screen. On 2016/08/25 09:09:46, Bernhard Bauer wrote: > On 2016/08/24 22:18:18, dgn wrote: > > On 2016/08/24 16:27:18, Bernhard Bauer wrote: > > > The name of the parameter describes the intent better here than the comment, > I > > > think -- it's the space that is available for the card, we just happen to > > > calculate it at the call site the way it's described in the comment. > > > > All right, removing. > > Sorry, I meant keep the parameter, but change the comment to describe what the > value is supposed to be (rather than how it's calculated) :) Done. https://codereview.chromium.org/2273713002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2273713002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:227: peekingCard.updatePeek(0, /*shouldAnimate*/ false); On 2016/08/25 09:09:46, Bernhard Bauer wrote: > Nit: space inside comment. Done.
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, peconn@chromium.org Link to the patchset: https://codereview.chromium.org/2273713002/#ps80001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL again. The issue was that the render tests don't instantiate a proper ATF layout, and just pass an empty view. Instantiating a NTPLayout there that would not ruin the tests would require test specific code to bypass needing to inflate it from XML. Instead I made the NTPLayout set hasSpaceForPeekingCard as a flag in the recyclerview, which is a dependency in the wrong direction, but avoids requiring test workarounds.
https://codereview.chromium.org/2273713002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/2273713002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:116: recyclerView.setHasSpaceForPeekingCard(false); I would probably use a local variable here and only call setHasSpaceForPeekingCard() once. https://codereview.chromium.org/2273713002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:168: * @param availableSpace space (pixels) above the screen fold where the card can be displayed. s/screen //
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, peconn@chromium.org Link to the patchset: https://codereview.chromium.org/2273713002/#ps120001 (title: "fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2273713002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/2273713002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:116: recyclerView.setHasSpaceForPeekingCard(false); On 2016/08/26 14:26:40, Bernhard (OOO until Sep 7) wrote: > I would probably use a local variable here and only call > setHasSpaceForPeekingCard() once. Done. https://codereview.chromium.org/2273713002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2273713002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:168: * @param availableSpace space (pixels) above the screen fold where the card can be displayed. On 2016/08/26 14:26:40, Bernhard (OOO until Sep 7) wrote: > s/screen // Done.
Message was sent while issue was closed.
Description was changed from ========== [NTP Client] Use the scroll offset to stop the card from peeking Don't use properties of the above-the-fold view holder to calculate how the peeking card should be, as it is sometimes not already attached to the recycler view and thus not accessible. BUG=633511 ========== to ========== [NTP Client] Use the scroll offset to stop the card from peeking Don't use properties of the above-the-fold view holder to calculate how the peeking card should be, as it is sometimes not already attached to the recycler view and thus not accessible. BUG=633511 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Client] Use the scroll offset to stop the card from peeking Don't use properties of the above-the-fold view holder to calculate how the peeking card should be, as it is sometimes not already attached to the recycler view and thus not accessible. BUG=633511 ========== to ========== [NTP Client] Use the scroll offset to stop the card from peeking Don't use properties of the above-the-fold view holder to calculate how the peeking card should be, as it is sometimes not already attached to the recycler view and thus not accessible. BUG=633511 Committed: https://crrev.com/8d28a70fc5513f85c16ea02abbf2e371cab83e97 Cr-Commit-Position: refs/heads/master@{#414715} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8d28a70fc5513f85c16ea02abbf2e371cab83e97 Cr-Commit-Position: refs/heads/master@{#414715} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
