Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285833002:
Fix suggestion dismissing when underlying data changes (Closed)
|
Created:
4 years, 3 months ago by Philipp Keck Modified:
4 years, 3 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, mvanouwerkerk, Marc Treib Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix suggestion dismissing when underlying data changes
When a suggestion is dismissed from the NewTabPage using the 'Remove'
context menu item, the content that it refers to cannot be looked up
from the corresponding ViewHolder, as onBindViewHolder may be called
while the context menu is still open (which would result in the wrong
suggestion being dismissed).
Instead, store the referenced suggestion in the listener for the
context menu items. This requires some additional refactorings.
When the 'Remove' item is clicked for a suggestion that has been
removed for other reasons (section became unavailable, suggestion was
invalidated) in the meantime, ignore the call. When it is clicked for
a suggestion that as moved out of the visible view, skip the
animation and just dismiss it from the underlying data model directly.
BUG=641313
Committed: https://crrev.com/1422e8bdc37459916e46959a4685ab16b0771f9b
Cr-Commit-Position: refs/heads/master@{#415597}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Rebase #Patch Set 3 : Peter's comments #Patch Set 4 : Michael's comments #Patch Set 5 : Rebase #
Total comments: 15
Patch Set 6 : Michael's comments #Patch Set 7 : Add SuppressFBWarnings for mImpressionTracker #Patch Set 8 : Import SuppressFBWarnings from the right namespace #
Messages
Total messages: 35 (20 generated)
pke@google.com changed reviewers: + bauerb@google.com
PTAL The TalkBack works as before.
Description was changed from ========== Fix suggestion dismissing when underlying data changes When a suggestion is dismissed from the NewTabPage using the 'Remove' context menu item, the content that it refers to cannot be looked up from the corresponding ViewHolder, as onBindViewHolder may be called while the context menu is still open (which would result in the wrong suggestion being dismissed). Instead, store the referenced suggestion in the listener for the context menu items. This requires some additional refactorings. When the 'Remove' item is clicked for a suggestion that has been removed for other reasons (section became unavailable, suggestion was invalidated) in the meantime, ignore the call. When it is clicked for a suggestion that as moved out of the visible view, skip the animation and just dismiss it from the underlying data model directly. BUG=641313 ========== to ========== Fix suggestion dismissing when underlying data changes When a suggestion is dismissed from the NewTabPage using the 'Remove' context menu item, the content that it refers to cannot be looked up from the corresponding ViewHolder, as onBindViewHolder may be called while the context menu is still open (which would result in the wrong suggestion being dismissed). Instead, store the referenced suggestion in the listener for the context menu items. This requires some additional refactorings. When the 'Remove' item is clicked for a suggestion that has been removed for other reasons (section became unavailable, suggestion was invalidated) in the meantime, ignore the call. When it is clicked for a suggestion that as moved out of the visible view, skip the animation and just dismiss it from the underlying data model directly. BUG=641313 ==========
pke@google.com changed reviewers: + bauerb@chromium.org - bauerb@google.com
The CQ bit was checked by mvanouwerkerk@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
peconn@chromium.org changed reviewers: + peconn@chromium.org
Just some cursory nits. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:103: this.mArticle = article; We don't need 'this' here? (I'm guessing you used eclipse's auto-generate) https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:218: final SnippetArticle article = mArticle; Is this variable used? (and if it is, any reason for the final?) https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:263: private void addContextMenuItem( This can be static now.
The CQ bit was checked by pke@google.com to run a CQ dry run
https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:103: this.mArticle = article; On 2016/08/26 17:21:20, PEConn wrote: > We don't need 'this' here? (I'm guessing you used eclipse's auto-generate) Removed all unnecessary 'this'. No auto-generate though, in other code bases it's best practice to write 'this' when it applies, so I'm used to it. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:218: final SnippetArticle article = mArticle; On 2016/08/26 17:21:19, PEConn wrote: > Is this variable used? (and if it is, any reason for the final?) Removed, that was a left-over from a previous solution (with an anonymous class here). I decided to make it a static class to prevent people from accidentally using mArticle (or other this-references) in the future. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:263: private void addContextMenuItem( On 2016/08/26 17:21:20, PEConn wrote: > This can be static now. True. Could also be removed alternatively.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:103: this.mArticle = article; On 2016/08/26 17:31:50, Philipp Keck wrote: > On 2016/08/26 17:21:20, PEConn wrote: > > We don't need 'this' here? (I'm guessing you used eclipse's auto-generate) > > Removed all unnecessary 'this'. No auto-generate though, in other code bases > it's best practice to write 'this' when it applies, so I'm used to it. I'm not sure if 'this' adds anything to code readability when we have a naming convention for member variables (you know mArticle will be a member).
https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:301: public int findSuggestion(String suggestionId) { Please rename this to getSuggestionPosition to align with the other methods in this class. Even though I kind of like this one better, the alternative would be a lot more renaming. Maybe add a TODO for lots of renaming if you strongly prefer the findFoo format. https://codereview.chromium.org/2285833002/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/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:402: final int position = ((NewTabPageAdapter) getAdapter()).findSuggestion(article.mId); getNewTabPageAdapter() https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:411: ((NewTabPageAdapter) getAdapter()).dismissItem(position); getNewTabPageAdapter() https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:421: // Any changes in the animation here should be reflected also in |updateViewStateForDismiss| nit: {@link #updateViewStateForDismiss} https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:422: // and reset in onBindViewHolder. I think this is an important part of the contract of how these methods relate. Could you pull this comment up to the main JavaDoc for this method and link to CardViewHolder#onBindViewHolder ? https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:437: ((NewTabPageAdapter) getAdapter()).dismissItem(position); getNewTabPageAdapter() https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:454: // Any changes in the animation here should be reflected also in |dismiss| Did you mean dismissItemWithAnimation? https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:100: public ContextMenuItemClickListener(SnippetArticle article, nit: blank line before this one https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:107: @Override nit: blank line before this one
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:301: public int findSuggestion(String suggestionId) { On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > Please rename this to getSuggestionPosition to align with the other methods in > this class. Even though I kind of like this one better, the alternative would be > a lot more renaming. Maybe add a TODO for lots of renaming if you strongly > prefer the findFoo format. Done. https://codereview.chromium.org/2285833002/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/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:402: final int position = ((NewTabPageAdapter) getAdapter()).findSuggestion(article.mId); On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > getNewTabPageAdapter() Done. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:411: ((NewTabPageAdapter) getAdapter()).dismissItem(position); On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > getNewTabPageAdapter() Done. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:421: // Any changes in the animation here should be reflected also in |updateViewStateForDismiss| On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > nit: {@link #updateViewStateForDismiss} Not sure about this one. {@link} does not work in inline comments because those are not compiled as JavaDoc, so the || format might be preferable here. Other code around here also uses that, and the place where I copied this code from also uses that. But now I moved it to the method's JavaDoc anyway... https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:422: // and reset in onBindViewHolder. On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > I think this is an important part of the contract of how these methods relate. > Could you pull this comment up to the main JavaDoc for this method and link to > CardViewHolder#onBindViewHolder ? Done. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:437: ((NewTabPageAdapter) getAdapter()).dismissItem(position); On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > getNewTabPageAdapter() Done. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:454: // Any changes in the animation here should be reflected also in |dismiss| On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > Did you mean dismissItemWithAnimation? Done. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:100: public ContextMenuItemClickListener(SnippetArticle article, On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > nit: blank line before this one Done. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:103: this.mArticle = article; On 2016/08/26 17:34:33, PEConn wrote: > On 2016/08/26 17:31:50, Philipp Keck wrote: > > On 2016/08/26 17:21:20, PEConn wrote: > > > We don't need 'this' here? (I'm guessing you used eclipse's auto-generate) > > > > Removed all unnecessary 'this'. No auto-generate though, in other code bases > > it's best practice to write 'this' when it applies, so I'm used to it. > > I'm not sure if 'this' adds anything to code readability when we have a naming > convention for member variables (you know mArticle will be a member). Agreed. https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:107: @Override On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > nit: blank line before this one Done.
On 2016/08/29 09:09:02, Philipp Keck wrote: > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java > (right): > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:301: > public int findSuggestion(String suggestionId) { > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > Please rename this to getSuggestionPosition to align with the other methods in > > this class. Even though I kind of like this one better, the alternative would > be > > a lot more renaming. Maybe add a TODO for lots of renaming if you strongly > > prefer the findFoo format. > > Done. > > https://codereview.chromium.org/2285833002/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/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:402: > final int position = ((NewTabPageAdapter) > getAdapter()).findSuggestion(article.mId); > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > getNewTabPageAdapter() > > Done. > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:411: > ((NewTabPageAdapter) getAdapter()).dismissItem(position); > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > getNewTabPageAdapter() > > Done. > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:421: > // Any changes in the animation here should be reflected also in > |updateViewStateForDismiss| > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > nit: {@link #updateViewStateForDismiss} > > Not sure about this one. {@link} does not work in inline comments because those > are not compiled as JavaDoc, so the || format might be preferable here. Other > code around here also uses that, and the place where I copied this code from > also uses that. > But now I moved it to the method's JavaDoc anyway... > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:422: > // and reset in onBindViewHolder. > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > I think this is an important part of the contract of how these methods relate. > > Could you pull this comment up to the main JavaDoc for this method and link to > > CardViewHolder#onBindViewHolder ? > > Done. > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:437: > ((NewTabPageAdapter) getAdapter()).dismissItem(position); > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > getNewTabPageAdapter() > > Done. > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:454: > // Any changes in the animation here should be reflected also in |dismiss| > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > Did you mean dismissItemWithAnimation? > > Done. > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java > (right): > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:100: > public ContextMenuItemClickListener(SnippetArticle article, > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > nit: blank line before this one > > Done. > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:103: > this.mArticle = article; > On 2016/08/26 17:34:33, PEConn wrote: > > On 2016/08/26 17:31:50, Philipp Keck wrote: > > > On 2016/08/26 17:21:20, PEConn wrote: > > > > We don't need 'this' here? (I'm guessing you used eclipse's auto-generate) > > > > > > Removed all unnecessary 'this'. No auto-generate though, in other code bases > > > it's best practice to write 'this' when it applies, so I'm used to it. > > > > I'm not sure if 'this' adds anything to code readability when we have a naming > > convention for member variables (you know mArticle will be a member). > > Agreed. > > https://codereview.chromium.org/2285833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:107: > @Override > On 2016/08/26 17:49:52, Michael van Ouwerkerk wrote: > > nit: blank line before this one > > Done. Seems like this previous email hasn't been sent (at least I haven't received it). Sending again to be sure.
https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:118: if (!ViewCompat.isAttachedToWindow(mNewTabPageRecyclerView)) return true; @peconn Please check this line, where I replaced itemView with mNewTabPageRecyclerView. Not sure if it works like this, but I also don't know how to test it.
The CQ bit was checked by mvanouwerkerk@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 with nits https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:414: public void dismissItemWithAnimation(SnippetArticle article) { nit: While the class is called SnippetArticle, the suggestions may also be e.g. bookmarks. Maybe rename this argument to "suggestion". https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:440: animation.setInterpolator(FADE_INTERPOLATOR); FADE_INTERPOLATOR is used for an animation that both fades and translates. How about DISMISS_INTERPOLATOR? That would also align with DISMISS_ANIMATION_TIME_MS. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:460: * @param dX The amount of horizontal displacement caused by user's action nit: end the sentence with a full stop. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:464: if (!(viewHolder instanceof CardViewHolder)) { Shouldn't this actually look at isDismissable? https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:99: private final NewTabPageManager mNewTabPageManager; nit: just mManager is sufficient https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:100: private final NewTabPageRecyclerView mNewTabPageRecyclerView; nit: just mRecyclerView is sufficient
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...)
https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:118: if (!ViewCompat.isAttachedToWindow(mNewTabPageRecyclerView)) return true; On 2016/08/30 08:18:18, Philipp Keck wrote: > @peconn Please check this line, where I replaced itemView with > mNewTabPageRecyclerView. Not sure if it works like this, but I also don't know > how to test it. That should work. You can test it by trying to reproduce the bug, essentially: - Does the Context Menu work? - If you tap a link, quickly long press on it to get the context menu and pick an action does it crash?
On 2016/08/30 13:56:39, PEConn wrote: > https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java > (right): > > https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:118: > if (!ViewCompat.isAttachedToWindow(mNewTabPageRecyclerView)) return true; > On 2016/08/30 08:18:18, Philipp Keck wrote: > > @peconn Please check this line, where I replaced itemView with > > mNewTabPageRecyclerView. Not sure if it works like this, but I also don't know > > how to test it. > > That should work. You can test it by trying to reproduce the bug, essentially: > - Does the Context Menu work? > - If you tap a link, quickly long press on it to get the context menu and pick > an action does it crash? LGTM with my previous comment and Michael's.
peconn@ Please see the last comment. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:414: public void dismissItemWithAnimation(SnippetArticle article) { On 2016/08/30 11:19:38, Michael van Ouwerkerk wrote: > nit: While the class is called SnippetArticle, the suggestions may also be e.g. > bookmarks. Maybe rename this argument to "suggestion". Done. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:440: animation.setInterpolator(FADE_INTERPOLATOR); On 2016/08/30 11:19:38, Michael van Ouwerkerk wrote: > FADE_INTERPOLATOR is used for an animation that both fades and translates. How > about DISMISS_INTERPOLATOR? That would also align with > DISMISS_ANIMATION_TIME_MS. Done. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:460: * @param dX The amount of horizontal displacement caused by user's action On 2016/08/30 11:19:38, Michael van Ouwerkerk wrote: > nit: end the sentence with a full stop. Done. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:464: if (!(viewHolder instanceof CardViewHolder)) { On 2016/08/30 11:19:38, Michael van Ouwerkerk wrote: > Shouldn't this actually look at isDismissable? This is code is equivalent to the previous implementation that used polymorphism. There are two differences: - Status and Action cards are not dismissable, but instanceof CardViewHolder. But then, this method would never be called for them, right? - A peeking card is not dismissable. In particular, maybe a card can become the peeking while it is being dismissed (seems rather unlikely). So I'm changing it to isDismissable -- seems more logical and roughly equivalent. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:99: private final NewTabPageManager mNewTabPageManager; On 2016/08/30 11:19:38, Michael van Ouwerkerk wrote: > nit: just mManager is sufficient Done. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:100: private final NewTabPageRecyclerView mNewTabPageRecyclerView; On 2016/08/30 11:19:38, Michael van Ouwerkerk wrote: > nit: just mRecyclerView is sufficient Done. https://codereview.chromium.org/2285833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:118: if (!ViewCompat.isAttachedToWindow(mNewTabPageRecyclerView)) return true; On 2016/08/30 13:56:39, PEConn wrote: > On 2016/08/30 08:18:18, Philipp Keck wrote: > > @peconn Please check this line, where I replaced itemView with > > mNewTabPageRecyclerView. Not sure if it works like this, but I also don't know > > how to test it. > > That should work. You can test it by trying to reproduce the bug, essentially: > - Does the Context Menu work? > - If you tap a link, quickly long press on it to get the context menu and pick > an action does it crash? Okay, I checked, it prevents the crash as intended. But if the last way (tap, quickly long press) is a way to reproduce it, doesn't make that the solution proposed in the TODO comment (line 117) ineffective? Because when the snippet is clicked, the context menu is not yet open. We would additionally need to disable the context menu when the tab is already loading another page.
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2285833002/#ps140001 (title: "Import SuppressFBWarnings from the right namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix suggestion dismissing when underlying data changes When a suggestion is dismissed from the NewTabPage using the 'Remove' context menu item, the content that it refers to cannot be looked up from the corresponding ViewHolder, as onBindViewHolder may be called while the context menu is still open (which would result in the wrong suggestion being dismissed). Instead, store the referenced suggestion in the listener for the context menu items. This requires some additional refactorings. When the 'Remove' item is clicked for a suggestion that has been removed for other reasons (section became unavailable, suggestion was invalidated) in the meantime, ignore the call. When it is clicked for a suggestion that as moved out of the visible view, skip the animation and just dismiss it from the underlying data model directly. BUG=641313 ========== to ========== Fix suggestion dismissing when underlying data changes When a suggestion is dismissed from the NewTabPage using the 'Remove' context menu item, the content that it refers to cannot be looked up from the corresponding ViewHolder, as onBindViewHolder may be called while the context menu is still open (which would result in the wrong suggestion being dismissed). Instead, store the referenced suggestion in the listener for the context menu items. This requires some additional refactorings. When the 'Remove' item is clicked for a suggestion that has been removed for other reasons (section became unavailable, suggestion was invalidated) in the meantime, ignore the call. When it is clicked for a suggestion that as moved out of the visible view, skip the animation and just dismiss it from the underlying data model directly. BUG=641313 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fix suggestion dismissing when underlying data changes When a suggestion is dismissed from the NewTabPage using the 'Remove' context menu item, the content that it refers to cannot be looked up from the corresponding ViewHolder, as onBindViewHolder may be called while the context menu is still open (which would result in the wrong suggestion being dismissed). Instead, store the referenced suggestion in the listener for the context menu items. This requires some additional refactorings. When the 'Remove' item is clicked for a suggestion that has been removed for other reasons (section became unavailable, suggestion was invalidated) in the meantime, ignore the call. When it is clicked for a suggestion that as moved out of the visible view, skip the animation and just dismiss it from the underlying data model directly. BUG=641313 ========== to ========== Fix suggestion dismissing when underlying data changes When a suggestion is dismissed from the NewTabPage using the 'Remove' context menu item, the content that it refers to cannot be looked up from the corresponding ViewHolder, as onBindViewHolder may be called while the context menu is still open (which would result in the wrong suggestion being dismissed). Instead, store the referenced suggestion in the listener for the context menu items. This requires some additional refactorings. When the 'Remove' item is clicked for a suggestion that has been removed for other reasons (section became unavailable, suggestion was invalidated) in the meantime, ignore the call. When it is clicked for a suggestion that as moved out of the visible view, skip the animation and just dismiss it from the underlying data model directly. BUG=641313 Committed: https://crrev.com/1422e8bdc37459916e46959a4685ab16b0771f9b Cr-Commit-Position: refs/heads/master@{#415597} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1422e8bdc37459916e46959a4685ab16b0771f9b Cr-Commit-Position: refs/heads/master@{#415597} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
