feat(xls-codec): read and write a sheet's real print settings - #905
Merged
Conversation
Mearman
marked this pull request as ready for review
September 4, 2026 07:21
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Mearman
force-pushed
the
feat/xls-print-settings
branch
from
September 4, 2026 11:11
95852e0 to
8c8e55a
Compare
….204 Every Palette citation in this package named section 2.4.204, which [MS-XLS]'s own "Record Enumeration, By Name" table assigns to PrintSize. Palette is 2.4.188. The package's own convention is that a field offset with no citation is a field offset nobody can check; one pointing at a different record is worse, since it reads as checkable and isn't.
The reader returned Excel's "Normal" preset for every sheet regardless of what the file said, so every print setting a .xls carried was lost on the way in and there was nothing to verify a writer against on the way out. Both directions now go through the records that actually carry them. BIFF8 splits one sheet's print settings across two substreams. The page setup lives in the worksheet substream, as the optional records of [MS-XLS] 2.1.7.20.6's own GLOBALS and PAGESETUP productions: Setup (2.4.257) for paper size, orientation, scale and fit-to-page counts, the four margin records (2.4.151/2.4.219/2.4.328/2.4.27), PrintGrid (2.4.202), PrintRowCol (2.4.203), WsBool's fFitToPage bit (2.4.351), and the two page-break records (2.4.142/2.4.343). The print range and the repeated header bands live in the GLOBALS substream instead, as the built-in Print_Area and Print_Titles defined names of an Lbl record (2.4.150), whose value is a compiled Ptg token stream naming the areas. Each field falls back to the Normal preset independently, so a sheet declaring one margin and nothing else keeps that margin. Two of BIFF8's own conditional rules are honoured rather than flattened: a Setup record whose fNoPls bit is set declares its own paper size and scale undefined, and WsBool's fFitToPage decides which of Setup's two mutually exclusive scaling fields is live -- real producers write both regardless, so reading both would report a scale and a page count that contradict each other. A page size no paper code names is refused rather than substituted: unlike xlsx's pageSetup element, Setup addresses paper only by code, so there is no honest way to write one. The writer also emits the calculation-state records the GLOBALS production requires ahead of PrintRowCol, which it previously omitted as bookkeeping. They are mandatory in the grammar, and LibreOffice's own importer silently discards whichever page-settings record comes first in a worksheet substream -- with PrintRowCol in that slot, a file written with row and column headers enabled opened with them off.
Moves print settings out of the read-side gaps and the writer's not-written table into the built-and-shipped list, with its own section covering the record map, the split between the worksheet and globals substreams, the two conditional rules that decide how to read Setup, the three things the format or the schema genuinely cannot carry, and how both directions were checked against real LibreOffice-produced BIFF8.
The package table listed print settings alongside formulas as a read-only gap; only formulas still are.
…failure A page size no iPaperSize code names was refused with a thrown BiffWriteError, which turned a presentational field nobody can express in BIFF8 into a whole failed conversion: a spreadsheet converted from a slide deck or a drawing carries that source's own canvas as its page size, which is almost never a named paper, so every such conversion died and lost the cells with it. iPaperSize 0 is [MS-XLS] 2.4.257's own "custom printer paper sizes", which is true of exactly this case, where substituting Letter would not be. The reading application then falls back to its own default paper -- this package to the Normal preset, LibreOffice to its locale's -- and every other print setting on the sheet still comes through. Also collapses an explicit 100% scale onto the absence that means the same thing. iScale is a mandatory field of a mandatory record with no spelling for "this sheet declares no scale", so reporting it put a scalePercent of 100 on every sheet of every workbook read, carrying nothing its absence did not already say, and meant a document written with no scale came back with one.
…ort smoke test package.json's `./*` wildcard advertises every module under dist/, and this suite is the only thing that proves the advertised subpaths really resolve -- publint and attw both pass a wildcard whose targets are missing. The two modules the print-settings work added were not in it.
The Fidelity note said the xls writer never touches cell decoration or anything print- or metadata-related, and rested the xls pairs' rating on that. All three are read and written for real by xls-codec now, so the rating rests on formulas and per-cell font alone; the ratings themselves are unchanged, since those two are what earned them.
…tup fields ContentSheetPrintSettings bounds neither a scalePercent nor a fitToPages count from above, and both land in 16-bit Setup fields, so a value past the field's own width wrapped: a 200000% scale went into the file as 3392%, and a fit-to-page width of 100000 as 34464. [MS-XLS] 2.4.257 caps iFitWidth/iFitHeight at 32767; iScale has only its field's own width, and a floor of 1, since 0 is not a scaling factor. Clamped rather than refused, matching how an unnameable paper size is handled a few lines above: a print scale is presentational, and losing a workbook's cells over an absurd one buys nothing.
…literal Every other record in this suite is built through the named constant record-types.ts carries its [MS-XLS] citation on; the print-name fixture spelled 0x0018 inline.
…peat band, or page break ContentSheetPrintRange/ContentSheetRepeatRange/manualBreaks bound no coordinate from above, and writeArea3d/writePageBreaksRecord wrote each one straight into a 16-bit field, which RecordBuilder.u16 masks with 0xffff. A coordinate past BIFF8's own 65536-row/256-column grid therefore wrapped to a smaller, plausible-looking in-grid value with no error -- a print range asked for through row 99999 silently landed at row 4463, confirmed against a real LibreOffice re-read of the written file. A print range or repeated header band is clamped to the grid's own last row/column instead: unlike a single-position page break, "to the bottom of the sheet" is genuinely what an end coordinate past the grid means once the grid is smaller than the caller assumed. A page break is dropped rather than clamped, since clamping one would insert a break at the grid's own edge nobody asked for -- the same presentational-field trade this file's own scale/fit-to-page clamp already makes, applied to the case where clamping is the wrong answer instead of the right one.
…g it readBooleanRecord tested a PrintGrid record's whole 16-bit word against zero, which is correct for PrintRowCol (a genuine 16-bit Boolean field) but not for PrintGrid: [MS-XLS] 2.4.202 defines only its first bit (fPrintGrid), with the remaining fifteen "Undefined, and MUST be ignored." A producer leaving anything set in those bits would read back as gridlines-on regardless of fPrintGrid's own value. No real producer does this -- LibreOffice writes 0x0000/0x0001 -- but masking to the one bit the field's own layout defines is what the spec says to do, and is a one-line fix now that PrintRowCol's identical-looking record has already established the pattern of reading each print-settings boolean by its own rules rather than a shared assumption.
Mearman
force-pushed
the
feat/xls-print-settings
branch
from
September 4, 2026 12:19
8c8e55a to
62b1a20
Compare
Contributor
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
1 participant
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.
Part of #815.
xls-codecreturned Excel's fixed "Normal" preset for every sheet'sprintSettingsregardless of what the file said, so every print setting a.xlscarried was lost on the way in — and, as the package README pointed out, that also meant there was nothing to verify a writer against on the way out. Both directions land here together.BIFF8 splits one sheet's print settings across two substreams, which is the main thing that made this more than a record-per-field exercise. The page setup is in the sheet's own substream, as the optional records of [MS-XLS] 2.1.7.20.6's
GLOBALSandPAGESETUPproductions —Setup(2.4.257) for paper size, orientation, scale and fit-to-page counts, the four margin records,PrintGrid,PrintRowCol,WsBool'sfFitToPagebit, and the two page-break records. The print range and the repeated header bands are not there at all: they live in the workbook globals substream, asLbldefined names (2.4.150) carrying a built-in name index (Print_Area0x06,Print_Titles0x07) rather than a user-typed name, scoped to a sheet by the record's own one-baseditab, with the range itself in a compiledPtgtoken stream. Writing one meant the globals writer also had to emit a self-referencingSupBookand anExternSheetfor thePtgArea3dto resolve through, which it previously had no reason to.Two of BIFF8's own conditional rules are honoured rather than flattened:
fNoPlsdeclaresSetup's own paper size and scale undefined, andWsBool'sfFitToPagedecides which ofSetup's two mutually exclusive scaling fields is live (real producers write both regardless, so reading both would report a scale and a page count that contradict each other). And aPrint_Titlesband has no field saying which axis it repeats along — the shape is the discriminant, a row band spanning every column and a column band spanning every row — so both directions classify by shape.Verification
Round trip, pinned in
src/write.test.ts, plus hand-built byte sequences per record insrc/workbook/sheet.test.ts/src/content.test.ts/src/biff/print-setup.test.tstaken from the spec's own field-layout tables.Against LibreOffice, in both directions, per field:
.xls.xlsre-read by LibreOfficepageSizemarginsgridlines,headerspageOrderoverThenDownanddownThenOverscalePercentfitToPagesmanualBreaksprintRangeB2:D6B2:D6repeatRows/repeatColumnsMethod: hand-authored
.fods→soffice --headless --convert-to xls→ read here; then written back out andsoffice --headless --convert-to fods, comparing LibreOffice's re-export of our file against its re-export of its own, attribute by attribute. ThePrint_Arearecord this writer emits is byte-for-byte the one LibreOffice writes for the same range, andsrc/workbook/print-names.test.tsasserts exactly that against the real bytes; thePrint_Titlesrecord differs by one byte, a trailingPtgParendisplay token.Two things the LibreOffice pass caught that a round trip would not have
LibreOffice's importer silently discards whichever page-settings record comes first in a worksheet substream. With
PrintRowColin that slot, a.xlswritten here with row and column headers enabled opened in LibreOffice with them off, while every other print setting in the same file came through. Bisected by stripping and reordering records in both its file and ours: moving any other record into that slot fixes it. The fix is to emit the calculation-state records (CalcCount,CalcRefMode,CalcIter,CalcDelta,CalcSaveRecalc) thatGLOBALSmakes mandatory ahead ofPrintRowColand this writer previously omitted as bookkeeping — the grammar already required them, and they are the honest thing to put there.Refusing an unnamed page size was the wrong trade.
Setupaddresses paper only by code, so a page size no code names genuinely cannot be written, and the first cut threw. That broke sixdocuments.jsconversion tests: a spreadsheet converted from a slide deck or a drawing carries that source's own canvas as its page size, which is almost never a named paper, so the conversion failed and lost the cells with it. It now writesiPaperSize0 — that section's own "custom printer paper sizes", which is true where substituting Letter would not be — and the reading application falls back to its own default paper. Verified: LibreOffice opens such a file with its own paper substituted and every other setting and cell intact.Not covered, and why
Plsrecord carrying a printer driver's opaqueDEVMODEblob, which this package has no way to produce.fitToPageswith an axis set toSetup's own "as many pages as necessary" (0) is not reported at all, rather than fabricated as 1 —ContentSheetPrintSettings.fitToPagesrequires both counts positive, and a fabricated 1 would claim the sheet is pinned to one page along an axis the file left free.iScaleis a mandatory field with no spelling for "no declared scale", and the schema already means actual size by carrying noscalePercent; the two print identically.Print_Areanaming several disjoint areas yields only the first (printRangemodels one rectangle), and a partial page break is carried as a full one (manualBreaksis an index with no extent).One smaller robustness fix in its own commit, found reviewing the same code:
scalePercentandfitToPagesare unbounded above in the schema and land in 16-bitSetupfields, so a 200000% scale wrapped into the file as 3392%. Both are now clamped —iFitWidth/iFitHeightto [MS-XLS]'s own 32767,iScaleto its field's width — rather than refused, for the same reason the paper code is.Worth a close look
The record layouts are all from [MS-XLS] rather than from an existing in-repo pattern, so the byte offsets are the thing to check hardest — every one is cited to its section at the point it is read or written, and every value in the LibreOffice fixtures decodes to what the source
.fodsdeclared, but a second pair of eyes on theSetupflag bit assignments (fLeftToRight/fPortrait/fNoPls/fNoOrient) and theLblfield order would be welcome. ThePrint_Titlesshape-based axis classification is the other place a wrong assumption would be quiet rather than loud.One drive-by in its own commit: every
Palettecitation in the package named [MS-XLS] 2.4.204, which isPrintSize;Paletteis 2.4.188.