feat: refactor snapshots when going back on Fabric#2134
Conversation
WoLewicki
changed the title
tboba
left a comment
tboba
left a comment
There was a problem hiding this comment.
Great work! Overall, this code looks good to me. Just left a few remarks there 🎉
| new_arch_enabled = ENV['RCT_NEW_ARCH_ENABLED'] == '1' | ||
| platform = new_arch_enabled ? "11.0" : "9.0" | ||
| source_files = new_arch_enabled ? 'ios/**/*.{h,m,mm,cpp}' : ["ios/**/*.{h,m,mm}", "cpp/**/*.{cpp,h}"] | ||
| source_files = new_arch_enabled ? 'ios/**/*.{h,m,mm,cpp}' : ["ios/**/*.{h,m,mm}", "cpp/RNScreensTurboModule.cpp", "cpp/RNScreensTurboModule.h"] |
There was a problem hiding this comment.
Why we're not scoping all .cpp and .h files right now?
There was a problem hiding this comment.
Because the new files added to the folder should not be compiled on old arch.
There was a problem hiding this comment.
Maybe it's worth to consider grouping the files. E.g. place screen-transition related staff in a cpp/transition / cpp/turbomodule directory & new screen listeners into cpp/mounting or sth similar.
There was a problem hiding this comment.
Can we do a cleanup of those in the follow-up PR?
|
Hi @WoLewicki! While I was testing all examples from FabricTestExample, I've stumbled onto one app crash while being on the branch from this PR. When I'm trying to run Exception from Logcat |
tboba
left a comment
tboba
left a comment
There was a problem hiding this comment.
Besides two comments, everything LGTM!
| PODFILE CHECKSUM: 67b3d295da87c29349179e51bb3526b67059b646 | ||
|
|
||
| COCOAPODS: 1.15.2 | ||
| COCOAPODS: 1.14.3 |
There was a problem hiding this comment.
I'm wondering if we shouldn't introduce some policy of using chosen version of cocoapods to omit this sort of changes from Podfiles 🤔 what do you think?
There was a problem hiding this comment.
Yeah, would be nice to just hardcode it so it does not land in many PRs by mistake
There was a problem hiding this comment.
This depends on cocoapods version you have installed locally I believe. I don't think we should include this in your PR, especially that it's a lower version, and there should be no need to downgrade.
| if(${IS_NEW_ARCHITECTURE_ENABLED}) | ||
| string(APPEND CMAKE_CXX_FLAGS " -DRCT_NEW_ARCH_ENABLED") | ||
| endif() |
There was a problem hiding this comment.
Why are we setting this? Is that because of some unmerged changes earlier for matching RN 0.74?
Summary: PR changing the single mountingOverrideDelegate to a vector of those, so other listeners can operate on the transaction. Used by `react-native-screens` in software-mansion/react-native-screens#2134 and `react-native-reanimated` in software-mansion/react-native-reanimated#6055. Till now, only one listener could be added there, meaning that e.g. `Layout Animations` from `react-native`, `Layout Animations` from `react-native-reanimated` and listening for `Screen` removal in `react-native-screens` could not operate at the same time. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [GENERAL] [FIXED] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [GENERAL] [FIXED] - Add option for multiple `mountingOverrideDelegates` Pull Request resolved: #44927 Test Plan: The code of `LayoutAnimations` inside `react-native` should work the same since it will add just one listener then. For other cases, different libraries can read/mutate transactions. Reviewed By: javache Differential Revision: D58530278 Pulled By: sammy-SC fbshipit-source-id: d6305963621000be11d51a50cffff64526cca934
There was a problem hiding this comment.
Okay, I think I have solid grasp of the changes. These looks solid 💪🏻 I still have some questions and change requests I've posted in the review.
Note I think we should vastly improve the PR description, with more information on the solution architecture, especially initialisation phase of the C++ layer and some simple flow of the solution.
| concurrent-ruby (~> 1.0) | ||
| json (2.7.2) | ||
| minitest (5.22.3) | ||
| minitest (5.22.2) |
There was a problem hiding this comment.
What's going on here, why do we change that?
| PODFILE CHECKSUM: 67b3d295da87c29349179e51bb3526b67059b646 | ||
|
|
||
| COCOAPODS: 1.15.2 | ||
| COCOAPODS: 1.14.3 |
There was a problem hiding this comment.
This depends on cocoapods version you have installed locally I believe. I don't think we should include this in your PR, especially that it's a lower version, and there should be no need to downgrade.
| new_arch_enabled = ENV['RCT_NEW_ARCH_ENABLED'] == '1' | ||
| platform = new_arch_enabled ? "11.0" : "9.0" | ||
| source_files = new_arch_enabled ? 'ios/**/*.{h,m,mm,cpp}' : ["ios/**/*.{h,m,mm}", "cpp/**/*.{cpp,h}"] | ||
| source_files = new_arch_enabled ? 'ios/**/*.{h,m,mm,cpp}' : ["ios/**/*.{h,m,mm}", "cpp/RNScreensTurboModule.cpp", "cpp/RNScreensTurboModule.h"] |
There was a problem hiding this comment.
Maybe it's worth to consider grouping the files. E.g. place screen-transition related staff in a cpp/transition / cpp/turbomodule directory & new screen listeners into cpp/mounting or sth similar.
| set_target_properties(rnscreens PROPERTIES | ||
| CXX_STANDARD 20 | ||
| CXX_STANDARD_REQUIRED ON | ||
| CXX_EXTENSIONS OFF | ||
| POSITION_INDEPENDENT_CODE ON |
There was a problem hiding this comment.
Why do we replace setting properties on particular target with setting the non-target-scoped variable CMAKE_CXX_STANDARD? Also why do we drop standard requirement, turning off extensions and enabling -fpic explicitly?
There was a problem hiding this comment.
I'm not sure why I removed it, reverted.
| target_compile_definitions( | ||
| rnscreens | ||
| PRIVATE | ||
| -DFOLLY_NO_CONFIG=1 |
There was a problem hiding this comment.
| -DFOLLY_NO_CONFIG=1 | |
| FOLLY_NO_CONFIG=1 |
I believe you can omit -D prefix in target_compile_definition (link)
There was a problem hiding this comment.
It doesn't work if I remove it so I think it is not true.
There was a problem hiding this comment.
Not gonna lie, I'm surprised as CMake docs I've linked explicitly state that you can omit -D, but that's a nitpick anyway, we can proceed w/o this change.
| struct RNSScreenRemovalListener : public MountingOverrideDelegate { | ||
| std::function<void(int)> listenerFunction_; | ||
| RNSScreenRemovalListener(std::function<void(int)> &&listenerFunction_) | ||
| : listenerFunction_(listenerFunction_) {} |
There was a problem hiding this comment.
haven't opened the code, writing from memory: don't we need here to have std::move / std::forward called to invoke move constructor of std::function?
| : listenerFunction_(listenerFunction_) {} | |
| : listenerFunction_(std::move(listenerFunction_)) {} |
There was a problem hiding this comment.
It works so I guess we don't?
There was a problem hiding this comment.
It is not matter of whether it works, it is matter of how it works.
See this snippet for the explanation.
Heres the example code in case the snippet times out (you can paste it to an online C++ compiler, e.g. this one
Code snippet
#include <iostream>
class SomeClass {
int someField;
public:
// DefaultConstructor
SomeClass() : someField{0} {
std::cout << "DefaultConstructor invoked\n";
}
// CopyConstructor
SomeClass(const SomeClass& other) {
std::cout << "CopyConstructor invoked\n";
this->someField = other.someField;
}
// MoveConstructor
SomeClass(SomeClass&& other) {
std::cout << "MoveConstructor invoked\n";
this->someField = std::move(other.someField);
}
};
class CompositeClass {
SomeClass someClassInstance;
public:
CompositeClass(SomeClass&& other) : someClassInstance(other) {}
// LOOK HERE!!!!
// Comment the line above and uncomment the line below to see the difference.
// CompositeClass(SomeClass&& other) : someClassInstance(std::move(other)) {}
};
// Type your code here, or load an example.
int main() {
SomeClass someClassInstance{};
CompositeClass ccInstance(std::move(someClassInstance));
return 0;
}There was a problem hiding this comment.
And now, after testing I'm sure we shall have std::move there.
There was a problem hiding this comment.
Can you explain what is the difference exactly and how it affects the code I added?
There was a problem hiding this comment.
I provided a code snippet with mimicking the setup you have here in this PR & appended comments explaining.
The difference comes down to which constructor is being called & that's why I've put "couts" in ctors of SomeClass class.
There was a problem hiding this comment.
Basically right now you take rvalue reference as RNSScreenRemovalListener constructor parameter just to later pass it by value to initialiser of listenerFunction_ field effectively copying it (invoking copy constructor). By moving there, as I've suggested, you invoke move constructor of std::function forwarding the parameter value correctly w/o unnecessary copy.
| ShadowViewMutationList mutations) const { | ||
| for (ShadowViewMutation mutation : mutations) { | ||
| if (mutation.type == ShadowViewMutation::Type::Remove && | ||
| mutation.oldChildShadowView.componentName != nullptr && |
There was a problem hiding this comment.
Why do we check oldChildShadowView.componentName for nullish value while later using parentShadowView.componentName for comparison purposes?
Also, are we looking for RNSScreenStack here, cause we want this fix to work only for native-stack? Isn't this (whiteflash) also an issue with ScreenContainer? (I don't have knowledge here, just wondering reading through the code).
There was a problem hiding this comment.
Why do we check oldChildShadowView.componentName for nullish value while later using parentShadowView.componentName for comparison purposes?
Because we use oldChildShadowView.componentName in listenerFunction_.
Also, are we looking for RNSScreenStack here, cause we want this fix to work only for native-stack? Isn't this (whiteflash) also an issue with ScreenContainer? (I don't have knowledge here, just wondering reading through the code).
Indeed it is only needed in RNSScreenStack since other navigators do not remove the view until the animation is completed.
…oftware-mansion#2261) ## Description > [!note] This PR applies to iOS only. Ok, so this PR is related to software-mansion#2247 & to get broader context I highly recommend to read [this comment](software-mansion#2247 (review)) at the very minimum. ### Issue context On Fabric during JS initialised Screen dismissal (view removing in general) children are unmounted before their parents, thus when dismissing screen from screen stack & starting a dismiss transition all Screen content is already removed & we're animating only a blank screen resulting in visual glitch. ### Current approaches Right now we're utilising `RCTMountingTransactionObserving` protocol, filter all mounting operations *before* they are applied and if screen dismissal is to be done, we take a snapshot of to-be-removed-screen. ### Alternative approaches software-mansion#2134 sets mounting coordinator delegate and effectively does the same as the current approach, however it can also be applied to Android. ### Proposed approach On iOS we can utilise the platform & how it works. Namely the fact of unmounting child view does not impact the hardware buffer, nor bitmap layer immediately, thus we can take the snapshot simply in `- [RNSScreen unmountChildComponentView: index:]` and the children will still be visible. This approach is safe and reliable, because: ##### 10k feet explanation Drawing is not performed immediately after an update to UIKit model (such as removing a view), the system handles all operations pending on main queue and just after that it schedules drawing. We're removing the views & making snapshot in the middle of block execution on the main thread, thus the drawing can't happen and just-unmounted-views will be visible on the snapshot. ##### More detailed explanation 1. the main thread run loop of Cocoa application drains the main queue till it's empty [[1]](https://opensource.apple.com/source/CF/CF-1153.18/CFRunLoop.c.auto.html) [[2]](https://fabernovel.github.io/2021-01-04/uikit-rendering-part-3) [[5]](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/RunLoopManagement/RunLoopManagement.html#//apple_ref/doc/uid/10000057i-CH16-SW1) 2. CoreAnimation framework integrates with the main run loop by registering an observer and listening for `kCFRunLoopBeforeWaiting` event (so after the main queue is drained & run loop is to become idle due to no more pending tasks). [[2]](https://fabernovel.github.io/2021-01-04/uikit-rendering-part-3) 3. CoreAnimation is responsible for applying all transactions from the last loop pass & sending them to render server (this happens on main thread), which in turn finally leads up to the changes being applied, drawn & displayed (this happens on different threads). [[2]](https://fabernovel.github.io/2021-01-04/uikit-rendering-part-3) [[3]](https://developer.apple.com/library/archive/documentation/WindowsViews/Conceptual/ViewPG_iPhoneOS/WindowsandViews/WindowsandViews.html#//apple_ref/doc/uid/TP40009503-CH2-SW1) 4. [We know that the RN's mounting stage will be executed on main thread](https://github.com/facebook/react-native/blob/91ecd7eb5330f5d725f5587744713064d614a6b3/packages/react-native/React/Fabric/Mounting/RCTMountingManager.mm#L258), because UIKit is thread-safe only in selected parts and requires calling from the main thread. 5. Single RN transaction is a complete diff between ["rendered tree" & "next tree"](https://reactnative.dev/architecture/render-pipeline#phase-2-commit-1) and is performed [atomically & synchronously](https://github.com/facebook/react-native/blob/91ecd7eb5330f5d725f5587744713064d614a6b3/packages/react-native/ReactCommon/react/renderer/mounting/TelemetryController.cpp#L18-L51) on [main thread](https://github.com/facebook/react-native/blob/91ecd7eb5330f5d725f5587744713064d614a6b3/packages/react-native/React/Fabric/Mounting/RCTMountingManager.mm#L258), thus whole batch of updates will be finished before drawing instructions will be send to render server. #### Reference: [[1]](https://opensource.apple.com/source/CF/CF-1153.18/CFRunLoop.c.auto.html) (Look for `__CFRunLoopDoBlocks(...)` & `__CFRunLoopRun(...)` functions) Important thing to notice if `__CFRunLoopDoBlocks` is that it locks the `rl` (run loop) lock, takes & copies reference to the list of the blocks to execute, clears the original list of blocks and releases the `rl` lock. Thus only the "already scheduled" blocks are executed in the single pass of this function. It is called multiple times in the single pass of the run loop, but I haven't dug deeper, it should be enough for our use case that we have guarantee that all the blocks are drained. [[2]](https://fabernovel.github.io/2021-01-04/uikit-rendering-part-3) (Blog post on rendering in UIKit) [[3]](https://developer.apple.com/library/archive/documentation/WindowsViews/Conceptual/ViewPG_iPhoneOS/WindowsandViews/WindowsandViews.html#//apple_ref/doc/uid/TP40009503-CH2-SW1) (Apple docs - The View Drawing Cycle section) [[4]](https://bou.io/RunRunLoopRun.html) (Blog post on the run loop) [[5]](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/RunLoopManagement/RunLoopManagement.html#//apple_ref/doc/uid/10000057i-CH16-SW1) (Apple docs on run loops) ## Changes * Snapshot is not done in `unmountChildComponentView: index:` & only when needed. * Removed old mechanism * Removed now unused implementation of `RCTMountingObserving` protocol ## Test code and steps to reproduce Run any example on Fabric, push a screen, initiate go-back via JS (e.g. by clicking a button with `navigation.goBack()` action), see that the screen transitions correctly (the content is visible throughout transition) ## Checklist - [x] Ensured that CI passes
…2134) PR adding snapshots when going back on Fabric on Android and changing the behavior a bit on iOS.
## Description This PR intents to fix current screen going blank on Android when returning to the previous one. The issue has been already fixed by #2134, but re-appeared recently. After a quick bisect it turned out that #2412 caused the regression. Bringing back the removed `NativeScreensModule` export fixes the issue. Fixes #1685 ## Changes - added missing export - modified `Test2282.tsx` repro ## Screenshots / GIFs ### Before https://github.com/user-attachments/assets/39bf6c66-39d2-4dfd-abc8-ee1c690417a4 ### After https://github.com/user-attachments/assets/003446b2-c976-4cdb-8ab4-348dd619ba79 ## Test code and steps to reproduce - use `Test2282.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
## Description Based on Expensify/App#49937 (comment) and the comments above, PR fixes the unnecessary checks for creating snapshot on the view on dismiss. It was refactored already in #2134 and #2261 (👏 to @kkafar). Those checks do nothing now since each screen is responsible for making its own snapshot. Having those checks can only lead to problems.
## Description This PR intents to fix current screen going blank on Android when returning to the previous one. The issue has been already fixed by software-mansion/react-native-screens#2134, but re-appeared recently. After a quick bisect it turned out that software-mansion/react-native-screens#2412 caused the regression. Bringing back the removed `NativeScreensModule` export fixes the issue. Fixes #1685 ## Changes - added missing export - modified `Test2282.tsx` repro ## Screenshots / GIFs ### Before https://github.com/user-attachments/assets/39bf6c66-39d2-4dfd-abc8-ee1c690417a4 ### After https://github.com/user-attachments/assets/003446b2-c976-4cdb-8ab4-348dd619ba79 ## Test code and steps to reproduce - use `Test2282.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
## Description Based on Expensify/App#49937 (comment) and the comments above, PR fixes the unnecessary checks for creating snapshot on the view on dismiss. It was refactored already in software-mansion/react-native-screens#2134 and software-mansion/react-native-screens#2261 (👏 to @kkafar). Those checks do nothing now since each screen is responsible for making its own snapshot. Having those checks can only lead to problems.
4 participants
Description
PR adding snapshots when going back on Fabric on Android and changing the behavior a bit on iOS.