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

Convert several Java files to Kotlin #2493

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Convert several Java files to Kotlin #2493

merged 2 commits into from
Jan 12, 2021

Conversation

FloEdelmann
Copy link
Member

@FloEdelmann FloEdelmann commented Jan 11, 2021

I hope this kind of PR is fine for you. I followed these steps for each of the commits:

  1. used Android Studio's "Convert Java File to Kotlin File" feature
  2. manually formatted the code
    • mostly restored empty lines and comment formatting
    • sometimes changed it to more idiomatic Kotlin
  3. fixed compile errors in other files, if any

Please tell me if I missed any obvious "idiomatic Kotlin" opportunities, as I'm new to Kotlin.

@westnordost
Copy link
Member

Yes, some of it is ok to change to Kotlin. Prefs is, ApplicationConstants is.

However, KryoSerializer is too critical that I want to touch it at all. For example, in the Java version, there was the java.util.HashMap at the first place in the registeredClasses array. In the Kotlin version, it is not clear if it is still java.util.HashMap or kotlin.collections.HashMap. If that makes any difference. In any case, I'd not like to take the risk that something could break there. Maybe one day I'll convert it, but at my own direction then.

The reason why CountryInfos.java and Abbreviations has not been converted yet is because I am playing with the thought or at least keep open the option to put each into a small separate library. For this reason, it should remain as .java.

@westnordost
Copy link
Member

So if you revert the changes not related to these two files, I can merge it.

@westnordost
Copy link
Member

westnordost commented Jan 12, 2021

If you feel like helping out by doing some small refactorings, there is something else that could be done - decoupling the data-side of quests (more) from UI and Android:

Task

For the cycleway quest, the cycleway answer options are cleanly seperated into two files: Cycleway.kt contains the "pure data" (the enum) and CyclewayItem contains the view-related part of a cycleway answer option.

This is better than how it is done for for example the surface quest (and others). There, in Surface.kt, the enum is not pure data because is contains reference to view stuff and also Android-specific stuff. So for these quest(s), the data class should be refactored to be like for the cycleway class.

Example

enum class Surface(val value: String) { // or osmValue?
  ASPHALT("asphalt"),
  ...
}

// in SurfaceItem.kt
fun Surface.asItem(): Item<Surface> = ...

Reason

Apart from it being good style, it is a step towards making #1892 a little less work to do

@westnordost
Copy link
Member

(If you are up for it, I can create a ticket for that so it can be properly referred to)

@FloEdelmann
Copy link
Member Author

I'm not sure how much time that will take for a Kotlin novice like me, but I will try :)

@westnordost westnordost merged commit 69e140d into streetcomplete:master Jan 12, 2021
@FloEdelmann FloEdelmann deleted the kotlin branch January 12, 2021 00:18
@westnordost
Copy link
Member

👍 thanks, merged!

@FloEdelmann
Copy link
Member Author

Are any of the remaining Java files eligible to be converted to Kotlin?

Critical → should not be converted

Could be extracted into a library → should not be converted

Dagger 2 can't find these as Kotlin files → should not be converted

Remaining → ❓

@westnordost
Copy link
Member

Serializer is closely related to KryoSerializer... and it is just 2 lines of code
For FlattenIterable and MultiIterable.... are they actually used? Maybe they can be converted. Kotlin is a little stricter regarding usage of generics (of generics).
LocationUtil... there was some other open PR I think, that touched that file, which is why I thought I'd rather postpone converting that.

@FloEdelmann
Copy link
Member Author

FloEdelmann commented Jan 15, 2021

  • Serializer.java is closely related to KryoSerializer.java → fair enough
  • FlattenIterable.java is not used at all (but there is a test for it) → should it be removed?
  • MultiIterable.java is used in three places, so I'll try to convert it to Kotlin and pay attention to generics
  • LocationUtil.java is indeed touched by Make changes for when the minSdk is set to 21. #2031 and Use more Compat classes. #2082.
    But so was MainActivity.java, which you already converted to Kotlin 😅
    @Isira-Seneviratne Would it be okay for you to resolve another merge conflict in those PRs when LocationUtil.java is converted to Kotlin?

@westnordost
Copy link
Member

MultiIterable.java is used in three places, so I'll try to convert it to Kotlin and pay attention to generics

Maybe there is a more kotlin-y way to do that. Something like
for(item in (collection1.asSequence() + ollection2.asSequence() + ...))
maybe?

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 this pull request may close these issues.

2 participants