-
-
Notifications
You must be signed in to change notification settings - Fork 514
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 8 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 |
---|---|---|
|
@@ -4,7 +4,7 @@ package = JSON.parse(File.read(File.join(__dir__, "package.json"))) | |
|
||
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. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 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 do a cleanup of those in the follow-up PR? |
||
|
||
folly_compiler_flags = '-DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1 -Wno-comma -Wno-shorten-64-to-32' | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,83 @@ | ||
cmake_minimum_required(VERSION 3.9.0) | ||
|
||
project(rnscreens) | ||
set(CMAKE_CXX_STANDARD 20) | ||
|
||
if(${IS_NEW_ARCHITECTURE_ENABLED}) | ||
string(APPEND CMAKE_CXX_FLAGS " -DRCT_NEW_ARCH_ENABLED") | ||
endif() | ||
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. Why are we setting this? Is that because of some unmerged changes earlier for matching RN 0.74? |
||
|
||
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 | ||
) | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we replace setting properties on particular target with setting the non-target-scoped variable 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. I'm not sure why I removed it, reverted. |
||
target_compile_definitions( | ||
rnscreens | ||
PRIVATE | ||
-DFOLLY_NO_CONFIG=1 | ||
) | ||
|
||
find_package(ReactAndroid REQUIRED CONFIG) | ||
|
||
target_link_libraries(rnscreens | ||
ReactAndroid::jsi | ||
android | ||
) | ||
if(${IS_NEW_ARCHITECTURE_ENABLED}) | ||
target_include_directories( | ||
rnscreens | ||
PRIVATE | ||
"${REACT_NATIVE_DIR}/ReactAndroid/src/main/jni/react/turbomodule" | ||
"${REACT_NATIVE_DIR}/ReactCommon" | ||
"${REACT_NATIVE_DIR}/ReactCommon/callinvoker" | ||
"${REACT_NATIVE_DIR}/ReactCommon/react/renderer/graphics/platform/cxx" | ||
"${REACT_NATIVE_DIR}/ReactCommon/runtimeexecutor" | ||
"${REACT_NATIVE_DIR}/ReactCommon/yoga" | ||
) | ||
|
||
find_package(fbjni REQUIRED CONFIG) | ||
|
||
target_link_libraries( | ||
rnscreens | ||
ReactAndroid::jsi | ||
ReactAndroid::folly_runtime | ||
ReactAndroid::react_nativemodule_core | ||
ReactAndroid::react_render_uimanager | ||
ReactAndroid::react_render_scheduler | ||
ReactAndroid::react_utils | ||
ReactAndroid::reactnativejni | ||
ReactAndroid::fabricjni | ||
ReactAndroid::react_debug | ||
ReactAndroid::react_render_core | ||
ReactAndroid::react_render_mounting | ||
ReactAndroid::runtimeexecutor | ||
ReactAndroid::react_render_graphics | ||
ReactAndroid::rrc_view | ||
ReactAndroid::yoga | ||
ReactAndroid::rrc_text | ||
ReactAndroid::rrc_textinput | ||
ReactAndroid::react_render_textlayoutmanager | ||
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() |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -77,6 +77,7 @@ def reactProperties = new Properties() | |||||
file("$reactNativeRootDir/ReactAndroid/gradle.properties").withInputStream { reactProperties.load(it) } | ||||||
def REACT_NATIVE_VERSION = reactProperties.getProperty("VERSION_NAME") | ||||||
def REACT_NATIVE_MINOR_VERSION = REACT_NATIVE_VERSION.startsWith("0.0.0-") ? 1000 : REACT_NATIVE_VERSION.split("\\.")[1].toInteger() | ||||||
def IS_NEW_ARCHITECTURE_ENABLED = isNewArchitectureEnabled() | ||||||
|
||||||
android { | ||||||
compileSdkVersion safeExtGet('compileSdkVersion', rnsDefaultCompileSdkVersion) | ||||||
|
@@ -102,13 +103,14 @@ android { | |||||
targetSdkVersion safeExtGet('targetSdkVersion', rnsDefaultTargetSdkVersion) | ||||||
versionCode 1 | ||||||
versionName "1.0" | ||||||
buildConfigField "boolean", "IS_NEW_ARCHITECTURE_ENABLED", isNewArchitectureEnabled().toString() | ||||||
buildConfigField "boolean", "IS_NEW_ARCHITECTURE_ENABLED", IS_NEW_ARCHITECTURE_ENABLED.toString() | ||||||
ndk { | ||||||
abiFilters (*reactNativeArchitectures()) | ||||||
} | ||||||
externalNativeBuild { | ||||||
cmake { | ||||||
arguments "-DANDROID_STL=c++_shared" | ||||||
arguments "-DANDROID_STL=c++_shared", | ||||||
"-DIS_NEW_ARCHITECTURE_ENABLED=${IS_NEW_ARCHITECTURE_ENABLED}" | ||||||
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
won't insist here, my only intention is to keep it consistent with iOS (I believe we even use 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. done |
||||||
} | ||||||
} | ||||||
} | ||||||
|
@@ -139,13 +141,14 @@ android { | |||||
"META-INF/**", | ||||||
"**/libjsi.so", | ||||||
"**/libc++_shared.so", | ||||||
"**/libreact_render*.so" | ||||||
"**/libreact_render*.so", | ||||||
"**/libreactnativejni.so" | ||||||
] | ||||||
} | ||||||
sourceSets.main { | ||||||
ext.androidResDir = "src/main/res" | ||||||
java { | ||||||
if (isNewArchitectureEnabled()) { | ||||||
if (IS_NEW_ARCHITECTURE_ENABLED) { | ||||||
srcDirs += [ | ||||||
"src/fabric/java", | ||||||
] | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
package com.swmansion.rnscreens | ||
|
||
import com.facebook.jni.HybridData | ||
import com.facebook.proguard.annotations.DoNotStrip | ||
import com.facebook.react.bridge.ReactApplicationContext | ||
import com.facebook.react.fabric.FabricUIManager | ||
import com.facebook.react.uimanager.UIManagerHelper | ||
import com.facebook.react.uimanager.common.UIManagerType | ||
import com.facebook.soloader.SoLoader | ||
|
||
class NativeProxy(private val reactContext: ReactApplicationContext) { | ||
@DoNotStrip | ||
@Suppress("unused") | ||
private val mHybridData: HybridData | ||
|
||
init { | ||
mHybridData = initHybrid() | ||
} | ||
|
||
private external fun initHybrid(): HybridData | ||
external fun nativeAddMutationsListener(fabricUIManager: FabricUIManager) | ||
|
||
@DoNotStrip | ||
public fun notifyScreenRemoved(screenTag: Int) { | ||
val uiManager = UIManagerHelper.getUIManager(reactContext, UIManagerType.FABRIC) | ||
val screen = uiManager?.resolveView(screenTag) | ||
if (screen is Screen) { | ||
screen.startRemovalTransition() | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
#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_); | ||
}); | ||
} | ||
|
||
jni::local_ref<NativeProxy::jhybriddata> | ||
NativeProxy::initHybrid(jni::alias_ref<jhybridobject> jThis) { | ||
return makeCxxInstance(jThis); | ||
} | ||
|
||
} |
WoLewicki marked this conversation as resolved.
Show resolved
Hide resolved
|
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); | ||
}; | ||
|
||
} |
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(); }); | ||
} |
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?