Fix crash in time zone picker due to race condition on view updates

am: 201c629fcc

Change-Id: Icd55bf16959da9e67b75ec93c53ef21fdcb9014f

src/com/android/settings/datetime/timezone/BaseTimeZoneAdapter.java

@@ -18,6 +18,7 @@ package com.android.settings.datetime.timezone;
 
 import android.icu.text.BreakIterator;
 import android.support.annotation.NonNull;
+import android.support.annotation.VisibleForTesting;
 import android.support.annotation.WorkerThread;
 import android.support.v7.widget.RecyclerView;
 import android.text.TextUtils;
@@ -42,14 +43,14 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem>
         extends RecyclerView.Adapter<BaseTimeZoneAdapter.ItemViewHolder> {
 
     private final List<T> mOriginalItems;
-    private final OnListItemClickListener mOnListItemClickListener;
+    private final OnListItemClickListener<T> mOnListItemClickListener;
     private final Locale mLocale;
     private final boolean mShowItemSummary;
 
     private List<T> mItems;
     private ArrayFilter mFilter;
 
-    public BaseTimeZoneAdapter(List<T> items, OnListItemClickListener
+    public BaseTimeZoneAdapter(List<T> items, OnListItemClickListener<T>
             onListItemClickListener, Locale locale, boolean showItemSummary) {
         mOriginalItems = items;
         mItems = items;
@@ -69,14 +70,8 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem>
 
     @Override
     public void onBindViewHolder(@NonNull ItemViewHolder holder, int position) {
-        final AdapterItem item = mItems.get(position);
+        holder.setAdapterItem(mItems.get(position));
-        holder.mSummaryFrame.setVisibility(
+        holder.mSummaryFrame.setVisibility(mShowItemSummary ? View.VISIBLE : View.GONE);
-                mShowItemSummary ? View.VISIBLE : View.GONE);
-        holder.mTitleView.setText(item.getTitle());
-        holder.mIconTextView.setText(item.getIconText());
-        holder.mSummaryView.setText(item.getSummary());
-        holder.mTimeView.setText(item.getCurrentTime());
-        holder.setPosition(position);
     }
 
     @Override
@@ -89,8 +84,7 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem>
         return mItems.size();
     }
 
-    public  @NonNull
+    public @NonNull ArrayFilter getFilter() {
-    Filter getFilter() {
         if (mFilter == null) {
             mFilter = new ArrayFilter();
         }
@@ -110,18 +104,18 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem>
         String[] getSearchKeys();
     }
 
-    public static class ItemViewHolder extends RecyclerView.ViewHolder
+    public static class ItemViewHolder<T extends BaseTimeZoneAdapter.AdapterItem>
-            implements View.OnClickListener{
+            extends RecyclerView.ViewHolder implements View.OnClickListener {
 
-        final OnListItemClickListener mOnListItemClickListener;
+        final OnListItemClickListener<T> mOnListItemClickListener;
         final View mSummaryFrame;
         final TextView mTitleView;
         final TextView mIconTextView;
         final TextView mSummaryView;
         final TextView mTimeView;
-        private int mPosition;
+        private T mItem;
 
-        public ItemViewHolder(View itemView, OnListItemClickListener onListItemClickListener) {
+        public ItemViewHolder(View itemView, OnListItemClickListener<T> onListItemClickListener) {
             super(itemView);
             itemView.setOnClickListener(this);
             mSummaryFrame = itemView.findViewById(R.id.summary_frame);
@@ -132,13 +126,17 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem>
             mOnListItemClickListener = onListItemClickListener;
         }
 
-        public void setPosition(int position) {
+        public void setAdapterItem(T item) {
-            mPosition = position;
+            mItem = item;
+            mTitleView.setText(item.getTitle());
+            mIconTextView.setText(item.getIconText());
+            mSummaryView.setText(item.getSummary());
+            mTimeView.setText(item.getCurrentTime());
         }
 
         @Override
         public void onClick(View v) {
-            mOnListItemClickListener.onListItemClick(mPosition);
+            mOnListItemClickListener.onListItemClick(mItem);
         }
     }
 
@@ -151,7 +149,8 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem>
      * require additional pre-processing. Potentially, a trie structure can be used to match
      * prefixes of the search keys.
      */
-    private class ArrayFilter extends Filter {
+    @VisibleForTesting
+    public class ArrayFilter extends Filter {
 
         private BreakIterator mBreakIterator = BreakIterator.getWordInstance(mLocale);
 
@@ -197,8 +196,9 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem>
             return results;
         }
 
+        @VisibleForTesting
         @Override
-        protected void publishResults(CharSequence constraint, FilterResults results) {
+        public void publishResults(CharSequence constraint, FilterResults results) {
             mItems = (List<T>) results.values;
             notifyDataSetChanged();
         }

src/com/android/settings/datetime/timezone/BaseTimeZoneInfoPicker.java

@@ -31,7 +31,6 @@ import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 import java.util.Locale;
-import java.util.Objects;
 
 /**
  * Render a list of {@class TimeZoneInfo} into the list view in {@class BaseTimeZonePicker}
@@ -52,8 +51,8 @@ public abstract class BaseTimeZoneInfoPicker extends BaseTimeZonePicker {
         return mAdapter;
     }
 
-    private void onListItemClick(int position) {
+    private void onListItemClick(TimeZoneInfoItem item) {
-        final TimeZoneInfo timeZoneInfo = mAdapter.getItem(position).mTimeZoneInfo;
+        final TimeZoneInfo timeZoneInfo = item.mTimeZoneInfo;
         getActivity().setResult(Activity.RESULT_OK, prepareResultData(timeZoneInfo));
         getActivity().finish();
     }
@@ -67,7 +66,7 @@ public abstract class BaseTimeZoneInfoPicker extends BaseTimeZonePicker {
     protected static class ZoneAdapter extends BaseTimeZoneAdapter<TimeZoneInfoItem> {
 
         public ZoneAdapter(Context context, List<TimeZoneInfo> timeZones,
-                OnListItemClickListener onListItemClickListener, Locale locale) {
+                OnListItemClickListener<TimeZoneInfoItem> onListItemClickListener, Locale locale) {
             super(createTimeZoneInfoItems(context, timeZones, locale),
                     onListItemClickListener, locale,  true /* showItemSummary */);
         }

src/com/android/settings/datetime/timezone/BaseTimeZonePicker.java

@@ -17,6 +17,7 @@
 package com.android.settings.datetime.timezone;
 
 import android.os.Bundle;
+import android.support.annotation.VisibleForTesting;
 import android.support.v7.widget.LinearLayoutManager;
 import android.support.v7.widget.RecyclerView;
 import android.view.LayoutInflater;
@@ -161,8 +162,8 @@ public abstract class BaseTimeZonePicker extends InstrumentedFragment
         return false;
     }
 
-    public interface OnListItemClickListener {
+    public interface OnListItemClickListener<T extends BaseTimeZoneAdapter.AdapterItem> {
-        void onListItemClick(int position);
+        void onListItemClick(T item);
     }
 
 }

src/com/android/settings/datetime/timezone/RegionSearchPicker.java

@@ -18,15 +18,16 @@ package com.android.settings.datetime.timezone;
 
 import android.app.Activity;
 import android.content.Intent;
-import android.graphics.Paint;
 import android.icu.text.Collator;
 import android.icu.text.LocaleDisplayNames;
 import android.os.Bundle;
+import android.support.annotation.VisibleForTesting;
 import android.util.Log;
 
 import com.android.internal.logging.nano.MetricsProto;
 import com.android.settings.R;
 import com.android.settings.core.SubSettingLauncher;
+import com.android.settings.datetime.timezone.BaseTimeZoneAdapter.AdapterItem;
 import com.android.settings.datetime.timezone.model.FilteredCountryTimeZones;
 import com.android.settings.datetime.timezone.model.TimeZoneData;
 
@@ -63,8 +64,8 @@ public class RegionSearchPicker extends BaseTimeZonePicker {
         return mAdapter;
     }
 
-    private void onListItemClick(int position) {
+    private void onListItemClick(RegionItem item) {
-        final String regionId = mAdapter.getItem(position).getId();
+        final String regionId = item.getId();
         final FilteredCountryTimeZones countryTimeZones = mTimeZoneData.lookupCountryTimeZones(
                 regionId);
         final Activity activity = getActivity();
@@ -119,7 +120,8 @@ public class RegionSearchPicker extends BaseTimeZonePicker {
         return new ArrayList<>(items);
     }
 
-    private static class RegionItem implements BaseTimeZoneAdapter.AdapterItem {
+    @VisibleForTesting
+    static class RegionItem implements AdapterItem {
 
         private final String mId;
         private final String mName;

tests/robotests/src/com/android/settings/datetime/timezone/RegionSearchPickerTest.java

@@ -16,7 +16,20 @@
 
 package com.android.settings.datetime.timezone;
 
+import static com.google.common.truth.Truth.assertThat;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import android.app.Activity;
+import android.app.Fragment;
+import android.widget.Filter;
+import android.widget.LinearLayout;
+
 import com.android.settings.datetime.timezone.BaseTimeZoneAdapter.AdapterItem;
+import com.android.settings.datetime.timezone.BaseTimeZoneAdapter.ItemViewHolder;
+import com.android.settings.datetime.timezone.RegionSearchPicker.RegionItem;
 import com.android.settings.datetime.timezone.model.TimeZoneData;
 import com.android.settings.testutils.SettingsRobolectricTestRunner;
 
@@ -24,17 +37,23 @@ import libcore.util.CountryZonesFinder;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.robolectric.Robolectric;
+import org.robolectric.RuntimeEnvironment;
+import org.robolectric.annotation.Config;
+import org.robolectric.annotation.Implementation;
+import org.robolectric.annotation.Implements;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Locale;
 
-import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.assertEquals;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
-
 @RunWith(SettingsRobolectricTestRunner.class)
+@Config(shadows = {
+        RegionSearchPickerTest.ShadowBaseTimeZonePicker.class,
+        RegionSearchPickerTest.ShadowFragment.class,
+    }
+)
 public class RegionSearchPickerTest {
 
     @Test
@@ -44,16 +63,90 @@ public class RegionSearchPickerTest {
         CountryZonesFinder finder = mock(CountryZonesFinder.class);
         when(finder.lookupAllCountryIsoCodes()).thenReturn(regionList);
 
-        RegionSearchPicker picker = new RegionSearchPicker() {
+        RegionSearchPicker picker = new RegionSearchPicker();
-            @Override
-            protected Locale getLocale() {
-                return Locale.US;
-            }
-        };
         BaseTimeZoneAdapter adapter = picker.createAdapter(new TimeZoneData(finder));
         assertEquals(1, adapter.getItemCount());
         AdapterItem item = adapter.getItem(0);
         assertEquals("United States", item.getTitle().toString());
         assertThat(Arrays.asList(item.getSearchKeys())).contains("United States");
     }
+
+    // Test RegionSearchPicker does not crash due to the wrong assumption that no view is clicked
+    // before all views are updated and after internal data structure is updated for text filtering.
+    // This test mocks the text filtering event and emit click event immediately
+    // http://b/75322108
+    @Test
+    public void clickItemView_duringRegionSearch_shouldNotCrash() {
+        List regionList = new ArrayList();
+        regionList.add("US");
+        CountryZonesFinder finder = mock(CountryZonesFinder.class);
+        when(finder.lookupAllCountryIsoCodes()).thenReturn(regionList);
+
+        // Prepare the picker and adapter
+        RegionSearchPicker picker = new RegionSearchPicker();
+        BaseTimeZoneAdapter<RegionItem> adapter = picker.createAdapter(new TimeZoneData(finder));
+        // Prepare and bind a new ItemViewHolder with United States
+        ItemViewHolder viewHolder = adapter.onCreateViewHolder(
+                new LinearLayout(RuntimeEnvironment.application), 0);
+        adapter.onBindViewHolder(viewHolder, 0);
+        assertEquals(1, adapter.getItemCount());
+
+        // Pretend to search for a unknown region and no result is found.
+        FilterWrapper filterWrapper = new FilterWrapper(adapter.getFilter());
+        filterWrapper.publishEmptyResult("Unknown region 1");
+
+        // Assert that the adapter should have been updated with no item
+        assertEquals(0, adapter.getItemCount());
+        viewHolder.itemView.performClick(); // This should not crash
+    }
+
+    // FilterResults is a protected inner class. Use FilterWrapper to create an empty FilterResults
+    // instance.
+    private static class FilterWrapper extends Filter {
+
+        private final BaseTimeZoneAdapter.ArrayFilter mFilter;
+
+        private FilterWrapper(BaseTimeZoneAdapter.ArrayFilter filter) {
+            mFilter = filter;
+        }
+
+        @Override
+        protected FilterResults performFiltering(CharSequence charSequence) {
+            return null;
+        }
+
+        private void publishEmptyResult(CharSequence charSequence) {
+            FilterResults filterResults = new FilterResults();
+            filterResults.count = 0;
+            filterResults.values = new ArrayList<>();
+            publishResults(charSequence, filterResults);
+        }
+
+        @Override
+        protected void publishResults(CharSequence charSequence, FilterResults filterResults) {
+            mFilter.publishResults(charSequence, filterResults);
+        }
+    }
+
+    // Robolectric can't start android.app.Fragment with support library v4 resources. Pretend
+    // the fragment has started, and provide the objects in context here.
+    @Implements(BaseTimeZonePicker.class)
+    public static class ShadowBaseTimeZonePicker extends ShadowFragment {
+
+        @Implementation
+        protected Locale getLocale() {
+            return Locale.US;
+        }
+    }
+
+    @Implements(Fragment.class)
+    public static class ShadowFragment {
+
+        private Activity mActivity = Robolectric.setupActivity(Activity.class);
+
+        @Implementation
+        public final Activity getActivity() {
+            return mActivity;
+        }
+    }
 }