Expose UIManager from Scheduler #33545

ShikaSD · 2022-04-01T13:24:21Z

Differential Revision: D35314058

facebook-github-bot · 2022-04-01T13:24:40Z

This pull request was exported from Phabricator. Differential Revision: D35314058

pull-bot · 2022-04-01T13:28:52Z

	Fails
🚫	❔ This pull request needs a description.

	Messages
📖	📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 2c6211c

analysis-bot · 2022-04-01T13:33:32Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: aeac6ab
Branch: main

facebook-github-bot · 2022-04-01T13:43:06Z

This pull request was exported from Phabricator. Differential Revision: D35314058

Summary: Pull Request resolved: facebook#33545 Differential Revision: D35314058 fbshipit-source-id: fe7a0feb37da3a691735c4315c64c7bc11c0a89d

tomekzaw

Thanks for this PR! I've left a couple of comments regarding some details.

tomekzaw · 2022-04-02T11:42:26Z

ReactCommon/react/renderer/core/EventListener.h

+  void removeListener(std::shared_ptr<EventListener const> listener);
+
+ private:
+  butter::shared_mutex lock_;


Suggested change

butter::shared_mutex lock_;

butter::shared_mutex mutex_;

I would like to suggest renaming this field to mutex_ so it is consistent with naming in ComponentDescriptorRegistry, EventQueue, ShadowNodeFamily etc. and can be used as std::unique_lock<butter::shared_mutex> lock(mutex_);.

Oh, must be a typo on my end :)

tomekzaw · 2022-04-02T11:42:31Z

ReactCommon/react/renderer/scheduler/Scheduler.cpp

                       jsi::Runtime &runtime,
                       const EventTarget *eventTarget,
                       const std::string &type,
                       ReactEventPriority priority,
                       const ValueFactory &payloadFactory) {
+    if (!eventListeners_.callListeners(
+            eventTarget, type, priority, payloadFactory)) {
+      return;


I would like to suggest adding a short comment here for future visitors why we return here (because event listeners not only intercept events but can also prevent them from being passed to JS).

tomekzaw · 2022-04-02T11:55:20Z

ReactCommon/react/renderer/core/EventListener.h

+    const EventTarget *eventTarget,
+    const std::string &type,
+    ReactEventPriority priority,
+    const ValueFactory &payloadFactory)>;


Just to make sure, I would like to ask whether it is safe for event listener to invoke payloadFactory even if it will decide to pass the event to JS?

In such case payloadFactory may be called twice – first time in event listener and then one more time in UIManagerBinding::dispatchEvent. (In fact, each event listener may want to call payloadFactory arbitrary number of times.)

I guess it depends on implementations of payload factory, e.g. if it decides to std::move some memory
From the implementations I saw in the core, it should be safe (usually we just create the value from struct ref or folly::dynamic)

tomekzaw · 2022-04-02T12:09:38Z

ReactCommon/react/renderer/core/EventListener.cpp

+  bool result = true;
+  for (auto const &listener : eventListeners_) {
+    result =
+        result && listener->operator()(eventTarget, type, priority, payloadFactory);
+  }
+  return result;


If I understand it correctly, all event listeners intercept the event (in the order of registration) and each one of them individually returns a boolean value whether the event should be passed to JS or not. If any of the event listeners returns false, the event will not be passed to JS.

Also, even if some event listener returns false (meaning it could handle the event), the rest of event listeners will still be called, which seems fair. (This guarantee will prevent third-party library developers from racing for first place as there is no priority in voting).

I know it's a matter of preference, but personally it makes more sense to me to return true from event listener if it successfully managed to handle the event (so it doesn't need to be sent to JS) and false otherwise (meaning it couldn't handle the event so it probably needs to be passed to JS). So we would start from result = false, use || instead of && here and don't need ! in Scheduler.cpp.

Also, a comment on the semantics of the return value from event listener would be helpful for the other developers as well.

BTW I'm not sure but it looks like the syntax without ->operator(), i.e. listener(eventTarget, type, priority, payloadFactory) would also work.

Agree on true/false comment, it will be more aligned with platform apis that way
I added the comment for the Event listener internally, but it probably didn't sync to GitHub.

Also about operator(), it is dereferencing shared pointer here, so just calling the function doesn't work, unfortunately
I could deref it into EventListener const & and call it that way, but not sure there is a point in doing so

You're right, I forgot that it's a shared pointer. In such case (*listener)(eventTarget, type, priority, payloadFactory) would do, but the original version with ->operator() is certainly better in terms of explicitness and readability.

facebook-github-bot · 2022-04-02T17:10:23Z

This pull request was exported from Phabricator. Differential Revision: D35314058

Summary: Pull Request resolved: facebook#33545 Exposes `UIManager` instance for access from third-party modules. Changelog: [Changed] Exposes `UIManager` instance for third-party access. Differential Revision: D35314058 fbshipit-source-id: 90752fb57c662b78d5355b84f35221d7050e810a

analysis-bot · 2022-04-02T17:38:41Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,775,951	+3,036
android	hermes	armeabi-v7a	7,181,695	+2,737
android	hermes	x86	8,085,263	+3,330
android	hermes	x86_64	8,065,446	+3,611
android	jsc	arm64-v8a	9,649,955	+3,046
android	jsc	armeabi-v7a	8,424,085	+2,736
android	jsc	x86	9,599,458	+3,326
android	jsc	x86_64	10,197,044	+3,614

Base commit: aeac6ab
Branch: main

tomekzaw · 2022-04-05T21:27:13Z

@ShikaSD We have just tried out this PR in Fabric version of react-native-reanimated. The good news is that we managed to implement and install a custom event listener on iOS (although we had some difficulties with obtaining a reference to RCTScheduler due to the fact that bridge.surfacePresenter was not initialized at the time we initialize Reanimated, but it looks like enabling requiresMainQueueSetup resolves this issue).

The bad news is that unfortunately I've overlooked the fact that the proposed event listeners are called on the JS thread, after the events are flushed from EventQueue. However, in Reanimated we need to intercept and handle events while still on the UI thread.

Can you please let us know if it would be possible to tweak this PR so we can handle the events still on the UI thread?

Summary: Exposes event listener through iOS scheduler wrapper (`RCTScheduler`) and exposes scheduler itself through `RCTSurfacePresenter`. Changelog: [Changed] Exposed `RCTScheduler` to allow setting event listeners. Differential Revision: D35313398 fbshipit-source-id: 67d27969d7a64f021810292f84ddb52484c11b68

Summary: Minimal set of changes to intercept events in external modules. Current intended use-case is for Reanimated to handle events for the Animated properties. Changelog: [Added] Add listeners to allow intercepting events in C++ core. Differential Revision: D35312534 fbshipit-source-id: 17ea760d016e68ee203b0106a113006f39fc159b

Differential Revision: D35313399 fbshipit-source-id: d53d9642ae351360b5825d280f4244ce701eebcd

Summary: Pull Request resolved: facebook#33545 Exposes `UIManager` instance for access from third-party modules. Changelog: [Changed] Exposes `UIManager` instance for third-party access. Reviewed By: javache Differential Revision: D35314058 fbshipit-source-id: 8640911c24a19c48fe6a4141cfec26c9427fb9c3

facebook-github-bot · 2022-04-21T15:46:12Z

This pull request was exported from Phabricator. Differential Revision: D35314058

Summary: Pull Request resolved: facebook#33545 Exposes `UIManager` instance for access from third-party modules. Changelog: [Changed] Exposes `UIManager` instance for third-party access. Differential Revision: D35314058 fbshipit-source-id: b80c5520185f272f66788c3392eccecbb2eb1def

react-native-bot · 2022-04-26T12:15:05Z

This pull request was successfully merged by @cortinico in 54db5f2.

^{When will my fix make it into a release? | Upcoming Releases}

Summary: Pull Request resolved: facebook#33545 Exposes `UIManager` instance for access from third-party modules. Changelog: [Changed] Exposes `UIManager` instance for third-party access. Reviewed By: javache Differential Revision: D35314058 fbshipit-source-id: 1922c2afc37b105b153a82f45e5bac9c0b0cdfae

Summary: Pull Request resolved: facebook#33545 Exposes `UIManager` instance for access from third-party modules. Changelog: [Changed] Exposes `UIManager` instance for third-party access. Reviewed By: javache Differential Revision: D35314058 fbshipit-source-id: 1922c2afc37b105b153a82f45e5bac9c0b0cdfae Co-authored-by: Nicola Corti <ncor@fb.com>

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Apr 1, 2022

facebook-github-bot added p: Facebook Partner: Facebook Partner labels Apr 1, 2022

ShikaSD pushed a commit to ShikaSD/react-native that referenced this pull request Apr 1, 2022

Expose UIManager from Scheduler (facebook#33545)

36e50f2

Summary: Pull Request resolved: facebook#33545 Differential Revision: D35314058 fbshipit-source-id: fe7a0feb37da3a691735c4315c64c7bc11c0a89d

ShikaSD force-pushed the export-D35314058 branch from 65c01d4 to 36e50f2 Compare April 1, 2022 13:43

tomekzaw reviewed Apr 2, 2022

View reviewed changes

ShikaSD force-pushed the export-D35314058 branch from 36e50f2 to fd4bec1 Compare April 2, 2022 17:10

Andrei Shikov added 4 commits April 21, 2022 06:28

Expose scheduler through FabricUIManager

f15406c

Differential Revision: D35313399 fbshipit-source-id: d53d9642ae351360b5825d280f4244ce701eebcd

ShikaSD force-pushed the export-D35314058 branch from fd4bec1 to 2c6211c Compare April 21, 2022 15:46

facebook-github-bot closed this in 54db5f2 Apr 26, 2022

react-native-bot added the Merged This PR has been merged. label Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose UIManager from Scheduler #33545

Expose UIManager from Scheduler #33545

ShikaSD commented Apr 1, 2022

facebook-github-bot commented Apr 1, 2022

pull-bot commented Apr 1, 2022 •

edited

Loading

analysis-bot commented Apr 1, 2022 •

edited

Loading

facebook-github-bot commented Apr 1, 2022

tomekzaw left a comment

tomekzaw Apr 2, 2022

ShikaSD Apr 2, 2022

tomekzaw Apr 2, 2022

tomekzaw Apr 2, 2022

ShikaSD Apr 2, 2022

tomekzaw Apr 2, 2022 •

edited

Loading

ShikaSD Apr 2, 2022

tomekzaw Apr 2, 2022

facebook-github-bot commented Apr 2, 2022

analysis-bot commented Apr 2, 2022 •

edited

Loading

tomekzaw commented Apr 5, 2022 •

edited

Loading

facebook-github-bot commented Apr 21, 2022

react-native-bot commented Apr 26, 2022

Expose UIManager from Scheduler #33545

Expose UIManager from Scheduler #33545

Conversation

ShikaSD commented Apr 1, 2022

facebook-github-bot commented Apr 1, 2022

pull-bot commented Apr 1, 2022 • edited Loading

analysis-bot commented Apr 1, 2022 • edited Loading

facebook-github-bot commented Apr 1, 2022

tomekzaw left a comment

Choose a reason for hiding this comment

tomekzaw Apr 2, 2022

Choose a reason for hiding this comment

ShikaSD Apr 2, 2022

Choose a reason for hiding this comment

tomekzaw Apr 2, 2022

Choose a reason for hiding this comment

tomekzaw Apr 2, 2022

Choose a reason for hiding this comment

ShikaSD Apr 2, 2022

Choose a reason for hiding this comment

tomekzaw Apr 2, 2022 • edited Loading

Choose a reason for hiding this comment

ShikaSD Apr 2, 2022

Choose a reason for hiding this comment

tomekzaw Apr 2, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Apr 2, 2022

analysis-bot commented Apr 2, 2022 • edited Loading

tomekzaw commented Apr 5, 2022 • edited Loading

facebook-github-bot commented Apr 21, 2022

react-native-bot commented Apr 26, 2022

pull-bot commented Apr 1, 2022 •

edited

Loading

analysis-bot commented Apr 1, 2022 •

edited

Loading

tomekzaw Apr 2, 2022 •

edited

Loading

analysis-bot commented Apr 2, 2022 •

edited

Loading

tomekzaw commented Apr 5, 2022 •

edited

Loading