natsuk4ze / gal

How to Save Image or Video to Photo Gallery in Flutter
https://pub.dev/packages/gal
BSD 3-Clause "New" or "Revised" License
85 stars 13 forks source link

Refactor the Android Plugin #170

Closed EchoEllet closed 5 months ago

EchoEllet commented 8 months ago

Overview

This pull request refactors the Android example and Plugin to use the latest technologies and Android best practices

Even though In my opinion it's better to remake the whole thing I don't wish to change too much to not introduce breaking changes

Consider using Kotlin coroutines instead of Java Thread, I didn't touch that because it's outside of the scope of this PR

Feel free to reject or merge the pull request, the decision will be based on your perspective but usually, you should remake or refactor the Android plugin even if this got rejected

EchoEllet commented 8 months ago

Thank you for your contribution @freshtechtips 🤝

The PR requires attention as the tests are failing.

However, I am opposed to migrating from Java to Kotlin. My past attempt at this migration did not lead to more concise code; instead, it resulted in increased verbosity, a trend also noticeable in this PR (you can see it with Codacy Static Code Analysis in test).

Moreover, the Java+Swift plugin configuration is a standard adopted by Flutter's official packages. While Kotlin's significant advantage is its lack of concern for version compatibility, this benefit seems minor for a small plugin.

you should remake or refactor the Android plugin even if this got rejected

How exactly do you plan to remake it?

Other small modifications in PRs are welcome.

Flutter is a little bit oudated when it comes to integration with Android

For example, it's still using Gradle Groovy, and native Android projects use Gradle kts

Also, I think you already know that kotlin is a primary language for Android even though it 100% compatible with Java

EchoEllet commented 8 months ago

Thank you for your contribution @freshtechtips 🤝

The PR requires attention as the tests are failing.

However, I am opposed to migrating from Java to Kotlin. My past attempt at this migration did not lead to more concise code; instead, it resulted in increased verbosity, a trend also noticeable in this PR (you can see it with Codacy Static Code Analysis in test).

Moreover, the Java+Swift plugin configuration is a standard adopted by Flutter's official packages. While Kotlin's significant advantage is its lack of concern for version compatibility, this benefit seems minor for a small plugin.

you should remake or refactor the Android plugin even if this got rejected

How exactly do you plan to remake it?

Other small modifications in PRs are welcome.

It's true but most official plugins don't support Android gradle plugin 8.0,

Take a look at this report

EchoEllet commented 8 months ago

Thank you for your contribution @freshtechtips 🤝

The PR requires attention as the tests are failing.

However, I am opposed to migrating from Java to Kotlin. My past attempt at this migration did not lead to more concise code; instead, it resulted in increased verbosity, a trend also noticeable in this PR (you can see it with Codacy Static Code Analysis in test).

Moreover, the Java+Swift plugin configuration is a standard adopted by Flutter's official packages. While Kotlin's significant advantage is its lack of concern for version compatibility, this benefit seems minor for a small plugin.

you should remake or refactor the Android plugin even if this got rejected

How exactly do you plan to remake it?

Other small modifications in PRs are welcome.

If you would like about this subject more then I suggest talking about it in private if you want to.

EchoEllet commented 8 months ago

Regarding the tests

image

It's passing in my local machine but I can check them and fix them

I need to test them in different Android emulator APIs

natsuk4ze commented 8 months ago

It's true but most official plugins don't support Android gradle plugin 8.0,

Take a look at this report

I think that is incorrect. It appears that AGP8 support is complete here.

The code for the namespace is already in build.gradle.

natsuk4ze commented 8 months ago

Regarding the tests

image

It's passing in my local machine but I can check them and fix them

I need to test them in different Android emulator APIs

Yes. Since different versions of Android have different permissions and different media storage locations, we must pass the test for all versions supported by this plugin.