Skip to content
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

Process the files in a separate thread on Android #2142

Merged

Conversation

renchap
Copy link
Contributor

@renchap renchap commented Jun 8, 2023

Motivation (required)

At the moment, when picking files on Android, all the processing (copying the file into the app's cache & extracting metadata) is made in the main thread.

This causes the app UI to become unavailable, and may trigger Android's Application Not Responding alerts, or have the application killed.

This changes runs the processing in a separate thread, that notifies the RN bridge once finished.

Ideally we could have a thread pool and have each file processed independently inside a thread, but I am not familiar enough with Java to do this properly.

Test Plan (required)

I tested this both in Emulator and on a physical Android 13 phone, using the example app.

It worked fine with many videos or photos and the app UI was responsive.

I am trying to resurrect an older Android device to run the tests on it.

Before this, the app's main thread was locked until all files
were copied and metadata were extracted. This means the
app's UI was not responsive.

Now the app's UI is responsive while those are processed, and
the JS Promise resolves once processed as expected.

This also prevents Application Not Responding alerts to
trigger when adding a lot of files at once.
@Johan-dutoit
Copy link
Collaborator

When does the executor get shutdown? Is that needed?

@renchap
Copy link
Contributor Author

renchap commented Jun 13, 2023

I am not a Java person, but this SO question gives some answers:

  • shutdown() is not needed except if to prevent new tasks to be added (not the case here, as its used once)
  • Executor should probably only be created once, and re-used on further onAssetsObtained calls. I am not sure how to do this properly in Java.

Also I did not test this with Fabric, I dont know if there might be issues here.

Copy link

@dev-aniketj dev-aniketj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice work buddy

@Johan-dutoit Johan-dutoit merged commit dc67109 into react-native-image-picker:main Jun 15, 2023
@renchap renchap deleted the android-use-thread branch June 15, 2023 08:34
@Johan-dutoit
Copy link
Collaborator

🎉 This PR is included in version 5.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants