feat: refactor snapshots when going back on Fabric #2134
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,27 +1,62 @@ | ||||||
| cmake_minimum_required(VERSION 3.9.0) | ||||||
|
|
||||||
| project(rnscreens) | ||||||
| set(CMAKE_CXX_STANDARD 20) | ||||||
|
|
||||||
| if(${IS_NEW_ARCHITECTURE_ENABLED}) | ||||||
| add_library(rnscreens | ||||||
| SHARED | ||||||
| ../cpp/RNScreensTurboModule.cpp | ||||||
| ../cpp/RNSScreenRemovalListener.cpp | ||||||
| ./src/main/cpp/jni-adapter.cpp | ||||||
| ./src/main/cpp/NativeProxy.cpp | ||||||
| ./src/main/cpp/OnLoad.cpp | ||||||
| ) | ||||||
| else() | ||||||
| add_library(rnscreens | ||||||
| SHARED | ||||||
| ../cpp/RNScreensTurboModule.cpp | ||||||
| ./src/main/cpp/jni-adapter.cpp | ||||||
| ) | ||||||
| endif() | ||||||
|
|
||||||
| include_directories( | ||||||
| ../cpp | ||||||
| ) | ||||||
|
|
||||||
|
There was a problem hiding this comment. Why do we replace setting properties on particular target with setting the non-target-scoped variable 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.
Suggested change
I believe you can omit 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 |
||||||
| ) | ||||||
|
|
||||||
| find_package(ReactAndroid REQUIRED CONFIG) | ||||||
|
|
||||||
| if(${IS_NEW_ARCHITECTURE_ENABLED}) | ||||||
| find_package(fbjni REQUIRED CONFIG) | ||||||
|
|
||||||
| target_link_libraries( | ||||||
| rnscreens | ||||||
| ReactAndroid::jsi | ||||||
| ReactAndroid::react_nativemodule_core | ||||||
| ReactAndroid::react_utils | ||||||
| ReactAndroid::reactnativejni | ||||||
| ReactAndroid::fabricjni | ||||||
| ReactAndroid::react_debug | ||||||
| ReactAndroid::react_render_core | ||||||
| ReactAndroid::runtimeexecutor | ||||||
| ReactAndroid::react_render_graphics | ||||||
| ReactAndroid::rrc_view | ||||||
| ReactAndroid::yoga | ||||||
| ReactAndroid::rrc_text | ||||||
| ReactAndroid::glog | ||||||
| ReactAndroid::react_render_componentregistry | ||||||
| fbjni::fbjni | ||||||
| android | ||||||
| ) | ||||||
WoLewicki marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| else() | ||||||
| target_link_libraries(rnscreens | ||||||
| ReactAndroid::jsi | ||||||
| android | ||||||
| ) | ||||||
| endif() | ||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| package com.swmansion.rnscreens | ||
|
|
||
| import android.util.Log | ||
| import com.facebook.jni.HybridData | ||
| import com.facebook.proguard.annotations.DoNotStrip | ||
| import com.facebook.react.fabric.FabricUIManager | ||
| import java.lang.ref.WeakReference | ||
| import java.util.concurrent.ConcurrentHashMap | ||
|
|
||
| class NativeProxy { | ||
| @DoNotStrip | ||
| @Suppress("unused") | ||
| private val mHybridData: HybridData | ||
|
|
||
| init { | ||
| mHybridData = initHybrid() | ||
| } | ||
|
|
||
| private external fun initHybrid(): HybridData | ||
| external fun nativeAddMutationsListener(fabricUIManager: FabricUIManager) | ||
|
|
||
| companion object { | ||
| // we use ConcurrentHashMap here since it will be read on the JS thread, | ||
| // and written to on the UI thread. | ||
| private val viewsMap = ConcurrentHashMap<Int, WeakReference<Screen>>() | ||
|
|
||
| fun addScreenToMap(tag: Int, view: Screen) { | ||
| viewsMap[tag] = WeakReference(view) | ||
| } | ||
|
|
||
| fun removeScreenFromMap(tag: Int) { | ||
| viewsMap.remove(tag) | ||
| } | ||
|
|
||
| fun clearMapOnInvalidate() { | ||
| viewsMap.clear() | ||
| } | ||
| } | ||
|
|
||
| @DoNotStrip | ||
| public fun notifyScreenRemoved(screenTag: Int) { | ||
| val screen = viewsMap[screenTag]?.get() | ||
| if (screen is Screen) { | ||
| screen.startRemovalTransition() | ||
| } else { | ||
| Log.w("[RNScreens]", "Did not find view with tag ${screenTag}.") | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| #include <fbjni/fbjni.h> | ||
| #include <react/fabric/Binding.h> | ||
| #include <react/renderer/scheduler/Scheduler.h> | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "NativeProxy.h" | ||
|
|
||
| using namespace facebook; | ||
| using namespace react; | ||
|
|
||
| namespace rnscreens { | ||
|
|
||
| NativeProxy::NativeProxy(jni::alias_ref<NativeProxy::javaobject> jThis) | ||
| : javaPart_(jni::make_global(jThis)) {} | ||
|
|
||
| NativeProxy::~NativeProxy() {} | ||
|
|
||
| void NativeProxy::registerNatives() { | ||
| registerHybrid( | ||
| {makeNativeMethod("initHybrid", NativeProxy::initHybrid), | ||
| makeNativeMethod( | ||
| "nativeAddMutationsListener", | ||
| NativeProxy::nativeAddMutationsListener)}); | ||
| } | ||
|
|
||
| void NativeProxy::nativeAddMutationsListener( | ||
| jni::alias_ref<facebook::react::JFabricUIManager::javaobject> | ||
| fabricUIManager) { | ||
| auto uiManager = | ||
| fabricUIManager->getBinding()->getScheduler()->getUIManager(); | ||
| screenRemovalListener_ = | ||
| std::make_shared<RNSScreenRemovalListener>([this](int tag) { | ||
| static const auto method = | ||
| javaPart_->getClass()->getMethod<void(jint)>("notifyScreenRemoved"); | ||
| method(javaPart_, tag); | ||
| }); | ||
|
|
||
| uiManager->getShadowTreeRegistry().enumerate( | ||
| [this](const facebook::react::ShadowTree &shadowTree, bool &stop) { | ||
| shadowTree.getMountingCoordinator()->setMountingOverrideDelegate( | ||
| screenRemovalListener_); | ||
| }); | ||
| } | ||
|
Comment on lines
+39
to
+44
There was a problem hiding this comment. We're setting the There was a problem hiding this comment. Cause if that is the case, we might want keep the old approach on iOS. There was a problem hiding this comment. With RN 0.74 it is possible that it will be overriden. I think I'll target 0.75 with this PR though since there is my commit already: facebook/react-native@358fe46 |
||
|
|
||
| jni::local_ref<NativeProxy::jhybriddata> NativeProxy::initHybrid( | ||
| jni::alias_ref<jhybridobject> jThis) { | ||
| return makeCxxInstance(jThis); | ||
| } | ||
|
|
||
| } // namespace rnscreens | ||
WoLewicki marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #pragma once | ||
|
|
||
| #include <fbjni/fbjni.h> | ||
| #include <react/fabric/JFabricUIManager.h> | ||
| #include "RNSScreenRemovalListener.h" | ||
|
|
||
| #include <string> | ||
|
|
||
| namespace rnscreens { | ||
| using namespace facebook; | ||
| using namespace facebook::jni; | ||
|
|
||
| class NativeProxy : public jni::HybridClass<NativeProxy> { | ||
| public: | ||
| std::shared_ptr<RNSScreenRemovalListener> screenRemovalListener_; | ||
| static auto constexpr kJavaDescriptor = | ||
| "Lcom/swmansion/rnscreens/NativeProxy;"; | ||
| static jni::local_ref<jhybriddata> initHybrid( | ||
| jni::alias_ref<jhybridobject> jThis); | ||
| static void registerNatives(); | ||
|
|
||
| ~NativeProxy(); | ||
|
|
||
| private: | ||
| friend HybridBase; | ||
| jni::global_ref<NativeProxy::javaobject> javaPart_; | ||
|
|
||
| explicit NativeProxy(jni::alias_ref<NativeProxy::javaobject> jThis); | ||
|
|
||
| void nativeAddMutationsListener( | ||
| jni::alias_ref<facebook::react::JFabricUIManager::javaobject> | ||
| fabricUIManager); | ||
| }; | ||
|
|
||
| } // namespace rnscreens |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| #include <fbjni/fbjni.h> | ||
|
|
||
| #include "NativeProxy.h" | ||
|
|
||
| JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *) { | ||
| return facebook::jni::initialize( | ||
| vm, [] { rnscreens::NativeProxy::registerNatives(); }); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import android.graphics.Paint | |
| import android.os.Parcelable | ||
| import android.util.SparseArray | ||
| import android.util.TypedValue | ||
| import android.view.View | ||
| import android.view.ViewGroup | ||
| import android.view.WindowManager | ||
| import android.webkit.WebView | ||
|
|
@@ -35,6 +36,7 @@ class Screen(context: ReactContext?) : FabricEnabledViewGroup(context) { | |
| var screenOrientation: Int? = null | ||
| private set | ||
| var isStatusBarAnimated: Boolean? = null | ||
| private var isBeingRemoved = false | ||
|
|
||
| init { | ||
| // we set layout params as WindowManager.LayoutParams to workaround the issue with TextInputs | ||
|
|
@@ -246,6 +248,40 @@ class Screen(context: ReactContext?) : FabricEnabledViewGroup(context) { | |
|
|
||
| var nativeBackButtonDismissalEnabled: Boolean = true | ||
|
|
||
| fun startRemovalTransition() { | ||
| if (!isBeingRemoved) { | ||
| isBeingRemoved = true | ||
| startTransitionRecursive(this) | ||
| } | ||
|
Comment on lines
+286
to
+289
There was a problem hiding this comment. Can we have it called multiple times? Why do we have the multiple-entry guard here? There was a problem hiding this comment. Yes, when navigating from JS it is called from the cpp and after that from |
||
| } | ||
|
|
||
| private fun startTransitionRecursive(parent: ViewGroup?) { | ||
| parent?.let { | ||
| for (i in 0 until it.childCount) { | ||
| val child = it.getChildAt(i) | ||
| if (child.javaClass.simpleName.equals("CircleImageView")) { | ||
| // SwipeRefreshLayout class which has CircleImageView as a child, | ||
| // does not handle `startViewTransition` properly. | ||
| // It has a custom `getChildDrawingOrder` method which returns | ||
| // wrong index if we called `startViewTransition` on the views on new arch. | ||
| // We add a simple View to make bump the number of children so it works. | ||
| // TODO: find a better way to handle this scenario | ||
| it.addView(View(context), i) | ||
|
There was a problem hiding this comment. Although it is something similar @maciekstosio is handling with the not respected zIndex during pop transition in custom header. There was a problem hiding this comment. Yeah, I spent enough time trying to handle it correctly and I have no idea how to do it better. |
||
| } else { | ||
| child?.let { view -> it.startViewTransition(view) } | ||
| } | ||
| if (child is ScreenStackHeaderConfig) { | ||
| // we want to start transition on children of the toolbar too, | ||
| // which is not a child of ScreenStackHeaderConfig | ||
| startTransitionRecursive(child.toolbar) | ||
| } | ||
| if (child is ViewGroup) { | ||
| startTransitionRecursive(child) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun calculateHeaderHeight(): Pair<Double, Double> { | ||
| val actionBarTv = TypedValue() | ||
| val resolvedActionBarSize = context.theme.resolveAttribute(android.R.attr.actionBarSize, actionBarTv, true) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,7 +151,7 @@ class ScreenStackHeaderConfigViewManager : ViewGroupManager<ScreenStackHeaderCon | |
|
|
||
| // TODO: Find better way to handle platform specific props | ||
| private fun logNotAvailable(propName: String) { | ||
| Log.w("[RNScreens]", "$propName prop is not available on Android") | ||
| } | ||
|
|
||
| override fun setBackTitle(view: ScreenStackHeaderConfig?, value: String?) { | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on here, why do we change that?