FirebaseExtended / mlkit-material-android

ML Kit Showcase App with Material Design
Apache License 2.0
491 stars 143 forks source link

Migrate to Kotlin. #1

Closed kenz closed 5 years ago

kenz commented 5 years ago

Announced over 50% of professional Android developers now use Kotlin. So migrated to Kotlin.

kenz commented 5 years ago

H, Yi Zhou,

Thank you your feedback for my pull request.

I changed my request. I tried not duplicate the layout files, however it result was complex code mixed Java and Kotlin. Because keep independence Java and Kotlin, I copied layout files that include custom view and modified each file for adapt Java and Kotlin.

Thank you.

thatfiredev commented 5 years ago

Hi @kenz ,

As @zhouyiself mentioned here:

2. Moves few classes (e.g. GraphicOverlay.java) that are used in the layout files to be under the top package in order to not duplicate the layout files.

The classes used in layout files should be under the top package so that we don't have to duplicate layouts, and these layouts can be used in Java and Kotlin both.

kenz commented 5 years ago

@rosariopfernandes thank you your reply.

Yes, I tried it. However, it became an unreadable code. The classes used in the Layout file and the it references class (eg CameraSource.java, Size.java and more ...) are either Java or Kotlin. The capsule was broken(eg com.google.firebase.ml.md.camera and com.google.firebase.ml.md.java.camera and com.google.firebase.ml.md.kotlin.camera).And I had to change some Protected function to Public.

I think so. Many developers need only one of Kotlin or Java. Therefore, it should be easier to separated Kotlin and Java. Duplicating the layout file(only 4 files.) is the easy solution.

thatfiredev commented 5 years ago

@kenz the solution used in firebase/quickstart-android was to have these classes written in Java in a package that could be used by both Java and Kotlin (have a look: https://github.com/firebase/quickstart-android/tree/master/mlkit/app/src/main/java/com/google/firebase/samples/apps/mlkit/common).

I'm not sure if we should keep these classes in Java or convert them to Kotlin. So before you make any changes, I'd suggest we hear @zhouyiself 's thoughts on this.

Also: although duplicating the layout files may seem like an easy solution now, it might be hard to maintain in the future

kenz commented 5 years ago

@rosariopfernandes Thank you advise. I have understand your solution how reduce duplicate layouts with common package. Sorry, I don't starting work it yet.

zhouyiself commented 5 years ago

Kenz, could we merge these commits into one final commit and described as something like "Adds kotlin version." to keep the commit history more clean without the ones originated from our discussions/reviews?

zhouyiself commented 5 years ago

Ignore my last comment.

Regarding whether forking layout files, gave it a second thought, it may be more clear to just do the fork for completeness purpose, that is both kotlin and java versions could have the whole implementation package. What do you think?

kenz commented 5 years ago

@zhouyiself Yes, I think, it can more clear to fork layout files. Each methods have both merits and demerits. Single layout file take few number of files. And only need to change single file if when want to change layout file. Duplicate layout files take to clearly separate to Kotlin and Java. Each layout files has like behavior, however each files has difference responsibility as sample. so it can keep SRP.

I think that fork layout files is readable, If decide to provide samples for two languages,