[US-1804] Object tree scalar values: one shared renderer, decoded text strings, binary carve-out - #20
Merged
Merged
Conversation
…ures and broken diff rows
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.
Description and links
A dictionary-entry scalar row in the object tree tells you a key exists and what type it is, and never what it says.
RowSpan numberis the whole problem: the cell has a row span, and the tree will not say it is 2. Answering a structural question means chasing object references one at a time withdump object --ref, or leaving for qpdf. JSON has the same hole and states it more plainly, since every scalar node carriesvalueTypeand novaluekey at all.Underneath that sat a second problem. Three copies of the same scalar switch existed:
scalarDisplay(tree.go) behind the tree and the diff,valueEntryFromObject(inspector.go) behind the object detail panel, andwriteScalar(objectsource.go) behinddump source. The detail panel does not callscalarDisplay, contrary to what the tree work would naturally assume, so any fix touching one of them leaves the tool saying different things about the same bytes on different screens.Jira: US-1804
Technical changes
internal/pdfcore/scalarrender.go, holding two renderings of the same object:scalarRaw(byte-exact, the oldscalarDisplaymoved verbatim) andscalarText(text strings decoded, everything else identical).semanticLabel,valueEntryFromObject,diffSummarizeand the newdiffCompareall route through them.writeScalaris deliberately left alone sodump sourcestays byte-frozen.TreeNodeaValueand aValueRaw, populated inbuildTreeNodeonly for dictionary-entry scalars.ValueRawis emitted from a content comparison, decoded text against the stored bytes with delimiters and escaping stripped, so an already-ASCII string emits no raw counterpart and its absence carries information rather than duplicating every node.dump treenow prints<key> <type> = <value>;--jsongains an additivevaluekey on scalar nodes andvalueRawwhere decoding changed the bytes. The JSON value is never truncated, because the point of--jsonis piping into jq.textstring.gointo the shared helper, so PDF text strings render decoded per ISO 32000-1 7.9.2.2 on all six renderers:dump tree,dump object,diff, the GUI TreePanel, DetailPanel and DiffView. SplitValueEntry.Display(decoded) fromValueEntry.Raw(byte-exact), which were previously written identically./Contentsand/Certbehind/Type /Sigor/DocTimeStampor a bare/ByteRange, and filespec/Params /CheckSumby name. They render as<binary, N bytes>rather than a hex blob, and the bytes stay reachable throughdump source --refanddump object --ref. A markup annotation's/Contentsis a genuine text string and still decodes.[truncated: N of M]marker, measured in runes so decoded UTF-16BE is never cut mid-character, and escaped control characters so one node is always exactly one output line. That also fixesdump object, where a/Altcontaining newlines rendered its continuation lines at column zero.flex-shrink-0, and the full value lives in the span'stitle. Addedfrontend/src/lib/escapeDisplayValue.tsas the GUI counterpart of the Go escaping, applied to TreePanel, DetailShared and DiffView.Considerations
Four shapes were on the table before this one. Decoding at the tree call site only, or inside
scalarDisplay, would each have been a much smaller change; both were argued from the assumption that the detail panel shared a renderer with the tree, and it does not, so both would have left the tool saying two or three different things about the same object. Shipping values with no decoding at all was internally consistent and cheaper, but then a UTF-16BE/Altreads as<FEFF0052...>in the primary inspection surface, and the encoding decision has to be made here anyway because the acceptance test must assert something for it.The consolidation is the part that made this worth doing as one change rather than three. The duplicated switch is exactly the drift risk that motivated a single formatter in the first place.
Two things turned up during review that are worth recording. Decoding nearly entered the diff's equality decision:
scalarLeaftook its changed/unchanged status by comparingdiffSummarizeon both sides, anddiffSummarizeis ascalarDisplaycaller, so<FEFF0041>and(A)would have compared equal and a real byte difference would have reportedunchanged. The comparison now runs throughdiffCompareon the byte-exact rendering while the displayed summaries decode. Separately, a literal string holding unescaped0x80-0xFFproduced invalid UTF-8, whichjson.Marshalrewrites to U+FFFD, destroying precisely what the raw counterpart exists to preserve;utf8Safefalls back to uppercase hex, the same policytextStringOrRawalready applied to decoded text.The plain-text line shape changes from
RowSpan numbertoRowSpan number = 2, which breaks an end-anchored grep such asgrep 'Type name$'. The only consumers are the CLI's own help text and thecli-tree-dumpandcli-object-querysuites, updated here as expected churn. Nothing is scripted against it in or out of the repo, and we are pre-1.0, so there is no compatibility shim.How was this tested?
go vet ./...clean,golangci-lint run0 issuesgo test ./...1049 passed across 13 packages;go test -race ./internal/pdfcore/ ./cmd/cli/928 passedscripts/test-all.shall per-module suites undertests/passed, including the newtests/object-tree-scalar-values/(51 black-box cases over the built binary) and the existingcli-tree-dump,cli-object-query,cli-output-format-normalization,structural-diffandshared-text-string-decodernpx tsc --noEmitclean,npx eslint .clean,npx vitest run945 passed in 73 filesdump tree,dump tree --json,dump objectanddump sourceagainsttestdata/signed.pdf,tagged.pdf,fonts-mixed.pdf,unsigned-sig-field.pdfandpdfa-1b-clean.pdf, plus a hand-assembled PDF covering odd-digit hex, an unpaired surrogate, an undefined escape, a#20name, a C1 run, a Latin-1 literal and a/Certarray nested inside another arrayNot covered by automation: the GUI tree row at narrow widths. Headless tests assert the class names and DOM order, but real layout needs a native build.
What could go wrong?
The binary carve-out is a key-plus-context match, so it depends on signature dictionaries carrying
/Type /Sig,/Type /DocTimeStampor a/ByteRange. A signature dictionary typed with something else and no/ByteRangewould have its DER run through the text decoder and land on the row. Nothing outside the signature family carries/ByteRange, so the guard cannot over-carve, but it can under-carve on a shape we have not seen.A signature
/Contentsor/Certarray reached as an indirect object cannot be carved out at all.GetObjectDetailrecovers the key from the node ID, and anobj:G:NID has no room for one, so the DER decodes when the value is selected as an object in its own right. The dictionary-entry tree row is unaffected. Fixing it means threading the key throughresolveNodeObjector changing the ID scheme, which is its own change.The decoder inherits pdfcpu's quirks, and they are now visible on six surfaces instead of one. The non-UTF-16BE fallback is Latin-1 despite the function being named
CP1252ToUTF8, so a Word-authored curly apostrophe (0x92) decodes to a C1 control rather than the character a reader expects; it is escaped rather than silently invisible, but it is still not the apostrophe.HexLiteralToStringruns escape processing over already-decoded bytes, so a0x5Cinside a hex text string is dropped:<415C42>givesAB, notA\B. Both are pinned by tests so the behaviour is at least known.tests/shared-text-string-decoder/TestDumpObject_RawStringRenderersUnchangedpinneddump objectrendering/Titleand/UFas raw hex, which is the contract this change deliberately overturns. It was rewritten asTestDumpObject_DisplayIsDecodedAndRawKeepsTheBytes, keeping the recoverability guarantee on the--jsonrawfield. That is a contract change to a prior change's suite, not a mechanical fix, and is worth a reviewer's eye.One unrelated flake:
frontend/src/components/App.drain.test.tsxfailed once at line 274, awaitForon real timers at the 1000 ms default, and passed 28 subsequent runs including a cold-transform full run. If it shows up on a CI leg here, it is that timeout and not this change.Screenshots/videos (if appropriate)
dump tree --resolve --resolve-depth 10 --depth 10on a tagged-table fixture, before:after:
A signature, where the bytes must not be decoded:
And the JSON contract, where the stored form is recoverable only when decoding changed it:
{"label":"Lang", "valueType":"string","value":"en-US"} {"label":"RowSpan","valueType":"number","value":"2"} {"label":"Alt", "valueType":"string","value":"Rapport cell","valueRaw":"<FEFF0052006100700070006F00720074002000630065006C006C>"}jq '.. | select(.valueRaw?)'now surveys every string in a document whose stored form differs from its display form, which previously took onedump source --refper object.Checklist
devbranch, notmaster