Skip to content

Commit

Permalink
Fabric Interop - Properly dispatch integer commands
Browse files Browse the repository at this point in the history
Summary:
This fixes a bug that got reported for the Fabric Interop for Android with Command dispatching.
(See terrylinla/react-native-sketch-canvas#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

Differential Revision: D47600094

fbshipit-source-id: a051eaf145cacaa275f520c46aeea95165f842a0
  • Loading branch information
cortinico authored and facebook-github-bot committed Jul 20, 2023
1 parent 2d3348e commit 8d0684a
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<UIManagerListener> mListeners = new CopyOnWriteArrayList<>();
Expand Down Expand Up @@ -213,14 +214,14 @@ public void executeItems(Queue<MountItem> 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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -427,7 +428,7 @@ public void onCatalystInstanceDestroy() {
// memory immediately.
mDispatchUIFrameCallback.stop();

mEventDispatcher.removeBatchEventDispatchedListener(mEventBeatManager);
mEventDispatcher.removeBatchEventDispatchedListener(mBatchEventDispatchedListener);
mEventDispatcher.unregisterEventEmitter(FABRIC);

mReactApplicationContext.unregisterComponentCallbacks(mViewManagerRegistry);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
}
}
Original file line number Diff line number Diff line change
@@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
public class MyLegacyViewManager extends SimpleViewManager<MyNativeView> {

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) {
Expand Down Expand Up @@ -71,8 +71,7 @@ public final Map<String, Object> getExportedViewConstants() {

@Override
public final Map<String, Object> getExportedCustomBubblingEventTypeConstants() {
Map<String, Object> eventTypeConstants = new HashMap<String, Object>();
eventTypeConstants.putAll(
return new HashMap<>(
MapBuilder.<String, Object>builder()
.put(
"onColorChanged",
Expand All @@ -81,18 +80,29 @@ public final Map<String, Object> 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_BAKGROUND_COLOR:
int sentColor = Color.parseColor(args.getString(0));
view.setBackgroundColor(sentColor);
break;
}
}

@Nullable
@Override
public Map<String, Integer> getCommandsMap() {
Expand Down

0 comments on commit 8d0684a

Please sign in to comment.