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

SVG Decoder with Batik #50

Merged
merged 5 commits into from
Jul 21, 2023
Merged

SVG Decoder with Batik #50

merged 5 commits into from
Jul 21, 2023

Conversation

DatL4g
Copy link
Contributor

@DatL4g DatL4g commented Jul 19, 2023

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.

# Conflicts:
#	gradle/libs.versions.toml
#	kamel-image/src/desktopMain/kotlin/io/kamel/image/config/DesktopKamelConfig.kt
#	kamel-image/src/desktopMain/kotlin/io/kamel/image/decoder/DesktopSvgDecoder.kt
@luca992
Copy link
Member

luca992 commented Jul 19, 2023

It would make more sense to make a new decoder

@DatL4g
Copy link
Contributor Author

DatL4g commented Jul 19, 2023

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
Copy link
Member

luca992 commented Jul 20, 2023

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
Copy link
Member

luca992 commented Jul 20, 2023

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

@DatL4g
Copy link
Contributor Author

DatL4g commented Jul 20, 2023

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
Copy link
Member

luca992 commented Jul 20, 2023

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
Copy link
Contributor Author

DatL4g commented Jul 20, 2023

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
Copy link
Member

luca992 commented Jul 21, 2023

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.

@luca992 luca992 merged commit 67a4a80 into Kamel-Media:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants