Move the original/optimised switch onto the canvas, and add a diff view - #12
Merged
Merged
Conversation
It was never a setting. It changes no byte of SVGO's output — which is why it sat in `unfingerprinted`, why `_saveSettings()` stripped it before persisting, and why `collectNotes()` needed a special case to silence every notice while it was on. A lens in a panel of things that produce the result, and a boolean that cannot express the third state a diff view needs. It becomes `ui/view-mode.js`: a self-created radio group at the top left of `.output`, outside the `.output-switcher` subtree so `SvgOutput`'s pan gestures can't swallow it. `MainController` keeps the latest result as `_resultItem` beside `_inputItem` and composes the two controls in `_renderOutput()`, which memoises the pair it last rendered. Two behaviour changes fall out of it, both deliberate: - Optimisation now runs while the original is showing. The `if (settings.original)` short-circuit in `_compressSvg()` is gone, so showing the original no longer buys silence on a slow file. That is the price of the mode being a lens rather than a switch on the pipeline. - The size readout always reports input → output. It used to collapse to a single number in original mode; the numbers describe the settings, not what is being looked at. Collision notices consequently hold in every mode, so the gate in `collectNotes()` and the `compareToFile` ternary in `_updateForFile()` both go. Removing the field from `globalFields` was fingerprint-neutral — the default fingerprint literal in `test/settings-model.test.js` is unchanged on purpose, which is what proves every result cached in the wild stayed valid. No migration: `SettingsModel.set()` ignores names it doesn't have, so a legacy payload carrying `original` passes through. The panel's View group had only `gzip` left in it, so that moves to the top of Output and the heading goes.
A third segment on the canvas control, composing with the toolbar's Preview/Markup toggler rather than replacing it: `Output` grows from two output types to four, and `MainController._renderOutput()` composes the pair of controls into a type and the file (or files) that go into it. The toggler is untouched — no coupling, no programmatic writes into it. In Preview it is a visual diff: both files rasterised into one box and compared with `pixelmatch`, on its own `PanZoom` so the checkerboard, centring and gestures match the preview it stands in for. The box comes from the *input* file, since pixelmatch compares buffers of equal length — when an optimisation changes the intrinsic size, the result landing elsewhere inside that box is the honest picture of that change. The rasterise half of `png-button.js` is factored out into `ui/rasterize.js` rather than written twice; Firefox's refusal to draw an SVG with no intrinsic size was already solved there. In Markup it is a line diff, and its design point is that with Prettify off — the default — the optimised markup is one line, so a naive line diff says "everything changed" and is worthless. Both sides are reflowed to one element per line, unconditionally, so the diff reads the same however that toggle moves; the view says so in a line of its own, since the reflow is cosmetic and reaches nothing that is exported, copied, measured, cached or fingerprinted. jsdiff runs bounded by `maxEditLength` because this is the main thread, and past the bound it degrades to a trimmed prefix and suffix around one removed and one added block rather than refusing. Two libraries, both bundled like svgo and css-tree with no gulpfile change: pixelmatch (ISC) and diff (jsdiff, BSD-3). Two fixes fall out of the work. `Output` now records its files synchronously in `set()` and clears them in `reset()`: its switch queue means a later `update()` can land mid-switch, and a type switched into between a new file and its first result would otherwise re-render the file that was just reset away. And `.code-output` gains the top padding that clears the view mode switch — the control landed over its first line in the previous commit.
ericges
force-pushed
the
feat/canvas-view-modes-and-diff
branch
from
August 18, 2026 16:31
e0efa00 to
55bb40b
Compare
…oots` Account for new bundled dependencies introduced by the diff view feature. Align `NOTICE.md` and tests to include missing `pixelmatch` and `diff` entries, ensuring accurate license declarations and compliance.
… `setting-reset` button design, and improve ripple effect transparency.
… in results and toasts styles.
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.
"Show original" was never a setting. It changes no byte of SVGO's output — which is why it lived in
unfingerprinted, why_saveSettings()stripped it before persisting, and whycollectNotes()needed a special case to silence every collision notice while it was on. It was a lens sitting in a panel of things that produce the result, and a boolean can't express the third state a diff needs.Two commits: the refactor first, green and hand-verifiable on its own, then the feature.
1. The view mode
src/js/page/ui/view-mode.jsis a self-created radio group at the top left of.output, outside the.output-switchersubtree soSvgOutput's pan gestures can't swallow it. It emitschangewith{ value };MainControllerowns what a mode means, keeping the latest result as_resultItembeside_inputItemand composing the two controls in_renderOutput().Two behaviour changes, both deliberate
if (settings.original)short-circuit in_compressSvg()is gone, so every settings change optimises whatever is on screen. Showing the original used to buy silence on a slow file; that's the price of the mode being a lens rather than a switch on the pipeline.Collision notices consequently hold in every mode, so the gate in
collectNotes()and thecompareToFileternary in_updateForFile()both go.Removing the field was fingerprint-neutral — it was excluded from the fingerprint all along, so every result cached in the wild stays valid. The default fingerprint literal in
test/settings-model.test.jsis deliberately unchanged, and that is what proves it. No migration either:SettingsModel.set()ignores names it doesn't have, so a legacy payload carryingoriginalpasses through untouched.The panel's View group had only
gzipleft in it, so that moves to the top of Output and the heading goes.2. Diff
A third segment composing with the toolbar rather than replacing it — the toggler is untouched, no coupling and no programmatic writes into it.
Outputgrows from two output types to four:imagerenderDiffcodemarkupDiffVisual diff: both files rasterised into one box and compared with
pixelmatch, on its ownPanZoom. The box comes from the input file, since pixelmatch compares buffers of equal length — an optimisation that changes the intrinsic size then lands elsewhere inside that box, which is the honest picture of that change. The rasterise half ofpng-button.jsis factored intoui/rasterize.jsrather than written twice. The anti-aliasing threshold is tuned against measurements on the demos, recorded in the constant's comment: at pixelmatch's stock0.1the tiger leaves 322 stray red pixels on a default load and the flag 37; at0.2they leave 25 and 8, while precision 0 on the tiger still lights up 943.Markup diff: with Prettify off — the default — the optimised markup is one line, so a naive line diff says "everything changed" and is worthless. Both sides are reflowed to one element per line, unconditionally, so the diff reads identically however that toggle moves (verified: same 554 rows and 247 runs either way). The reflow is cosmetic and reaches nothing that is exported, copied, measured, cached or fingerprinted, and the view says so in a line of its own. jsdiff runs bounded by
maxEditLengthbecause this is the main thread; past the bound it degrades to a trimmed prefix and suffix around one removed and one added block rather than refusing.Row type is carried by a
+/-/space gutter character as well as a tint — colour alone reaches nobody who can't tell the two apart. No syntax highlighting: Prism highlights a document, not a diff of two.Dependencies
pixelmatch7.2.0pngjsdep is CLI-only). Takes two equal-sized buffers and writes the highlight image.diff9.0.0diffLines, rather than a hand-rolled Myers implementation.Both bundle through Rollup like
svgoandcss-tree, with no gulpfile change.Two fixes that fell out
Outputnow records its files synchronously inset()and clears them inreset(). Its switch queue means a laterupdate()can land mid-switch, and a type switched into between a new file and its first result would otherwise re-render the file that was just reset away..code-outputgains the top padding that clears the new control, which landed over its first line in commit 1.Verified by hand
In
npm run dev, service worker cleared first:Worker.prototype.postMessage) — a mode change re-renders from the files in hand..svg-containertransform is byte-identical across the switch while the iframesrcchanges.maxEditLengthbail and degrades to exactlycontext(1), remove(2500), add(2500), context(1)without hanging.originalkey.One thing worth recording: the first switch between output types takes ~1s because
transitionToClass()loses itstransitionendrace and waits out the timeout. That is pre-existing — I measured 1034ms for the same switch onorigin/main— so it is left alone here.CLAUDE.mdandREADME.mdare updated with the PR, not after it.