Make ResourceReader a public API #5334

lamba92 · 2025-06-13T14:06:45Z

Describe proposed changes and the issue being fixed

Fixes CMP-8134

Testing

Add Test to desktopTest SourceSet
(Optional) This should be tested by QA

Release Notes

Features - Resources

Added JvmResourceReader API and made LocalResourceReader public to allow providing a custom classloader for desktop target

...ces/library/src/androidMain/kotlin/org/jetbrains/compose/resources/ResourceReader.android.kt

...ces/library/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceReader.desktop.kt

components/resources/library/build.gradle.kts

...ces/library/src/androidMain/kotlin/org/jetbrains/compose/resources/ResourceReader.android.kt

...rces/library/src/desktopTest/kotlin/org/jetbrains/compose/resources/CustomClassloaderTest.kt

terrakok · 2025-06-16T10:30:25Z

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

lamba92 · 2025-06-16T12:44:04Z

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

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.

components/resources/library/build.gradle.kts

...rces/library/src/desktopTest/kotlin/org/jetbrains/compose/resources/CustomClassloaderTest.kt

...s/resources/library/src/iosMain/kotlin/org/jetbrains/compose/resources/ResourceReader.ios.kt

...nts/resources/library/src/jsMain/kotlin/org/jetbrains/compose/resources/ResourceReader.js.kt

...sources/library/src/macosMain/kotlin/org/jetbrains/compose/resources/ResourceReader.macos.kt

...urces/library/src/wasmJsMain/kotlin/org/jetbrains/compose/resources/ResourceReader.wasmJs.kt

...ces/library/src/androidMain/kotlin/org/jetbrains/compose/resources/ResourceReader.android.kt

...esources/library/src/commonTest/kotlin/org/jetbrains/compose/resources/TestResourceReader.kt

...ces/library/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceReader.desktop.kt

...s/resources/library/src/iosMain/kotlin/org/jetbrains/compose/resources/ResourceReader.ios.kt

rock3r · 2025-06-17T10:36:34Z

@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 · 2025-06-17T14:46:06Z

@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.

rock3r · 2025-06-17T14:54:37Z

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

igordmn

Besides the comments, LGTM. @terrakok should perform the main review

...ces/library/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceReader.desktop.kt

igordmn · 2025-06-17T18:19:18Z

...ces/library/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceReader.desktop.kt

+) : ResourceReader {
+
+    companion object {
+        val DEFAULT = JvmResourceReader()


Consider making a top-level val DefaultJvmResourceReader to be consistent with the other targets

I also changed other platform implementations to be objects. What do you mean exactly?

igordmn · 2025-06-17T18:20:19Z

...ts/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceReader.kt

-internal expect fun getPlatformResourceReader(): ResourceReader
-
-internal val DefaultResourceReader = getPlatformResourceReader()
+expect fun getDefaultResourceReader(): ResourceReader


Consider making val DefaultResourceReader to be consistent with the platform implementations

I also changed other platform implementations to be objects. What do you mean exactly?

lamba92 · 2025-06-18T14:36:51Z

I added @ExperimentalResourceApi to the APIs I made public in this PR

components/resources/library/build.gradle.kts

...ts/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceReader.kt

...esources/library/src/commonTest/kotlin/org/jetbrains/compose/resources/TestResourceReader.kt

...ts/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceReader.kt

...ces/library/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceReader.desktop.kt

...s/resources/library/src/iosMain/kotlin/org/jetbrains/compose/resources/ResourceReader.ios.kt

...urces/library/src/wasmJsMain/kotlin/org/jetbrains/compose/resources/ResourceReader.wasmJs.kt

terrakok

I still see you deleted the lazyness of test reader on web platforms.
Please, make only the JVM resource reader class public.

...ces/library/src/androidMain/kotlin/org/jetbrains/compose/resources/ResourceReader.android.kt

terrakok · 2025-06-26T10:42:49Z

...ces/library/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceReader.desktop.kt

+@ExperimentalResourceApi
+internal actual fun getPlatformResourceReader(): ResourceReader =
+    JvmResourceReader.Default
+
+@ExperimentalResourceApi
+class JvmResourceReader(
+    private val classLoader: ClassLoader = JvmResourceReader::class.java.classLoader
+) : ResourceReader {
+
+    companion object {
+        val Default = JvmResourceReader()
+    }
+


I guess it is better

@ExperimentalResourceApi internal actual fun getPlatformResourceReader(): ResourceReader = JvmResourceReader() @ExperimentalResourceApi class JvmResourceReader( private val classLoader: ClassLoader = JvmResourceReader::class.java.classLoader ) : ResourceReader { //no companion object }

oh. I see the difference now. then make

companion object { internal val Default = JvmResourceReader() }

In Kotlin all objects are initialized on their first symbol invokation. As such, a top level object is lazy by default. The lazy has been designed for inlining lazyness in a function or a class property.

Please, make only the JVM resource reader class public.

On it

Make ResourceReader implementations public This change makes `ResourceReader` and its platform-specific implementations public. This allows users to provide custom implementations, for example, to load resources from a custom ClassLoader on JVM. A new test (`CustomClassloaderTest`) is added to verify that resources can be loaded from a custom ClassLoader. Additionally, some internal resource readers have been refactored from anonymous objects to regular objects for better organization and potential future reuse.

lamba92 · 2025-07-03T10:49:43Z

❤️

MatkovIvan requested a review from terrakok June 13, 2025 14:08

rock3r reviewed Jun 13, 2025

View reviewed changes

...ces/library/src/androidMain/kotlin/org/jetbrains/compose/resources/ResourceReader.android.kt Show resolved Hide resolved

rock3r reviewed Jun 13, 2025

View reviewed changes

...ces/library/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceReader.desktop.kt Show resolved Hide resolved

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch 3 times, most recently from e4f6326 to e8d66a6 Compare June 13, 2025 19:43

rock3r reviewed Jun 16, 2025

View reviewed changes

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch 2 times, most recently from bbf9835 to d69a6b4 Compare June 16, 2025 12:48

lamba92 requested a review from rock3r June 16, 2025 13:12

terrakok requested review from igordmn and removed request for rock3r June 16, 2025 13:19

lamba92 requested a review from rock3r June 16, 2025 13:20

rock3r reviewed Jun 16, 2025

View reviewed changes

igordmn reviewed Jun 17, 2025

View reviewed changes

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch from 132e8c3 to 8cf1eb7 Compare June 18, 2025 14:32

terrakok requested changes Jun 19, 2025

View reviewed changes

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch from 1cb579c to e0f882a Compare June 23, 2025 13:08

lamba92 requested a review from terrakok June 23, 2025 13:08

lamba92 assigned terrakok Jun 23, 2025

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch from e0f882a to 9c1571d Compare June 23, 2025 13:11

terrakok requested changes Jun 24, 2025

View reviewed changes

...urces/library/src/wasmJsMain/kotlin/org/jetbrains/compose/resources/ResourceReader.wasmJs.kt Show resolved Hide resolved

TBSten mentioned this pull request Jun 25, 2025

Add ResourceConfiguration API #5315

Closed

1 task

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch from 9c1571d to 1739345 Compare June 25, 2025 11:59

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch from 1739345 to b91d0f6 Compare June 25, 2025 12:00

lamba92 requested review from igordmn and terrakok June 25, 2025 12:01

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch from b91d0f6 to bf20c32 Compare June 25, 2025 12:05

terrakok requested changes Jun 26, 2025

View reviewed changes

lamba92 added 2 commits July 1, 2025 13:15

lamba92 force-pushed the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch from be999b1 to 1a662db Compare July 1, 2025 11:16

lamba92 requested a review from terrakok July 1, 2025 11:16

terrakok approved these changes Jul 2, 2025

View reviewed changes

terrakok merged commit c4cf4c5 into master Jul 2, 2025
1 of 2 checks passed

terrakok deleted the lamberto.basti/CMP-8134-Switch-the-ClassLoader-used-by-ResourceReader branch July 2, 2025 09:03

henryqwe pushed a commit to henryqwe/compose-multiplatform that referenced this pull request

Make ResourceReader a public API #5334

Make ResourceReader a public API #5334

Uh oh!

Conversation

lamba92 commented Jun 13, 2025 • edited by terrakok Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Release Notes

Features - Resources

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

terrakok commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lamba92 commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rock3r commented Jun 17, 2025

Uh oh!

lamba92 commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rock3r commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igordmn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igordmn Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

lamba92 Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

igordmn Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

lamba92 Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lamba92 commented Jun 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

terrakok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

terrakok Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

terrakok Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

lamba92 Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lamba92 commented Jun 13, 2025 •

edited by terrakok

Loading

terrakok commented Jun 16, 2025 •

edited

Loading

lamba92 commented Jun 16, 2025 •

edited

Loading

lamba92 commented Jun 17, 2025 •

edited

Loading

rock3r commented Jun 17, 2025 •

edited

Loading

lamba92 Jun 18, 2025 •

edited

Loading

lamba92 Jun 27, 2025 •

edited

Loading