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 PolylinesSerializer to kotlinx-io #5307

Conversation

neonowy
Copy link
Contributor

@neonowy neonowy commented Oct 13, 2023

One small step in getting rid of Java dependencies, as noted in #1892 (comment) :)

Just a few things I'd like to get feedback on before finishing this PR:

  1. Regarding the new dependency: kotlinx-io is still in alpha, but since the new code is using only a few basic library APIs, any breaking changes are unlikely to affect it? Alternatively, Okio is a more stable option with a nearly identical API.
  2. Since no Kotlin library reproduces Java's Object Serialization Stream Protocol used by java.io.Object{Input,Output}Stream classes, this introduces a breaking change to how polylines are stored in the database. How would you prefer to handle it?
    A) Add custom logic for upgrading geometry tables by deserializing existing java.io-backed polylines and then serializing them using kotlinx-io. In the future this would have to be wrapped to be a no-op on non-JVM platforms.
    B) Recreate (DROP+CREATE) geometry tables like it's been done with osm_element_edits.

cc @westnordost

@westnordost
Copy link
Member

Cool, thank you for this! I didn't know that a multiplatform substitute for these io classes already existed... albeit in alpha.
Then again, there is no hurry to get this PR merged, so it could just sit here, approved and ready for merge for a while and at the time it makes sense to merge, maybe it is not in alpha anymore, or alternatively, the library gets abandoned and then we use okio directly instead.

Yeah, that Java adds some extra bytes here and there is something I also noticed when I ported https://github.com/westnordost/countryboundaries/ to Rust. It uses Data{Input,Output}Stream on the Java-side now which is really just the raw bytes.

So, I think option 2A) is pretty much out of question because that requires to replicate the Java object serialization stream protocol. I think it would be easier to clear local map data (element geometry + elements) on DB upgrade instead. After all, this is cleared all the time anyway (i.e. old data is refreshed or dropped latest some weeks after it was downloaded)

@neonowy
Copy link
Contributor Author

neonowy commented Oct 26, 2023

Thanks for the feedback! 👍

As suggested, I went with the second option: deleting all elements + element geometry + DownloadedTilesTable (if I understand correctly, the app would otherwise think it still has the map data we've just deleted). Used DELETE FROM as it's just as fast and less lines of code.

@neonowy neonowy marked this pull request as ready for review October 26, 2023 18:24
@westnordost westnordost mentioned this pull request Oct 31, 2023
@FloEdelmann FloEdelmann added code cleanup hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event labels Oct 31, 2023
@westnordost
Copy link
Member

(Just assigning the people who did the work to the completed ticket so that https://github.com/orgs/streetcomplete/projects/1/views/1 looks nicer :-) )

@westnordost westnordost merged commit 8748863 into streetcomplete:master Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event
Projects
Status: Released
Development

Successfully merging this pull request may close these issues.

3 participants