Add source metadata for SVG model commands#513
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3193d50324
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| if (node.LocalModel is { } localModel) | ||
| { | ||
| ApplySourceMetadata(localModel, node, overwrite: false); |
There was a problem hiding this comment.
When LocalModel is a text model, it can contain draw commands for multiple child text elements such as <tspan id="a"> and <tspan id="b">; SvgSceneTextCompiler records those runs directly from each run's StyleSource, but the recorder never pushes per-run source metadata. Backfilling the whole local model with the outer node here means FindCommandsBySourceElementId("a") returns nothing, or all commands are attributed to the parent <text> instead of the child element, which breaks the advertised element-targeted editing for common text SVGs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 3db12bb.
This now records source metadata at the text-run level while SvgSceneTextCompiler records local text-model pictures. The compiler pushes the resolved SvgTextBase or parent anchor source around each run, using the same element-address callback as the scene compiler, so commands from child runs such as <tspan id="a"> and <tspan id="b"> carry their own SourceElementId, address, and type. The existing renderer backfill remains a fallback only (overwrite: false), so parent <text> metadata no longer replaces metadata already attached to child-run commands.
I also added regression coverage in SKSvgRebuildFromModelTests.ModelCommands_PreserveChildTextRunSourceMetadata: it checks that FindCommandsBySourceElementId("a") and "b" return the correct DrawTextCanvasCommand text, that the commands are typed as SvgTextSpan, and that text-root does not claim the child run draw commands.
Verification:
dotnet format Svg.Skia.slnx --no-restore --exclude externalsdotnet build Svg.Skia.slnx -c Releasedotnet test Svg.Skia.slnx -c Release
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3db12bb291
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| canvas.SaveLayer(simpleFilterPaint); | ||
| DrawPositionedTextPathRun(run, geometryBounds, ignoreAttributes, canvas, assetLoader, includeFill: true, includeStroke: true, includeDecorations: true); | ||
| DrawPositionedTextPathRun(run, geometryBounds, ignoreAttributes, canvas, assetLoader, getElementAddressKey, includeFill: true, includeStroke: true, includeDecorations: true); |
There was a problem hiding this comment.
When a filtered <textPath> run takes this branch, the surrounding Save, optional ClipRect, SaveLayer, and Restore commands are emitted outside the PushTextCommandSource scope created inside DrawPositionedTextPathRun; the complex filter branch below has the same gap around canvas.DrawPicture(sourceGraphic). This leaves the filter layer commands with null source metadata, so FindCommandsBySourceElementId cannot select or edit the full command set for that text-path element and source-targeted removals can leave orphan layer commands behind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 5f3ba90.
The filtered text-path path now pushes the text run source metadata before emitting the filter wrapper commands. That puts the Save, optional ClipRect, SaveLayer, DrawPicture for the complex filter path, and matching Restore commands under the same source scope as the text-path run. The existing per-run source metadata inside DrawPositionedTextPathRun is still preserved for the nested glyph draw commands, but source-targeted editing now also sees the surrounding layer commands for the <textPath> element instead of leaving them untagged.
I added regression coverage for both branches:
ModelCommands_PreserveSimpleFilteredTextPathWrapperSourceMetadatacovers the simplefeGaussianBlur in="SourceGraphic"path and asserts the wrapperSave,ClipRect,SaveLayer, andRestorecommands are tagged aspath-text/SvgTextPath.ModelCommands_PreserveComplexFilteredTextPathWrapperSourceMetadatacovers the general filter path withfeOffsetand additionally asserts the outerDrawPicturecommand is tagged with the same text-path source metadata.
Verification:
dotnet test tests/Svg.Skia.UnitTests/Svg.Skia.UnitTests.csproj -f net10.0 -c Release --no-restore --filter "FullyQualifiedName~SKSvgRebuildFromModelTests.ModelCommands_Preserve"dotnet format Svg.Skia.slnx --no-restore --exclude externalsdotnet build Svg.Skia.slnx -c Releasedotnet test Svg.Skia.slnx -c Release
1 participant
PR Summary: Source Metadata for Model Commands
Background
Discussion #493 asked whether commands generated in
SKSvg.ModelorRebuildFromModel()can be traced back to the SVG elements that produced them. The main use case is targeted editing, such as changing one path's fill, without reparsing the SVG document or manually matching draw commands back to source elements.Changes
CanvasCommand:SourceElementIdSourceElementAddressSourceElementTypeNameSKCanvas.PushCommandSource(...)so renderers can scope subsequently recorded commands to the currently rendered source node.AddCommand(...)helper, which applies the active source metadata to commands as they are appended.CanvasCommand.DeepClone().FindCommandsBySourceElementId(...)FindCommandsBySourceElementId<TCommand>(...)FindCommandsBySourceElementAddress(...)FindCommandsBySourceElementAddress<TCommand>(...)SvgSceneRendererto push source metadata while rendering scene nodes and document background commands.SKPicturemodels generated for text and images when nested commands do not already carry source metadata.README.md.Review Fix
During review,
SKCanvas.DeepClone()was adjusted so cloned canvases do not copy the active transient command-source scope. Cloned commands still preserve their existing metadata, but future commands recorded on the cloned canvas no longer inherit a stale source scope that cannot be unwound.Tests
Validation
dotnet format Svg.Skia.slnx --no-restore --include README.md src/ShimSkiaSharp/SKCanvas.cs src/ShimSkiaSharp/Editing/SKPictureEditingExtensions.cs src/Svg.SceneGraph/SvgSceneRenderer.cs tests/ShimSkiaSharp.UnitTests/CloneCommandTests.cs tests/ShimSkiaSharp.UnitTests/EditingHelpersTests.cs tests/ShimSkiaSharp.UnitTests/SKCanvasTests.cs tests/Svg.Skia.UnitTests/SKSvgRebuildFromModelTests.csdotnet test tests/ShimSkiaSharp.UnitTests/ShimSkiaSharp.UnitTests.csproj -c Release --filter "FullyQualifiedName~SKCanvasTests|FullyQualifiedName~EditingHelpersTests|FullyQualifiedName~CloneCommandTests"dotnet test tests/Svg.Skia.UnitTests/Svg.Skia.UnitTests.csproj -f net10.0 -c Release --filter "FullyQualifiedName~SKSvgRebuildFromModelTests"dotnet build Svg.Skia.slnx -c Releasedotnet test Svg.Skia.slnx -c Releasegit diff --checkNotes
The full build and test run completed successfully. The build still reports existing package vulnerability, nullable, and obsolete API warnings, but no errors.