Kamel-Media / Kamel

Kotlin asynchronous media loading and caching library for Compose.
Apache License 2.0
636 stars 24 forks source link

File URLs should be handled as such on JVM platform #88

Open eskatos opened 8 months ago

eskatos commented 8 months ago

Doing asyncPainterResource("file://path/to/image.png") currently fails with the following exception:

io.ktor.http.URLParserException: Fail to parse url: file://path/to/image.png
    at io.ktor.http.URLParserKt.takeFrom(URLParser.kt:21)
    at io.ktor.http.URLUtilsKt.URLBuilder(URLUtils.kt:25)
    at io.ktor.http.URLUtilsKt.Url(URLUtils.kt:13)
    at io.kamel.core.mapper.MappersKt$StringMapper$1.map(Mappers.kt:14)
    at io.kamel.core.mapper.MappersKt$StringMapper$1.map(Mappers.kt:8)

I would have expected this to load the local image from the filesystem.

I do this from a commonMain source set where I can't use java.io.File.

For the time being I worked around it with an expect/actual function to call asyncPainterResource(File) for file: URLs on the JVM platform. This is a totally viable workaround but supporting file: URLs out of the box would make Kamel easier to use.

luca992 commented 8 months ago

that's java URI syntax right?

So the better way to do it would be either to write your own string mapper and use it in your KamelConfig:

https://github.com/Kamel-Media/Kamel/blob/86f216c1df9fafbe54ffb60ad3a11f5861ba5fb4/kamel-core/src/commonMain/kotlin/io/kamel/core/mapper/Mappers.kt#L8-L16

Or.. the default common string mapper could be modified in the library and split into expect actual definitions, and extended to add uri support on jvm. Or is the java uri format useful on other targets as well? In that case the default common string mapper could be extended for other targets too.

Lmk what you think

luca992 commented 8 months ago

Also fyi if you did instead asyncPainterResource(URI("file://path/to/image.png")) it should work as is.

eskatos commented 8 months ago

The file URI scheme is not specific to the JVM https://datatracker.ietf.org/doc/html/rfc8089

There's no URI type in Kotlin common so asyncPainterResource(URI("file://path/to/image.png")) doesn't work.

I'll give the custom mapper way a try.

luca992 commented 8 months ago

Thanks for the reference.

io.kamel.core.utils.URI technically has the same constructor for all targets.. so I could add a way to create one in common if that is desired

eskatos commented 8 months ago

Using asyncPainterResource(io.kamel.core.utils.URI("file://path/to/image.png")) in my JVM actual doesn't load the image. I'll stick with my "dirty" workaround for now that basically does this on the JVM:

when {
    url.startsWith("file:") -> asyncPainterResource(Paths.get(URI.create(url)).toFile())
    else -> asyncPainterResource(url)
}
luca992 commented 8 months ago

I just exposed the URI(str: String) constructor in common. And it's loading the example...

Screenshot 2024-01-23 at 9 44 32 PM

but also looks like it works with just a string too 🤔:

Screenshot 2024-01-23 at 9 47 08 PM
luca992 commented 8 months ago

It might just be an issue with your path

eskatos commented 8 months ago

Ha! :)

The path is right as I can load the image if I pass a java.io.File built from it. I tried many variations but it doesn't work on Linux without my workaround! :thinking:

luca992 commented 8 months ago

Ha! :)

The path is right as I can load the image if I pass a java.io.File built from it. I tried many variations but it doesn't work on Linux without my workaround! 🤔

So it appears, file:// is scoped to the jar's resources. That's why it is working for me.. But when I use an absolute path it does not

eskatos commented 8 months ago

Right, the image path I use is for a png file from the OS file system, outside the application resources.

luca992 commented 8 months ago

Lol, I see now I was editing the ResourcesFetcher test... so obviously loading resources was working 😅

Anyways: https://github.com/Kamel-Media/Kamel/pull/90

you can try it with: 0.10.0-SNAPSHOT It should be working for local files with or without the "file://" prefix

luca992 commented 8 months ago

@eskatos lmk if you get a chance to try it out

eskatos commented 8 months ago

I just tried and it fails by default parsing a file:/path/to/image.png URL which is what I get from other APIs. If I manually change the URL to file:///path/to/image.png then it works out of the box. It looks like KTor URL parser is not correct as this syntax is described in Section 2 of the RFC as shown in examples in the Appendix B of the RFC.

I would understand you push this upstream to KTor at this point.

luca992 commented 8 months ago

I just tried and it fails by default parsing a file:/path/to/image.png URL which is what I get from other APIs. If I manually change the URL to file:///path/to/image.png then it works out of the box. It looks like KTor URL parser is not correct as this syntax is described in Section 2 of the RFC as shown in examples in the Appendix B of the RFC.

I would understand you push this upstream to KTor at this point.

If that's common I can add a case here:

kamel-core/src/commonMain/kotlin/io/kamel/core/mapper/Mappers.kt

But you might want to submit that issue to ktor anyway and tag it here

eskatos commented 8 months ago

Here's the KTOR issue https://youtrack.jetbrains.com/issue/KTOR-6709

luca992 commented 8 months ago

@eskatos I re-published with support for file:/ and file:/// in kamel-core/src/commonMain/kotlin/io/kamel/core/mapper/Mappers.kt

I got rid of assuming a / prefix is a file because that would break things when using a defaultRequest with a base url.

eskatos commented 4 days ago

Sorry it took me so long to get back to you. All good with you latest publication, thank you very much :heart: Any chance this could get into a released version?

luca992 commented 2 days ago

@eskatos it should be in the 1.0 beta. The only reason it's beta is because it uses ktor 3.0 beta still. I'm waiting on a release.