Skip to content

Read sheet hidden state - #50

Merged
borgar merged 6 commits into
borgar:masterfrom
steinnes:read-sheet-hidden-state
Mar 9, 2026
Merged

Read sheet hidden state#50
borgar merged 6 commits into
borgar:masterfrom
steinnes:read-sheet-hidden-state

Conversation

@steinnes

@steinnes steinnes commented Mar 7, 2026

Copy link
Copy Markdown
Contributor

What

Read and set hidden: 0 | 1 | 2 from workbook > sheets > sheet[state] attribute (ECMA-376 - SpreadsheetML Reference section 18.2.19).

Why

JSF supports it, we just had to read and map the states to the numerical representation (which matches the ST_SheetState definition, probably not by accident).

How

The parser has a SheetLink object which is constructed when reading the workbook, collecting other information in the workbook sheet list. I added this property there, since that's where it is stored in the OOXML and a list of these SheetLink objects is included in ConversionContext for handlers such as the worksheet handler which generates the JSF Sheet object for output.

steinnes added 2 commits March 7, 2026 09:42
…igate whether the sheet state attribute can have more interesting values and whether adding this to SheetLink array is the right approach (seems fairly benign)
@steinnes

steinnes commented Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

I had to change one unrelated fixture which turned out to have a hidden sheet 😺

@steinnes

steinnes commented Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

Hmm.. I'm not getting this test failure for charts-and-images.xlsx locally. Looking into this, might recreate the branch and see if getting rid of this merge commit helps 🤷

charts-and-images.xlsx has images in two sheets (Sheet1 background
image5.png, Sheet2 drawing image6.png). The test asserts both appear
in wb.images. Currently fails because context.images is shared mutable
state across the Promise.all sheet processing loop — the last sheet to
write wb.images wins, losing the other sheet's images.
@steinnes

steinnes commented Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

Hmm.. it looks like there's actually a race condition because charts-and-images.xlsx contains two images. I have a new test in convertBinary.spec.ts which reproduces the problem reliably, looking into replacing await Promise.all(context.sheetLinks.map(async (sheetLink, index) => with sequential handling of the sheets.

steinnes added 2 commits March 7, 2026 16:55
Process sheets sequentially instead of with Promise.all to eliminate
shared-state races on context.images and context.drawingRels between
await points. The async zip decompression operates on an in-memory
buffer so parallelism provided no meaningful benefit.

Also accumulate wb.images across sheets instead of overwriting.
Sequential sheet processing changes the order formulas are encountered,
shifting formula indices. The formulas themselves are unchanged.
@steinnes

steinnes commented Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

Processing the sheets sequentially required regenerating at least one more fixture json (tables.xslx), but all is passing now. Ready to discuss with you @borgar ... I'd be happy to change the approach and contribute any other desired improvements :-)

@borgar

borgar commented Mar 9, 2026

Copy link
Copy Markdown
Owner

That race is something I hit occasionally and have been intending to fix so I'm very happy you've cleaned that up. The approach is good, there is really no special advantage to parallel-running sheet conversion as the whole thing runs single threaded anyway.

I'm sure hidden sheets we're working at some point but I must have regressed it, very good find!

@borgar
borgar merged commit 8d516ea into borgar:master Mar 9, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants