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

Review-Url: https://codereview.chromium.org/2285833002
Cr-Commit-Position: refs/heads/master@{#415597}

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java

@@ -4,12 +4,7 @@
 
 package org.chromium.chrome.browser.ntp.cards;
 
-import android.animation.Animator;
-import android.animation.AnimatorListenerAdapter;
-import android.animation.AnimatorSet;
-import android.animation.ObjectAnimator;
 import android.support.annotation.DrawableRes;
-import android.support.v4.view.animation.FastOutLinearInInterpolator;
 import android.support.v4.view.animation.FastOutSlowInInterpolator;
 import android.support.v7.widget.RecyclerView;
 import android.view.ContextMenu;
@@ -44,9 +39,7 @@ import org.chromium.chrome.browser.util.ViewUtils;
  * parent implementation to reset the private state when a card is recycled.
  */
 public class CardViewHolder extends NewTabPageViewHolder {
-    private static final Interpolator FADE_INTERPOLATOR = new FastOutLinearInInterpolator();
     private static final Interpolator TRANSITION_INTERPOLATOR = new FastOutSlowInInterpolator();
-    private static final int DISMISS_ANIMATION_TIME_MS = 300;
 
     /** Value used for max peeking card height and padding. */
     private final int mMaxPeekPadding;
@@ -192,15 +185,6 @@ public class CardViewHolder extends NewTabPageViewHolder {
         return mPeekingPercentage > 0f;
     }
 
-    @Override
-    public void updateViewStateForDismiss(float dX) {
-        // Any changes in the animation here should be reflected also in |dismiss|
-        // and reset in onBindViewHolder.
-        float input = Math.abs(dX) / itemView.getMeasuredWidth();
-        float alpha = 1 - FADE_INTERPOLATOR.getInterpolation(input);
-        itemView.setAlpha(alpha);
-    }
-
     /**
      * Override this to react when the card is tapped. This method will not be called if the card is
      * currently peeking.
@@ -248,40 +232,6 @@ public class CardViewHolder extends NewTabPageViewHolder {
         return LayoutInflater.from(parent.getContext()).inflate(resourceId, parent, false);
     }
 
-    /**
-     * Animates the card being swiped to the right as if the user had dismissed it. You must check
-     * {@link #isDismissable()} before calling.
-     */
-    protected void dismiss() {
-        assert isDismissable();
-
-        // In case the user pressed dismiss on the context menu after swiping to dismiss.
-        if (getAdapterPosition() == RecyclerView.NO_POSITION) return;
-
-        // Any changes in the animation here should be reflected also in |updateViewStateForDismiss|
-        // and reset in onBindViewHolder.
-        AnimatorSet animation = new AnimatorSet();
-        animation.playTogether(
-                ObjectAnimator.ofFloat(itemView, View.ALPHA, 0f),
-                ObjectAnimator.ofFloat(itemView, View.TRANSLATION_X, (float) itemView.getWidth()));
-
-        animation.setDuration(DISMISS_ANIMATION_TIME_MS);
-        animation.setInterpolator(FADE_INTERPOLATOR);
-        animation.addListener(new AnimatorListenerAdapter() {
-            @Override
-            public void onAnimationStart(Animator animation) {
-                mRecyclerView.onItemDismissStarted(itemView);
-            }
-
-            @Override
-            public void onAnimationEnd(Animator animation) {
-                ((NewTabPageAdapter) mRecyclerView.getAdapter()).dismissItem(CardViewHolder.this);
-                mRecyclerView.onItemDismissFinished(itemView);
-            }
-        });
-        animation.start();
-    }
-
     private static boolean isCard(@NewTabPageItem.ViewType int type) {
         switch (type) {
             case NewTabPageItem.VIEW_TYPE_SNIPPET:
@@ -307,4 +257,8 @@ public class CardViewHolder extends NewTabPageViewHolder {
         if (hasCardAbove && !hasCardBelow) return R.drawable.ntp_card_bottom;
         return R.drawable.ntp_card_single;
     }
+
+    protected NewTabPageRecyclerView getRecyclerView() {
+        return mRecyclerView;
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java

@@ -69,8 +69,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
         @Override
         public void onSwiped(ViewHolder viewHolder, int direction) {
             mRecyclerView.onItemDismissStarted(viewHolder.itemView);
-
-            NewTabPageAdapter.this.dismissItem(viewHolder);
+            NewTabPageAdapter.this.dismissItem(viewHolder.getAdapterPosition());
         }
 
         @Override
@@ -108,7 +107,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
                 float dX, float dY, int actionState, boolean isCurrentlyActive) {
             assert viewHolder instanceof NewTabPageViewHolder;
 
-            ((NewTabPageViewHolder) viewHolder).updateViewStateForDismiss(dX);
+            mRecyclerView.updateViewStateForDismiss(dX, viewHolder);
 
             // The super implementation performs animation and elevation, but only the animation is
             // needed.
@@ -311,6 +310,18 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
         return getGroupPositionOffset(mBottomSpacer);
     }
 
+    public int getSuggestionPosition(String suggestionId) {
+        List<NewTabPageItem> items = getItems();
+        for (int i = 0; i < items.size(); i++) {
+            NewTabPageItem item = items.get(i);
+            if (item instanceof SnippetArticle
+                    && ((SnippetArticle) item).mId.equals(suggestionId)) {
+                return i;
+            }
+        }
+        return RecyclerView.NO_POSITION;
+    }
+
     /** Start a request for new snippets. */
     public void reloadSnippets() {
         SnippetsBridge.fetchSnippets(/*forceRequest=*/true);
@@ -360,10 +371,8 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
         mRecyclerView = (NewTabPageRecyclerView) recyclerView;
     }
 
-    public void dismissItem(ViewHolder itemViewHolder) {
-        int position = itemViewHolder.getAdapterPosition();
+    public void dismissItem(int position) {
         SnippetArticle suggestion = (SnippetArticle) getItems().get(position);
-
         mSuggestionsSource.getSuggestionVisited(suggestion, new Callback<Boolean>() {
             @Override
             public void onResult(Boolean result) {
@@ -373,9 +382,8 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
             }
         });
 
-        itemViewHolder.itemView.announceForAccessibility(
-                itemViewHolder.itemView.getResources().getString(
-                        R.string.ntp_accessibility_item_removed, suggestion.mTitle));
+        mRecyclerView.announceForAccessibility(mRecyclerView.getResources().getString(
+                R.string.ntp_accessibility_item_removed, suggestion.mTitle));
 
         mSuggestionsSource.dismissSuggestion(suggestion);
         SuggestionsSection section = (SuggestionsSection) getGroup(position);

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java

@@ -4,15 +4,21 @@
 
 package org.chromium.chrome.browser.ntp.cards;
 
+import android.animation.Animator;
+import android.animation.AnimatorListenerAdapter;
+import android.animation.AnimatorSet;
+import android.animation.ObjectAnimator;
 import android.content.Context;
 import android.content.res.Resources;
 import android.graphics.Region;
+import android.support.v4.view.animation.FastOutLinearInInterpolator;
 import android.support.v7.widget.LinearLayoutManager;
 import android.support.v7.widget.RecyclerView;
 import android.util.AttributeSet;
 import android.view.GestureDetector;
 import android.view.MotionEvent;
 import android.view.View;
+import android.view.animation.Interpolator;
 import android.view.inputmethod.EditorInfo;
 import android.view.inputmethod.InputConnection;
 
@@ -20,6 +26,7 @@ import org.chromium.base.Log;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ntp.NewTabPageLayout;
 import org.chromium.chrome.browser.ntp.snippets.SectionHeaderViewHolder;
+import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
 import org.chromium.chrome.browser.util.ViewUtils;
 
 /**
@@ -28,6 +35,8 @@ import org.chromium.chrome.browser.util.ViewUtils;
  */
 public class NewTabPageRecyclerView extends RecyclerView {
     private static final String TAG = "NtpCards";
+    private static final Interpolator DISMISS_INTERPOLATOR = new FastOutLinearInInterpolator();
+    private static final int DISMISS_ANIMATION_TIME_MS = 300;
 
     private final GestureDetector mGestureDetector;
     private final LinearLayoutManager mLayoutManager;
@@ -394,4 +403,68 @@ public class NewTabPageRecyclerView extends RecyclerView {
         ViewUtils.gatherTransparentRegionsForOpaqueView(this, region);
         return true;
     }
+
+    /**
+     * Animates the card being swiped to the right as if the user had dismissed it. Any changes to
+     * the animation here should be reflected also in
+     * {@link #updateViewStateForDismiss(float, ViewHolder)} and reset in
+     * {@link CardViewHolder#onBindViewHolder(NewTabPageItem)}.
+     * @param article The item to be dismissed.
+     */
+    public void dismissItemWithAnimation(SnippetArticle suggestion) {
+        // We need to recompute the position, as it might have changed.
+        final int position = getNewTabPageAdapter().getSuggestionPosition(suggestion.mId);
+        if (position == RecyclerView.NO_POSITION) {
+            // The item does not exist anymore, so ignore.
+            return;
+        }
+
+        final View itemView = mLayoutManager.findViewByPosition(position);
+        if (itemView == null) {
+            // The view is not visible anymore, skip the animation.
+            getNewTabPageAdapter().dismissItem(position);
+            return;
+        }
+
+        final ViewHolder viewHolder = getChildViewHolder(itemView);
+        if (!((NewTabPageViewHolder) viewHolder).isDismissable()) {
+            // The item is not dismissable (anymore), so ignore.
+            return;
+        }
+
+        AnimatorSet animation = new AnimatorSet();
+        animation.playTogether(ObjectAnimator.ofFloat(itemView, View.ALPHA, 0f),
+                ObjectAnimator.ofFloat(itemView, View.TRANSLATION_X, (float) itemView.getWidth()));
+
+        animation.setDuration(DISMISS_ANIMATION_TIME_MS);
+        animation.setInterpolator(DISMISS_INTERPOLATOR);
+        animation.addListener(new AnimatorListenerAdapter() {
+            @Override
+            public void onAnimationStart(Animator animation) {
+                NewTabPageRecyclerView.this.onItemDismissStarted(itemView);
+            }
+
+            @Override
+            public void onAnimationEnd(Animator animation) {
+                getNewTabPageAdapter().dismissItem(position);
+                NewTabPageRecyclerView.this.onItemDismissFinished(itemView);
+            }
+        });
+        animation.start();
+    }
+
+    /**
+     * Update the view's state as it is being swiped away. Any changes to the animation here should
+     * be reflected also in {@link #dismissItemWithAnimation(SnippetArticle)} and reset in
+     * {@link CardViewHolder#onBindViewHolder(NewTabPageItem)}.
+     * @param dX The amount of horizontal displacement caused by user's action.
+     * @param viewHolder The view holder containing the view to be updated.
+     */
+    public void updateViewStateForDismiss(float dX, ViewHolder viewHolder) {
+        if (!((NewTabPageViewHolder) viewHolder).isDismissable()) return;
+
+        float input = Math.abs(dX) / viewHolder.itemView.getMeasuredWidth();
+        float alpha = 1 - DISMISS_INTERPOLATOR.getInterpolation(input);
+        viewHolder.itemView.setAlpha(alpha);
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java

@@ -39,12 +39,6 @@ public class NewTabPageViewHolder extends RecyclerView.ViewHolder {
         return false;
     }
 
-    /**
-     * Update the wrapped view's state as it is being swiped away.
-     * @param dX The amount of horizontal displacement caused by user's action
-     */
-    public void updateViewStateForDismiss(float dX) {}
-
     /**
      * Update the layout params for the view holder.
      */

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java

@@ -18,6 +18,7 @@ import android.text.format.DateUtils;
 import android.view.ContextMenu;
 import android.view.Menu;
 import android.view.MenuItem;
+import android.view.MenuItem.OnMenuItemClickListener;
 import android.view.View;
 import android.view.View.MeasureSpec;
 import android.widget.ImageView;
@@ -26,6 +27,7 @@ import android.widget.TextView;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.Callback;
+import org.chromium.base.annotations.SuppressFBWarnings;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.favicon.FaviconHelper.FaviconImageCallback;
@@ -51,8 +53,7 @@ import java.util.concurrent.TimeUnit;
 /**
  * A class that represents the view for a single card snippet.
  */
-public class SnippetArticleViewHolder extends CardViewHolder
-        implements MenuItem.OnMenuItemClickListener, ImpressionTracker.Listener {
+public class SnippetArticleViewHolder extends CardViewHolder implements ImpressionTracker.Listener {
     private static final String PUBLISHER_FORMAT_STRING = "%s - %s";
     private static final int FADE_IN_ANIMATION_TIME_MS = 300;
     private static final int[] FAVICON_SERVICE_SUPPORTED_SIZES = {16, 24, 32, 48, 64};
@@ -84,6 +85,7 @@ public class SnippetArticleViewHolder extends CardViewHolder
     private final boolean mUseFaviconService;
     private final UiConfig mUiConfig;
 
+    @SuppressFBWarnings("URF_UNREAD_FIELD")
     private ImpressionTracker mImpressionTracker;
 
     /**
@@ -94,6 +96,61 @@ public class SnippetArticleViewHolder extends CardViewHolder
         void onCreateContextMenu();
     }
 
+    private static class ContextMenuItemClickListener implements OnMenuItemClickListener {
+        private final SnippetArticle mArticle;
+        private final NewTabPageManager mManager;
+        private final NewTabPageRecyclerView mRecyclerView;
+
+        public ContextMenuItemClickListener(SnippetArticle article,
+                NewTabPageManager newTabPageManager,
+                NewTabPageRecyclerView newTabPageRecyclerView) {
+            mArticle = article;
+            mManager = newTabPageManager;
+            mRecyclerView = newTabPageRecyclerView;
+        }
+
+        @Override
+        public boolean onMenuItemClick(MenuItem item) {
+            // If the user clicks a snippet then immediately long presses they will create a context
+            // menu while the snippet's URL loads in the background. This means that when they press
+            // an item on context menu the NTP will not actually be open. We add this check here to
+            // prevent taking any action if the user has already left the NTP.
+            // https://crbug.com/640468.
+            // TODO(peconn): Instead, close the context menu when a snippet is clicked.
+            if (!ViewCompat.isAttachedToWindow(mRecyclerView)) return true;
+
+            // The UMA is used to compare how the user views the article linked from a snippet.
+            switch (item.getItemId()) {
+                case ID_OPEN_IN_NEW_WINDOW:
+                    NewTabPageUma.recordOpenSnippetMethod(
+                            NewTabPageUma.OPEN_SNIPPET_METHODS_NEW_WINDOW);
+                    mManager.openSnippet(WindowOpenDisposition.NEW_WINDOW, mArticle);
+                    return true;
+                case ID_OPEN_IN_NEW_TAB:
+                    NewTabPageUma.recordOpenSnippetMethod(
+                            NewTabPageUma.OPEN_SNIPPET_METHODS_NEW_TAB);
+                    mManager.openSnippet(WindowOpenDisposition.NEW_FOREGROUND_TAB, mArticle);
+                    return true;
+                case ID_OPEN_IN_INCOGNITO_TAB:
+                    NewTabPageUma.recordOpenSnippetMethod(
+                            NewTabPageUma.OPEN_SNIPPET_METHODS_INCOGNITO);
+                    mManager.openSnippet(WindowOpenDisposition.OFF_THE_RECORD, mArticle);
+                    return true;
+                case ID_SAVE_FOR_OFFLINE:
+                    NewTabPageUma.recordOpenSnippetMethod(
+                            NewTabPageUma.OPEN_SNIPPET_METHODS_SAVE_FOR_OFFLINE);
+                    mManager.openSnippet(WindowOpenDisposition.SAVE_TO_DISK, mArticle);
+                    return true;
+                case ID_REMOVE:
+                    // UMA is recorded during dismissal.
+                    mRecyclerView.dismissItemWithAnimation(mArticle);
+                    return true;
+                default:
+                    return false;
+            }
+        }
+    }
+
     /**
      * Constructs a SnippetCardItemView item used to display snippets
      *
@@ -147,27 +204,31 @@ public class SnippetArticleViewHolder extends CardViewHolder
                 "NewTabPage.Snippets.CardLongPressed", mArticle.mPosition);
         mArticle.recordAgeAndScore("NewTabPage.Snippets.CardLongPressed");
 
+        OnMenuItemClickListener listener =
+                new ContextMenuItemClickListener(mArticle, mNewTabPageManager, getRecyclerView());
+
         // Create a context menu akin to the one shown for MostVisitedItems.
         if (mNewTabPageManager.isOpenInNewWindowEnabled()) {
-            addContextMenuItem(
-                    menu, ID_OPEN_IN_NEW_WINDOW, R.string.contextmenu_open_in_other_window);
+            addContextMenuItem(menu, ID_OPEN_IN_NEW_WINDOW,
+                    R.string.contextmenu_open_in_other_window, listener);
         }
 
-        addContextMenuItem(menu, ID_OPEN_IN_NEW_TAB, R.string.contextmenu_open_in_new_tab);
+        addContextMenuItem(
+                menu, ID_OPEN_IN_NEW_TAB, R.string.contextmenu_open_in_new_tab, listener);
 
         if (mNewTabPageManager.isOpenInIncognitoEnabled()) {
             addContextMenuItem(menu, ID_OPEN_IN_INCOGNITO_TAB,
-                    R.string.contextmenu_open_in_incognito_tab);
+                    R.string.contextmenu_open_in_incognito_tab, listener);
         }
 
         // TODO(peconn): Only show 'Save for Offline' for appropriate snippet types.
         if (SnippetsConfig.isSaveToOfflineEnabled()
                 && OfflinePageBridge.canSavePage(mArticle.mUrl)) {
-            addContextMenuItem(menu, ID_SAVE_FOR_OFFLINE,
-                    R.string.contextmenu_save_offline);
+            addContextMenuItem(
+                    menu, ID_SAVE_FOR_OFFLINE, R.string.contextmenu_save_offline, listener);
         }
 
-        addContextMenuItem(menu, ID_REMOVE, R.string.remove);
+        addContextMenuItem(menu, ID_REMOVE, R.string.remove, listener);
 
         // Disable touch events on the RecyclerView while the context menu is open. This is to
         // prevent the user long pressing to get the context menu then on the same press scrolling
@@ -187,49 +248,9 @@ public class SnippetArticleViewHolder extends CardViewHolder
     /**
      * Convenience method to reduce multi-line function call to single line.
      */
-    private void addContextMenuItem(ContextMenu menu, int id, int resourceId) {
-        menu.add(Menu.NONE, id, Menu.NONE, resourceId).setOnMenuItemClickListener(this);
-    }
-
-    @Override
-    public boolean onMenuItemClick(MenuItem item) {
-        // If the user clicks a snippet then immediately long presses they will create a context
-        // menu while the snippet's URL loads in the background. This means that when they press
-        // an item on context menu the NTP will not actually be open. We add this check here to
-        // prevent taking any action if the user has already left the NTP. https://crbug.com/640468.
-        // TODO(peconn): Instead, close the context menu when a snippet is clicked.
-        if (!ViewCompat.isAttachedToWindow(itemView)) return true;
-
-        // The UMA is used to compare how the user views the article linked from a snippet.
-        switch (item.getItemId()) {
-            case ID_OPEN_IN_NEW_WINDOW:
-                NewTabPageUma.recordOpenSnippetMethod(
-                        NewTabPageUma.OPEN_SNIPPET_METHODS_NEW_WINDOW);
-                mNewTabPageManager.openSnippet(WindowOpenDisposition.NEW_WINDOW, mArticle);
-                return true;
-            case ID_OPEN_IN_NEW_TAB:
-                NewTabPageUma.recordOpenSnippetMethod(
-                        NewTabPageUma.OPEN_SNIPPET_METHODS_NEW_TAB);
-                mNewTabPageManager.openSnippet(WindowOpenDisposition.NEW_FOREGROUND_TAB, mArticle);
-                return true;
-            case ID_OPEN_IN_INCOGNITO_TAB:
-                NewTabPageUma.recordOpenSnippetMethod(
-                        NewTabPageUma.OPEN_SNIPPET_METHODS_INCOGNITO);
-                mNewTabPageManager.openSnippet(WindowOpenDisposition.OFF_THE_RECORD, mArticle);
-                return true;
-            case ID_SAVE_FOR_OFFLINE:
-                NewTabPageUma.recordOpenSnippetMethod(
-                        NewTabPageUma.OPEN_SNIPPET_METHODS_SAVE_FOR_OFFLINE);
-                mNewTabPageManager.openSnippet(WindowOpenDisposition.SAVE_TO_DISK, mArticle);
-                return true;
-            case ID_REMOVE:
-                assert isDismissable() : "Context menu should not be shown for peeking card.";
-                // UMA is recorded during dismissal.
-                dismiss();
-                return true;
-            default:
-                return false;
-        }
+    private static void addContextMenuItem(
+            ContextMenu menu, int id, int resourceId, OnMenuItemClickListener listener) {
+        menu.add(Menu.NONE, id, Menu.NONE, resourceId).setOnMenuItemClickListener(listener);
     }
 
     /**