coil

coil copied to clipboard

Scales SVGs correctly

Issue: https://github.com/coil-kt/coil/issues/2048

Please run ./test.sh before submitting to ensure your pull request passes the automated checks.

Thanks! I think we can ship this with a few changes:

• Let's put these changes behind a flag in SvgDecoder.Factory's constructor. We need to preserve the existing behaviour for existing users.
• Do we need to make the same changes to SvgDecoder.nonAndroid? Ideally we want the Android and non-Android implementations to behave as similarly as possible.
• Please add a test in AbstractSvgDecoderTest. This'll make sure that we don't break the behaviour in the future.

Hey Colin,

The changes that you suggest sound reasonable, but I'm not able to make those modifications right now. We (Google) are using the prior version, and upgrading to 3.x so that I can make the change to head is going to take a little longer.

On Tue, Jan 30, 2024 at 10:59 AM Colin White @.***> wrote:

Thanks! I think we can ship this with a few changes:

• Let's put these changes behind a flag in SvgDecoder.Factory's constructor. We need to preserve the existing behaviour for existing users.
• Do we need to make the same changes to SvgDecoder.nonAndroid? Ideally we want the Android and non-Android implementations to behave as similarly as possible.
• Please add a test in AbstractSvgDecoderTest. This'll make sure that we don't break the behaviour in the future.

— Reply to this email directly, view it on GitHub https://github.com/coil-kt/coil/pull/2090#issuecomment-1917697109, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFUHJ72FROI5XYRQPNR74DYRE7K5AVCNFSM6AAAAABCRWKZOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJXGY4TOMJQHE . You are receiving this because you authored the thread.Message ID: @.***>

@seanpont Ah gotcha! If you'd like to make the change to the 2.x releases you'll need to target your PR to the 2.x branch. main currently has all the 3.0 alpha code which supports multiplatform.

I think we should still add a flag to SvgDecoder's constructor to avoid changing the behaviour for existing users. Maybe densityScalingEnabled: Boolean = false?

I can work on releasing 2.6.0 with those changes once they're merged.

Looking at the commits does svg.scale even exist? I attempted to copy this code and it did not work. Is there something I am missing to get svg.scale be available?

Is there any obvious downsides of doing an implementation like this by default eventually?

Gonna +1 getting this merged in some form. For us, SVGs render at a miniscule size on Android when using coil