Read a string in a cell as a string, in the binary dictionary too - #37
Merged
Merged
Conversation
`{"a"}` in a compressed-binary .sldd displayed as `{<1x1 string>}`: the summary
form, so the text was not merely mis-typed, it was not on screen at all and the
cell offered no editor. The text dictionary showed `{"a"}` the whole time, which
is how this was reported -- the same value read two ways depending on which
format it had been saved in.
A `string` is not a JSON value in the XML channel, it is an object with a
`saveobj` payload, and BinarySlddParser nests one in three places:
parseEntryValue for an entry's own value, parsePropContent for a struct field or
an object property, and parseCellElement for a cell element. The first two tested
`Class="string"` and called parseStringValue; the third never did, so the element
fell through to the generic nested-object tail and became an object of class
`string` whose text sat in the saveobj bag, where no formatter looks. One rule,
three paths, two of them right.
The display was the visible tip. Nothing decoded that bag back, so rebuilding the
chunk wrote the element out as an empty `<Element Class="string">` and the text
was GONE from the file -- silent data loss on any save of a dictionary that held
one, with no edit to it required. Re-serializing an untouched MATLAB-authored
dictionary with the fix stashed loses all four string-bearing entries.
Fixed by lifting `string` out of the object tail, the same two lines its two
siblings already had. probe_cell_string.m settles the shape the branch keys on,
which is a level deeper than it looks: a cell element holding an object is a
CLASSLESS <Element> wrapping the object's own <Element Class="...">, so the tail
was right for every other class and the fix had to be a lift rather than a
replacement. Its two fixtures are one dictionary in both flavours, which is what
lets the channels be asserted against EACH OTHER rather than each against its own
literal.
Three tests in cellElementShape.test.ts had pinned `{<1x1 string>}` as a known
limitation of the binary channel, blamed on the undecoded MCOS payload of a .mat.
That was wrong twice over -- an .sldd carries no MCOS blob, and the write side was
already byte-correct -- so the limitation bullet it cited now says so.
Two neighbours the same measurement turned up, both recorded in DESIGN.md and
neither touched here: we write `Dimension="1*1"` on a 1x1 cell where MATLAB omits
the attribute (save churn, round-trips correctly, predates this), and MATLAB has
stopped emitting `_fields` in a text dictionary, which is the only thing
StructNode.parse builds a scalar struct's field rows from.
A value that could not be read and was destroyed on save now reads and round-trips, and three PRs merged since v1.22.0 ride along, so this is a minor bump. The release tag has to match this field, and the pin in data-explorer-vscode's package.json cannot move until the tag exists. package-lock.json is left alone deliberately, as every bump from 1.14.0 on has been: its `version` field is cosmetic for a git dependency, and a global version substitution in that file has corrupted an unrelated package's entry before.
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.
{"a"}in a compressed-binary.sldddisplayed as{<1x1 string>}— the summary form, so the text was not on screen at all and the cell offered no editor. The text dictionary showed{"a"}the whole time, which is how it was reported: the same value read two ways depending on which format it had been saved in.A
stringis not a JSON value in the XML channel, it is an object with asaveobjpayload, andBinarySlddParsernests one in three places —parseEntryValue(an entry's own value),parsePropContent(a struct field or an object property),parseCellElement(a cell element). The first two testedClass="string"; the third never did, so the element fell through to the generic nested-object tail and became an object of classstringwhose text sat in the saveobj bag, where no formatter looks.The display was the visible tip. Nothing decoded that bag back, so rebuilding the chunk wrote the element out as an empty
<Element Class="string">and the text was gone from the file — silent data loss on any save of a dictionary that held one, with no edit to it required.Fixed by lifting
stringout of the object tail inparseCellElement, the same two lines its two siblings already had.How the shape was settled
test/parity/matlab/probe_cell_string.m, against R2027a: a cell element holding an object is a classless<Element>wrapping the object's own<Element Class="...">, one level deeper than it looks — so the tail is right for every other class and the fix had to be a lift, not a replacement. Its two fixtures (cellstr_text.sldd,cellstr_binary.sldd) are one dictionary in both flavours, which lets the channels be asserted against each other rather than each against its own literal.Tests
binaryWriteBackGate.test.ts— reads MATLAB's own bytes, compares both channels, and pins that an untouched rebuild keeps all sixsaveobjpayloads and reopens as the values MATLAB wrote; two entries are asserted byte-identical to MATLAB's chunk.binarySlddValues.test.ts— the reader-level regression, failing before the fix.cellElementShape.test.ts— three assertions that had pinned{<1x1 string>}as a known limitation of the binary channel, blamed on a.matMCOS payload, now assert both channels agree. An.slddcarries no MCOS blob and the write side was already byte-correct, so the limitation bullet they cited now says so.Full
npm run verifygreen: 4868 tests.Two neighbours recorded, neither touched
Dimension="1*1"on a 1x1 cell's<P Name="Value">where MATLAB omits the attribute. Save churn, not data loss; it round-trips, and it predates this defect.27.1.0.3393633no longer emits_fieldsin a text dictionary, andStructNode.parsebuilds a scalar struct's field rows from_fieldsalone — so a struct in a current-MATLAB text.slddshows no field row at all. Wider blast radius than this defect and not fixed here; measured with a nine-struct probe and written up inDESIGN.md.