Fix clear selection of bookmarks after sync

Currently, the selected bookmarks are all cleared whenever there is a
change on another device. This behavior does hurt user experience and is
also different from that in iOS version. This CL only clears bookmarks
which are removed on other devices and keeps the rest of selected ones.

Bug: 999269
Change-Id: Ic86a8e6f76b0160118caf712966697c9b3af136c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808014
Commit-Queue: Lijin Shen <lazzzis@google.com>
Reviewed-by: Theresa  <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698968}

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java

@@ -15,7 +15,6 @@ import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkItem;
-import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkModelObserver;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.tabmodel.TabLaunchType;
 import org.chromium.chrome.browser.tabmodel.document.TabDelegate;
@@ -38,13 +37,6 @@ public class BookmarkActionBar extends SelectableListToolbar<BookmarkId>
     private BookmarkItem mCurrentFolder;
     private BookmarkDelegate mDelegate;
 
-    private BookmarkModelObserver mBookmarkModelObserver = new BookmarkModelObserver() {
-        @Override
-        public void bookmarkModelChanged() {
-            onSelectionStateChange(mDelegate.getSelectionDelegate().getSelectedItemsAsList());
-        }
-    };
-
     public BookmarkActionBar(Context context, AttributeSet attrs) {
         super(context, attrs);
         setNavigationOnClickListener(this);
@@ -163,7 +155,6 @@ public class BookmarkActionBar extends SelectableListToolbar<BookmarkId>
         mDelegate = delegate;
         mDelegate.addUIObserver(this);
         if (!delegate.isDialogUi()) getMenu().removeItem(R.id.close_menu_id);
-        delegate.getModel().addObserver(mBookmarkModelObserver);
 
         getMenu().setGroupEnabled(R.id.selection_mode_menu_group, true);
     }
@@ -175,7 +166,6 @@ public class BookmarkActionBar extends SelectableListToolbar<BookmarkId>
         if (mDelegate == null) return;
 
         mDelegate.removeUIObserver(this);
-        mDelegate.getModel().removeObserver(mBookmarkModelObserver);
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBridge.java

@@ -5,6 +5,7 @@
 package org.chromium.chrome.browser.bookmarks;
 
 import android.os.SystemClock;
+import android.support.annotation.Nullable;
 import android.text.TextUtils;
 import android.util.Pair;
 
@@ -361,6 +362,7 @@ public class BookmarkBridge {
      * @return A BookmarkItem instance for the given BookmarkId.
      *         <code>null</code> if it doesn't exist.
      */
+    @Nullable
     public BookmarkItem getBookmarkById(BookmarkId id) {
         assert mIsNativeBookmarkModelLoaded;
         return BookmarkBridgeJni.get().getBookmarkByID(
@@ -754,7 +756,8 @@ public class BookmarkBridge {
     /**
      * Notifies the observer that bookmark model has been loaded.
      */
-    protected void notifyBookmarkModelLoaded() {
+    @VisibleForTesting
+    public void notifyBookmarkModelLoaded() {
         // Call isBookmarkModelLoaded() to do the check since it could be overridden by the child
         // class to add the addition logic.
         if (isBookmarkModelLoaded()) {

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java

@@ -146,7 +146,8 @@ class BookmarkItemsAdapter extends RecyclerView.Adapter<RecyclerView.ViewHolder>
     /**
      * @return The position of the given bookmark in adapter. Will return -1 if not found.
      */
-    private int getPositionForBookmark(BookmarkId bookmark) {
+    @Override
+    public int getPositionForBookmark(BookmarkId bookmark) {
         assert bookmark != null;
         int position = -1;
         for (int i = 0; i < getItemCount(); i++) {

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkManager.java

@@ -8,6 +8,7 @@ import android.app.Activity;
 import android.app.ActivityManager;
 import android.content.Context;
 import android.support.v7.widget.RecyclerView;
+import android.support.v7.widget.RecyclerView.AdapterDataObserver;
 import android.text.TextUtils;
 import android.view.View;
 import android.view.ViewGroup;
@@ -76,6 +77,7 @@ public class BookmarkManager
     // TODO(crbug.com/160194): Clean up after bookmark reordering launches.
     private ItemsAdapter mAdapter;
     private BookmarkDragStateDelegate mDragStateDelegate;
+    private AdapterDataObserver mAdapterDataObserver;
 
     /**
      * An adapter responsible for managing bookmark items.
@@ -85,11 +87,14 @@ public class BookmarkManager
         void notifyDataSetChanged();
         void onBookmarkDelegateInitialized(BookmarkDelegate bookmarkDelegate);
         void search(String query);
+        void registerAdapterDataObserver(AdapterDataObserver observer);
+        void unregisterAdapterDataObserver(AdapterDataObserver observer);
 
         void moveUpOne(BookmarkId bookmarkId);
         void moveDownOne(BookmarkId bookmarkId);
 
         void highlightBookmark(BookmarkId bookmarkId);
+        int getPositionForBookmark(BookmarkId bookmarkId);
     }
 
     private final BookmarkModelObserver mBookmarkModelObserver = new BookmarkModelObserver() {
@@ -112,7 +117,6 @@ public class BookmarkManager
                     openFolder(parent.getId());
                 }
             }
-            mSelectionDelegate.clearSelection();
 
             // This is necessary as long as we rely on RecyclerView.ItemDecorations to apply padding
             // at the bottom of the bookmarks list to avoid the bottom navigation menu. This ensures
@@ -121,12 +125,6 @@ public class BookmarkManager
             mAdapter.notifyDataSetChanged();
         }
 
-        @Override
-        public void bookmarkNodeMoved(BookmarkItem oldParent, int oldIndex, BookmarkItem newParent,
-                int newIndex) {
-            mSelectionDelegate.clearSelection();
-        }
-
         @Override
         public void bookmarkModelChanged() {
             // If the folder no longer exists in folder mode, we need to fall back. Relying on the
@@ -134,7 +132,6 @@ public class BookmarkManager
             if (getCurrentState() == BookmarkUIState.STATE_FOLDER) {
                 setState(mStateStack.peek());
             }
-            mSelectionDelegate.clearSelection();
         }
     };
 
@@ -203,7 +200,10 @@ public class BookmarkManager
         mSelectionDelegate = new SelectionDelegate<BookmarkId>() {
             @Override
             public boolean toggleSelectionForItem(BookmarkId bookmark) {
-                if (!mBookmarkModel.getBookmarkById(bookmark).isEditable()) return false;
+                if (mBookmarkModel.getBookmarkById(bookmark) != null
+                        && !mBookmarkModel.getBookmarkById(bookmark).isEditable()) {
+                    return false;
+                }
                 return super.toggleSelectionForItem(bookmark);
             }
         };
@@ -227,6 +227,18 @@ public class BookmarkManager
         } else {
             mAdapter = new BookmarkItemsAdapter(activity);
         }
+        mAdapterDataObserver = new AdapterDataObserver() {
+            @Override
+            public void onItemRangeRemoved(int positionStart, int itemCount) {
+                syncAdapterAndSelectionDelegate();
+            }
+
+            @Override
+            public void onChanged() {
+                syncAdapterAndSelectionDelegate();
+            }
+        };
+        mAdapter.registerAdapterDataObserver(mAdapterDataObserver);
         mRecyclerView = mSelectableListLayout.initializeRecyclerView(
                 (RecyclerView.Adapter<RecyclerView.ViewHolder>) mAdapter);
 
@@ -296,6 +308,7 @@ public class BookmarkManager
      * Destroys and cleans up itself. This must be called after done using this class.
      */
     public void onDestroyed() {
+        mAdapter.unregisterAdapterDataObserver(mAdapterDataObserver);
         mIsDestroyed = true;
         RecordUserAction.record("MobileBookmarkManagerClose");
         mSelectableListLayout.onDestroyed();
@@ -446,13 +459,25 @@ public class BookmarkManager
             }
         }
 
-        mSelectionDelegate.clearSelection();
-
         for (BookmarkUIObserver observer : mUIObservers) {
             notifyStateChange(observer);
         }
     }
 
+    // TODO(lazzzis): This method can be moved to adapter after bookmark reordering launches.
+    /**
+     * Some bookmarks may be moved to another folder or removed in another devices. However, it may
+     * still be stored by {@link #mSelectionDelegate}, which causes incorrect selection counting.
+     */
+    private void syncAdapterAndSelectionDelegate() {
+        for (BookmarkId node : mSelectionDelegate.getSelectedItemsAsList()) {
+            if (mSelectionDelegate.isItemSelected(node)
+                    && mAdapter.getPositionForBookmark(node) == -1) {
+                mSelectionDelegate.toggleSelectionForItem(node);
+            }
+        }
+    }
+
     @Override
     public void moveDownOne(BookmarkId bookmarkId) {
         mAdapter.moveDownOne(bookmarkId);

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkRow.java

@@ -265,7 +265,8 @@ abstract class BookmarkRow extends SelectableItemView<BookmarkId>
     @Override
     public void onSearchStateSet() {}
 
-    boolean isItemSelected() {
+    @VisibleForTesting
+    public boolean isItemSelected() {
         return mDelegate.getSelectionDelegate().isItemSelected(mBookmarkId);
     }
 

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/ReorderBookmarkItemsAdapter.java

@@ -127,7 +127,8 @@ class ReorderBookmarkItemsAdapter extends DragReorderableListAdapter<BookmarkIte
     /**
      * @return The position of the given bookmark in adapter. Will return -1 if not found.
      */
-    private int getPositionForBookmark(BookmarkId bookmark) {
+    @Override
+    public int getPositionForBookmark(BookmarkId bookmark) {
         assert bookmark != null;
         int position = -1;
         for (int i = 0; i < getItemCount(); i++) {

chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java

@@ -32,6 +32,7 @@ import android.widget.TextView;
 import android.widget.TextView.OnEditorActionListener;
 
 import androidx.annotation.CallSuper;
+import androidx.annotation.IntDef;
 import androidx.annotation.StringRes;
 
 import org.chromium.base.ApiCompatibilityUtils;
@@ -51,6 +52,8 @@ import org.chromium.chrome.browser.widget.selection.SelectionDelegate.SelectionO
 import org.chromium.ui.KeyboardVisibilityDelegate;
 import org.chromium.ui.UiUtils;
 
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.util.List;
 
 /**
@@ -79,6 +82,14 @@ public class SelectableListToolbar<E>
         void onEndSearch();
     }
 
+    @IntDef({ViewType.NORMAL_VIEW, ViewType.SELECTION_VIEW, ViewType.SEARCH_VIEW})
+    @Retention(RetentionPolicy.SOURCE)
+    public @interface ViewType {
+        int NORMAL_VIEW = 0;
+        int SELECTION_VIEW = 1;
+        int SEARCH_VIEW = 2;
+    }
+
     /** No navigation button is displayed. **/
     public static final int NAVIGATION_BUTTON_NONE = 0;
     /** Button to navigate back. This calls {@link #onNavigationBack()}. **/
@@ -133,6 +144,9 @@ public class SelectableListToolbar<E>
     private int mHideInfoStringId;
     private int mExtraMenuItemId;
 
+    // current view type that SelectableListToolbar is showing
+    private int mViewType;
+
     /**
      * Constructor for inflating from XML.
      */
@@ -527,6 +541,10 @@ public class SelectableListToolbar<E>
     }
 
     protected void showNormalView() {
+        // hide overflow menu explicitly: crbug.com/999269
+        hideOverflowMenu();
+
+        mViewType = ViewType.NORMAL_VIEW;
         getMenu().setGroupVisible(mNormalGroupResId, true);
         getMenu().setGroupVisible(mSelectedGroupResId, false);
         if (mHasSearchView) {
@@ -546,6 +564,8 @@ public class SelectableListToolbar<E>
     }
 
     protected void showSelectionView(List<E> selectedItems, boolean wasSelectionEnabled) {
+        mViewType = ViewType.SELECTION_VIEW;
+
         getMenu().setGroupVisible(mNormalGroupResId, false);
         getMenu().setGroupVisible(mSelectedGroupResId, true);
         getMenu().setGroupEnabled(mSelectedGroupResId, !selectedItems.isEmpty());
@@ -563,6 +583,8 @@ public class SelectableListToolbar<E>
     }
 
     private void showSearchViewInternal() {
+        mViewType = ViewType.SEARCH_VIEW;
+
         getMenu().setGroupVisible(mNormalGroupResId, false);
         getMenu().setGroupVisible(mSelectedGroupResId, false);
         mNumberRollView.setVisibility(View.GONE);
@@ -757,4 +779,9 @@ public class SelectableListToolbar<E>
             child.setFocusableInTouchMode(true);
         }
     }
+
+    @VisibleForTesting
+    public @ViewType int getCurrentViewType() {
+        return mViewType;
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java

@@ -161,7 +161,7 @@ public class BookmarkTest {
         }
     }
 
-    private boolean isItemPresentInBookmarkList(final String expectedTitle) {
+    protected boolean isItemPresentInBookmarkList(final String expectedTitle) {
         return TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
             @Override
             public Boolean call() throws Exception {
@@ -531,7 +531,7 @@ public class BookmarkTest {
                 () -> mBookmarkModel.addFolder(mBookmarkModel.getDefaultFolder(), 0, title));
     }
 
-    private void removeBookmark(final BookmarkId bookmarkId) {
+    protected void removeBookmark(final BookmarkId bookmarkId) {
         TestThreadUtils.runOnUiThreadBlocking(() -> mBookmarkModel.deleteBookmark(bookmarkId));
     }