refactor: move rules computation to SvgRootNode#107
Conversation
fix: apply opacity when fill-opacity or stroke-opacity is not present fix: gradient not considering viewport translation when gradientUnits == "objectBoundingBox
WalkthroughThe pull request introduces comprehensive modifications across multiple SVG-related Kotlin files in the svg-to-compose project. The primary focus is on enhancing the handling of opacity and alpha values for various SVG node types, including circles, ellipses, paths, and polygons. Additionally, the changes involve restructuring CSS rule processing, modifying gradient coordinate calculations, and improving style resolution mechanisms. The modifications aim to create more robust and flexible SVG-to-Compose rendering capabilities. Changes
Sequence DiagramsequenceDiagram
participant SVGNode as SVG Node
participant StyleResolver as Style Resolver
participant RuleProcessor as Rule Processor
participant RenderEngine as Render Engine
SVGNode->>StyleResolver: Resolve attributes from style
StyleResolver->>RuleProcessor: Process computed rules
RuleProcessor-->>StyleResolver: Return processed rules
StyleResolver->>SVGNode: Update node attributes
SVGNode->>RenderEngine: Render with resolved attributes
Possibly Related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgNode.kt (1)
146-155: Consider adding a brief KDoc for the ComputedRule class.
The code overall looks sound, with the compareTo implementation using CSS specificity for sorting. To further enhance maintainability, provide a short KDoc explaining the purpose of this class and how it impacts rule application.svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgRootNode.kt (1)
241-259: Efficient approach for CSS style resolution.The logic in resolveStyleTags() leverages Kotlin’s functional style effectively. Potential performance improvements could come from caching selectors or from early filtering of relevant rules. However, this is already a clear and maintainable solution.
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgPolygonNode.kt (1)
Line range hint
13-26: Consider enhancing point parsing robustnessThe point parsing logic could benefit from additional error handling to gracefully handle malformed input. Consider:
- Adding documentation for the regex pattern
- Implementing validation for the number of coordinates
- Adding error handling for invalid numeric values
Example implementation:
val points by attribute<String?, List<Point2D>> { points -> if (points.isNullOrBlank()) { emptyList() } else { + try { listOfPointsRegex .findAll(points) .map { it.value } .windowed(size = 2, step = 2) + .onEach { if (it.size != 2) throw IllegalArgumentException("Invalid coordinate pair") } .map { (x, y) -> Point2D(x.toFloat(), y.toFloat()) } .toList() + } catch (e: NumberFormatException) { + throw IllegalArgumentException("Invalid coordinate value in points: $points", e) + } } }svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgPolylineNode.kt (1)
Line range hint
1-85: Architecture feedback: Well-structured implementationThe implementation demonstrates good software engineering practices:
- Clear separation of concerns between path building and node creation
- Efficient point processing using buildList
- Proper parameter handling and transformation
- Consistent with Compose's vector graphics model
Consider adding KDoc documentation to describe the behavior of opacity inheritance for future maintainers.
svg-to-compose/src/jvmMain/kotlin/Main.kt (1)
22-23: Consider adding file existence validation.The switch to
Fileclass provides more flexibility for testing different SVG files. However, consider adding validation to ensure the SVG file exists before processing.val suffix = "css/flower.svg" + val svgFile = Paths.get("samples/svg/$suffix") + require(Files.exists(svgFile)) { "SVG file not found: $svgFile" } val (pkg, path, output) = SampleFile.Svg.File( SampleAppPackage("dev.tonholo.svg_to_compose.playground.ui.icon.jvm"), suffix, )svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/gradient/SvgGradient.kt (2)
44-44: Consider adding a safe cast check for SvgRootNodeThe direct cast to
SvgRootNodecould throw aClassCastExceptionif the hierarchy is unexpected. Consider using a safe cast with error handling:- .onEach { it.resolveAttributesFromStyle((rootParent as SvgRootNode).rules) } + .onEach { + (rootParent as? SvgRootNode)?.let { root -> + it.resolveAttributesFromStyle(root.rules) + } ?: throw IllegalStateException("Expected SvgRootNode as root parent") + }
75-77: Consider reducing duplication in coordinate calculationsThe X and Y coordinate calculation methods share similar logic. Consider extracting the common pattern:
private fun calculateGradientCoordinate( length: SvgLength, target: List<PathNodes>, getBoundingBoxDimension: (BoundingBox) -> Float, getBoundingBoxOffset: (BoundingBox) -> Float, getViewportTranslation: (SvgRootNode) -> Float, getViewportDimension: (SvgRootNode) -> Float, ): Float { val root = rootParent as SvgRootNode return if (gradientUnits == "objectBoundingBox") { check(target.isNotEmpty()) val boundingBox = target.boundingBox() length.toFloat(getBoundingBoxDimension(boundingBox)) + getBoundingBoxOffset(boundingBox) } else { val translation = getViewportTranslation(root) val baseDimension = getViewportDimension(root) translation + length.toFloat(baseDimension) } }This would allow the X and Y methods to be simplified to:
internal fun calculateGradientXCoordinate(...) = calculateGradientCoordinate( length, target, { it.width.toFloat() }, { it.x.toFloat() }, { -it.viewportX }, { it.viewportWidth } )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
samples/svg/css/flower.svgis excluded by!**/*.svg,!**/samples/**
📒 Files selected for processing (17)
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgCircleNode.kt(2 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgClipPath.kt(0 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgColor.kt(1 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgElementNode.kt(0 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgEllipseNode.kt(1 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgGraphicNode.kt(1 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgGroupNode.kt(0 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgNode.kt(2 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgPathNode.kt(1 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgPolygonNode.kt(1 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgPolylineNode.kt(1 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgRectNode.kt(1 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgRootNode.kt(5 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgStyleNode.kt(1 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/gradient/SvgGradient.kt(3 hunks)svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/parser/ImageParser.kt(1 hunks)svg-to-compose/src/jvmMain/kotlin/Main.kt(1 hunks)
💤 Files with no reviewable changes (3)
- svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgElementNode.kt
- svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgClipPath.kt
- svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgGroupNode.kt
🔇 Additional comments (17)
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgEllipseNode.kt (1)
48-48: LGTM! Correct implementation of opacity fallback mechanism
The changes properly implement the opacity fallback mechanism using the Elvis operator, ensuring that when fillOpacity or strokeOpacity is not present, the general opacity value is used as a fallback. This aligns with SVG specifications and fixes the opacity application issue mentioned in the PR objectives.
Also applies to: 51-51
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgNode.kt (1)
10-10: No issues with newly introduced import.
The imported CssSpecificity is properly used by the ComputedRule class, so this addition is valid and necessary.
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgRootNode.kt (4)
15-17: Imports look valid and self-contained.
It makes sense to import CSS-related classes for rule parsing. No issues found here.
97-97: Making viewportX public might be intentional, but verify external usages.
Now that this property is publicly exposed, ensure that any external dependencies can safely interact with the property. Also confirm whether renaming “viewportX” to something like “xOffset” or “viewBoxMinX” could add clarity.
107-107: Same consideration applies to viewportY.
With viewportY becoming public, verify if external code references or modifies it. Ensure consistent naming for better code readability.
200-200: Adding 'rules' property is well-structured.
Declaring rules as a lazily computed property is clean and ensures styles are only processed when needed. This is a good approach for performance and clarity.
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgPathNode.kt (1)
66-66: LGTM! Proper implementation of opacity fallback mechanism
The changes correctly implement the fallback mechanism for opacity values, ensuring that when specific opacities (fillOpacity, strokeOpacity) are not present, the general opacity value is used. This aligns with SVG specifications and fixes the opacity application issue.
Also applies to: 69-69
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgPolygonNode.kt (1)
52-52: LGTM! Proper opacity fallback implementation
The changes correctly implement the fallback mechanism for opacity values, ensuring that when specific opacity values (fillOpacity and strokeOpacity) are not present, the general opacity value is used instead. This implementation:
- Aligns with SVG specifications
- Uses idiomatic Kotlin null coalescing
- Maintains consistency across the codebase
Also applies to: 55-55
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgCircleNode.kt (1)
90-93: LGTM: Proper opacity fallback implementation
The changes correctly implement the opacity fallback mechanism, ensuring that when fillOpacity or strokeOpacity are not present, the general opacity value is used. This aligns with SVG specifications and fixes the opacity application issue mentioned in the PR objectives.
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgStyleNode.kt (1)
20-21: Excellent improvement in content retrieval robustness!
The new implementation correctly handles multiple text nodes by concatenating their contents, which is more robust than the previous approach of taking only the first text node. This change ensures that no style content is lost when the style node contains multiple text nodes, which is a valid XML structure.
Let's verify if there are any SVG files in the codebase that actually benefit from this change:
✅ Verification successful
Excellent improvement in content retrieval robustness - verified with real use cases!
The search results confirm that this change is crucial for the library's functionality. The codebase contains multiple SVG files with complex style tags that span multiple lines and contain various CSS rules, comments, and whitespace. For example:
samples/svg/css/flower.svg: Contains multi-line styles with comments and multiple CSS rulessamples/svg/css/mountain-with-clouds.svg: Has extensive styling with multiple class definitionssamples/svg/css/house-with-trees.svg: Includes complex styling with hover states and comments
The new implementation using filterIsInstance and fold is essential for correctly handling these real-world SVG files, as it properly concatenates all text content including whitespace, comments, and CSS rules spread across multiple text nodes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find SVG files with style tags containing multiple text nodes
# Expected: Files containing style tags with line breaks or multiple text content
# Search for SVG files with style tags spanning multiple lines
rg -U --type xml "<style[^>]*>(\n|.)*?</style>" -g "*.svg" -A 2
Length of output: 6219
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgRectNode.kt (1)
53-53: LGTM! The opacity fallback implementation looks correct.
The changes properly implement the fallback mechanism for opacity values:
fillAlpha = fillOpacity ?: opacityensures that whenfillOpacityis null, it falls back to the generalopacitystrokeAlpha = strokeOpacity ?: opacitydoes the same for stroke opacity
This aligns with the PR objective to fix opacity application when specific opacity values are not present.
Let's verify the opacity inheritance behavior across other SVG node types:
Also applies to: 56-56
✅ Verification successful
Opacity fallback implementation is consistently applied across all SVG node types
The verification confirms that the opacity fallback pattern fillOpacity ?: opacity and strokeOpacity ?: opacity is uniformly implemented across all SVG node types:
- SvgCircleNode
- SvgPolylineNode
- SvgPathNode
- SvgEllipseNode
- SvgRectNode
- SvgPolygonNode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the opacity fallback pattern is consistently implemented across other SVG node types
# Expected: Similar opacity fallback implementations in other node files
# Search for opacity assignments in SVG node files
rg -U "(?:fill|stroke)Alpha.*=.*opacity" --type kotlin
Length of output: 1773
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgPolylineNode.kt (1)
27-27: LGTM: Proper fallback handling for opacity values
The implementation correctly addresses the opacity inheritance by using the Elvis operator to fallback to the general opacity value when specific opacity values (fillOpacity, strokeOpacity) are not present. This aligns with SVG specifications where general opacity should be used as a fallback.
Let's verify if this pattern is consistently applied across other SVG node types:
Also applies to: 30-30
✅ Verification successful
Opacity fallback pattern is consistently implemented across SVG nodes
The verification shows that the opacity fallback pattern using the Elvis operator (?:) is consistently implemented across various SVG node implementations. All nodes follow the same pattern where:
fillAlpha = fillOpacity ?: opacitystrokeAlpha = strokeOpacity ?: opacity
This confirms that the implementation in SvgPolylineNode.kt follows the established pattern in the codebase and correctly handles opacity inheritance according to SVG specifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the opacity fallback pattern is consistently implemented across SVG nodes
# Expected: All SVG node files should use the same opacity fallback pattern
# Search for asNode functions in SVG node files and check their opacity handling
ast-grep --pattern 'fun $_Node.asNode($_) {
$$$
fillAlpha = $_ ?: opacity
$$$
strokeAlpha = $_ ?: opacity
$$$
}'
Length of output: 124
Script:
#!/bin/bash
# Let's try a different approach to find opacity handling patterns
# Search for files containing SVG node implementations
fd "Svg.*Node.kt" --exec rg -A 2 -B 2 "(?:fill|stroke)(?:Alpha|Opacity|opacity)" {}
Length of output: 4023
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/parser/ImageParser.kt (1)
166-166: Verify the rules computation delegation to SvgRootNode
The change correctly delegates the rules computation to SvgRootNode by using svg.rules instead of maintaining rules locally. This aligns with the PR objective of moving rules computation and should improve opacity handling.
Let's verify the rules computation in SvgRootNode and its impact on opacity:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Verified: Rules computation is correctly delegated to SvgRootNode
The change correctly moves rules computation to SvgRootNode, which lazily resolves style tags through the rules property. The opacity handling is properly implemented across various SVG nodes (SvgGraphicNode, SvgCircleNode, SvgPathNode, etc.) with support for both general opacity and specific fill/stroke opacity attributes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the rules computation implementation in SvgRootNode
# and check for any opacity-related computations
# Check for rules computation in SvgRootNode
ast-grep --pattern 'class SvgRootNode {
$$$
val rules: $_
$$$
}'
# Check for opacity handling in the rules computation
rg -A 5 'opacity|fill-opacity|stroke-opacity' --type kotlin
Length of output: 17212
Script:
#!/bin/bash
# Let's verify the rules computation in SvgRootNode by checking its implementation
ast-grep --pattern 'class SvgRootNode {
$$$
fun computeRules($_) {
$$$
}
$$$
}'
# Also check for any rules-related code in SvgRootNode
rg -A 5 'rules' --type kotlin --glob 'SvgRootNode.kt'
Length of output: 843
Script:
#!/bin/bash
# Let's check the implementation of resolveStyleTags method and ComputedRule class
ast-grep --pattern 'private fun resolveStyleTags() {
$$$
}'
# Check for ComputedRule class definition
ast-grep --pattern 'class ComputedRule {
$$$
}'
# Also check for any style tag handling
rg -A 5 'style.*tag' --type kotlin
Length of output: 5250
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/gradient/SvgGradient.kt (1)
59-61: LGTM! Verify viewport translation behavior
The viewport translation handling looks correct and aligns with the PR objectives. The translation is properly applied only when not in objectBoundingBox mode.
Let's verify the usage of viewportX in the codebase:
✅ Verification successful
Let me check the usage of viewportX in the context of transformations to ensure consistency.
Viewport translation implementation is consistent
The viewport translation handling in the gradient coordinate calculation aligns perfectly with the root node transformation logic. Both locations correctly apply the negative viewportX translation:
- In SvgRootNode.kt:
transform = "translate(${-viewportX}, ${-viewportY})" - In SvgGradient.kt:
val translateX = -root.viewportX
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of viewportX to ensure consistent translation handling
rg "viewportX" --type kotlin
Length of output: 672
Script:
#!/bin/bash
# Search for transform-related code to verify viewport translation consistency
ast-grep --pattern 'transform = $_'
Length of output: 186
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgColor.kt (1)
11-11: Verify the "Color.Transparent" string format compatibility.
The string "Color.Transparent" doesn't follow the hexadecimal format used by other colors in the namedColors map. This might cause issues in toComposeColor() and toBrush() methods.
Let's verify the usage of this color format in the codebase:
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgGraphicNode.kt (2)
28-28: LGTM: Making opacity inheritable aligns with SVG spec
The addition of inherited = true to the opacity attribute is correct as per SVG specifications where opacity should cascade down the element tree.
28-28: Consider implementing opacity computation method
While opacity, fillOpacity, and strokeOpacity are correctly marked as inheritable, there's no visible implementation for computing the final opacity values when some values are missing. According to the PR objectives, this was one of the fixes needed.
Let's verify if this computation exists elsewhere:
Consider adding a protected method to compute final opacity values:
protected fun computeFinalOpacity(
opacity: Float?,
specificOpacity: Float?
): Float = when {
specificOpacity != null -> specificOpacity
opacity != null -> opacity
else -> 1.0f
}Also applies to: 30-30
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgCircleNode.kt
Show resolved
Hide resolved
svg-to-compose/src/commonMain/kotlin/dev/tonholo/s2c/domain/svg/SvgColor.kt
Outdated
Show resolved
Hide resolved
1 participant
fix: apply opacity when fill-opacity or stroke-opacity is not present
fix: gradient not considering viewport translation when gradientUnits == "objectBoundingBox
Summary by CodeRabbit
New Features
ComputedRuleclass for improved CSS rule handling.SvgRootNodeclass.Bug Fixes
Documentation
Chores