-
-
Notifications
You must be signed in to change notification settings - Fork 587
fix(Android): crash API 25 when going back from screen with flat list #2964
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
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.
Yeah I understand the issue. This won't be the way-to-go to fix it. mostly because concurrency problems. What we should do in lieu of this approach, is to schedule calls to startTransitionRecursive
on UI thread.
I was trying to do this already some time ago, but decided to not merge since "there weren't any problems with current approach".
Can we have a call so that I can explain the fix I see here to you?
This is the approach I've mentioned to you: https://github.com/software-mansion/react-native-screens/pull/2532/files, however I'm not happy with this PR neither. Let's have a talk today & I'll explain. |
NativeProxy We should be fine doing this, because the `notifyScreenRemoved` code is run on JS thread BEFORE transaction execution is scheduled on UI thread. This code works only under assumption, that the transaction is scheduled in a SERIAL, no prioritized manner. TOOD: We need to confirm that this is a case on Android.
@maciekstosio Do you remember, what was the conclusion for this PR? Were there any issues? I have something in the back of my head, that there was an issue, but can not recall exactly. |
This problem would not be solved with? fun startRemovalTransition() {
if (!isBeingRemoved) {
isBeingRemoved = true
post { startTransitionRecursive(this) }
}
} |
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've tested the runtime & it seems to work fine.
This PR will be also helpful in landing of #3250, therefore I decide to proceed here.
Description
Fixes #2810
This PR should fix a crash when going back from the screen that has flat list.
Changes
There are two exceptions thrown:
First:
Second:
And it fails around this place in
Screen.kt
:Based on the comment, I assume
it.addView(View(context), i)
is to prevent the second crash. Because the first exception, we do not add "workaround" view, thus we run into second. We get the first exception, as the operation is performed from different thread, thus I ensure the adding view is performed from UIThread.Test code and steps to reproduce
Source code
Checklist