search

g0dkar / qrcode-kotlin

QRCode Generator implemented in pure Kotlin
https://qrcodekotlin.com/
MIT License
172 stars 16 forks source link

Replace BiFunction with lambda #9

Closed Doomsdayrs closed 2 years ago

Doomsdayrs commented 2 years ago

Please never use a java specific function like that.

g0dkar commented 2 years ago

Heya! Thanks a lot for the contribution! Only now I've found it on the repo, sorry for the late answer 🤦

Anyway, I know I shouldn't be using a BiFunction like that hahaha 😅

I did it like that to keep the interoperability of the lib with Java without any extra libraries at all (not even kotlin-stdlib).

As far as I've used the library myself, Kotlin seemed to have abstracted that (the BiFunction instead of an actual lambda) away from me.

Even though internally it looks not-the-best-nor-the-most-professional-ever (aka "ugly af") that will prevent Java users of the library of needing to import kotlin-stdlib.

That was the only reason I've done that ^^

I know this might be frustrating, but does this explain why it was done as it was?

I can alter the documentation (and comment the code as well) to explain this.

I'd love to know which use case or issue you ran into so that I could see if we can work out a solution :D

Once again, thanks a lot for the contribution! Your point is a good one!

Doomsdayrs commented 2 years ago

You're going about this in a severely wrong way IMO.

Kotlin is a multi platform language. There is an MPP module so you can target different platforms.

The reason I made this PR is that BiFunction is not compatible with android. This PR addresses BiFunction specifically.

A PR that would fully fix the core issue is to make this a MP library that can target different platforms.

Doomsdayrs commented 2 years ago

I was honestly horrified that you were checking for a class existing.

g0dkar commented 2 years ago

Thanks for the follow up =D

I'll try to address the points you brought up, let me know if I left something out 😀

  1. About the BiFunction not existing on Android: Are you sure? It seems to exist: https://developer.android.com/reference/java/util/function/BiFunction - I might be wrong, it's been some years since I last developed Android stuff 😅 - Could you share more on which error were you having? Maybe I'm not understanding this properly 😞
  2. About Android itself: This library is not intended to be an Android library, but a Java/Kotlin ecosystem one, which is currently as a matter of fact also compatible with Android. It's end goal is to be an alternative, smaller and simpler to use library for server-ish applications than Google ZXing, which seems to be pretty much the only one available. I'm trying to keep it compatible as much as I can - Java, Android and Kotlin itself - while also keeping the dependency footprint and lib size to a minimum
  3. About the class checking: I'm open to any reliable platform/feature detection methods 😅 (see the next point) - That is the only one I know, and I've successfully used before so that's why I went with it. The idea is simply to provide a platform-dependent implementation of QRCodeCanvas. At v1 I had an issue where since the library would always use a BufferedImage, which as far as I know doesn't exist in Android, and that would throw ClassNotFound errors in runtime. Because of that, I rewrote the whole image rendering part of the library to support different platform-dependent implementations of renderers :)
  4. About Kotlin Multiplatform: That looks like it'll address the previous point! I'll look into this and might implement this soon! I wasn't aware of this and your comment helped me find that - thanks!

I hope to have addressed all your points! Let me know if I forgot something or if there is anything else!

Doomsdayrs commented 2 years ago

About the BiFunction not existing on Android: Are you sure? It seems to exist: https://developer.android.com/reference/java/util/function/BiFunction - I might be wrong, it's been some years since I last developed Android stuff sweat_smile - Could you share more on which error were you having? Maybe I'm not understanding this properly disappointed

While it does exist, Cast from BiFunction to String requires API level 24 (current min is 22). It is not available for all versions for android.

About Kotlin Multiplatform: That looks like it'll address the previous point! I'll look into this and might implement this soon! I wasn't aware of this and your comment helped me find that - thanks!

If you need help, I am a Kotlin MP programmer, and have been worked on MP projects. I can happily convert the entire library on my free time.

Doomsdayrs commented 2 years ago

I apologize if I was a bit harsh before, Your work with this library is great.

g0dkar commented 2 years ago

Cast from BiFunction to String requires API level 24 (current min is 22)

Android is weird hahaha... I'll change those to actual lambdas soon ^^

I can happily convert the entire library on my free time.

Don't worry! I'm really interested in the whole concept of the Kotlin Multiplatform Library thing and I'll most certainly implement it! I'd love to try it out!

I'll definitely get start implementing this over the weekend!

I apologize if I was a bit harsh before

It happens, thanks for following up on the discussion :)

Your work with this library is great.

Thanks a lot! This really means a lot to me :D - Even though this might look simple and all this is a passion project to me, and having even the smallest bits of recognition makes me very happy :P

g0dkar commented 2 years ago

Heya! Took me a while, but mind checking the new version of the library?

I'd love to have your input on how it is going!

Also, feel free to suggest any changes and improvements - I'm sure there's a lot to improve still hahahaha ^^

Finally, do you mind if I close the PR given that it might have been fixed by the whole v3.0.0 shtick?

g0dkar commented 2 years ago

Closing due to inactivity.