JetBrains
Make ResourceReader a public API
Describe proposed changes and the issue being fixed
(Optional) Fixes [link to the issue]
Testing
(Optional) Describe how you tested your changes (provide a snippet or/and steps)
- [x] Add Test to desktopTest SourceSet (Optional) This should be tested by QA
Release Notes
Features - Resources
- Added
JvmResourceReaderAPI and madeLocalResourceReaderpublic to allow providing a custom classloader for desktop target
When you open the ResourceReader API to public it means you finalize a format of the internal strings serialization format:
suspend fun readPart(path: String, offset: Long, size: Long): ByteArray
that is why I haven't been ready to do that. The format was invented on the go and I haven't think to stabilize it yet.
One of the problem with these offset and size: CMP-5039 Overriding string resources causes crashes or inelligible strings
When you open the ResourceReader API to public it means you finalize a format of the internal strings serialization format:
suspend fun readPart(path: String, offset: Long, size: Long): ByteArraythat is why I haven't been ready to do that. The format was invented on the go and I haven't think to stabilize it yet.
One of the problem with these offset and size: CMP-5039 Overriding string resources causes crashes or inelligible strings
We could annotate ResourceReader as internal. Alternatively we could provide a Configuration in the local composition for resource loaders. I would be far more incline on exposing unstable APIs with an optin-in marker rather creating a contraption. I believe that documenting properly in code can be sufficient to let users understand.
@lamba92 you marked all the "shouldn't this be private" comments as resolved without addressing them nor replying. I cannot unresolve them myself, but they are not resolved at all.
@lamba92 you marked all the "shouldn't this be private" comments as resolved without addressing them nor replying. I cannot unresolve them myself, but they are not resolved at all.
Sorry, I forgot to send the comment. The whole point of this MR is to expose ResourceReader interface and its implementations to users so that it could be replaced. JvmResourceReader should be public because a user should be able to:
@Composable
fun WithCustomResourceReader(reader: ResourceLoader, content: @Composable () -> Unit) {
CompositionLocalProvider(LocalResourceReader provides loader, content)
}
...
val reader = JvmResourceReader(this::class.java.classloader)
setContent {
WithCustomResourceReader(reader) {
...
}
}
Given that the JVM implementation is public, I do not see why the default ones for the other platforms should not be as well.
I can see why you'd want the JVM one to be public, but making others public is a bad idea for two reasons:
- You're adding public APIs you do not need and won't be able to easily change in the future (in a library you should keep a tight control of what API you publish, not just dump everything into the public API)
- Most (all?) other implementations are
objects that can't be either extended nor instantiated, and it is thus pointless to expose them to begin with