From 72d0ddc16f2f631003c3486e0a59e50c145ec613 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Thu, 28 Jan 2021 14:01:07 -0800 Subject: [PATCH] Refactor Event to encourage and ease migration to `RCTModernEventEmitter` support Summary: The `Event` interface has been improved such that: 1. `getEventData` is now a default method on Event that returns null 2. `dispatch` has a default implementation that relies on getEventName() and getEventData() 3. `dispatchModern` can detect if surfaceId and event data are present; if not, it falls back to dispatch This will dramatically ease future migrations: at some point in the (distant) future, we can simply delete RCTEventEmitter and all use-cases will be supported by the current `Event` class without needing to introduce a 3rd transitional interface; and 99% of all Event classes can be simplified, delet their `dispatch` implementation and need no further work. At their core, all Events are simply: (1) a name, (2) data, (3) a target (surfaceId and tag). The interface now reflects that but still allows for flexibility of the data and names being generated on-demand if necessary; but for the vast majority of Event classes, code will be dramatically simplified. I also migrate a single Event class, ContentSizeChangeEvent, to use this new method of dispatch. Changelog: [Android][Changed] Added convenience methods to simplify native Event classes and ease migrations Reviewed By: mdvacca Differential Revision: D26043325 fbshipit-source-id: bc308105f7f6e654d45fd156dbf4a2bcbc45819c --- .../events/ContentSizeChangeEvent.java | 11 ++-- .../react/uimanager/events/Event.java | 54 ++++++++++++++++--- .../react/views/image/ImageLoadEvent.java | 18 +------ 3 files changed, 58 insertions(+), 25 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/ContentSizeChangeEvent.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/ContentSizeChangeEvent.java index a28bd72b12e..d65c810b095 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/ContentSizeChangeEvent.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/ContentSizeChangeEvent.java @@ -19,8 +19,13 @@ public class ContentSizeChangeEvent extends Event { private final int mWidth; private final int mHeight; + @Deprecated public ContentSizeChangeEvent(int viewTag, int width, int height) { - super(viewTag); + this(-1, viewTag, width, height); + } + + public ContentSizeChangeEvent(int surfaceId, int viewTag, int width, int height) { + super(surfaceId, viewTag); mWidth = width; mHeight = height; } @@ -31,10 +36,10 @@ public class ContentSizeChangeEvent extends Event { } @Override - public void dispatch(RCTEventEmitter rctEventEmitter) { + protected WritableMap getEventData() { WritableMap data = Arguments.createMap(); data.putDouble("width", PixelUtil.toDIPFromPixel(mWidth)); data.putDouble("height", PixelUtil.toDIPFromPixel(mHeight)); - rctEventEmitter.receiveEvent(getViewTag(), EVENT_NAME, data); + return data; } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/Event.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/Event.java index d1a0c1aa3fa..bd3b06b16e6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/Event.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/Event.java @@ -7,14 +7,26 @@ package com.facebook.react.uimanager.events; +import androidx.annotation.Nullable; +import com.facebook.react.bridge.WritableMap; import com.facebook.react.common.SystemClock; +import com.facebook.react.uimanager.IllegalViewOperationException; /** * A UI event that can be dispatched to JS. * - *

For dispatching events {@link EventDispatcher#dispatchEvent} should be used. Once event object - * is passed to the EventDispatched it should no longer be used as EventDispatcher may decide to - * recycle that object (by calling {@link #dispose}). + *

For dispatching events {@code getEventData} should be used. Once event object is passed to the + * EventDispatched it should no longer be used as EventDispatcher may decide to recycle that object + * (by calling {@link #dispose}). + * + *

If you need advanced customizations and overriding only {@code getEventData} doesn't work for + * you, you must override both {@code dispatch} and {@code dispatchModern}. Both of these will be + * deleted in the distant future and it is highly recommended to use only {@code getEventData}. + * + *

Old, pre-Fabric Events only used viewTag as the identifier, but Fabric needs surfaceId as well + * as viewTag. You may use {@code UIManagerHelper.getSurfaceId} on a Fabric-managed View to get the + * surfaceId. Fabric will work without surfaceId - making {@code Event} backwards-compatible - but + * Events without SurfaceId are slightly slower to propagate. */ public abstract class Event { @@ -119,15 +131,45 @@ public abstract class Event { /** * Dispatch this event to JS using the given event emitter. Compatible with old and new renderer. + * Instead of using this or dispatchModern, it is recommended that you simply override + * `getEventData`. In the future */ @Deprecated - public abstract void dispatch(RCTEventEmitter rctEventEmitter); + public void dispatch(RCTEventEmitter rctEventEmitter) { + WritableMap eventData = getEventData(); + if (eventData == null) { + throw new IllegalViewOperationException( + "Event: you must return a valid, non-null value from `getEventData`, or override `dispatch` and `disatchModern`. Event: " + + getEventName()); + } + rctEventEmitter.receiveEvent(getViewTag(), getEventName(), eventData); + } /** - * Dispatch this event to JS using a V2 EventEmitter. Events must explicitly override this, by - * default it uses the V1 dispatcher. + * Can be overridden by classes to make migrating to RCTModernEventEmitter support easier. If this + * class returns null, the RCTEventEmitter interface will be used instead of + * RCTModernEventEmitter. In the future, returning null here will be an error. */ + @Nullable + protected WritableMap getEventData() { + return null; + } + + /** + * Dispatch this event to JS using a V2 EventEmitter. If surfaceId is not -1 and `getEventData` is + * non-null, this will use the RCTModernEventEmitter API. Otherwise, it falls back to the + * old-style dispatch function. For Event classes that need to do something different, this method + * can always be overridden entirely, but it is not recommended. + */ + @Deprecated public void dispatchModern(RCTModernEventEmitter rctEventEmitter) { + if (getSurfaceId() != -1) { + WritableMap eventData = getEventData(); + if (eventData != null) { + rctEventEmitter.receiveEvent(getSurfaceId(), getViewTag(), getEventName(), getEventData()); + return; + } + } dispatch(rctEventEmitter); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/image/ImageLoadEvent.java b/ReactAndroid/src/main/java/com/facebook/react/views/image/ImageLoadEvent.java index 8dacc3cffc1..130cb67ad8b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/image/ImageLoadEvent.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/image/ImageLoadEvent.java @@ -12,8 +12,6 @@ import androidx.annotation.Nullable; import com.facebook.react.bridge.Arguments; import com.facebook.react.bridge.WritableMap; import com.facebook.react.uimanager.events.Event; -import com.facebook.react.uimanager.events.RCTEventEmitter; -import com.facebook.react.uimanager.events.RCTModernEventEmitter; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -146,30 +144,18 @@ public class ImageLoadEvent extends Event { } @Override - public void dispatch(RCTEventEmitter rctEventEmitter) { - rctEventEmitter.receiveEvent(getViewTag(), getEventName(), getEventData()); - } - - @Override - public void dispatchModern(RCTModernEventEmitter rctEventEmitter) { - rctEventEmitter.receiveEvent(getSurfaceId(), getViewTag(), getEventName(), getEventData()); - } - - private WritableMap getEventData() { - WritableMap eventData = null; + protected WritableMap getEventData() { + WritableMap eventData = Arguments.createMap(); switch (mEventType) { case ON_PROGRESS: - eventData = Arguments.createMap(); eventData.putInt("loaded", mLoaded); eventData.putInt("total", mTotal); break; case ON_LOAD: - eventData = Arguments.createMap(); eventData.putMap("source", createEventDataSource()); break; case ON_ERROR: - eventData = Arguments.createMap(); eventData.putString("error", mErrorMessage); break; }