search

burhanrashid52 / PhotoEditor

A Photo Editor library with simple, easy support for image editing using paints,text,filters,emoji and Sticker like stories.
MIT License
4.12k stars 989 forks source link

PE-406: Convert tests to Kotlin #407

Closed lucianocheng closed 2 years ago

lucianocheng commented 2 years ago
burhanrashid52 commented 2 years ago

Thanks. Will have a look at it soon.

lucianocheng commented 2 years ago

@burhanrashid52 ping on this?

burhanrashid52 commented 2 years ago

@lucianocheng I was thinking instead of directly merging in master, we merge it in the another branch i.e kotlin-migration, and keep merging all the Kotlin changes there. Once all the files are migrated to Kotlin, we can release a new photoeditor library with Kotlin support Ex: com.burhanrashid52.photoeditor-ktx-2.0.0 and keep the java library as it is. Let me know your thoughts ?

lucianocheng commented 2 years ago

@burhanrashid52 I'm not explicitly against it, but I'm not sure I understand the tradeoff. Kotlin/Java is interchangable and this now creates effectively a fork. Bugs will inevitably get fixed in one fork but not the other.

burhanrashid52 commented 2 years ago

@burhanrashid52 I'm not explicitly against it, but I'm not sure I understand the tradeoff. Kotlin/Java is interchangable and this now creates effectively a fork. Bugs will inevitably get fixed in one fork but not the other.

Hmmm...let me review and will get back to you.

lucianocheng commented 2 years ago

@burhanrashid52 ping.

burhanrashid52 commented 2 years ago

@burhanrashid52 ping.

Sorry, have not been able to find time to review this. Will try to close this by the weekend.

lucianocheng commented 2 years ago

@burhanrashid52 can you hand this review off to someone else? This review is now 4 weeks old.

burhanrashid52 commented 2 years ago

@burhanrashid52 can you hand this review off to someone else? This review is now 4 weeks old.

Do you have anyone in mind ? Feel free to add them as reviewers. I am would be happy if someone else can review it.

lucianocheng commented 2 years ago

I don't have reviewer-adding permissions, but @anderbytes would make sense since he's trying to convert more.

anderbytes commented 2 years ago

I don't have reviewer-adding permissions, but @anderbytes would make sense since he's trying to convert more.

I'd be happy to help , just need some guidance on what to do, because I have never done that before.

burhanrashid52 commented 2 years ago

@lucianocheng I don't know why but I am not able to add @anderbytes as a reviewer.

lucianocheng commented 2 years ago

@lucianocheng I don't know why but I am not able to add @anderbytes as a reviewer.

@burhanrashid52 You need to add one or both of us as outside collaborators and permission us appropriately.

https://docs.github.com/en/organizations/managing-peoples-access-to-your-organization-with-roles/roles-in-an-organization

lucianocheng commented 2 years ago

@burhanrashid52 alternatively just set yourself as a reviewer and allow @anderbytes to comment. Then once we both agree you can just merge it.

burhanrashid52 commented 2 years ago

@lucianocheng I don't know why but I am not able to add @anderbytes as a reviewer.

@burhanrashid52 You need to add one or both of us as outside collaborators and permission us appropriately.

https://docs.github.com/en/organizations/managing-peoples-access-to-your-organization-with-roles/roles-in-an-organization

I've added you as collaborators in this project.

anderbytes commented 2 years ago

@lucianocheng I'm not used to formal testing using JUnit or anything as such. Can you tell me the needed steps to execute this review? I want to collaborate but I lack the knowledge in this matter

lucianocheng commented 2 years ago

@anderbytes assigned you as reviewer.

@lucianocheng I'm not used to formal testing using JUnit or anything as such. Can you tell me the needed steps to execute this review? I want to collaborate but I lack the knowledge in this matter

Yep. Since I'm not adding any functionality this one should be fast. I would recommend:

  1. Checking out the repo and running all the tests locally on your machine.
  2. Reading each test and making sure you understand what the test is doing and why it is testing it.
  3. Ask any questions about any Kotlin or code you don't understand or seem unclear.
  4. Get yourself comfortable where if you were asked to modify this code in my absence, you could do it.

If any of 1-4 aren't clear for anything, leave a comment on the review and I will answer.

anderbytes commented 2 years ago

Everything seems to be running perfectly. See : image

anderbytes commented 2 years ago

@lucianocheng after the tests conversion get updated to source code, what do you think of converting the whole library/app to Kotlin?

I want to collaborate more but I don't like Java and Kotlin is much better to work with, in my opinion.

lucianocheng commented 2 years ago

37 Converted tests passing with no issues

👍

@lucianocheng after the tests conversion get updated to source code, what do you think of converting the whole library/app to Kotlin?

My plan is to do the app next, then the library.

However I would like @burhanrashid52 to do a release first with just this PR. This would flush out any issues with introducing Kotlin to the repository before proceeding.

Also I labeled this review as "Convert tests to Kotlin" but the automated conversion didn't quite work perfectly. It did actually take some time to deal with the edge cases. I suspect converting the app to kotlin will produce more complex edge cases. This will probably take time to address.

Lastly before getting to the library, there are some Android 6.0 MediaStore permission bugs I found in the app. Once the app is converted to Kotlin, I would like to address those before proceeding.

lucianocheng commented 2 years ago

@anderbytes once you're ready please formally approve the review in the github UI.

@burhanrashid52 if you see no blockers I will merge shortly after approval.

burhanrashid52 commented 2 years ago

@anderbytes I see you have approved the PR. Should I merge it ? @lucianocheng By release, you mean merging to master right ? or do you mean a library release ? If it's the second case I will probably wait to convert the full library in Kotlin and release it as a 2.0 version.

anderbytes commented 2 years ago

I guess that doing all the Kotlin conversions before releasing makes more sense.

@lucianocheng Once I tried locally the Kotlin conversion of the app+library and it wasnt that painfil, just a few tweaks, as you said.

The only "catch" I can see is that Kotlin has coroutines and probably they wont just appear from the automated conversion. So the correct async code will probably have to be coded.

burhanrashid52 commented 2 years ago

I guess that doing all the Kotlin conversions before releasing makes more sense.

@lucianocheng Once I tried locally the Kotlin conversion of the app+library and it wasnt that painfil, just a few tweaks, as you said.

The only "catch" I can see is that Kotlin has coroutines and probably they wont just appear from the automated conversion. So the correct async code will probably have to be coded.

Yeah. The kotlin friendly API is something we can plan in the future. As a first step, conversion seems enough. @lucianocheng We will track all the kotlin progress in kotlin-conversion branch and once it's ready will release the library with 2.0 and merge it to master.

burhanrashid52 commented 2 years ago

@lucianocheng Please create an issue to track other conversion in kotlin.

lucianocheng commented 2 years ago

@lucianocheng By release, you mean merging to master right ? or do you mean a library release ? If it's the second case I will probably wait to convert the full library in Kotlin and release it as a 2.0 version. @lucianocheng We will track all the kotlin progress in kotlin-conversion branch and once it's ready will release the library with 2.0 and merge it to master.

We can do this, but this may create drift between the java and kotlin versions. Specifically, I won't be pushing my Android 6.0 fixes to Java. Once the conversion is complete, you should try to release 2.0 quickly. We shouldn't wait.

@lucianocheng Once I tried locally the Kotlin conversion of the app+library and it wasnt that painfil, just a few tweaks, as you said.

@anderbytes I believe you mentioned in the original issue (https://github.com/burhanrashid52/PhotoEditor/issues/360) that there were file provider issues. I was seeing these too. I think there will be additional subtle bugs and it will take longer than it appears to complete and get it through review. This will contribute to the drift I mentioned above, which is why I would prefer to not release it as a 2.0.

@lucianocheng Please create an issue to track other conversion in kotlin.

@burhanrashid52 lets just use #360

burhanrashid52 commented 2 years ago

@lucianocheng We can merge your Android 6.0 and other bug fixes in kotlin into kotlin-conversion branch. If migration takes a long time then I will make it a release anyway.

anderbytes commented 2 years ago

My main test phone is Android 8.0 so anything below that I'll let you test.

@lucianocheng did you see my latest pull request? I believe I concluded the conversion. Hopefully solved the bugs that appeared.