Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass mutation list to RCTMountingTransactionObserving callbacks #33510

Closed
wants to merge 10 commits into from
Closed

Pass mutation list to RCTMountingTransactionObserving callbacks #33510

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f840a22
Stashing work
kmagiera Feb 22, 2022
33d26c3
Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…
kmagiera Mar 28, 2022
d125f38
Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…
kmagiera Mar 28, 2022
edff89a
Drop unused variables
kmagiera Mar 29, 2022
f2a6105
Remove MountingTransactionMetadata
kmagiera Apr 1, 2022
124c731
Revert changes in mounting observer for modal component
kmagiera Apr 1, 2022
87084d0
Commit podfile lock changes (perhaps not a valid ones) to see if CI p…
kmagiera Apr 1, 2022
d08c67f
Revert changes in rn-tester's podfile.lock
kmagiera Apr 4, 2022
d9b3edb
Add SurfaceTelemetry as additional argument to all transaction observ…
kmagiera Apr 5, 2022
f03b034
Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…
kmagiera Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,14 @@ - (void)ensurePresentedOnlyIfNeeded

#pragma mark - RCTMountingTransactionObserving

- (void)mountingTransactionWillMountWithMetadata:(MountingTransactionMetadata const &)metadata
- (void)mountingTransactionWillMount:(MountingTransaction const &)transaction
{
_modalContentsSnapshot = [self.viewController.view snapshotViewAfterScreenUpdates:NO];
for (auto mutation : transaction.getMutations()) {
ShikaSD marked this conversation as resolved.
Show resolved Hide resolved
if (mutation.type == ShadowViewMutation::Type::Delete && mutation.parentShadowView.componentName == ModalHostViewComponentName) {
_modalContentsSnapshot = [self.viewController.view snapshotViewAfterScreenUpdates:NO];
return;
}
}
}

#pragma mark - UIView methods
Expand Down
2 changes: 1 addition & 1 deletion React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ - (void)dealloc

#pragma mark - RCTMountingTransactionObserving

- (void)mountingTransactionDidMountWithMetadata:(MountingTransactionMetadata const &)metadata
- (void)mountingTransactionDidMount:(MountingTransaction const &)transaction
{
[self _remountChildren];
}
Expand Down
4 changes: 2 additions & 2 deletions React/Fabric/Mounting/RCTComponentViewFactory.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ - (RCTComponentViewClassDescriptor)_componentViewClassDescriptorFromClass:(Class
{
.viewClass = viewClass,
.observesMountingTransactionWillMount =
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionWillMountWithMetadata:)),
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionWillMount:)),
.observesMountingTransactionDidMount =
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionDidMountWithMetadata:)),
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionDidMount:)),
};
#pragma clang diagnostic pop
}
Expand Down
12 changes: 6 additions & 6 deletions React/Fabric/Mounting/RCTMountingManager.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,15 @@ - (void)performTransaction:(MountingCoordinator::Shared const &)mountingCoordina
auto surfaceId = mountingCoordinator->getSurfaceId();

mountingCoordinator->getTelemetryController().pullTransaction(
[&](MountingTransactionMetadata metadata) {
[&](MountingTransaction const &transaction) {
[self.delegate mountingManager:self willMountComponentsWithRootTag:surfaceId];
_observerCoordinator.notifyObserversMountingTransactionWillMount(metadata);
_observerCoordinator.notifyObserversMountingTransactionWillMount(transaction);
},
[&](ShadowViewMutationList const &mutations) {
RCTPerformMountInstructions(mutations, _componentViewRegistry, _observerCoordinator, surfaceId);
[&](MountingTransaction const &transaction) {
RCTPerformMountInstructions(transaction.getMutations(), _componentViewRegistry, _observerCoordinator, surfaceId);
},
[&](MountingTransactionMetadata metadata) {
_observerCoordinator.notifyObserversMountingTransactionDidMount(metadata);
[&](MountingTransaction const &transaction) {
_observerCoordinator.notifyObserversMountingTransactionDidMount(transaction);
[self.delegate mountingManager:self didMountComponentsWithRootTag:surfaceId];
});
}
Expand Down
4 changes: 2 additions & 2 deletions React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ class RCTMountingTransactionObserverCoordinator final {
* To be called from `RCTMountingManager`.
*/
void notifyObserversMountingTransactionWillMount(
facebook::react::MountingTransactionMetadata const &metadata) const;
facebook::react::MountingTransaction const &transaction) const;
void notifyObserversMountingTransactionDidMount(
facebook::react::MountingTransactionMetadata const &metadata) const;
facebook::react::MountingTransaction const &transaction) const;

private:
facebook::butter::map<
Expand Down
12 changes: 6 additions & 6 deletions React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@
}

void RCTMountingTransactionObserverCoordinator::notifyObserversMountingTransactionWillMount(
MountingTransactionMetadata const &metadata) const
MountingTransaction const &transaction) const
{
auto surfaceId = metadata.surfaceId;
auto surfaceId = transaction.getSurfaceId();
auto surfaceRegistryIterator = registry_.find(surfaceId);
if (surfaceRegistryIterator == registry_.end()) {
return;
Expand All @@ -51,15 +51,15 @@
for (auto const &componentViewDescriptor : surfaceRegistry) {
if (componentViewDescriptor.observesMountingTransactionWillMount) {
[(id<RCTMountingTransactionObserving>)componentViewDescriptor.view
mountingTransactionWillMountWithMetadata:metadata];
mountingTransactionWillMount:transaction];
}
}
}

void RCTMountingTransactionObserverCoordinator::notifyObserversMountingTransactionDidMount(
MountingTransactionMetadata const &metadata) const
MountingTransaction const &transaction) const
{
auto surfaceId = metadata.surfaceId;
auto surfaceId = transaction.getSurfaceId();
auto surfaceRegistryIterator = registry_.find(surfaceId);
if (surfaceRegistryIterator == registry_.end()) {
return;
Expand All @@ -68,7 +68,7 @@
for (auto const &componentViewDescriptor : surfaceRegistry) {
if (componentViewDescriptor.observesMountingTransactionDidMount) {
[(id<RCTMountingTransactionObserving>)componentViewDescriptor.view
mountingTransactionDidMountWithMetadata:metadata];
mountingTransactionDidMount:transaction];
}
}
}
4 changes: 2 additions & 2 deletions React/Fabric/Mounting/RCTMountingTransactionObserving.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ 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;

/*
* 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;

@end

Expand Down
1 change: 1 addition & 0 deletions ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class MountingTransactionMetadata final {
MountingTransaction::Number number;
TransactionTelemetry telemetry;
SurfaceTelemetry surfaceTelemetry;
ShadowViewMutationList const &mutations;
ShikaSD marked this conversation as resolved.
Show resolved Hide resolved
};

} // namespace react
Expand Down
14 changes: 6 additions & 8 deletions ReactCommon/react/renderer/mounting/TelemetryController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ TelemetryController::TelemetryController(
: mountingCoordinator_(mountingCoordinator) {}

bool TelemetryController::pullTransaction(
std::function<void(MountingTransactionMetadata metadata)> const &willMount,
std::function<void(ShadowViewMutationList const &mutations)> const &doMount,
std::function<void(MountingTransactionMetadata metadata)> const &didMount)
std::function<void(MountingTransaction const &transaction)> const &willMount,
std::function<void(MountingTransaction const &transaction)> const &doMount,
std::function<void(MountingTransaction const &transaction)> const &didMount)
const {
auto optional = mountingCoordinator_.pullTransaction();
if (!optional.has_value()) {
Expand All @@ -28,24 +28,22 @@ bool TelemetryController::pullTransaction(

auto transaction = std::move(*optional);

auto surfaceId = transaction.getSurfaceId();
auto number = transaction.getNumber();
auto telemetry = transaction.getTelemetry();
auto numberOfMutations = static_cast<int>(transaction.getMutations().size());

mutex_.lock();
auto compoundTelemetry = compoundTelemetry_;
mutex_.unlock();

willMount({surfaceId, number, telemetry, compoundTelemetry});
willMount(transaction);

telemetry.willMount();
doMount(transaction.getMutations());
doMount(transaction);
telemetry.didMount();

compoundTelemetry.incorporate(telemetry, numberOfMutations);

didMount({surfaceId, number, telemetry, compoundTelemetry});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started updating internal usages of this method, seems like compoundTelemetry is not available in the transaction .
I am not sure what's the best course of action here, maybe we could pass both transaction and telemetry as separate parameters or rollback deletion of metadata and just have it as a { transaction, compoundTelemetry } type of object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about passing SurfaceTelemetry as a second argument to all the observer callbacks? For me it'd seem more natural than adding additional struct to carry them both. If you'd prefer to have such struct then I'd consider either naming it differently than "*Metadata" as it'd seem unintuitive that "TransactionMetadata" actually carries "Transaction". Alternatively I can revert "Metadata" to its previous form and add only a reference to "mutations" to it as that's the only thing we need from transaction to fulfil out usecase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, SurfaceTelemetry as a separate parameter sounds good to me :)

didMount(transaction);

mutex_.lock();
compoundTelemetry_ = compoundTelemetry;
Expand Down
6 changes: 3 additions & 3 deletions ReactCommon/react/renderer/mounting/TelemetryController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ class TelemetryController final {
* Calls `MountingCoordinator::pullTransaction()` and aggregates telemetry.
*/
bool pullTransaction(
std::function<void(MountingTransactionMetadata metadata)> const
std::function<void(MountingTransaction const &transaction)> const
&willMount,
std::function<void(ShadowViewMutationList const &mutations)> const
std::function<void(MountingTransaction const &transaction)> const
&doMount,
std::function<void(MountingTransactionMetadata metadata)> const &didMount)
std::function<void(MountingTransaction const &transaction)> const &didMount)
const;

private:
Expand Down