-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[screen-capture] Fix crash on bridgeless #28244
Conversation
The Pull Request introduced fingerprint changes against the base commit: b30d376 Fingerprint diff[
{
"type": "dir",
"filePath": "../../packages/expo-screen-capture/android",
"reasons": [
"expoAutolinkingAndroid"
],
"hash": "aec51f980b0e08b59e09ec7b98ed9a8bf6617e2d"
}
] Generated by PR labeler 🤖 |
@@ -32,7 +33,6 @@ class ScreenCaptureModule : Module() { | |||
screenCaptureCallback = Activity.ScreenCaptureCallback { | |||
sendEvent(eventName) | |||
} | |||
currentActivity.registerScreenCaptureCallback(currentActivity.mainExecutor, screenCaptureCallback!!) |
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 believe that, in theory, the current activity should be available at this point. Maybe it's connected with #28200? I'm okay with merging your fix, but generally speaking, I don't like that the order of initialization was changed.
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.
It could be. AFAIK it's an upstream issue that Kudo has brought up with them. I just made this change for now because this is the only module that stops the app from running on bridgeless.
Why
Fixes a crash on bridgeless caused by accessing the activity too early
How
Refactored to register the listener when the user first uses the api.
Test Plan
bare-expo on bridgeless. Crash does not occur and the module works as expected