Port to Kotlin Multiplatform

[Ercalvez]

This port to KMP is required as osmfeatures is a dependency for Streetcomplete which will be ported to KMP or Jetpack Compose.

Questions:

• Should we convert all the codebase to Kotlin or just create a folder library-kmp on par with library-android ?

[westnordost]

Port to KMP should be what's in the library folder. library-android is android-specific, i.e. would have the kotlinized library as a dependency only

Currently, this library uses org.json:json for JSON parsing, which must be replaced with a multiplatform json parser like org.jetbrains.kotlinx:kotlinx-serialization-json. Additionally, as Locale only exists in Java, the interface would likely have to be changed to use language and country codes instead.

Also, note that whoever takes this on would probably end up being at least a (co-)maintainer of this repository (if it remains in this repository), as whoever does the port basically made the code his own by going through it from beginning to end, getting to know it as well as myself or better. (If anybody wants to work on this, please indicate this before, so that noone else tries to work on it at the same time!)

[Ercalvez]

Regarding Locale, a possible solution might be to use the expect keywords for a Locale class used in library. Then we need to use the actual keyword to define the platform specific implementation (i.e java.util.Locale for library-android and NSLocale in a new library-ios folder). Let me know if it sounds good or not

[westnordost]

Looking through the code, it looks like Locale in the library is used simply as a container for language code plus optional country code plus optional script code. So, it could be an interface whose implementation in jvm/ios each use the Locale implementations of the corresponding platforms, but it sounds much easier to me to just have a simple data class for that. (Or use a string, i.e. the IETF BCP 47 language tag, e.g. "en-US")

[westnordost]

Since the library also loads files from a file system, a multiplatform way to load files is necessary.

Okio seems to be the biggest and most well maintained library for that as the kotlin.io API in the standard library is not multiplatform. There's also kotlinx-io-core but this is actually based on Okio but documentation is lacking. Right now, I am not sure what exactly it features.

Though IIRC the file access stuff is somewhat seperated from the core logic already.

As I wrote, for JSON parsing, the most standard way would be to use kotlinx-serialization-json, so that should be used unless there is a good reason not to use it.

[susrisha]

Me and @iamrajeshk would love to attempt this. We will keep you posted on the progress.

[Ercalvez]

Me and @iamrajeshk would love to attempt this. We will keep you posted on the progress.

I've forked the project on my repo https://github.com/Ercalvez/osmfeatures. I'm committing changes today. I can grant you developer or maintainer roles. I've ported the whole code base to Kotlin and set it up as a kmp project but lots of unit tests are still failing.

[susrisha]

@Ercalvez I cloned your repository. Do let us know when you are done committing changes. we will start with unit tests when you are ok.

[westnordost]

Well, it's good that @susrisha prevented a collision of several people tackling this at once and thus avoiding the work to be done twice by announcing their intention first 👍

Now, 4+ eyes see more than 2 eyes, so if several people are interested in porting it, it can be a boon, too, as you can cross-review your changes to the code - thus identifying probable bugs (don't trust that the tests cover every possible bug, maybe it makes sense to also add new tests), sections whose readability could be improved, etc.

Since you, @Ercalvez , wrote that you advanced quite far, I suggest that you just create a draft PR as soon as you'd like some feedback or help from any of the rest (@iamrajeshk, @susrisha or me).

[Ercalvez]

@Ercalvez I cloned your repository. Do let us know when you are done committing changes. we will start with unit tests when you are ok.

I pushed commits few minutes ago

[westnordost]

I quickly browsed the source code, here is a 10min pre-pre-review :-)

• typo in FeatureDictionnary.kt filename
• if JsonUtils.kt is necessary, I am kind of disappointed in kotlinx.serialization. Pretty sure it is not.
• check for import java.* and any import .*jvm.*
• for (most of) the stuff in CollectionUtils, there should instead be convenience functions in Kotlin (putOrGet etc)
• can Feature.geometry really be null?
• Locale should be a simple immutable data class: data class Locale(val language: String, val country: String? = null, val script: String? = null, ...)
• I attach great importance on having as little dependencies at possible. In particular, I think normalize, fluid.locale are not necessary.
• doesn't //endregion always have to be on a new line?

[westnordost]

Nevermind normalize, can't think of another way to do it.

[iamrajeshk]

@Ercalvez Thanks for the commit. I'll start with fixing the existing unit test cases. @westnordost Will look into the review findings after that

[Ercalvez]

@Ercalvez Thanks for the commit. I'll start with fixing the existing unit test cases.

Yes, I started trying to debug this. The classloader in Java has access to the resource path for test. If you try to debug the getResourceAsStream method, you will see the module is unnamed. I think it's due to the JAR build files not available to commonTest

[westnordost]

You mean these resources are not available in commonTest (because bundling whatever is in resources into the JAR is a feature of Java specifically)?

Couldn't the tests access these resources through normal IO (so in that case, through okio) on the file system? Regardless of whether okio is used as a dependency for the library, it could be used as a dependency of the tests. Another way I found is this gradle plugin and this article, though in general I find it more straightforward to just use okio, if possible.

[Ercalvez]

You mean these resources are not available in commonTest (because bundling whatever is in resources into the JAR is a feature of Java specifically)?

Couldn't the tests access these resources through normal IO (so in that case, through okio) on the file system? Regardless of whether okio is used as a dependency for the library, it could be used as a dependency of the tests. Another way I found is this gradle plugin and this article, though in general I find it more straightforward to just use okio, if possible.

I came up with this solution

private fun getSource(file: String): Source {
        val resourcePath = "src/commonTest/resources/${file}".toPath()
        return FileSystem.SYSTEM.source(resourcePath)
    }

[Ercalvez]

@susrisha @iamrajeshk I pushed new commits. Now the issues with resource path are solved

[westnordost]

@Ercalvez , I'd say, if you would like to continue working on it, go ahead. There hasn't been any visible activity from them yet since their announcement over a month ago and the prospect of them maybe helping out with your PR shouldn't block you from being able to completing it.

[Ercalvez]

I have this unit test failing:

assertNull(dictionary.byId("shop/bakery/Ditsch").inCountry("AT-9").get())

with disch:

private val ditsch: Feature = feature( // brand in DE for shop=bakery
        "shop/bakery/Ditsch",
        mapOf("shop" to "bakery", "name" to "Ditsch"),
        POINT,
        listOf("Ditsch"),
        listOf(),
        listOf("DE", "AT"),
        listOf("AT-9"),
        true,
        1.0f,
        mapOf("wikipedia" to "de:Brezelb%C3%A4ckerei_Ditsch", "brand" to "Ditsch"),
        true,
        null
    )

But a feature is found in this method of TestPerCountryFeatureCollection:

override operator fun get(id: String, countryCodes: List<String?>): Feature? {
        return features.find { feature ->
            feature.id == id
                    && countryCodes.any { countryCode ->
                        feature.includeCountryCodes.contains(countryCode) || countryCode == null && feature.includeCountryCodes.isEmpty()
                    }
        }

    }

"AT-9" is included in the excluded country codes but it is never checked in the get method. Has this unit test ever passed?

[westnordost]

The test case is definitely correct and it looks like the Java version of that method also did not take into account the excluded country codes:

    public Feature get(String id, List<String> countryCodes) {
        for (Feature feature : features) {
            if (!feature.getId().equals(id)) continue;
            for (String countryCode : countryCodes) {
                List<String> includeCountryCodes = feature.getIncludeCountryCodes();
                if (includeCountryCodes.contains(countryCode) || countryCode == null && includeCountryCodes.isEmpty()) {
                    return feature;
                }
            }
        }
        return null;
    }

So, based on that, it looks like the test could never have passed.

Right now, I am unable to run the project myself, some gradle configuration problem, will have to look into it.

[westnordost]

Ok, was able to execute the test and indeed, that one test case fails also on master.

[westnordost]

(So that means, don't worry about that failing test)

[Ercalvez]

I modified the get operator to take excluded country codes into account now. All tests passed. Should I make a PR to a new branch or to master? This PR might introduce breaking changes, I guess it should be merged to a new branch for now?

[westnordost]

A PR to master would be great! However, did you already address the things I found in my pre-review here: https://github.com/westnordost/osmfeatures/issues/22#issuecomment-1894588926 ?

[Ercalvez]

A PR to master would be great! However, did you already address the things I found in my pre-review here: #22 (comment) ?

Yes! not everything tho.

• typo in FeatureDictionnary.kt filename ✅
• if JsonUtils.kt is necessary, I am kind of disappointed in kotlinx.serialization. Pretty sure it is not. -> I kept it
• check for import java. and any import .jvm.* ✅
• for (most of) the stuff in CollectionUtils, there should instead be convenience functions in Kotlin (putOrGet etc) ✅ except synchronizedGetOrCreate
• can Feature.geometry really be null? ✅
• Locale should be a simple immutable data class: data class Locale(val language: String, val country: String? = null, val script: String? = null, ...) ✅
• I attach great importance on having as little dependencies at possible. In particular, I think normalize, fluid.locale are not necessary. -> removed fluid.locale but kept normalize, is there a way to normalize strings without using this package?
• doesn't //endregion always have to be on a new line? -> No idea

[westnordost]

Nevermind normalize.

doesn't //endregion always have to be on a new line? -> No idea

Pretty sure it is. Please put any //endregion in an own line, I made that comment because I found one that wasn't

if JsonUtils.kt is necessary, I am kind of disappointed in kotlinx.serialization. Pretty sure it is not. -> I kept it

Hm ok, I will have another look at it during review, then