From 91fc2c00919af98f248b2544f780d63e1056e1af Mon Sep 17 00:00:00 2001 From: Krzysztof Magiera Date: Thu, 21 Apr 2022 05:10:21 -0700 Subject: [PATCH] Pass mutation list to RCTMountingTransactionObserving callbacks (#33510) Summary: This PR updates `RCTMountingTransactionObserving` protocol to accept full `MountingTransaction` object as an argument to its methods as opposed to just `MountingTransactionMetdata` which contained only some subset of information. This change makes it possible for components implementing the protocol to analyze the list of mutations and hence only take action when certain mutations are about to happen. One of the use cases for `RCTMountingTransactionObserving` protocol is to allow for Modal to take view snapshot before it is closed, such that an animated close transition can be performed over the snapshotted content. Note that when modal is removed from the view hierarchy its children are gone too and therefore the snapshot mechanism makes it possible for children to still be visible while the animated closing transition is ongoing. A similar use-case to that can be seen in react-native-screens library, where we use the same snapshot mechanism for views that are removed from navigation stack. Before this change, we'd use `mountingTransactionDidMountWithMetadata` to take a snapshot. However, making a snapshot of relatively complex view hierarchy can be expensive, and we'd like to make sure that we only perform a snapshot when the given modal is about to be removed. Because the mentioned method does not provide an information about what changes are going to be performed in a given transaction, we'd make the snapshot for every single view transaction that happens while the modal is mounted. In this PR we're updating `RCTMountingTransactionObserving` protocol's methods, in particular, we rename methods to no longer contain "Metadata" in them and to accept `MountingTransaction` as the only argument instead of `MountingTransactionMetadata` object. With this change we are also deleting `MountingTransactionMetadata` altogether as it has no uses outside the protocol. Finally, we update the two uses of the protocol in `RCTScrollViewComponentView` and `RCTModalHostViewComponentView`. ## Changelog [iOS][Fabric] - Update RCTMountingTransactionObserving protocol to consume MountingTransaction objects Pull Request resolved: https://github.com/facebook/react-native/pull/33510 Test Plan: As there are not that many uses of `RCTMountingTransactionObserving` protocol during testing I focused on checking if the updated method is called and if the provided objects contains the proper data. Unfortunately, despite code for the modal protocol being present in OSS version it does seem like some parts of modal implementation are still missing and the component only renders an unimplemented view (checked this with rn-tester). I only managed to verify the use in `RCTScrollViewComponentView` with the following steps: 1. Build for iOS 2. Put a breakpoint in mountingTransactionDidMount method in `RCTScrollViewComponentView.mm` 3. Verify that the program stops on the breakpoint when a scrollview is rendered (use any screen on rn-tester app) 4. Inspect the provided object in the debugger (ensure the list of transactions is not empty) Outside of that we verified the transactions can be processed in `mountingTransactionDidMount` after the changes from this PR are applied in FabricExample app in [react-native-screens](https://github.com/software-mansion/react-native-screens/tree/main/FabricExample) repo. Reviewed By: cipolleschi Differential Revision: D35214478 Pulled By: ShikaSD fbshipit-source-id: f40afc512f2c8cfa6262d2fb82fb1ccb05aa734c --- .../Modal/RCTModalHostViewComponentView.mm | 3 +- .../ScrollView/RCTScrollViewComponentView.mm | 3 +- .../Mounting/RCTComponentViewFactory.mm | 4 +-- React/Fabric/Mounting/RCTMountingManager.mm | 13 ++++---- ...CTMountingTransactionObserverCoordinator.h | 8 +++-- ...TMountingTransactionObserverCoordinator.mm | 18 ++++++----- .../RCTMountingTransactionObserving.h | 8 +++-- .../mounting/MountingTransactionMetadata.cpp | 12 ------- .../mounting/MountingTransactionMetadata.h | 32 ------------------- .../renderer/mounting/TelemetryController.cpp | 15 ++++----- .../renderer/mounting/TelemetryController.h | 14 ++++---- 11 files changed, 46 insertions(+), 84 deletions(-) delete mode 100644 ReactCommon/react/renderer/mounting/MountingTransactionMetadata.cpp delete mode 100644 ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h diff --git a/React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm index 6cbe904eec9d2e..28c4bd227b7cc9 100644 --- a/React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm @@ -192,7 +192,8 @@ - (void)ensurePresentedOnlyIfNeeded #pragma mark - RCTMountingTransactionObserving -- (void)mountingTransactionWillMountWithMetadata:(MountingTransactionMetadata const &)metadata +- (void)mountingTransactionWillMount:(MountingTransaction const &)transaction + withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry { _modalContentsSnapshot = [self.viewController.view snapshotViewAfterScreenUpdates:NO]; } diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index 7752626d2d8363..2d339aead9dc1a 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -147,7 +147,8 @@ - (void)dealloc #pragma mark - RCTMountingTransactionObserving -- (void)mountingTransactionDidMountWithMetadata:(MountingTransactionMetadata const &)metadata +- (void)mountingTransactionDidMount:(MountingTransaction const &)transaction + withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry { [self _remountChildren]; } diff --git a/React/Fabric/Mounting/RCTComponentViewFactory.mm b/React/Fabric/Mounting/RCTComponentViewFactory.mm index cf5a9181243b78..1487beaa2a92ce 100644 --- a/React/Fabric/Mounting/RCTComponentViewFactory.mm +++ b/React/Fabric/Mounting/RCTComponentViewFactory.mm @@ -95,9 +95,9 @@ - (RCTComponentViewClassDescriptor)_componentViewClassDescriptorFromClass:(Class { .viewClass = viewClass, .observesMountingTransactionWillMount = - (bool)class_respondsToSelector(viewClass, @selector(mountingTransactionWillMountWithMetadata:)), + (bool)class_respondsToSelector(viewClass, @selector(mountingTransactionWillMount:withSurfaceTelemetry:)), .observesMountingTransactionDidMount = - (bool)class_respondsToSelector(viewClass, @selector(mountingTransactionDidMountWithMetadata:)), + (bool)class_respondsToSelector(viewClass, @selector(mountingTransactionDidMount:withSurfaceTelemetry:)), }; #pragma clang diagnostic pop } diff --git a/React/Fabric/Mounting/RCTMountingManager.mm b/React/Fabric/Mounting/RCTMountingManager.mm index fb18cf4e96d15b..23d57bc42fe364 100644 --- a/React/Fabric/Mounting/RCTMountingManager.mm +++ b/React/Fabric/Mounting/RCTMountingManager.mm @@ -266,15 +266,16 @@ - (void)performTransaction:(MountingCoordinator::Shared const &)mountingCoordina auto surfaceId = mountingCoordinator->getSurfaceId(); mountingCoordinator->getTelemetryController().pullTransaction( - [&](MountingTransactionMetadata metadata) { + [&](MountingTransaction const &transaction, SurfaceTelemetry const &surfaceTelemetry) { [self.delegate mountingManager:self willMountComponentsWithRootTag:surfaceId]; - _observerCoordinator.notifyObserversMountingTransactionWillMount(metadata); + _observerCoordinator.notifyObserversMountingTransactionWillMount(transaction, surfaceTelemetry); }, - [&](ShadowViewMutationList const &mutations) { - RCTPerformMountInstructions(mutations, _componentViewRegistry, _observerCoordinator, surfaceId); + [&](MountingTransaction const &transaction, SurfaceTelemetry const &surfaceTelemetry) { + RCTPerformMountInstructions( + transaction.getMutations(), _componentViewRegistry, _observerCoordinator, surfaceId); }, - [&](MountingTransactionMetadata metadata) { - _observerCoordinator.notifyObserversMountingTransactionDidMount(metadata); + [&](MountingTransaction const &transaction, SurfaceTelemetry const &surfaceTelemetry) { + _observerCoordinator.notifyObserversMountingTransactionDidMount(transaction, surfaceTelemetry); [self.delegate mountingManager:self didMountComponentsWithRootTag:surfaceId]; }); } diff --git a/React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.h b/React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.h index 8f0f57234e2db3..f8178140491aed 100644 --- a/React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.h +++ b/React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.h @@ -11,7 +11,7 @@ #import #import -#import +#include class RCTMountingTransactionObserverCoordinator final { public: @@ -31,9 +31,11 @@ class RCTMountingTransactionObserverCoordinator final { * To be called from `RCTMountingManager`. */ void notifyObserversMountingTransactionWillMount( - facebook::react::MountingTransactionMetadata const &metadata) const; + facebook::react::MountingTransaction const &transaction, + facebook::react::SurfaceTelemetry const &surfaceTelemetry) const; void notifyObserversMountingTransactionDidMount( - facebook::react::MountingTransactionMetadata const &metadata) const; + facebook::react::MountingTransaction const &transaction, + facebook::react::SurfaceTelemetry const &surfaceTelemetry) const; private: facebook::butter::map< diff --git a/React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.mm b/React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.mm index 959c14603733a9..52424ac048f6b1 100644 --- a/React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.mm +++ b/React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.mm @@ -40,9 +40,10 @@ } void RCTMountingTransactionObserverCoordinator::notifyObserversMountingTransactionWillMount( - MountingTransactionMetadata const &metadata) const + MountingTransaction const &transaction, + SurfaceTelemetry const &surfaceTelemetry) const { - auto surfaceId = metadata.surfaceId; + auto surfaceId = transaction.getSurfaceId(); auto surfaceRegistryIterator = registry_.find(surfaceId); if (surfaceRegistryIterator == registry_.end()) { return; @@ -50,16 +51,17 @@ auto &surfaceRegistry = surfaceRegistryIterator->second; for (auto const &componentViewDescriptor : surfaceRegistry) { if (componentViewDescriptor.observesMountingTransactionWillMount) { - [(id)componentViewDescriptor.view - mountingTransactionWillMountWithMetadata:metadata]; + [(id)componentViewDescriptor.view mountingTransactionWillMount:transaction + withSurfaceTelemetry:surfaceTelemetry]; } } } void RCTMountingTransactionObserverCoordinator::notifyObserversMountingTransactionDidMount( - MountingTransactionMetadata const &metadata) const + MountingTransaction const &transaction, + SurfaceTelemetry const &surfaceTelemetry) const { - auto surfaceId = metadata.surfaceId; + auto surfaceId = transaction.getSurfaceId(); auto surfaceRegistryIterator = registry_.find(surfaceId); if (surfaceRegistryIterator == registry_.end()) { return; @@ -67,8 +69,8 @@ auto &surfaceRegistry = surfaceRegistryIterator->second; for (auto const &componentViewDescriptor : surfaceRegistry) { if (componentViewDescriptor.observesMountingTransactionDidMount) { - [(id)componentViewDescriptor.view - mountingTransactionDidMountWithMetadata:metadata]; + [(id)componentViewDescriptor.view mountingTransactionDidMount:transaction + withSurfaceTelemetry:surfaceTelemetry]; } } } diff --git a/React/Fabric/Mounting/RCTMountingTransactionObserving.h b/React/Fabric/Mounting/RCTMountingTransactionObserving.h index 35594d87ad61e7..c24029a2d81ab1 100644 --- a/React/Fabric/Mounting/RCTMountingTransactionObserving.h +++ b/React/Fabric/Mounting/RCTMountingTransactionObserving.h @@ -7,7 +7,7 @@ #import -#import +#include NS_ASSUME_NONNULL_BEGIN @@ -50,14 +50,16 @@ NS_ASSUME_NONNULL_BEGIN * Is not being called for a component view which is being mounted as part of the transaction (because the view is not * registered as an observer yet). */ -- (void)mountingTransactionWillMountWithMetadata:(facebook::react::MountingTransactionMetadata const &)metadata; +- (void)mountingTransactionWillMount:(facebook::react::MountingTransaction const &)transaction + withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry; /* * Called right after the last mutation instruction is executed. * Is not being called for a component view which was being unmounted as part of the transaction (because the view is * not registered as an observer already). */ -- (void)mountingTransactionDidMountWithMetadata:(facebook::react::MountingTransactionMetadata const &)metadata; +- (void)mountingTransactionDidMount:(facebook::react::MountingTransaction const &)transaction + withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry; @end diff --git a/ReactCommon/react/renderer/mounting/MountingTransactionMetadata.cpp b/ReactCommon/react/renderer/mounting/MountingTransactionMetadata.cpp deleted file mode 100644 index e6106cda4f0447..00000000000000 --- a/ReactCommon/react/renderer/mounting/MountingTransactionMetadata.cpp +++ /dev/null @@ -1,12 +0,0 @@ -/* - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#include "MountingTransactionMetadata.h" - -namespace facebook { -namespace react {} // namespace react -} // namespace facebook diff --git a/ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h b/ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h deleted file mode 100644 index d0eda81002cf7e..00000000000000 --- a/ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#pragma once - -#include -#include - -namespace facebook { -namespace react { - -/* - * Contains all (meta)information related to a MountingTransaction except a list - * of mutation instructions. - * The class is meant to be used when a consumer should not have access to all - * information about the transaction (incapsulation) but still needs to observe - * it to produce some side-effects. - */ -class MountingTransactionMetadata final { - public: - SurfaceId surfaceId; - MountingTransaction::Number number; - TransactionTelemetry telemetry; - SurfaceTelemetry surfaceTelemetry; -}; - -} // namespace react -} // namespace facebook diff --git a/ReactCommon/react/renderer/mounting/TelemetryController.cpp b/ReactCommon/react/renderer/mounting/TelemetryController.cpp index f660768a5cd7b2..98d4eecba9cbb1 100644 --- a/ReactCommon/react/renderer/mounting/TelemetryController.cpp +++ b/ReactCommon/react/renderer/mounting/TelemetryController.cpp @@ -17,10 +17,9 @@ TelemetryController::TelemetryController( : mountingCoordinator_(mountingCoordinator) {} bool TelemetryController::pullTransaction( - std::function const &willMount, - std::function const &doMount, - std::function const &didMount) - const { + MountingTransactionCallback const &willMount, + MountingTransactionCallback const &doMount, + MountingTransactionCallback const &didMount) const { auto optional = mountingCoordinator_.pullTransaction(); if (!optional.has_value()) { return false; @@ -28,8 +27,6 @@ bool TelemetryController::pullTransaction( auto transaction = std::move(*optional); - auto surfaceId = transaction.getSurfaceId(); - auto number = transaction.getNumber(); auto telemetry = transaction.getTelemetry(); auto numberOfMutations = static_cast(transaction.getMutations().size()); @@ -37,15 +34,15 @@ bool TelemetryController::pullTransaction( auto compoundTelemetry = compoundTelemetry_; mutex_.unlock(); - willMount({surfaceId, number, telemetry, compoundTelemetry}); + willMount(transaction, compoundTelemetry); telemetry.willMount(); - doMount(transaction.getMutations()); + doMount(transaction, compoundTelemetry); telemetry.didMount(); compoundTelemetry.incorporate(telemetry, numberOfMutations); - didMount({surfaceId, number, telemetry, compoundTelemetry}); + didMount(transaction, compoundTelemetry); mutex_.lock(); compoundTelemetry_ = compoundTelemetry; diff --git a/ReactCommon/react/renderer/mounting/TelemetryController.h b/ReactCommon/react/renderer/mounting/TelemetryController.h index 8ea445e8f0a32d..23ecd25c806a54 100644 --- a/ReactCommon/react/renderer/mounting/TelemetryController.h +++ b/ReactCommon/react/renderer/mounting/TelemetryController.h @@ -11,7 +11,6 @@ #include #include -#include #include namespace facebook { @@ -19,6 +18,10 @@ namespace react { class MountingCoordinator; +using MountingTransactionCallback = std::function; + /* * Provides convenient tools for aggregating and accessing telemetry data * associated with running Surface. @@ -43,12 +46,9 @@ class TelemetryController final { * Calls `MountingCoordinator::pullTransaction()` and aggregates telemetry. */ bool pullTransaction( - std::function const - &willMount, - std::function const - &doMount, - std::function const &didMount) - const; + MountingTransactionCallback const &willMount, + MountingTransactionCallback const &doMount, + MountingTransactionCallback const &didMount) const; private: MountingCoordinator const &mountingCoordinator_;