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

[android][expo-media-library] migrate to new modules API #20232

Merged

Conversation

alanjhughes
Copy link
Collaborator

@alanjhughes alanjhughes commented Nov 27, 2022

Why

Migrate native module to use new modules API

How

This was a much larger migration than any I have attempted before so I will need feedback from the team. Maybe something of this size is not wanted from external contributors.

Followed the typical migration steps.

Note

  • One of the functions getAssetsAsync accepts an options object. One of the parameters is an array that can be a mixed type. A string or a tuple. I wasn't sure how to type this. The Either type is still marked as experimental so I didn't want to use it. Also, regarding this options type, the first variable was used as anInt sometimes and a
    Double at other times. I settled on a Double.
  • The module was renamed from ExponentMediaLibrary to ExpoMediaLibrary
  • There are cases where I still use promise.resolve(null) I'm aware this is not usually necessary but without it, some functions would never resolve
  • There are some instances where an exception is caught and rejected in the same way it was with the old API. I've got varied feedback on this so I'm not sure which way is preferred
  • There are cases where functions use things that require a min SDK of 29. In the original code sometimes this was handled and other times it wasn't. Didn't know whether to handle this or leave as it

Test Plan

Checked by running the bare expo app and running the existing test suite.
After the first iteration, the checks in the bare app all passed but the native unit tests were broken because of the changes in the Pomise API. These were fixed, and now all 43 native tests pass. The JS tests all pass as before.

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Nov 27, 2022
@alanjhughes alanjhughes changed the title @alanhughes/android/modules expo media library [android[expo-media-library] migrate to new modules API Nov 27, 2022
@alanjhughes alanjhughes changed the title [android[expo-media-library] migrate to new modules API [android][expo-media-library] migrate to new modules API Nov 27, 2022
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Nov 27, 2022
@alanjhughes alanjhughes marked this pull request as ready for review November 27, 2022 13:56
@alanjhughes
Copy link
Collaborator Author

alanjhughes commented Nov 27, 2022

Not sure why the spotless check is failing here. I have ran it

Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

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

Looking very promising, but I'll have to run through your changes one more time ;)

CodedException("Media library is corrupted")

class EmptyAlbumException :
CodedException("Found album is empty.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CodedException("Found album is empty.")
CodedException("Found album is empty")

We stoped using dots in the error messages.

CodedException("Found album is empty.")

class AlbumPathException :
CodedException("Couldn't get album path.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CodedException("Couldn't get album path.")
CodedException("Couldn't get album path")

CodedException("Couldn't find album")

class AssetQueryException :
CodedException("Could not get asset. Query returns null.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CodedException("Could not get asset. Query returns null.")
CodedException("Could not get asset. Query returns null")

CodedException(message)

class AssetException :
CodedException("Could not add image to gallery.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CodedException("Could not add image to gallery.")
CodedException("Could not add image to gallery")

CodedException("Could not add image to gallery.")

class ContentEntryException :
CodedException("Could not create content entry.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CodedException("Could not create content entry.")
CodedException("Could not create content entry")

Comment on lines 82 to 83
CreateAsset(context, localUri, promise, false)
.execute()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CreateAsset(context, localUri, promise, false)
.execute()
throwUnlessPermissionsGranted {
CreateAsset(context, localUri, promise, false)
.execute()
}

@alanjhughes
Copy link
Collaborator Author

Thanks @lukmccall I'll wait until you finish your review before making any changes


videosObserver =
MediaStoreContentObserver(handler, MediaStore.Files.FileColumns.MEDIA_TYPE_VIDEO)
AsyncFunction("startObserving") { _: Promise ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
AsyncFunction("startObserving") { _: Promise ->
OnStartObserving {

You can use the dedicated component to implement startObserving.

contentResolver.unregisterContentObserver(it)
videosObserver = null

AsyncFunction("stopObserving") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
AsyncFunction("stopObserving") {
OnStopObserving {

The same applies to stopObserving

state = PromiseState.REJECTED
rejectCode = code
rejectThrowable = e
rejectThrowable = exception
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You're sure that those functions aren't used by anything else and can be safely deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, you are right. This was a mistake. I'll revert. Probably need a mock for the new Promise type

Copy link
Collaborator Author

@alanjhughes alanjhughes Nov 29, 2022

Choose a reason for hiding this comment

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

Actually, there is one, just need to change the imports

val albumsMap = assets
// All files should have mime type, but if not, we can safely assume that
// those without mime type shouldn't be move
.groupBy { it.parentFile }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.groupBy { it.parentFile }
.filter { it.mimeType != null }
.groupBy { it.parentFile }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can mimeType ever be null? It's typed as String

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. We don't have to check that. But in that case, we can remove the comment above those lines.

@@ -24,11 +19,11 @@ internal class AddAssetsToAlbum(
private val albumId: String,
copyToAlbum: Boolean,
private val promise: Promise
) : AsyncTask<Void?, Void?, Void?>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

One very important thing here. You can't just remove async tasks and not replace them with some other forms of async solutions like coroutines. Your current solution will block the native module queue because all the disk operations will be dispatched to the same place where all calls from js are processed. This can lead to a lot of lags in your application, especially on low-end devices.

Could you replace the AsyncTask with coroutines? To do it, you have to create a coroutine scope in the module and then dispatch all the work there. Here you can find a small example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem, I was wondering about this too. I wasn't sure whether to use the Coroutine builder on the AsyncFunction or create a scope myself.

@alanjhughes
Copy link
Collaborator Author

@lukmccall would adding the promise assertion functions from org.unimodules.test.core to the TestUtils in expo.modules.test.core be ok?

@lukmccall
Copy link
Contributor

@lukmccall would adding the promise assertion functions from org.unimodules.test.core to the TestUtils in expo.modules.test.core be ok?

Yes, we want to migrate functionalities from the legacy package anyway ;)

Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

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

Great job 🔥
Thanks for applying all my comments ;)

val albumsMap = assets
// All files should have mime type, but if not, we can safely assume that
// those without mime type shouldn't be move
.groupBy { it.parentFile }
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. We don't have to check that. But in that case, we can remove the comment above those lines.

@lukmccall lukmccall merged commit 4cc76f9 into expo:main Dec 1, 2022
@alanjhughes alanjhughes deleted the @alanhughes/android/modules-expo-media-library branch December 1, 2022 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants