-
-
Notifications
You must be signed in to change notification settings - Fork 597
fix(Android): add custom fragment factory & modify library installation steps to prevent on-restoration crashes #3089
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
Conversation
kkafar
left a comment
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.
This looks really good overall. I have only few cosmetic changes requests.
I haven't tested the runtime though, will do when I tackle upgrade to 0.81.
Let's also give a credit to @bojanlozanovski77, who suggested this idea to us in the PR description. I see that you added him as co-author of commits - and that's awesome!
|
|
||
| class RNScreensFragmentFactory : FragmentFactory() { | ||
| override fun instantiate(classLoader: ClassLoader, className: String): Fragment { | ||
| return if (className.startsWith(BuildConfig.LIBRARY_PACKAGE_NAME)) { |
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.
I like this!
android/src/main/java/com/swmansion/rnscreens/RNScreensFragmentFactory.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: bojanlozanovski77 <[email protected]>
Co-authored-by: bojanlozanovski77 <[email protected]>
Co-authored-by: bojanlozanovski77 <[email protected]>
898b35d to
29de0d8
Compare
So that these do not reside in top level library namespace
kkafar
left a comment
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.
Seems that runtime works fine.
I've done some small refactors & will land this once the CI passes.
Thanks!
|
@Skalakid hello. I didn't quite understand from your example, do I need to leave this line in AndroidManifest.xml now? |
|
@bulkinav yes, you can leave this line as it is. I suggested removing it in the test steps to be able to test if the fix works and there aren't any crashes. If you remove this line, RN will lose its state during configuration changes. So in my opinion, the most common types should be specified there |
|
We saw a increased number of IllegalStateException about the fragement resotration after enabling the RNScreensFragmentFactory as the newer README suggested, and as far as I can see it should not cause any behaviour change if users continue to use I'll open a separate issue when I successfully reproduces the IllegalStateException, but for now I'm mainly looking for workaorunds. |
|
This is correct. If this turns out to be problematic you can stay on |
|
This is not a solution for preserving application state. Whether the state will be preserved or not depends on your particular application setup & how the react native is set up. |
Description
This PR replaces the current workaround for crashes connected to the configuration changes on Android. The previous fix required disabling passing saved instance state in Activity onCreate -
onCreate(null). Such a solution generates a problem with state persistence on the native side of brownfield applications. Because of that, we decided to replace it with a custom fragment factory that will limit the scope of the solution and fix the problem directly only on the React Native side. All fragments connected to the RN Screens package will be replaced with self-destructing ones, which will prevent Android from recreating the Fragment after the configuration change. We will leave this responsibility for React Native. Thanks to that, there won't be any conflict between the Android system and React Native, and we will prevent the app from crashing or any other app appearance-related issues.Credits to @bojanlozanovski77 for bringing up the idea in #2917
Fixes: #17
Changes
MainActivityfiles ofFabricExampleandExampleappsTest code and steps to reproduce
AndroidManifest.xmlfile<activity android:name=".MainActivity" android:label="@string/app_name" - android:configChanges="keyboard|keyboardHidden|orientation|screenLayout|screenSize|smallestScreenSize|uiMode" android:launchMode="singleTask" ...Checklist