From 582f4a961e2c4033068432f38f0d13b8b75ab991 Mon Sep 17 00:00:00 2001
From: Tsung-Mao Fang <tmfang@google.com>
Date: Thu, 13 May 2021 21:22:56 +0800
Subject: [PATCH] Fix search highlight issues

This cl fixes two kinds of issues.
1) Highlight two different preferences while clicking a search result.
2) Preference only is  highlighted one time.

For the first problem, we don't allow to reuse view holder in the
highlight scenario, and then allow it later.

The root cause of second problem is scrolling to target preference
needs x milliseconds. Thus, we should wait a few milliseconds, and
then start to highlight preference.

Test: Test some search results, and see the correct behavior
Fix: 186060148
Fix: 186010165
Fix: 187886982
Change-Id: I7df3e34efe39ee386fe9ce91d7d6c52cf390e2e7
---
 .../HighlightablePreferenceGroupAdapter.java  | 29 +++++++++++++------
 ...ghlightablePreferenceGroupAdapterTest.java |  6 ++--
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/src/com/android/settings/widget/HighlightablePreferenceGroupAdapter.java b/src/com/android/settings/widget/HighlightablePreferenceGroupAdapter.java
index 82b9e763a12..6e73382915e 100644
--- a/src/com/android/settings/widget/HighlightablePreferenceGroupAdapter.java
+++ b/src/com/android/settings/widget/HighlightablePreferenceGroupAdapter.java
@@ -121,10 +121,10 @@ public class HighlightablePreferenceGroupAdapter extends PreferenceGroupAdapter
                 && (mHighlightKey != null
                 && TextUtils.equals(mHighlightKey, getItem(position).getKey()))) {
             // This position should be highlighted. If it's highlighted before - skip animation.
-            addHighlightBackground(v, !mFadeInAnimated);
+            addHighlightBackground(holder, !mFadeInAnimated);
         } else if (Boolean.TRUE.equals(v.getTag(R.id.preference_highlighted))) {
             // View with highlight is reused for a view that should not have highlight
-            removeHighlightBackground(v, false /* animate */);
+            removeHighlightBackground(holder, false /* animate */);
         }
     }
 
@@ -141,20 +141,26 @@ public class HighlightablePreferenceGroupAdapter extends PreferenceGroupAdapter
             return;
         }
 
+        // Collapse app bar after 300 milliseconds.
         if (appBarLayout != null) {
             root.postDelayed(() -> {
                 appBarLayout.setExpanded(false, true);
             }, DELAY_COLLAPSE_DURATION_MILLIS);
         }
 
+        // Scroll to correct position after 600 milliseconds.
         root.postDelayed(() -> {
             mHighlightRequested = true;
             // Remove the animator to avoid a RecyclerView crash.
             recyclerView.setItemAnimator(null);
             recyclerView.smoothScrollToPosition(position);
             mHighlightPosition = position;
-            notifyItemChanged(position);
         }, DELAY_HIGHLIGHT_DURATION_MILLIS);
+
+        // Highlight preference after 900 milliseconds.
+        root.postDelayed(() -> {
+            notifyItemChanged(position);
+        }, DELAY_COLLAPSE_DURATION_MILLIS + DELAY_HIGHLIGHT_DURATION_MILLIS);
     }
 
     public boolean isHighlightRequested() {
@@ -162,19 +168,21 @@ public class HighlightablePreferenceGroupAdapter extends PreferenceGroupAdapter
     }
 
     @VisibleForTesting
-    void requestRemoveHighlightDelayed(View v) {
+    void requestRemoveHighlightDelayed(PreferenceViewHolder holder) {
+        final View v = holder.itemView;
         v.postDelayed(() -> {
             mHighlightPosition = RecyclerView.NO_POSITION;
-            removeHighlightBackground(v, true /* animate */);
+            removeHighlightBackground(holder, true /* animate */);
         }, HIGHLIGHT_DURATION);
     }
 
-    private void addHighlightBackground(View v, boolean animate) {
+    private void addHighlightBackground(PreferenceViewHolder holder, boolean animate) {
+        final View v = holder.itemView;
         v.setTag(R.id.preference_highlighted, true);
         if (!animate) {
             v.setBackgroundColor(mHighlightColor);
             Log.d(TAG, "AddHighlight: Not animation requested - setting highlight background");
-            requestRemoveHighlightDelayed(v);
+            requestRemoveHighlightDelayed(holder);
             return;
         }
         mFadeInAnimated = true;
@@ -189,10 +197,12 @@ public class HighlightablePreferenceGroupAdapter extends PreferenceGroupAdapter
         fadeInLoop.setRepeatCount(4);
         fadeInLoop.start();
         Log.d(TAG, "AddHighlight: starting fade in animation");
-        requestRemoveHighlightDelayed(v);
+        holder.setIsRecyclable(false);
+        requestRemoveHighlightDelayed(holder);
     }
 
-    private void removeHighlightBackground(View v, boolean animate) {
+    private void removeHighlightBackground(PreferenceViewHolder holder, boolean animate) {
+        final View v = holder.itemView;
         if (!animate) {
             v.setTag(R.id.preference_highlighted, false);
             v.setBackgroundResource(mNormalBackgroundRes);
@@ -220,6 +230,7 @@ public class HighlightablePreferenceGroupAdapter extends PreferenceGroupAdapter
                 // Animation complete - the background is now white. Change to mNormalBackgroundRes
                 // so it is white and has ripple on touch.
                 v.setBackgroundResource(mNormalBackgroundRes);
+                holder.setIsRecyclable(true);
             }
         });
         colorAnimation.start();
diff --git a/tests/robotests/src/com/android/settings/widget/HighlightablePreferenceGroupAdapterTest.java b/tests/robotests/src/com/android/settings/widget/HighlightablePreferenceGroupAdapterTest.java
index f5e2a50c6fc..62c7bd8091c 100644
--- a/tests/robotests/src/com/android/settings/widget/HighlightablePreferenceGroupAdapterTest.java
+++ b/tests/robotests/src/com/android/settings/widget/HighlightablePreferenceGroupAdapterTest.java
@@ -184,7 +184,7 @@ public class HighlightablePreferenceGroupAdapterTest {
         assertThat(mAdapter.mFadeInAnimated).isTrue();
         assertThat(mViewHolder.itemView.getBackground()).isInstanceOf(ColorDrawable.class);
         assertThat(mViewHolder.itemView.getTag(R.id.preference_highlighted)).isEqualTo(true);
-        verify(mAdapter).requestRemoveHighlightDelayed(mViewHolder.itemView);
+        verify(mAdapter).requestRemoveHighlightDelayed(mViewHolder);
     }
 
     @Test
@@ -197,14 +197,14 @@ public class HighlightablePreferenceGroupAdapterTest {
         // through animation.
         assertThat(mAdapter.mFadeInAnimated).isTrue();
         // remove highlight should be requested.
-        verify(mAdapter).requestRemoveHighlightDelayed(mViewHolder.itemView);
+        verify(mAdapter).requestRemoveHighlightDelayed(mViewHolder);
 
         ReflectionHelpers.setField(mAdapter, "mHighlightPosition", 10);
         mAdapter.updateBackground(mViewHolder, 10);
         // only sets background color once - if it's animation this would be called many times
         verify(mViewHolder.itemView).setBackgroundColor(mAdapter.mHighlightColor);
         // remove highlight should be requested.
-        verify(mAdapter, times(2)).requestRemoveHighlightDelayed(mViewHolder.itemView);
+        verify(mAdapter, times(2)).requestRemoveHighlightDelayed(mViewHolder);
     }
 
     @Test