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

[BUG] - setOnSetImageUriCompleteListener can't use lambda form anymore for the interface OnSetImageUriCompleteListener #102

Closed
AndroidDeveloperLB opened this issue Mar 30, 2021 · 7 comments · Fixed by #104

Comments

@AndroidDeveloperLB
Copy link
Contributor

AndroidDeveloperLB commented Mar 30, 2021

  • Lib Version [e.g. 1.1.0]
    2.3.0

Describe the bug
When it was in Java, I could use :

cropImageView.setOnSetImageUriCompleteListener { _, _, error ->
...

Now I have to use :

cropImageView.setOnSetImageUriCompleteListener(object : CropImageView.OnSetImageUriCompleteListener {
    override fun onSetImageUriComplete(view: CropImageView, uri: Uri, error: Exception?) {

To Reproduce

  1. Just try the code I wrote above, of before this version and after.

Expected behavior
Should still allow to do it.

Screenshots
No need

Smartphone (please complete the following information):
None.

Additional context
I think whoever converted to Kotlin didn't look enough at the resulting code...
To fix this, I think it's possible to use "fun interface" instead of just "interface". This should be added for each interface in the project that has a single function.

@Canato
Copy link
Member

Canato commented Mar 30, 2021

Thanks for pointing this, for sure is an improvement we can do it.

We didn't when we convert from Java because this would change the method notation, right know the difference is from using lambda to usage of creating the Listener class.

If we decide to use the Kotlin we don't need to define an interface anymore, I mean the code interface OnSetImageUriCompleteListener doing this we need to be sure that Java and Kotlin keep working.

Was not that we didn't look enough, the library was not update since 2018 there was a lot of bugs and fixes to put in place we cannot do all at once. So we decide to not break.

Please feel free to join us and drop a PR. This would be an amazing momento to create the Java Sample code, so we can test this changes. Probably the version 3.0

@AndroidDeveloperLB
Copy link
Contributor Author

@Canato I didn't mean I wrote in Java. this is in Kotlin.
When the library is in Java, it allows Kotlin (and Java) developers to use the shorter form.
All you need to do is to add "fun" for this interface (and others) in Kotlin, when possible.

@Canato
Copy link
Member

Canato commented Mar 30, 2021

Sorry, maybe I was confusing on my worlds. I meant we need to keep the library working for Kotlin and Java after the changes. So we need to test, something simple.

Same as the other ticket, for sure is something we should do and something I want to do, but this is low on my priority now.
I try to order: Samples (so we can test), refactor, fixing bugs, dealing with deprecation, adding very asked features, improve in general (lamba/comments/doc/etc)

But feel free to add it ^^

@AndroidDeveloperLB
Copy link
Contributor Author

@Canato As I'm not so familiar with how the library works, I prefer only to give advice and feedback as a user of it.
In this case, it's quite easy. Add "fun" for each interface in Kotlin that you can.

@Canato
Copy link
Member

Canato commented Mar 30, 2021

Don't worry, I don't wanna be the only merging in the lib, I just handover the repo, but I hope this can be a community work. If you make the changes and test the samples in the sample app everything should be safe.

If something is missing and I know I can tell on the PR, but myself just have 4 months touching the lib. So much to learn too

@AndroidDeveloperLB
Copy link
Contributor Author

AndroidDeveloperLB commented Mar 31, 2021

OK here:
main...AndroidDeveloperLB:main
#104

@AndroidDeveloperLB
Copy link
Contributor Author

Seems now it's possible again to use the short form again. Weird that the IDE doesn't suggest it though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants