-
-
Notifications
You must be signed in to change notification settings - Fork 546
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.
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
feat: refactor snapshots when going back on Fabric #2134
Changes from all commits
b975b0e
639b67d
dae22d1
ef3baa4
1e2bebb
d1af500
99be7d1
ca2f34e
3de04f5
7909666
7cf4390
05dfb34
16f9498
8fd8ae7
33a6ac5
f467091
b28c1c5
18a7aaa
c8e4ba7
31c2f2e
1019731
d0c24ff
bd7a30e
8dfcdb0
e127cde
0cfd980
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,11 +2,22 @@ cmake_minimum_required(VERSION 3.9.0) | |||||
|
||||||
project(rnscreens) | ||||||
|
||||||
if(${RNS_NEW_ARCH_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 | ||||||
|
@@ -19,9 +30,42 @@ set_target_properties(rnscreens PROPERTIES | |||||
POSITION_INDEPENDENT_CODE ON | ||||||
) | ||||||
|
||||||
target_compile_definitions( | ||||||
rnscreens | ||||||
PRIVATE | ||||||
-DFOLLY_NO_CONFIG=1 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I believe you can omit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't work if I remove it so I think it is not true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not gonna lie, I'm surprised as CMake docs I've linked explicitly state that you can omit |
||||||
) | ||||||
|
||||||
find_package(ReactAndroid REQUIRED CONFIG) | ||||||
|
||||||
target_link_libraries(rnscreens | ||||||
ReactAndroid::jsi | ||||||
android | ||||||
) | ||||||
if(${RNS_NEW_ARCH_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 | ||||||
ReactAndroid::react_render_consistency | ||||||
ReactAndroid::react_performance_timeline | ||||||
ReactAndroid::react_render_observers_events | ||||||
fbjni::fbjni | ||||||
android | ||||||
) | ||||||
else() | ||||||
target_link_libraries(rnscreens | ||||||
ReactAndroid::jsi | ||||||
android | ||||||
) | ||||||
endif() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
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.") | ||
} | ||
} | ||
} |
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're setting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cause if that is the case, we might want keep the old approach on iOS. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
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 |
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(); }); | ||
} |
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 | ||
|
@@ -37,6 +38,7 @@ class Screen( | |
var screenOrientation: Int? = null | ||
private set | ||
var isStatusBarAnimated: Boolean? = null | ||
var isBeingRemoved = false | ||
|
||
init { | ||
// we set layout params as WindowManager.LayoutParams to workaround the issue with TextInputs | ||
|
@@ -280,6 +282,40 @@ class Screen( | |
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have it called multiple times? Why do we have the multiple-entry guard here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 bump the number of children to make it work. | ||
// TODO: find a better way to handle this scenario | ||
it.addView(View(context), i) | ||
} 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 = | ||
|
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.
Why we're not scoping all .cpp and .h files right now?
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.
Because the new files added to the folder should not be compiled on old arch.
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.
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 intocpp/mounting
or sth similar.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.
Can we do a cleanup of those in the follow-up PR?