search

openvehicles / Open-Vehicle-Android

Android App for OVMS
Other
54 stars 45 forks source link

Migrate code to Kotlin #150

Closed francos closed 5 months ago

francos commented 1 year ago

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.

dexterbg commented 1 year ago

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 migrate-kotlin for this purpose, please post your PR against this branch.

Regards, Michael

francos commented 1 year ago

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 :)

francos commented 10 months ago

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?

dexterbg commented 10 months ago

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.

francos commented 9 months ago

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.

dexterbg commented 9 months ago

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.

dexterbg commented 8 months ago

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:

Everyone,

for those not following the Android repository updates: thanks to the great help & efforts of Franco Sabadini (https://github.com/francos, not sure if he's also subscribed here), the Android App is now heading towards a full migration from Java to Kotlin.

This is currently done in the dedicated "migrate-kotlin" branch, but "master" is now frozen until this is finished & merged, as Java changes cannot be merged into Kotlin automatically. So, for any new submissions, please base your code on the "migrate-kotlin" branch until further notice.

To test the current Kotlin state, please install the latest build from the "migrate-kotlin" branch:

For those not familiar with Kotlin: it's basically a clean Java concept reimplementation, very similar but more readable & versatile. It's compiled into Java VM byte code using the same ABIs, so coexists & cooperates with Java classes in the same application. That's also the current state of the implementation, Franco has reworked the core classes, the UI classes are still Java.

Please leave feedback on any issues you might find here:

Regards, Michael

francos commented 8 months ago

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 migrate-kotlin into master first and then for me to continue working from there as to make it easier for other contributors that might not be subscribed to the developers list, as it might take me a bit to finish the migration given the UI has lots of files there.

Let me know if you agree or if you prefer to continue with the plan as is 👍

dexterbg commented 8 months ago

I agree, and have just merged migrate-kotlin into master to resolve this.