From cbf1b39c662eeede356214ba276a105604b75bb2 Mon Sep 17 00:00:00 2001 From: Kudo Chien Date: Thu, 20 Jun 2019 07:44:50 -0700 Subject: [PATCH] Fix Android Picker ArrayOutOfBoundsException during Picker.Item update from a long list (#25276) Summary: axe-fb reported this side effect from my previous commit in https://github.com/facebook/react-native/pull/24793#issuecomment-502202082 After revisited the implementation of Android Spinner, it seems the formal way to update existing adapter is mutating it, i.e. `arrayAdapter.clear()` & `arrayAdapter.addAll()` to update a Spinner Adapter. `setAdapter()` will reset everything including `mDataChanged`. A race condition may happens between rendering a long picker list and reseting adapter. Here is a code snippet: https://snack.expo.io/kudochien/80f810 To reproduce the issue, please select large item (e.g. 500) first and click the button right hand side. Please not to verify this on Expo directly in the meantime, because Expo with RN 0.59 does not include my previous commit. ## Changelog [Android] [Fixed] - Fix Picker ArrayOutOfBoundsException during Picker.Item update from a long list Pull Request resolved: https://github.com/facebook/react-native/pull/25276 Test Plan: 1. Check the test case https://snack.expo.io/kudochien/80f810 will have exception or not. 2. Regression of https://snack.expo.io/Sy1JClEag from https://github.com/facebook/react-native/issues/13351 3. Regression of https://snack.expo.io/kudochien/android-picker-issue from https://github.com/facebook/react-native/issues/22821 4. RNTester Picker example Reviewed By: mdvacca Differential Revision: D15857426 Pulled By: axe-fb fbshipit-source-id: 8ef902447fdd1b8aeab50ad061545cd14c735e51 --- .../react/views/picker/ReactPicker.java | 53 +++++++----- .../views/picker/ReactPickerAdapter.java | 68 ++++++++++++++++ .../react/views/picker/ReactPickerItem.java | 39 +++++++++ .../views/picker/ReactPickerManager.java | 80 ++----------------- 4 files changed, 146 insertions(+), 94 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerAdapter.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerItem.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java index a030a454908..4a529892274 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java @@ -8,24 +8,27 @@ package com.facebook.react.views.picker; import android.content.Context; -import androidx.appcompat.widget.AppCompatSpinner; import android.util.AttributeSet; import android.view.View; import android.widget.AdapterView; import android.widget.Spinner; -import android.widget.SpinnerAdapter; + +import androidx.appcompat.widget.AppCompatSpinner; import com.facebook.react.common.annotations.VisibleForTesting; +import java.util.List; + import javax.annotation.Nullable; public class ReactPicker extends AppCompatSpinner { private int mMode = Spinner.MODE_DIALOG; - private @Nullable Integer mPrimaryColor; private @Nullable OnSelectListener mOnSelectListener; - private @Nullable SpinnerAdapter mStagedAdapter; + private @Nullable List mItems; + private @Nullable List mStagedItems; private @Nullable Integer mStagedSelection; + private @Nullable Integer mStagedPrimaryTextColor; private final OnItemSelectedListener mItemSelectedListener = new OnItemSelectedListener() { @Override @@ -113,8 +116,8 @@ public class ReactPicker extends AppCompatSpinner { return mOnSelectListener; } - /* package */ void setStagedAdapter(final SpinnerAdapter adapter) { - mStagedAdapter = adapter; + /* package */ void setStagedItems(final @Nullable List items) { + mStagedItems = items; } /** @@ -125,6 +128,10 @@ public class ReactPicker extends AppCompatSpinner { mStagedSelection = selection; } + /* package */ void setStagedPrimaryTextColor(@Nullable Integer primaryColor) { + mStagedPrimaryTextColor = primaryColor; + } + /** * Used to commit staged data into ReactPicker view. * During this period, we will disable {@link OnSelectListener#onItemSelected(int)} temporarily, @@ -133,14 +140,19 @@ public class ReactPicker extends AppCompatSpinner { /* package */ void commitStagedData() { setOnItemSelectedListener(null); + ReactPickerAdapter adapter = (ReactPickerAdapter) getAdapter(); final int origSelection = getSelectedItemPosition(); - if (mStagedAdapter != null && mStagedAdapter != getAdapter()) { - setAdapter(mStagedAdapter); - // After setAdapter(), Spinner will reset selection and cause unnecessary onValueChange event. - // Explicitly setup selection again to prevent this. - // Ref: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/widget/AbsSpinner.java#123 - setSelection(origSelection, false); - mStagedAdapter = null; + if (mStagedItems != null && mStagedItems != mItems) { + mItems = mStagedItems; + mStagedItems = null; + if (adapter == null) { + adapter = new ReactPickerAdapter(getContext(), mItems); + setAdapter(adapter); + } else { + adapter.clear(); + adapter.addAll(mItems); + adapter.notifyDataSetChanged(); + } } if (mStagedSelection != null && mStagedSelection != origSelection) { @@ -148,17 +160,16 @@ public class ReactPicker extends AppCompatSpinner { mStagedSelection = null; } + if (mStagedPrimaryTextColor != null && + adapter != null && + mStagedPrimaryTextColor != adapter.getPrimaryTextColor()) { + adapter.setPrimaryTextColor(mStagedPrimaryTextColor); + mStagedPrimaryTextColor = null; + } + setOnItemSelectedListener(mItemSelectedListener); } - public @Nullable Integer getPrimaryColor() { - return mPrimaryColor; - } - - public void setPrimaryColor(@Nullable Integer primaryColor) { - mPrimaryColor = primaryColor; - } - @VisibleForTesting public int getMode() { return mMode; diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerAdapter.java b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerAdapter.java new file mode 100644 index 00000000000..b2415d88df6 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerAdapter.java @@ -0,0 +1,68 @@ +package com.facebook.react.views.picker; + +import android.content.Context; +import android.view.LayoutInflater; +import android.view.View; +import android.view.ViewGroup; +import android.widget.ArrayAdapter; +import android.widget.TextView; + +import com.facebook.infer.annotation.Assertions; + +import java.util.List; + +import javax.annotation.Nullable; + +/* package */ +class ReactPickerAdapter extends ArrayAdapter { + + private final LayoutInflater mInflater; + private @Nullable + Integer mPrimaryTextColor; + + public ReactPickerAdapter(Context context, List data) { + super(context, 0, data); + + mInflater = (LayoutInflater) Assertions.assertNotNull( + context.getSystemService(Context.LAYOUT_INFLATER_SERVICE)); + } + + @Override + public View getView(int position, View convertView, ViewGroup parent) { + return getView(position, convertView, parent, false); + } + + @Override + public View getDropDownView(int position, View convertView, ViewGroup parent) { + return getView(position, convertView, parent, true); + } + + private View getView(int position, View convertView, ViewGroup parent, boolean isDropdown) { + ReactPickerItem item = getItem(position); + if (convertView == null) { + int layoutResId = isDropdown + ? android.R.layout.simple_spinner_dropdown_item + : android.R.layout.simple_spinner_item; + convertView = mInflater.inflate(layoutResId, parent, false); + } + + TextView textView = (TextView) convertView; + textView.setText(item.label); + if (!isDropdown && mPrimaryTextColor != null) { + textView.setTextColor(mPrimaryTextColor); + } else if (item.color != null) { + textView.setTextColor(item.color); + } + + return convertView; + } + + public @Nullable Integer getPrimaryTextColor() { + return mPrimaryTextColor; + } + + public void setPrimaryTextColor(@Nullable Integer primaryTextColor) { + mPrimaryTextColor = primaryTextColor; + notifyDataSetChanged(); + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerItem.java b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerItem.java new file mode 100644 index 00000000000..0473183773d --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerItem.java @@ -0,0 +1,39 @@ +package com.facebook.react.views.picker; + +import com.facebook.react.bridge.ReadableArray; +import com.facebook.react.bridge.ReadableMap; + +import java.util.ArrayList; +import java.util.List; + +import javax.annotation.Nullable; + +/* package */ +class ReactPickerItem { + public final String label; + @Nullable + public final Integer color; + + public ReactPickerItem(final ReadableMap jsMapData) { + label = jsMapData.getString("label"); + + if (jsMapData.hasKey("color") && !jsMapData.isNull("color")) { + color = jsMapData.getInt("color"); + } else { + color = null; + } + } + + @Nullable + public static List createFromJsArrayMap(final ReadableArray jsArrayMap) { + if (jsArrayMap == null) { + return null; + } + + final List items = new ArrayList<>(jsArrayMap.size()); + for (int i = 0; i < jsArrayMap.size(); ++i) { + items.add(new ReactPickerItem(jsArrayMap.getMap(i))); + } + return items; + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerManager.java index 9e70d661f33..a4fc550d9ea 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPickerManager.java @@ -7,16 +7,9 @@ package com.facebook.react.views.picker; -import android.content.Context; -import android.view.LayoutInflater; -import android.view.View; -import android.view.ViewGroup; -import android.widget.ArrayAdapter; import android.widget.Spinner; -import android.widget.TextView; -import com.facebook.infer.annotation.Assertions; + import com.facebook.react.bridge.ReadableArray; -import com.facebook.react.bridge.ReadableMap; import com.facebook.react.uimanager.SimpleViewManager; import com.facebook.react.uimanager.ThemedReactContext; import com.facebook.react.uimanager.UIManagerModule; @@ -24,6 +17,9 @@ import com.facebook.react.uimanager.ViewProps; import com.facebook.react.uimanager.annotations.ReactProp; import com.facebook.react.uimanager.events.EventDispatcher; import com.facebook.react.views.picker.events.PickerItemSelectEvent; + +import java.util.List; + import javax.annotation.Nullable; /** @@ -37,26 +33,13 @@ public abstract class ReactPickerManager extends SimpleViewManager @ReactProp(name = "items") public void setItems(ReactPicker view, @Nullable ReadableArray items) { - if (items != null) { - ReadableMap[] data = new ReadableMap[items.size()]; - for (int i = 0; i < items.size(); i++) { - data[i] = items.getMap(i); - } - ReactPickerAdapter adapter = new ReactPickerAdapter(view.getContext(), data); - adapter.setPrimaryTextColor(view.getPrimaryColor()); - view.setStagedAdapter(adapter); - } else { - view.setStagedAdapter(null); - } + final List pickerItems = ReactPickerItem.createFromJsArrayMap(items); + view.setStagedItems(pickerItems); } @ReactProp(name = ViewProps.COLOR, customType = "Color") public void setColor(ReactPicker view, @Nullable Integer color) { - view.setPrimaryColor(color); - ReactPickerAdapter adapter = (ReactPickerAdapter) view.getAdapter(); - if (adapter != null) { - adapter.setPrimaryTextColor(color); - } + view.setStagedPrimaryTextColor(color); } @ReactProp(name = "prompt") @@ -90,55 +73,6 @@ public abstract class ReactPickerManager extends SimpleViewManager reactContext.getNativeModule(UIManagerModule.class).getEventDispatcher())); } - private static class ReactPickerAdapter extends ArrayAdapter { - - private final LayoutInflater mInflater; - private @Nullable Integer mPrimaryTextColor; - - public ReactPickerAdapter(Context context, ReadableMap[] data) { - super(context, 0, data); - - mInflater = (LayoutInflater) Assertions.assertNotNull( - context.getSystemService(Context.LAYOUT_INFLATER_SERVICE)); - } - - @Override - public View getView(int position, View convertView, ViewGroup parent) { - return getView(position, convertView, parent, false); - } - - @Override - public View getDropDownView(int position, View convertView, ViewGroup parent) { - return getView(position, convertView, parent, true); - } - - private View getView(int position, View convertView, ViewGroup parent, boolean isDropdown) { - ReadableMap item = getItem(position); - - if (convertView == null) { - int layoutResId = isDropdown - ? android.R.layout.simple_spinner_dropdown_item - : android.R.layout.simple_spinner_item; - convertView = mInflater.inflate(layoutResId, parent, false); - } - - TextView textView = (TextView) convertView; - textView.setText(item.getString("label")); - if (!isDropdown && mPrimaryTextColor != null) { - textView.setTextColor(mPrimaryTextColor); - } else if (item.hasKey("color") && !item.isNull("color")) { - textView.setTextColor(item.getInt("color")); - } - - return convertView; - } - - public void setPrimaryTextColor(@Nullable Integer primaryTextColor) { - mPrimaryTextColor = primaryTextColor; - notifyDataSetChanged(); - } - } - private static class PickerEventEmitter implements ReactPicker.OnSelectListener { private final ReactPicker mReactPicker;