-
Notifications
You must be signed in to change notification settings - Fork 42
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
Migrate code to Kotlin #150
Comments
Welcome Franco & sorry for the late reply. Kotlin has some advantages over Java, and Google are committed to Kotlin, so this seems to be the way to go. I expect issues though, as the App code in some places still isn't as clean as it should be, especially regarding concurrency. On my todo list is a rewrite of the notifications storage. The push notifications API used also is deprecated. Also, our use of a background task for the TCP channel seems to be something, Google really discourages / disapproves now, may become necessary to rework when moving to Kotlin. There are probably more places that will need more than a simple code rework. You're of course welcome to work on this. We should introduce a dedicated migration branch for this, so we can maintain the Java version as long as necessary. I've created the branch Regards, |
Hi @dexterbg, sorry I just saw this now. The notification of your response got lost in my inbox :/ I am a bit busy now but I'll try to work on this towards the end of this year if I have some time :) |
Hey @dexterbg, I started working on this today. I was wondering if you could give me a short explanation of why the code in the Luttu library is kept separate from the main project? Is it used also in another project? |
I don't know where that library came from, AFAICT it's only used here. It's possibly an artifact from the App's history. The App source code once got lost. Mark hat to use a Java decompiler to recover some parts: http://lists.openvehicles.com/pipermail/ovmsdev/2012-July/007797.html Feel free to refactor the library functions into the main part. |
Hi @dexterbg, I'm working on this and I have a follow-up question: There are some model classes which don't really follow good Java naming conventions, e.g., the properties in ChargePoint start with upper case and the properties in CarData use snake case. Is this because of some serialization necessity or just something that was overlooked when the classes were created? If it's the latter I can rename the properties, otherwise I'll leave them as they are for now. |
ChargePoint is deserialized from JSON, so IIRC needs to follow the JSON naming scheme. CarData naming was legacy, but should be kept now, as we've exposed the same naming scheme to the scripting API. For classes without serialization / API field uses, you're free to rename & fix spelling. |
Franco, I've uploaded the officially signed build into the branch's latest directory. I've also posted an info to the OVMS developers list:
Regards, |
Thanks for the update and the kind words @dexterbg :) I was thinking about this approach and, if you are ok with it, I think it might be best to merge the Let me know if you agree or if you prefer to continue with the plan as is 👍 |
I agree, and have just merged |
I'm happy to do this migration and create a PR, I wanted to check if it'd be useful before starting though. Let me know what you think @dexterbg.
The text was updated successfully, but these errors were encountered: