Kamel-Media / Kamel

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

SVG Decoder with Batik #50

Closed DatL4g closed 11 months ago

DatL4g commented 11 months ago

This adds Batik support for the SVG Decoder on the JVM.

As described in the KamelConfigBuilder extension function this is useful for this bug https://bugs.chromium.org/p/skia/issues/detail?id=12251

When using svgDecoder(useBatik = true) images like this are displayed properly, however they may result in lower quality therefore this options is disabled by default.

luca992 commented 11 months ago

It would make more sense to make a new decoder

DatL4g commented 11 months ago

Mhm I don't know. They serve the same purpose and only one decoder can be used at a time. An internal change would be required since the default KamelConfig already uses the SVGDecoder.

With the current implementation the decoder can be easily overwritten

luca992 commented 11 months ago

Mhm I don't know. They serve the same purpose and only one decoder can be used at a time. An internal change would be required since the default KamelConfig already uses the SVGDecoder.

With the current implementation the decoder can be easily overwritten

I think it should work something like this where the last added decoder of a type should replace any previously added decoders of the same type.

KamelConfig {
    takeFrom(KamelConfig.Default)
    resourcesFetcher()
    // the last added svg decoder should replace the default
    batikSvgDecoder()
}
luca992 commented 11 months ago

Also... we should probably separate decoders into their own modules people can import independently.... but I can figure that out after

DatL4g commented 11 months ago

I updated the PR so there is a separate decoder for Batik.

I don't think we should separate decoders into their own modules as this is a lot of overhead in development, I can tell as someone who developed other multi-module libraries.

I think a feature-like separation makes more sense.

Example:

We have a core image processing library which contains just default compose implementation. Then we have a module for web resources, so all fetchers and decoders related to HttpRequests etc. are going in one extra module, as some people don't need this if they work with resources and system files only. Then there is a svg module for example where all extra svg decoders (like Batik) take a place.

However there has to be a decision made about something like the android svg decoder. As the core images module would include the skia implementation and people could get confused why SVGs work on any other than android

luca992 commented 11 months ago

Yeah I agree it should definitely be split up by features as well. But, if for example a svg decoder feature module one day has 5 different implementations backed by 5 different dependencies. People aren't going to like all that extra baggage.

I think it might be good to split all the decoders / fetchers into their own modules then have a module called something like kamel-defaults or something similar which gives you access to KamelConfig.Default and exposes all decoders and fetchers which are considered default. And then you can also have another module which bundles by feature like kamel-decoder-svg which would expose all svg decoder implementations. And finally you can granular imports with each decoder/fetcher implementation like: kamel-decoder-svg-batik. Ideally I think it should be a lot like how ktor handles adding dependencies for engines.

dependencies {
    implementation("media.kamel:kamel-image:0.6.0")
    // imports all default implementations giving you access to KamelConfig.Default
    implementation("media.kamel:kamel-defaults:0.6.0") 
    // imports all svg decoders the (standard + batik)
    implementation("media.kamel:kamel-decoder-svg:0.6.0")
    // imports just the batik svg decoder
    implementation("media.kamel:kamel-decoder-svg-batik:0.6.0")
    // ...
}
DatL4g commented 11 months ago

Done, btw not sure if the work for new modules is worth since Coil goes Multiplatform which will probably replace most users of this library

luca992 commented 11 months ago

Done, btw not sure if the work for new modules is worth since Coil goes Multiplatform which will probably replace most users of this library

True. However it's really not that much work imo.