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 4 - Asset management #14565

Merged
merged 9 commits into from
Oct 11, 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. [media-library] Refactor to Kotlin 2 - Albums #14563
  3. [media-library] Refactor to Kotlin 3 - Album migration #14564
  4. This PR - rewritten asset-related stuff to Kotlin, extracted it to expo.modules.medialibrary.assets package
  5. [media-library] Refactor to Kotlin 5 - Utilities #14566
  6. Coroutines + refactor error handling (No PR yet)

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 29, 2021
@barthap barthap changed the base branch from master to @barthap/media-lib/kotlin/pt3-migration September 29, 2021 11:48
@barthap barthap force-pushed the @barthap/media-lib/kotlin/pt3-migration branch from 71e3655 to d1839c3 Compare October 5, 2021 08:21
Base automatically changed from @barthap/media-lib/kotlin/pt3-migration to master October 6, 2021 14:11
@barthap barthap force-pushed the @barthap/media-lib/kotlin/pt4-assets branch from 6b525ba to ef2aa82 Compare October 7, 2021 11:44
@barthap barthap marked this pull request as ready for review October 8, 2021 09:20
Copy link
Contributor

@mstach60161 mstach60161 left a comment

Choose a reason for hiding this comment

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

👍

fun getExifFullInfo(exifInterface: ExifInterface, response: Bundle) {
val exifMap = Bundle()
for ((type, name) in MediaLibraryConstants.exifTags) {
if (exifInterface.getAttribute(name!!) != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why name isn't null safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

result of MediaLibraryConstants not being kotlinized yet. They're constants defined here

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 🔥

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!

Comment on lines 9 to 13

/**
* If the receiver is instance of `T`, returns the receiver, otherwise returns `null`
*/
inline fun <reified T> Any?.takeIfInstanceOf(): T? = if (this is T) this else null
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can achieve this functionality with built-in as? operator. As documentation is rather laconic, I looked up some cases in playground

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, totally forgot about existence of as? operator 😅

What do you think - which one looks better?

// I have to add parenthesis here
(input["createdAfter"] as? Number)?.let {

or

input["createdAfter"].takeIfInstanceOf<Number>()?.let {

If we'd like to leave takeIfInstanceOf, then I could just replace:

Suggested change
/**
* If the receiver is instance of `T`, returns the receiver, otherwise returns `null`
*/
inline fun <reified T> Any?.takeIfInstanceOf(): T? = if (this is T) this else null
/**
* If the receiver is instance of `T`, returns the receiver, otherwise returns `null`
*/
inline fun <reified T> Any?.takeIfInstanceOf(): T? = this as? T

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 can. However, I don't like it. From my perspective is less readable.

For example instead of writing:

input["first"].takeIfInstanceOf<Number>()?.toInt() ?: 20

you will have to do:

(input["first"] as? Number)?.toInt() ?: 20

And I know, it's less character... but you have added ugly brackets.

From the perspective of underlying code is doing the same thing. Safe cast operator compiles to something like this:

Any temp = value;
if (!(temp is T)) {
    temp = null;
}
T output = (T) temp;

Copy link
Contributor Author

@barthap barthap Oct 8, 2021

Choose a reason for hiding this comment

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

Ok, I've decided the following:

  • When there is a method chaining, e.g. input["first"].takeIfInstanceOf<Number>()?.toInt()?.let { ... }, I've left the takeIfInstanceOf - the reason is that I don't like these parentheses too.
    Hypotethic situation, when this approach has a huge advantage:
    // everything goes in clean, readable order
    val x = input["a"]
                  ?.takeIfInstanceOf<Bundle>()
                  ?.get("b")
                  ?.takeIfInstanceOf<String>()
                  ?.let { ... }
    
    // maybe shorter, but I got lost with parentheses
    val y = ((input["a"] as? Bundle)?.get("b") as? String)?.let { ... }
  • When there's no such chaining, I replaced the occurrences with the as? operator - in such cases it's much cleaner:
    val sortBy = input["sortBy"] as? List<*>

I also updated the description of the takeIfInstanceOf method - added an example similar to the above

Anyway, nice catch, thanks for this suggestion 😁

@expo-bot
Copy link
Collaborator

expo-bot commented Oct 8, 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 c8732e0

@barthap barthap merged commit 3621ae2 into master Oct 11, 2021
@barthap barthap deleted the @barthap/media-lib/kotlin/pt4-assets branch October 11, 2021 10:48
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.

5 participants