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

Migrate code to Kotlin #150

Open
francos opened this issue Mar 16, 2023 · 9 comments
Open

Migrate code to Kotlin #150

francos opened this issue Mar 16, 2023 · 9 comments

Comments

@francos
Copy link
Contributor

francos commented Mar 16, 2023

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
Copy link
Member

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
Copy link
Contributor Author

francos commented Oct 25, 2023

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
Copy link
Contributor Author

francos commented Dec 21, 2023

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
Copy link
Member

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
Copy link
Contributor Author

francos commented Feb 5, 2024

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
Copy link
Member

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
Copy link
Member

dexterbg commented Mar 2, 2024

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
Copy link
Contributor Author

francos commented Mar 4, 2024

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
Copy link
Member

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

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

No branches or pull requests

2 participants