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

[media-library] Refactor to Kotlin 2 - Albums #14563

Merged
merged 10 commits into from
Oct 6, 2021

Conversation

barthap
Copy link
Contributor

@barthap barthap commented Sep 29, 2021

Why, How etc...

See #14562 description for details.


This is part of a PR stack:

  1. [media-library] Refactor to Kotlin 1 - Base module #14562
  2. This PR - rewritten album-related stuff to Kotlin, extracted it to expo.modules.medialibrary.albums package
  3. [media-library] Refactor to Kotlin 3 - Album migration #14564
  4. [media-library] Refactor to Kotlin 4 - Asset management #14565
  5. [media-library] Refactor to Kotlin 5 - Utilities #14566
  6. Coroutines + refactor error handling (No PR yet)

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.

LGTM 🚀

promise.reject(ERROR_MEDIA_LIBRARY_CORRUPTED, "Media library is corrupted")
return null
}
return File(fileInAlbum.parent!!)
Copy link
Contributor

Choose a reason for hiding this comment

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

Must the parent of the fileInAlbum exist?

Copy link
Contributor Author

@barthap barthap Oct 5, 2021

Choose a reason for hiding this comment

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

When it is a file (the if above), it must have a parent directory 😉

Comment on lines +12 to +34
internal fun interface AssetFileStrategy {
@Throws(IOException::class)
fun apply(src: File, dir: File, context: Context): File

companion object {
val copyStrategy = AssetFileStrategy { src, dir, _ -> MediaLibraryUtils.safeCopyFile(src, dir) }
val moveStrategy = AssetFileStrategy strategy@{ src, dir, context ->
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R && src is MediaLibraryUtils.AssetFile) {
val assetId = src.assetId
val assetUri = ContentUris.withAppendedId(MediaStore.Images.Media.EXTERNAL_CONTENT_URI, assetId.toLong())
val newFile = MediaLibraryUtils.safeCopyFile(src, dir)
context.contentResolver.delete(assetUri, null)
return@strategy newFile
}
val newFile = MediaLibraryUtils.safeMoveFile(src, dir)
context.contentResolver.delete(
MediaLibraryConstants.EXTERNAL_CONTENT,
"${MediaStore.MediaColumns.DATA}=?", arrayOf(src.path)
)
newFile
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of putting implementation in the companion object in that case 💪

return album
}

public override fun doInBackground(vararg params: 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.

The type of this function looks stupid 😛 But we can't do anything with it right now.

Base automatically changed from @barthap/media-lib/kotlin/pt1-base to master October 4, 2021 17:08
@barthap barthap force-pushed the @barthap/media-lib/kotlin/pt2-albums branch from 0fe430b to aae9ecc Compare October 4, 2021 17:17
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Oct 4, 2021
@expo-bot
Copy link
Collaborator

expo-bot commented Oct 4, 2021

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider (it's optional) adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against d0c7106

Copy link
Contributor

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@barthap barthap merged commit 1d6514e into master Oct 6, 2021
@barthap barthap deleted the @barthap/media-lib/kotlin/pt2-albums branch October 6, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: suggestions ExpoBot has some suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants