feat: Render referenced elements in use tags#238
Open
theminecoder wants to merge 3 commits intomeyfa:mainfrom
Open
feat: Render referenced elements in use tags#238theminecoder wants to merge 3 commits intomeyfa:mainfrom
theminecoder wants to merge 3 commits intomeyfa:mainfrom
Conversation
meyfa
requested changes
Nov 6, 2024
Owner
There was a problem hiding this comment.
Thanks for the contribution! I've left some ideas to further improve this.
The most important thing I would like to have is a check for cycles. Currently, the <use> tag could even reference itself, or reference a parent node, which would cause infinite recursion.
What makes this complicated are cases like:
<use id="foo" href="#bar" />
<use id="bar" href="#foo" />One solution I can think of would be:
- Store an array of SVGUse elements on the SVGRasterizer (initially empty)
- At the beginning of
SVGUse::rasterize(), check if$thisis part of the array; if yes, return; if no, add it to the array, and continue as usual. - At the end of
SVGUse::rasterize(), remove$thisfrom the array.
| $element = $this->getHref(); | ||
| if(empty($element)) return; | ||
|
|
||
| /** @var SVGDocumentFragment $root */ |
Owner
There was a problem hiding this comment.
Please remove this doc comment
Suggested change
| /** @var SVGDocumentFragment $root */ |
| do { | ||
| $root = $this->getParent(); | ||
| } while ($root->getParent() != null); | ||
| $element = $root->getElementById(substr($element, strpos($element, "#") + 1 ?: 0)); |
Owner
There was a problem hiding this comment.
- Please avoid reusing variables, especially when the type changes (
$elementwasstringbefore this line, but is now a node object). Maybe rename thestringvariable to$hrefto reduce confusion. strpos($element, "#") + 1 ?: 0is problematic - if the string is not found,strposreturnsfalse, andfalse + 1makes little sense.
However, it seems to me that the href always has to start with #, and so this can (and should) be rewritten:
- At the start of the function, if
$hrefdoes not start with#, return. - Otherwise, remove the first character, and use that for
getElementById.
Co-authored-by: Fabian Meyer <3982806+meyfa@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
<use>elements lists to reference another element by id. This enables the codebase to render such element when rasterising.I am aware this is not completely to spec in regards to x,y,width,height but this does the base job for when transforms are at play, which is what my current case exposes. I may look into that in another pr