Add multiplatform support to coil-svg #1965

MohamedRejeb · 2023-12-21T19:19:55Z

Migrate coil-svg module to multiplatform
Add SvgDecoder for non android platforms based on skia svg
Add a public API to create an SvgDecoder from commonMain
Add tests to jvm target

colinrtwhite · 2023-12-25T05:33:46Z

coil-svg/src/commonMain/kotlin/coil3/decode/SvgDecoder.kt

@@ -0,0 +1,11 @@
+package coil3.decode
+
+object SvgDecoder {


Let's use an expect/actual class instead of factory functions:

expect class SvgDecoder @JvmOverloads constructor( source: ImageSource, options: Options, useViewBoundsAsIntrinsicSize: Boolean = true, ) : Decoder { class Factory : Decoder.Factory }

colinrtwhite · 2023-12-25T08:22:06Z

Btw you can run the CI checks locally using ./test.sh. Unfortunately I can't set it to run on each push

MohamedRejeb · 2023-12-25T12:25:34Z

It should run successfully now.

colinrtwhite · 2023-12-25T19:31:35Z

Great! This looks pretty good. I think we should be able to merge it once the tests are updated to run on Android and the JVM

colinrtwhite · 2023-12-28T05:32:18Z

coil-svg/src/androidMain/kotlin/coil3/decode/SvgDecoder.android.kt

@@ -20,16 +20,17 @@ import kotlin.math.roundToInt
 import kotlinx.coroutines.runInterruptible

 /**
- * A [Decoder] that uses [AndroidSVG](https://bigbadaboom.github.io/androidsvg/) to decode SVG
+ * A [Decoder] that uses [AndroidSVG](https://bigbadaboom.github.io/androidsvg/)
+ * and [SVGDOM](https://api.skia.org/classSkSVGDOM.html/) to decode SVG


Seems like this should be the docs for the expect declaration

colinrtwhite · 2023-12-28T05:32:43Z

coil-svg/src/commonMain/kotlin/coil3/decode/SvgDecoder.kt

+import coil3.request.Options
+
+/**
+ * A [Decoder] that uses [AndroidSVG](https://bigbadaboom.github.io/androidsvg/) and  to decode SVG


This should be swapped with the Android implementation docs

colinrtwhite · 2023-12-28T05:35:03Z

coil-svg/src/jvmTest/kotlin/coil3/decode/SvgDecoderTest.jvm.kt

+    private val decoderFactory = SvgDecoder.Factory()
+
+    @Test
+    fun handlesSvgMimeType() {


Is it possible to implement these tests in a common way? If you can't put a common test class in jvmCommonTest, you could create AbstractSvgDecoderTest in jvmCommonTest which defines all the tests then have SvgDecoderTestAndroid and SvgDecoderTestJvm extend that class.

I was trying to do this, but I can't access jvmCommonTest or even commonTest from androidInstrumentedTest.

colinrtwhite · 2023-12-28T05:42:55Z

internal/test-utils/src/jvmMain/kotlin/coil3/test/utils/bitmaps.jvm.kt

+import kotlinx.coroutines.runBlocking
+import org.jetbrains.skia.Bitmap
+
+val Bitmap.size: Size


Thanks for converting these! Could we create a common Bitmap type (only for our tests)? Looks like something like this should be enough:

expect class Bitmap { val width: Int val height: Int fun getPixels(): ByteArray }

This way we can put the rest of these functions in commonMain, which will be useful for migrating more of Coil's tests to commonMain.

MohamedRejeb · 2024-01-01T09:24:08Z

I created AbstractSvgDecoderTest in the internal test utils module, and now the svg decoder test is shared between jvm and android. Also, I created CoilBitmap for testing and the bitmap comparison functions are shared between jvm and android. I didn't name it Bitmap to avoid confusion.
The only thing that prevents us from moving these tests to common is the ability to read resources from common test.
Now some functions are still duplicated in android for Bitmap and in common for CoilBitmap just to keep the old android tests working, But after migrating those tests, these functions can be removed from android.

MohamedRejeb · 2024-01-01T20:44:41Z

A little mistake in the dependencies caused this failure. It should be running successfully now.

colinrtwhite · 2024-01-02T17:33:43Z

coil-svg/src/androidInstrumentedTest/kotlin/coil3/decode/SvgDecoderTest.android.kt

+
+class SvgDecoderTestAndroid : AbstractSvgDecoderTest(SvgDecoder.Factory()) {
+    @Test
+    override fun handlesSvgMimeType() =


I think you can annotate the methods of the superclass so you don't need to override. We do something similar in Redwood: https://github.com/cashapp/redwood/blob/trunk/redwood-layout-shared-test/src/commonMain/kotlin/app/cash/redwood/layout/AbstractFlexContainerTest.kt#L71

colinrtwhite · 2024-01-02T17:34:38Z

coil-svg/src/jvmTest/kotlin/coil3/decode/SvgDecoderTest.jvm.kt

+
+class SvgDecoderTestJvm : AbstractSvgDecoderTest(SvgDecoder.Factory()) {
+    @Test
+    override fun handlesSvgMimeType() =


Same for here.

colinrtwhite · 2024-01-02T17:40:58Z

internal/test-utils/src/jvmCommonMain/kotlin/coil3/test/utils/AbstractSvgDecoderTest.kt


-class SvgDecoderTest {
+abstract class AbstractSvgDecoderTest(


Can this class live in coil-svg/jvmCommonTest instead of moving it to test-utils?

When it was in jvmCommonTest it was not visible to android instrumented test.

colinrtwhite · 2024-01-02T17:46:06Z

internal/test-utils/src/androidMain/kotlin/coil3/test/utils/bitmaps.android.kt

@@ -100,7 +129,7 @@ fun Bitmap.isSimilarTo(
 */
 fun Bitmap.assertIsSimilarTo(
    expected: Bitmap,
-    threshold: Double = 0.99
+    threshold: Double = 0.987


Let's use 0.98

I changed it back to 0.99 in android and 0.98 in common.

colinrtwhite

Thanks!

* Add multiplatform support to coil-svg * Remove commented code * Move SvgDecoderTest to coil3/decode * Add public API for creating SvgDecoder.Factory from commonMain * Move isSvg_newLine test to common * Apply spotless * Run apiDump * Change SvgDecoder to expect/actual class * Apply spotless * Add test to jvm target * Run spotlessApply * Use Okio FileSystem.RESOURCES * Remove unnecessary code * Fix SvgDecoder documentation * Share SvgDecoderTest between jvm and android * Apply spotless * Bring back old android Bitmap test functions * Fix SvgDecoder android documentation * Rename common decodeBitmapAsset to decodeBitmapResource * Fix test failure * Update threshold default value * Add Test annotation to AbstractSvgDecoderTest methods * Apply spotless

KapilYadav-dev · 2024-01-03T07:52:55Z

@MohamedRejeb awesome to see your contribution in coil also < 3

Add multiplatform support to coil-svg

1d5a254

MohamedRejeb mentioned this pull request Dec 21, 2023

Compose Multiplatform support #842

Closed

MohamedRejeb added 5 commits December 21, 2023 20:43

Remove commented code

c8f8a28

Move SvgDecoderTest to coil3/decode

7310a51

Add public API for creating SvgDecoder.Factory from commonMain

cdc93b1

Move isSvg_newLine test to common

9f14776

Apply spotless

c7f20b6

MohamedRejeb marked this pull request as ready for review December 24, 2023 16:27

Run apiDump

28fd2f2

colinrtwhite reviewed Dec 25, 2023

View reviewed changes

Change SvgDecoder to expect/actual class

e5d9482

Apply spotless

928d9fc

MohamedRejeb added 4 commits December 26, 2023 12:38

Add test to jvm target

30511bf

Run spotlessApply

90abbb9

Use Okio FileSystem.RESOURCES

1341f3d

Remove unnecessary code

2767ae6

MohamedRejeb requested a review from colinrtwhite December 26, 2023 12:12

colinrtwhite reviewed Dec 28, 2023

View reviewed changes

colinrtwhite deleted the branch coil-kt:3.x December 30, 2023 21:08

colinrtwhite closed this Dec 30, 2023

colinrtwhite reopened this Dec 30, 2023

MohamedRejeb added 3 commits December 31, 2023 21:36

Fix SvgDecoder documentation

9e8f3f4

Share SvgDecoderTest between jvm and android

5c3d6e8

Apply spotless

dff3582

Bring back old android Bitmap test functions

1da194b

MohamedRejeb requested a review from colinrtwhite January 1, 2024 09:24

MohamedRejeb added 3 commits January 1, 2024 10:39

Fix SvgDecoder android documentation

f76c6ee

Rename common decodeBitmapAsset to decodeBitmapResource

3b1891f

Fix test failure

221ec8d

colinrtwhite reviewed Jan 2, 2024

View reviewed changes

MohamedRejeb added 3 commits January 2, 2024 22:03

Update threshold default value

68a08c7

Add Test annotation to AbstractSvgDecoderTest methods

fb64ee5

Apply spotless

14cbcc4

MohamedRejeb requested a review from colinrtwhite January 2, 2024 21:12

colinrtwhite approved these changes Jan 3, 2024

View reviewed changes

colinrtwhite merged commit d9a99c7 into coil-kt:3.x Jan 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiplatform support to coil-svg #1965

Add multiplatform support to coil-svg #1965

MohamedRejeb commented Dec 21, 2023 •

edited

Loading

colinrtwhite Dec 25, 2023 •

edited

Loading

MohamedRejeb Dec 25, 2023

colinrtwhite commented Dec 25, 2023

MohamedRejeb commented Dec 25, 2023

colinrtwhite commented Dec 25, 2023

colinrtwhite Dec 28, 2023

MohamedRejeb Dec 31, 2023

colinrtwhite Dec 28, 2023

MohamedRejeb Dec 31, 2023

colinrtwhite Dec 28, 2023

MohamedRejeb Dec 31, 2023

colinrtwhite Dec 28, 2023

MohamedRejeb commented Jan 1, 2024

MohamedRejeb commented Jan 1, 2024

colinrtwhite Jan 2, 2024

MohamedRejeb Jan 2, 2024

colinrtwhite Jan 2, 2024

colinrtwhite Jan 2, 2024

MohamedRejeb Jan 2, 2024

colinrtwhite Jan 2, 2024

MohamedRejeb Jan 2, 2024

colinrtwhite left a comment

KapilYadav-dev commented Jan 3, 2024


		class SvgDecoderTest {
		abstract class AbstractSvgDecoderTest(

Add multiplatform support to coil-svg #1965

Add multiplatform support to coil-svg #1965

Conversation

MohamedRejeb commented Dec 21, 2023 • edited Loading

colinrtwhite Dec 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrtwhite commented Dec 25, 2023

MohamedRejeb commented Dec 25, 2023

colinrtwhite commented Dec 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedRejeb commented Jan 1, 2024

MohamedRejeb commented Jan 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrtwhite left a comment

Choose a reason for hiding this comment

KapilYadav-dev commented Jan 3, 2024

MohamedRejeb commented Dec 21, 2023 •

edited

Loading

colinrtwhite Dec 25, 2023 •

edited

Loading