From f445acf58df111a08846d8c747fa98a4fdb9d7c8 Mon Sep 17 00:00:00 2001 From: Nicola Corti Date: Thu, 20 Jul 2023 03:59:30 -0700 Subject: [PATCH] Fabric Interop - Properly dispatch integer commands (#38527) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/38527 This fixes a bug that got reported for the Fabric Interop for Android with Command dispatching. (See https://github.com/terrylinla/react-native-sketch-canvas/issues/236) The problem is that libraries that were receiving commands as ints with: ``` public void receiveCommand( int surfaceId, int reactTag, int commandId, Nullable ReadableArray commandArgs) { ``` would not receive command with the Fabric Interop for Android. The problem is that with Fabric, events are dispatched as string always. cipolleschi took care of this for iOS, but we realized that the Android part was missing. I'm adding it here. The logic is, if the event is dispatched as a string that represents a number (say `"42"`) and the user has Fabric Interop enabled, then we dispatch the event as `int` (so libraries will keep on working). Changelog: [Android] [Fixed] - Fabric Interop - Properly dispatch integer commands Reviewed By: cipolleschi Differential Revision: D47600094 fbshipit-source-id: 73fd3f5bdee860f1d371541b3bd0aa60a62e2981 --- .../react/fabric/FabricUIManager.java | 47 +++++++++++++---- .../react/fabric/FabricUIManagerTest.kt | 52 +++++++++++++++++++ .../fakes/FakeBatchEventDispatchedListener.kt | 12 +++++ .../uiapp/component/MyLegacyViewManager.java | 20 +++++-- 4 files changed, 117 insertions(+), 14 deletions(-) create mode 100644 packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerTest.kt create mode 100644 packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/fakes/FakeBatchEventDispatchedListener.kt diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index f03d2bafbe6701..a7e80e784b5996 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -55,7 +55,6 @@ import com.facebook.react.common.build.ReactBuildConfig; import com.facebook.react.common.mapbuffer.ReadableMapBuffer; import com.facebook.react.config.ReactFeatureFlags; -import com.facebook.react.fabric.events.EventBeatManager; import com.facebook.react.fabric.events.EventEmitterWrapper; import com.facebook.react.fabric.events.FabricEventEmitter; import com.facebook.react.fabric.interfaces.SurfaceHandler; @@ -65,6 +64,7 @@ import com.facebook.react.fabric.mounting.SurfaceMountingManager; import com.facebook.react.fabric.mounting.SurfaceMountingManager.ViewEvent; import com.facebook.react.fabric.mounting.mountitems.BatchMountItem; +import com.facebook.react.fabric.mounting.mountitems.DispatchCommandMountItem; import com.facebook.react.fabric.mounting.mountitems.MountItem; import com.facebook.react.fabric.mounting.mountitems.MountItemFactory; import com.facebook.react.modules.core.ReactChoreographer; @@ -79,6 +79,7 @@ import com.facebook.react.uimanager.UIManagerHelper; import com.facebook.react.uimanager.ViewManagerPropertyUpdater; import com.facebook.react.uimanager.ViewManagerRegistry; +import com.facebook.react.uimanager.events.BatchEventDispatchedListener; import com.facebook.react.uimanager.events.EventCategoryDef; import com.facebook.react.uimanager.events.EventDispatcher; import com.facebook.react.uimanager.events.EventDispatcherImpl; @@ -168,7 +169,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @NonNull private final MountItemDispatcher mMountItemDispatcher; @NonNull private final ViewManagerRegistry mViewManagerRegistry; - @NonNull private final EventBeatManager mEventBeatManager; + @NonNull private final BatchEventDispatchedListener mBatchEventDispatchedListener; @NonNull private final CopyOnWriteArrayList mListeners = new CopyOnWriteArrayList<>(); @@ -213,14 +214,14 @@ public void executeItems(Queue items) { public FabricUIManager( @NonNull ReactApplicationContext reactContext, @NonNull ViewManagerRegistry viewManagerRegistry, - @NonNull EventBeatManager eventBeatManager) { + @NonNull BatchEventDispatchedListener batchEventDispatchedListener) { mDispatchUIFrameCallback = new DispatchUIFrameCallback(reactContext); mReactApplicationContext = reactContext; mMountingManager = new MountingManager(viewManagerRegistry, mMountItemExecutor); mMountItemDispatcher = new MountItemDispatcher(mMountingManager, new MountItemDispatchListener()); mEventDispatcher = new EventDispatcherImpl(reactContext); - mEventBeatManager = eventBeatManager; + mBatchEventDispatchedListener = batchEventDispatchedListener; mReactApplicationContext.addLifecycleEventListener(this); mViewManagerRegistry = viewManagerRegistry; @@ -388,7 +389,7 @@ public void stopSurface(final int surfaceID) { @Override public void initialize() { mEventDispatcher.registerEventEmitter(FABRIC, new FabricEventEmitter(this)); - mEventDispatcher.addBatchEventDispatchedListener(mEventBeatManager); + mEventDispatcher.addBatchEventDispatchedListener(mBatchEventDispatchedListener); if (ENABLE_FABRIC_PERF_LOGS) { mDevToolsReactPerfLogger = new DevToolsReactPerfLogger(); mDevToolsReactPerfLogger.addDevToolsReactPerfLoggerListener(FABRIC_PERF_LOGGER); @@ -427,7 +428,7 @@ public void onCatalystInstanceDestroy() { // memory immediately. mDispatchUIFrameCallback.stop(); - mEventDispatcher.removeBatchEventDispatchedListener(mEventBeatManager); + mEventDispatcher.removeBatchEventDispatchedListener(mBatchEventDispatchedListener); mEventDispatcher.unregisterEventEmitter(FABRIC); mReactApplicationContext.unregisterComponentCallbacks(mViewManagerRegistry); @@ -1047,9 +1048,17 @@ public void dispatchCommand( final int reactTag, final String commandId, @Nullable final ReadableArray commandArgs) { - mMountItemDispatcher.dispatchCommandMountItem( - MountItemFactory.createDispatchCommandMountItem( - surfaceId, reactTag, commandId, commandArgs)); + if (ReactFeatureFlags.unstable_useFabricInterop) { + // For Fabric Interop, we check if the commandId is an integer. If it is, we use the integer + // overload of dispatchCommand. Otherwise, we use the string overload. + // and the events won't be correctly dispatched. + mMountItemDispatcher.dispatchCommandMountItem( + createDispatchCommandMountItemForInterop(surfaceId, reactTag, commandId, commandArgs)); + } else { + mMountItemDispatcher.dispatchCommandMountItem( + MountItemFactory.createDispatchCommandMountItem( + surfaceId, reactTag, commandId, commandArgs)); + } } @Override @@ -1246,6 +1255,26 @@ public void didDispatchMountItems() { } } + /** + * Util function that takes care of handling commands for Fabric Interop. If the command is a + * string that represents a number (say "42"), it will be parsed as an integer and the + * corresponding dispatch command mount item will be created. + */ + /* package */ DispatchCommandMountItem createDispatchCommandMountItemForInterop( + final int surfaceId, + final int reactTag, + final String commandId, + @Nullable final ReadableArray commandArgs) { + try { + int commandIdInteger = Integer.parseInt(commandId); + return MountItemFactory.createDispatchCommandMountItem( + surfaceId, reactTag, commandIdInteger, commandArgs); + } catch (NumberFormatException e) { + return MountItemFactory.createDispatchCommandMountItem( + surfaceId, reactTag, commandId, commandArgs); + } + } + private class DispatchUIFrameCallback extends GuardedFrameCallback { private volatile boolean mIsMountingEnabled = true; diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerTest.kt new file mode 100644 index 00000000000000..35d68ac6656a74 --- /dev/null +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerTest.kt @@ -0,0 +1,52 @@ +package com.facebook.react.fabric + +import com.facebook.react.bridge.ReactApplicationContext +import com.facebook.react.fabric.events.EventBeatManager +import com.facebook.react.uimanager.ViewManagerRegistry +import com.facebook.react.uimanager.events.BatchEventDispatchedListener +import com.facebook.testutils.fakes.FakeBatchEventDispatchedListener +import com.facebook.testutils.shadows.ShadowSoLoader +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.annotation.Config + +@RunWith(RobolectricTestRunner::class) +@Config(shadows = [ShadowSoLoader::class]) +class FabricUIManagerTest { + + private lateinit var reactContext: ReactApplicationContext + private lateinit var viewManagerRegistry: ViewManagerRegistry + private lateinit var batchEventDispatchedListener: BatchEventDispatchedListener + private lateinit var underTest: FabricUIManager + + @Before + fun setup() { + reactContext = ReactApplicationContext(RuntimeEnvironment.getApplication()) + viewManagerRegistry = ViewManagerRegistry(emptyList()) + batchEventDispatchedListener = FakeBatchEventDispatchedListener() + underTest = FabricUIManager(reactContext, viewManagerRegistry, batchEventDispatchedListener) + } + + @Test + fun createDispatchCommandMountItemForInterop_withValidString_returnsStringEvent() { + val command = underTest.createDispatchCommandMountItemForInterop(11, 1, "anEvent", null) + + // DispatchStringCommandMountItem is package private so we can `as` check it. + val className = command::class.java.name.substringAfterLast(".") + assertEquals("DispatchStringCommandMountItem", className) + } + + @Test + fun createDispatchCommandMountItemForInterop_withValidInt_returnsIntEvent() { + val command = underTest.createDispatchCommandMountItemForInterop(11, 1, "42", null) + + // DispatchIntCommandMountItem is package private so we can `as` check it. + val className = command::class.java.name.substringAfterLast(".") + assertEquals("DispatchIntCommandMountItem", className) + } +} \ No newline at end of file diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/fakes/FakeBatchEventDispatchedListener.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/fakes/FakeBatchEventDispatchedListener.kt new file mode 100644 index 00000000000000..4ef8849d9b096f --- /dev/null +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/fakes/FakeBatchEventDispatchedListener.kt @@ -0,0 +1,12 @@ +package com.facebook.testutils.fakes + +import com.facebook.react.uimanager.events.BatchEventDispatchedListener + +/** + * A fake [BatchEventDispatchedListener] for testing that does nothing. + */ +class FakeBatchEventDispatchedListener : BatchEventDispatchedListener { + override fun onBatchEventDispatched() { + // do nothing + } +} \ No newline at end of file diff --git a/packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/component/MyLegacyViewManager.java b/packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/component/MyLegacyViewManager.java index a192837166ffa1..e2f34691472d88 100644 --- a/packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/component/MyLegacyViewManager.java +++ b/packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/component/MyLegacyViewManager.java @@ -27,7 +27,7 @@ public class MyLegacyViewManager extends SimpleViewManager { public static final String REACT_CLASS = "RNTMyLegacyNativeView"; - private static final Integer COMMAND_CHANGE_BACKGROUND_COLOR = 42; + private static final int COMMAND_CHANGE_BACKGROUND_COLOR = 42; private final ReactApplicationContext mCallerContext; public MyLegacyViewManager(ReactApplicationContext reactContext) { @@ -71,8 +71,7 @@ public final Map getExportedViewConstants() { @Override public final Map getExportedCustomBubblingEventTypeConstants() { - Map eventTypeConstants = new HashMap(); - eventTypeConstants.putAll( + return new HashMap<>( MapBuilder.builder() .put( "onColorChanged", @@ -81,18 +80,29 @@ public final Map getExportedCustomBubblingEventTypeConstants() { MapBuilder.of( "bubbled", "onColorChanged", "captured", "onColorChangedCapture"))) .build()); - return eventTypeConstants; } @Override public void receiveCommand( @NonNull MyNativeView view, String commandId, @Nullable ReadableArray args) { - if (commandId.contentEquals(COMMAND_CHANGE_BACKGROUND_COLOR.toString())) { + if (commandId.contentEquals("changeBackgroundColor")) { int sentColor = Color.parseColor(args.getString(0)); view.setBackgroundColor(sentColor); } } + @SuppressWarnings("deprecation") // We intentionally want to test against the legacy API here. + @Override + public void receiveCommand( + @NonNull MyNativeView view, int commandId, @Nullable ReadableArray args) { + switch (commandId) { + case COMMAND_CHANGE_BACKGROUND_COLOR: + int sentColor = Color.parseColor(args.getString(0)); + view.setBackgroundColor(sentColor); + break; + } + } + @Nullable @Override public Map getCommandsMap() {