[KMP] Add Gradient Support to Parser #749
Conversation
WalkthroughAdds end-to-end gradient support for vector drawables: new SDK types VectorDrawable.Gradient, GradientItem, and AaptAttr; Path now carries AAPT attributes. Parser maps VectorDrawable gradients (linear and radial, with color stops) into IrFill gradient representations. Generator maps IrFill gradients back into VectorDrawable.AaptAttr for XML output and hoists the aapt namespace to the root during serialization. Tests added and re-enabled to cover parsing and round-trip generation for linear and radial gradients, color stops, multiple paths, and gradients inside groups. Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/parser/kmp/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParser.kt (1)
114-131: Consider handling empty color stops gracefully.If
itemsis empty and bothstartColor/endColorare null (or have invalid colors),buildColorStops()returns an empty list. This could produce an invalid gradient with no colors.Consider returning
nullfromtoIrFill()when no valid color stops can be built:private fun VectorDrawable.Gradient.toIrFill(): IrFill? { + val colorStops = buildColorStops() + if (colorStops.isEmpty()) return null + return when (type.lowercase()) { "linear" -> { IrFill.LinearGradient( startX = startX ?: 0f, startY = startY ?: 0f, endX = endX ?: 0f, endY = endY ?: 0f, - colorStops = buildColorStops(), + colorStops = colorStops, ) } "radial" -> { IrFill.RadialGradient( radius = gradientRadius ?: 0f, centerX = centerX ?: 0f, centerY = centerY ?: 0f, - colorStops = buildColorStops(), + colorStops = colorStops, ) } else -> null } }sdk/generator/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGenerator.kt (1)
107-135: Potential crash ifcolorStopsis empty.
colorStops.first()andcolorStops.last()on lines 110-111 and 122-123 will throwNoSuchElementExceptionif the list is empty. While this is unlikely in practice, defensive checks would make the code more robust.private fun IrFill.LinearGradient.toVectorDrawableGradient(): VectorDrawable.Gradient { + if (colorStops.isEmpty()) { + return VectorDrawable.Gradient( + type = "linear", + startX = startX, + startY = startY, + endX = endX, + endY = endY, + centerX = null, + centerY = null, + gradientRadius = null, + startColor = null, + endColor = null, + items = emptyList(), + ) + } + // Use startColor/endColor for simple 2-color gradients (0.0 to 1.0) val useSimpleFormat = colorStops.size == 2 &&components/parser/kmp/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParserTest.kt (2)
353-414: Multiple paths with different gradient types are well‑covered
parse_multiple_paths_with_gradientsvalidates:
- Two separate paths inside one vector.
- First path filled with a linear gradient, second with a radial gradient, each with correct coordinates and colors.
One optional improvement: you could also assert the expected pathData/
pathsfor eachIrPathto guard against accidental node reordering, but the current checks are already valuable.
477-494: vectorWithGradient helper avoids namespace duplicationThe
vectorWithGradienthelper sensibly mirrorsvectorbut adds thexmlns:aaptdeclaration. This keeps individual tests focused on content instead of boilerplate XML.If you end up adding more AAPT‑backed tests, consider parameterizing extra namespaces on
vector(...)instead of maintaining two helpers, but that’s purely optional at this point.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/parser/kmp/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParser.kt(2 hunks)components/parser/kmp/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParserTest.kt(3 hunks)components/parser/unified/src/jvmTest/kotlin/io/github/composegears/valkyrie/parser/unified/XmlToImageVectorTest.kt(0 hunks)sdk/core/xml/api/xml.api(5 hunks)sdk/core/xml/api/xml.klib.api(6 hunks)sdk/core/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/core/xml/VectorDrawable.kt(3 hunks)sdk/generator/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGenerator.kt(1 hunks)sdk/generator/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/XmlSerializer.kt(1 hunks)sdk/generator/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGeneratorTest.kt(4 hunks)
💤 Files with no reviewable changes (1)
- components/parser/unified/src/jvmTest/kotlin/io/github/composegears/valkyrie/parser/unified/XmlToImageVectorTest.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-27T22:19:48.762Z
Learnt from: t-regbs
Repo: ComposeGears/Valkyrie PR: 587
File: sdk/generator/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/XmlSerializer.kt:13-16
Timestamp: 2025-09-27T22:19:48.762Z
Learning: The VectorDrawable.Child sealed interface in the Valkyrie project only has Group and Path as implementations. ClipPath is not implemented in this project, despite being supported in Android's official VectorDrawable specification.
Applied to files:
components/parser/kmp/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParser.ktsdk/core/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/core/xml/VectorDrawable.ktsdk/core/xml/api/xml.klib.apicomponents/parser/kmp/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParserTest.ktsdk/core/xml/api/xml.api
🧬 Code graph analysis (2)
sdk/generator/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGeneratorTest.kt (1)
components/parser/kmp/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParserTest.kt (1)
imageVector(496-502)
components/parser/kmp/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParserTest.kt (1)
sdk/generator/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGeneratorTest.kt (1)
imageVector(729-735)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (27)
components/parser/kmp/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParser.kt (2)
40-45: LGTM - Clean gradient fill precedence logic.The implementation correctly prioritizes gradient fill over color fill, with a clean fallback pattern.
91-112: LGTM - Gradient type mapping is correct.The lowercase comparison handles case-insensitive type matching properly. Defaulting missing coordinates to
0faligns with Android's VectorDrawable behavior.Note: "sweep" gradients are not supported (returns
null), which is consistent with the PR scope focusing on linear and radial gradients.sdk/generator/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGeneratorTest.kt (4)
594-632: LGTM - Good coverage for linear gradient serialization.The test validates all essential attributes: namespace declaration, aapt:attr structure, gradient type, coordinates, and start/end colors.
634-670: LGTM - Comprehensive radial gradient test.Test correctly validates radial-specific attributes (
centerX,centerY,gradientRadius) alongside common gradient elements.
672-709: LGTM - Multi-stop gradient test validates item serialization.This test ensures the generator correctly uses
<item>elements with explicit offsets when there are more than 2 color stops, rather than the simplifiedstartColor/endColorformat.
420-431: LGTM - Round-trip test properly validates gradient structure.The updated assertions verify that both linear and radial gradients are preserved through the parse-generate cycle.
sdk/core/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/core/xml/VectorDrawable.kt (3)
38-53: LGTM - Well-structured Gradient data class.The design appropriately uses nullable fields to handle optional attributes for different gradient types (linear vs radial). The
itemslist with empty default enables the simple 2-color format viastartColor/endColor.
63-69: LGTM - Extensible AaptAttr design.The structure allows future extension to other aapt attribute types beyond gradients.
103-104: LGTM - Backward-compatible Path extension.The nullable
aaptAttrfield with defaultnullensures existing code continues to work unchanged.sdk/generator/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGenerator.kt (2)
64-90: Potential issue:fillColormay be set alongsideaaptAttrfor gradient fills.When
fillis a gradient,fillColorStron line 65-68 returnsnull(correct), but this relies on theelse -> nullbranch. The logic works correctly, but could be made more explicit.Additionally, both
fillColorandaaptAttrfields are set on the Path. WhilefillColorwill benullfor gradients (due to thewhenbranch), this implicit coupling could be fragile ifIrFillgets new subtypes.Consider making the mutual exclusivity explicit in a future refactor if needed.
137-164: LGTM - Radial gradient conversion mirrors linear pattern.The implementation correctly handles radial-specific attributes (
centerX,centerY,gradientRadius). The sameuseSimpleFormatoptimization applies.Note: The same empty
colorStopsconsideration applies here as noted for the linear gradient function.sdk/generator/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/XmlSerializer.kt (1)
41-54: Regex pattern handles multiline vector tags correctly.The character class
[^>]in the regex on line 49 matches across newlines, so the pattern will work even if the vector tag spans multiple lines withxmlns:androidon a separate line. No regex fix is required.Likely an incorrect or invalid review comment.
sdk/core/xml/api/xml.klib.api (4)
44-71: AaptAttr ABI looks consistent and safely extensibleThe new
VectorDrawable.AaptAttrtype (name + optional gradient) plus its serializer/companion are coherent and additive to the existing ABI. The nullable gradient matches AAPT usage where other value kinds could be added later.
102-158: Gradient model matches VectorDrawable semantics
VectorDrawable.Gradientexposes all expected coordinates (start/end, center, radius), optional start/end colors, and an items list, with full data‑class style API and serialization hooks. This should cover both linear and radial gradients without constraining future extensions.
160-187: GradientItem is a minimal, correct representation of color stops
VectorDrawable.GradientItemas (color, offset) with proper copy/component/serializer support is exactly what’s needed for gradient stops. No issues spotted.
241-281: Path.aaptAttr integration and new GRADIENT constants are ABI‑safeExtending
VectorDrawable.Path’s constructor/copy/components with trailingAaptAttr?while keeping defaults is binary compatible and gives a clean hook for gradient fills. The addedGRADIENT/GRADIENT_ITEMconstants onCompanionalign with the new types.Also applies to: 312-315
components/parser/kmp/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParserTest.kt (6)
7-9: Explicit IrPathNode imports keep expectations readableImporting
MoveTo,LineTo, andClosedirectly improves clarity in the new path expectations without impacting behavior.
173-214: Linear gradient parsing test matches XML and IR expectationsThe
parse_path_with_linear_gradienttest correctly wires:
- XML
android:type="linear"and start/end coordinates.- Opaque
#FF0000/#0000FFmapped to0xFFFF0000/0xFF0000FF.- Path data
M0,0 L10,0 L10,10 L0,10 Zto the correspondingMoveTo/LineTo/Closesequence.This is a solid baseline check that gradient parsing doesn’t disturb path geometry.
216-261: Radial gradient parsing test is consistent with vector XMLFor
parse_path_with_radial_gradient, center, radius, and color mapping from XML (#00FF00→0xFF00FF00,#FFFF00→0xFFFFFF00) are all consistent, and the expectedIrFill.RadialGradientmirrors the XML configuration. No issues.
263-307: Linear gradient with explicit color stops covers item‑based gradients well
parse_path_with_linear_gradient_and_color_stopsexercises the<item>‑only case (nostartColor/endColor), checks offsets0/0.5/1, and verifies ARGB values for red/green/blue. This is a good edge case to ensure items take precedence and ordering is preserved.
309-351: Radial gradient with color stops test is correctly specifiedThe radial gradient with three
<item>stops has matching expected offsets and colors (#FFFFFF,#888888,#000000→0xFFFFFFFF,0xFF888888,0xFF000000). This nicely complements the linear‑gradient stops test.
416-457: Gradient inside group verifies nested structure and fill mapping
parse_path_with_gradient_in_groupasserts:
- Top‑level node is an
IrGroupnamed"gradientGroup".- Group has a single
IrPathchild.- Child fill is the expected linear gradient with correct start/end coords and colors.
This is a good regression test for gradients under groups.
sdk/core/xml/api/xml.api (5)
4-7: New GRADIENT/GRADIENT_ITEM constants are a non‑breaking extensionAdding
GRADIENTandGRADIENT_ITEMalongside existingCLIP_PATH,GROUP, andPATHis a straightforward extension of the public API and should not impact existing consumers.
44-72: AaptAttr JVM ABI aligns with the KMP design
VectorDrawable$AaptAttrexposes (name, gradient) with proper component/copy methods plus serializer/companion, matching the KLib view. This gives a clear JVM surface for AAPT attributes.
111-158: Gradient JVM ABI faithfully represents the gradient modelThe
VectorDrawable$GradientJVM signature includes all the same fields, copy/component methods, and serializer as in the KLib ABI. This looks correct and keeps the common gradient contract consistent across targets.
159-187: GradientItem JVM ABI is consistent with KLib and tests
VectorDrawable$GradientItemcarries (color, offset) with the expected methods and serializer, matching how color stops are used in the new tests.
234-251: Path extended with AaptAttr remains binary compatible
VectorDrawable$Pathnow has constructors/copy/component12/getAaptAttr wired toAaptAttr, mirroring the wasm ABI. Because the new parameter is appended with a defaulted synthetic constructor, this remains source‑ and binary‑compatible for existing callers.
…ctor drawable tests and implementation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
sdk/core/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/core/xml/VectorDrawable.kt (1)
38-53: Consider defaultingtypeto linear to better match VectorDrawable behaviorAs modeled,
type: Stringis required and has no default, so deserialization will fail ifandroid:typeis omitted, whereas Android treats the default aslinear. If you expect to consume such vectors, consider givingtypea default:- @XmlSerialName("type", ANDROID_NAMESPACE, ANDROID_PREFIX) val type: String, + @XmlSerialName("type", ANDROID_NAMESPACE, ANDROID_PREFIX) val type: String = "linear",Also, this relies on
GradientItem’s@XmlSerialNameto emit<item>children directly fromitems; it sounds correct, but worth double‑checking against xmlutil’s list handling and your round‑trip tests.sdk/generator/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGeneratorTest.kt (4)
411-432:ic_brushgradient assertions look good;<item>counting is slightly brittleThe extended checks for
<aapt:attr>,<gradient>, and bothandroid:type="linear"/"radial"give good coverage that brush gradients survive a round‑trip, and the explicit pathData asserts help guard against regressions in non‑gradient parts.One minor robustness nit:
gradientItemCountis derived fromgeneratedXml.split("<item").lastIndex, which will also count any other<item>tags that might be introduced later (e.g., unrelated aapt items). That’s fine today, but if this XML evolves it could make the test fail for reasons unrelated to gradient stops. If it ever becomes noisy, consider narrowing the count to items within the gradient block (e.g. by substringing around<gradient…</gradient>before splitting).
464-491: Round‑trip gradient validations are solid; alignassertEqualsargument order for clarityThe added checks in the gradient‑focused round‑trip tests (
ic_full_qualified,ic_compose_color_*_gradient,ic_linear_radial_gradient_with_offset) give good signal that:
<aapt:attr>and<gradient>are preserved in the output.- All expected paths are present.
- The number of gradient definitions matches the number of gradient paths (e.g. 16 gradients for 16 paths).
Two small test‑hygiene suggestions:
In several places you call
assertEquals(pathCount, 3, ...)/assertEquals(pathCount, 16, ...)(Lines 470, 483, 500). In Kotlin the first argument is the expected value and the second is the actual, so swapping them toassertEquals(3, pathCount, ...)/assertEquals(16, pathCount, ...)will yield clearer failure messages and match the style used elsewhere in this file.For
gradientCountin theic_linear_radial_gradient_with_offsettest (Lines 533‑534), you only assert> 0. If the underlying asset’s gradient count is stable, tightening this to an exactassertEqualswould catch accidental extra/missing gradient definitions; if you purposely want flexibility here, the current check is fine.Also applies to: 494-507, 511-535
594-632: New linear gradient IR → XML test covers key attributes wellThis test exercises the new
IrFill.LinearGradientpath nicely: you validate that the aapt namespace is present, thatandroid:fillColoris expressed via<aapt:attr>, that a<gradient android:type="linear">is emitted with the right geometry and start/end colors, and that the</aapt:attr>wrapper is closed.If you ever need to harden it further, you could assert that
xmlns:aaptappears on the<vector>element (not just somewhere in the document), but for now this provides good coverage of the intended behavior.
672-708: Multi‑stop linear gradient test effectively validates color stops; consider reusing aapt/namespace checksThis test nicely verifies:
android:type="linear"is used.- All three colors are present with their expected offsets.
- The number of
<item>elements is exactly 3.That gives good coverage of the multi‑stop mapping. Since this is also going through the gradient machinery, you might optionally add the same
xmlns:aapt/<aapt:attr name="android:fillColor">assertions you use in the single‑gradient tests for consistency, but it’s not strictly necessary given those behaviors are already validated elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sdk/core/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/core/xml/VectorDrawable.kt(3 hunks)sdk/generator/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGeneratorTest.kt(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: t-regbs
Repo: ComposeGears/Valkyrie PR: 587
File: sdk/generator/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/XmlSerializer.kt:13-16
Timestamp: 2025-09-27T22:19:48.762Z
Learning: The VectorDrawable.Child sealed interface in the Valkyrie project only has Group and Path as implementations. ClipPath is not implemented in this project, despite being supported in Android's official VectorDrawable specification.
📚 Learning: 2025-09-27T22:19:48.762Z
Learnt from: t-regbs
Repo: ComposeGears/Valkyrie PR: 587
File: sdk/generator/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/XmlSerializer.kt:13-16
Timestamp: 2025-09-27T22:19:48.762Z
Learning: The VectorDrawable.Child sealed interface in the Valkyrie project only has Group and Path as implementations. ClipPath is not implemented in this project, despite being supported in Android's official VectorDrawable specification.
Applied to files:
sdk/core/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/core/xml/VectorDrawable.kt
🧬 Code graph analysis (1)
sdk/generator/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGeneratorTest.kt (1)
components/parser/kmp/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/kmp/xml/XmlToImageVectorParserTest.kt (1)
imageVector(496-502)
🔇 Additional comments (6)
sdk/core/xml/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/core/xml/VectorDrawable.kt (5)
11-12: AAPT namespace/prefix constants align with existing patternAdding
AAPT_NAMESPACE/AAPT_PREFIXmirrors the existing ANDROID constants and keeps the XML naming concerns nicely centralized; no changes needed here.
32-33: Gradient element name constants are consistent and reusable
GRADIENT/GRADIENT_ITEMfollow the same pattern asGROUP/PATH/CLIP_PATHand de‑duplicate the literal strings across annotations and logic; looks good.
55-61: GradientItem shape matches typical VectorDrawable expectations
coloras a non‑nullableStringandoffsetasFloatare a straightforward mapping for<item android:color="" android:offset=""/>; no issues from a modeling perspective.
63-69: AaptAttr is a focused model for gradient-based attributes
AaptAttrcorrectly captures<aapt:attr name="…">plus an optionalGradient; this keeps AAPT concerns scoped and avoids polluting the main Path with more fields. Looks appropriate for the current gradient use case.
103-103: Path.aaptAttr integration is minimal and source-compatibleAdding
aaptAttr: AaptAttr? = nullat the end of the Path constructor keeps source compatibility for most callers and cleanly tucks the gradient machinery behind a single optional field; the shape looks good.sdk/generator/xml/src/commonTest/kotlin/io/github/composegears/valkyrie/sdk/generator/xml/IrToXmlGeneratorTest.kt (1)
634-670: Radial gradient IR → XML test is comprehensiveThe radial gradient test mirrors the linear case well: it checks the aapt namespace,
name="android:fillColor",android:type="radial", center coordinates,gradientRadius, and start/end colors. This should catch most regressions in radial gradient generation without being overly strict.No issues from my side here.
2 participants
Uh oh!
There was an error while loading. Please reload this page.