Split SvgRenderer into multiple functions #213
Merged
Conversation
This file contains 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
adroitwhiz
commented
Feb 5, 2021
As far as I can tell, no public Scratch repository is calling this.
This will allow us to make these methods static / move them into a separate "fixup" function
The VM uses the SvgRenderer solely to convert SVGs from version 2. This allows the VM to simply import loadSvgString without bringing in the entire SvgRenderer along with it. The next commit will move toString to a separate file as well, so the VM's dependency on SvgRenderer can be entirely removed.
I may be going a bit overboard with the exports here.
The details of what "normalizing" an SVG does should be easily viewable to those using the publicly exported `loadSvgString` function.
adroitwhiz
force-pushed
the
split-svg-renderer
branch
from
1eab850
to
51c80da
Compare
fsih
reviewed
Feb 22, 2021
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.
Depends on #212
Resolves
Resolves #211
Proposed Changes
This PR splits up the varied functionality of
SvgRenderer(quirks-mode SVG conversion, SVG parsing, SVG serialization) into multiple individually exported functions, and deprecates theSvgRendererclass.Reason for Changes
The
SvgRendererclass previously contained a lot of disparate functionality, most of which was called from one specific part of the codebase (e.g. the VM's costume loading called into the "quirks mode conversion" part, the renderer called the drawing part).This PR separates all that functionality into different separately-exported parts:
loadSvgStringloads an SVG string into an element, normalizing it (and optionally doing quirks-mode conversion).serializeSvgToStringreplacesSvgRenderer.toString, serializing an SVG element and optionally inlining fonts. I'm not sure if this wafer-thin wrapper aroundXMLSerializerneeds to live here--it's called once byV2SVGAdapterwhen converting a V2 SVG string (without inlining fonts) and once in the renderer when setting anSVGSkin(inlining fonts).V2SVGAdapteris a simple wrapper function that loads an SVG string with "quirks mode" then re-serializes it. Despite being a one-liner, it should probably be here because a few different places in the codebase attach it (including tests in the renderer, VM, and GUI).SVGin caps--the convention seems to be varied butV2SVGAdapteris what theSvgRendererclass was imported as in most tests, despiteSvgRenderer,inlineSvgFonts, andSvgElementusing theSvgconvention. Should I rename this toV2SvgAdapterto standardize this?Functionality relating to actually, well, rendering the SVG will be moved back to the
SVGSkinclass inscratch-render, since that's the only place it's ever used.Test Coverage
Tested manually :/