From b10724890ee4ffc98949d1689c36a564da182483 Mon Sep 17 00:00:00 2001 From: Thomas Nardone Date: Wed, 17 Jul 2024 18:00:48 -0700 Subject: [PATCH] View recycling - fix API access and disable when not possible (#45484) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/45484 Prior to Android SDK 28, there was no way to tell if a View's pivotX or pivotY were set. Unfortunately this breaks view recycling due to the default behavior of: - `getPivotX()` and `getPivotY()` [initialize as 0](https://android.googlesource.com/platform/frameworks/base/+/android-8.1.0_r81/libs/hwui/RenderProperties.h#643) - As long as they haven't been set, [they actually default to width/2 and height/2](https://android.googlesource.com/platform/frameworks/base/+/android-8.1.0_r81/libs/hwui/RenderProperties.cpp#195). Thus even if we were to check for `getPixotX() == 0`, we wouldn't know if it was specifically set to 0 (making the pivot actually 0), or still just the default value. We'd then need to reset the pivot any time the width or height changed. [`View.resetPivot()`](https://developer.android.com/reference/android/view/View#resetPivot%28%29) was presumably added to fix this in API 28. This diff adds nullability to `prepareToRecycleView()` so we can act accordingly - returning null if the view can't be recycled. Also added a version check for [`setAnimationMatrix()`](https://developer.android.com/reference/android/view/View#setAnimationMatrix%28android.graphics.Matrix%29), which is only available in 29+. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D59827328 fbshipit-source-id: d1729bba347e8af7fb2b57c95ed2e0b66a15d155 --- .../ReactAndroid/api/ReactAndroid.api | 3 ++- .../react/uimanager/BaseViewManager.java | 14 +++++++++++--- .../facebook/react/uimanager/ViewManager.java | 12 ++++++++---- .../react/views/text/ReactRawTextManager.java | 8 ++++++++ .../react/views/text/ReactTextViewManager.java | 17 ++++++++--------- .../react/views/view/ReactViewManager.java | 10 +++++----- .../ReactPropAnnotationSetterSpecTest.kt | 2 ++ .../uimanager/ReactPropAnnotationSetterTest.kt | 4 ++++ .../react/uimanager/ReactPropConstantsTest.kt | 4 ++++ .../uimanager/ReactPropForShadowNodeSpecTest.kt | 2 ++ 10 files changed, 54 insertions(+), 22 deletions(-) diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api index 7d27ba44c69..8cc66c6cd97 100644 --- a/packages/react-native/ReactAndroid/api/ReactAndroid.api +++ b/packages/react-native/ReactAndroid/api/ReactAndroid.api @@ -5286,7 +5286,7 @@ public abstract class com/facebook/react/uimanager/ViewManager : com/facebook/re protected fun onAfterUpdateTransaction (Landroid/view/View;)V public fun onDropViewInstance (Landroid/view/View;)V public fun onSurfaceStopped (I)V - protected fun prepareToRecycleView (Lcom/facebook/react/uimanager/ThemedReactContext;Landroid/view/View;)Landroid/view/View; + protected abstract fun prepareToRecycleView (Lcom/facebook/react/uimanager/ThemedReactContext;Landroid/view/View;)Landroid/view/View; public fun receiveCommand (Landroid/view/View;ILcom/facebook/react/bridge/ReadableArray;)V public fun receiveCommand (Landroid/view/View;Ljava/lang/String;Lcom/facebook/react/bridge/ReadableArray;)V protected fun recycleView (Lcom/facebook/react/uimanager/ThemedReactContext;Landroid/view/View;)Landroid/view/View; @@ -7266,6 +7266,7 @@ public class com/facebook/react/views/text/ReactRawTextManager : com/facebook/re public fun createViewInstance (Lcom/facebook/react/uimanager/ThemedReactContext;)Lcom/facebook/react/views/text/ReactTextView; public fun getName ()Ljava/lang/String; public fun getShadowNodeClass ()Ljava/lang/Class; + protected fun prepareToRecycleView (Lcom/facebook/react/uimanager/ThemedReactContext;Landroid/view/View;)Landroid/view/View; public fun updateExtraData (Landroid/view/View;Ljava/lang/Object;)V } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java index 84bf68d23d0..28262eb7ce1 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java @@ -68,7 +68,7 @@ public abstract class BaseViewManager= Build.VERSION_CODES.P) { + view.resetPivot(); + } else { + // no way of resetting pivot, or knowing whether it is set + return null; + } view.setTop(0); view.setBottom(0); view.setLeft(0); view.setRight(0); view.setElevation(0); - view.setAnimationMatrix(null); + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + // failsafe - should already be set to null when animation finishes + view.setAnimationMatrix(null); + } view.setTag(R.id.transform, null); view.setTag(R.id.transform_origin, null); diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java index aedc311fa38..7ebe7909d94 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java @@ -226,16 +226,20 @@ public abstract class ViewManager int surfaceId = themedReactContext.getSurfaceId(); @Nullable Stack recyclableViews = getRecyclableViewStack(surfaceId); if (recyclableViews != null) { - recyclableViews.push(prepareToRecycleView(themedReactContext, view)); + T recyclableView = prepareToRecycleView(themedReactContext, view); + if (recyclableView != null) { + recyclableViews.push(recyclableView); + } } } /** * Called when a View is removed from the hierarchy. This should be used to reset any properties. + * + * @return {@code view} if it was properly recycled, or {@code null} if it could not be recycled */ - protected T prepareToRecycleView(@NonNull ThemedReactContext reactContext, @NonNull T view) { - return view; - } + protected abstract @Nullable T prepareToRecycleView( + @NonNull ThemedReactContext reactContext, @NonNull T view); /** Called when a View is going to be reused. */ protected T recycleView(@NonNull ThemedReactContext reactContext, @NonNull T view) { diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactRawTextManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactRawTextManager.java index 4a6901198f0..4c2a87352f4 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactRawTextManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactRawTextManager.java @@ -8,6 +8,8 @@ package com.facebook.react.views.text; import android.view.View; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; import com.facebook.react.common.annotations.VisibleForTesting; import com.facebook.react.module.annotations.ReactModule; import com.facebook.react.uimanager.ThemedReactContext; @@ -32,6 +34,12 @@ public class ReactRawTextManager extends ViewManager { } @Override - protected ReactViewGroup prepareToRecycleView( + protected @Nullable ReactViewGroup prepareToRecycleView( @NonNull ThemedReactContext reactContext, ReactViewGroup view) { // BaseViewManager - super.prepareToRecycleView(reactContext, view); - - view.recycleView(); - + ReactViewGroup preparedView = super.prepareToRecycleView(reactContext, view); + if (preparedView != null) { + preparedView.recycleView(); + } return view; } diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropAnnotationSetterSpecTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropAnnotationSetterSpecTest.kt index dbd41441dfc..130531a81ef 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropAnnotationSetterSpecTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropAnnotationSetterSpecTest.kt @@ -29,6 +29,8 @@ class ReactPropAnnotationSetterSpecTest { override fun createViewInstance(reactContext: ThemedReactContext): View = createViewInstance(reactContext) + override fun prepareToRecycleView(reactContext: ThemedReactContext, view: View): View? = null + override fun updateExtraData(root: View, extraData: Any) = Unit } diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropAnnotationSetterTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropAnnotationSetterTest.kt index 996a7617432..8c39f9f9579 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropAnnotationSetterTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropAnnotationSetterTest.kt @@ -65,6 +65,10 @@ class ReactPropAnnotationSetterTest { override fun createViewInstance(reactContext: ThemedReactContext): View = error("This method should not be executed as a part of this test") + override fun prepareToRecycleView(reactContext: ThemedReactContext, view: View): View? { + error("This method should not be executed as a part of this test") + } + override fun updateExtraData(root: View, extraData: Any) = error("This method should not be executed as a part of this test") diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt index 37a700aa3bb..7b2c341c2c7 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt @@ -37,6 +37,10 @@ class ReactPropConstantsTest { error("This method should not be executed as a part of this test") } + override fun prepareToRecycleView(reactContext: ThemedReactContext, view: View): View? { + error("This method should not be executed as a part of this test") + } + override fun getShadowNodeClass(): Class> { return ReactShadowNode::class.java } diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropForShadowNodeSpecTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropForShadowNodeSpecTest.kt index 33b81f311c5..accfd039d9f 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropForShadowNodeSpecTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropForShadowNodeSpecTest.kt @@ -110,6 +110,8 @@ class ReactPropForShadowNodeSpecTest { override fun createViewInstance(reactContext: ThemedReactContext): View = View(null) + override fun prepareToRecycleView(reactContext: ThemedReactContext, view: View): View? = null + override fun updateExtraData(root: View, extraData: Any?) = Unit } }