Skip to content

fix(odf): address a repeated sheet cell by position, not by index - #857

Merged
andiwand merged 1 commit into
mainfrom
feat/sheet-repeat-identity
Sep 7, 2026
Merged

fix(odf): address a repeated sheet cell by position, not by index#857
andiwand merged 1 commit into
mainfrom
feat/sheet-repeat-identity

Conversation

@andiwand

@andiwand andiwand commented Sep 7, 2026

Copy link
Copy Markdown
Member

🤖 Generated with Claude Code

The bug

One element stands for every position a repeat covers, so an index cannot say which of them a handle means:

SheetCell c = sheet.cell(5, 0);   // run anchored at column 2
c.position();                     // → (2, 0)
sheet.cell(5, 0) == sheet.cell(2, 0);  // → true

DocumentPath inherits it, since extract_path spells a cell by sheet_cell_position. This is the half of the repeated-cell change in v6.12.0 (#776) that a consumer could observe — it was marked Breaking at the time and justified with "spreadsheets are not editable", which #856 has now retired.

The fix

sheet_cell(sheet, column, row) hands out a positional id for a repeat:

tag(1) | ordinal(15) | column(24) | row(24)

the ordinal counting sheets in document order. A cell standing for one position alone keeps its index, so only a repeat pays for the indirection and the element store stays a flat array.

The decode is one hook — ElementRegistry::resolve_id, identity for every engine that does not shadow it. odf's shadow looks the position up in the sheet's existing run index (a binary search that was already there), and every payload accessor goes through it.

Two properties fall out:

  • RegistryElementAdapter navigates entirely through element_at, so all navigation works untouched, and element_parent drops the position for free — a sheet's stored parent_id is a plain index.
  • The id resolves through the index rather than pinning the element it found, so a handle follows the index. That is what will let the next step split a run under a handle without invalidating it.

extract_path/navigate_path needed no change: they already go out by position and back through sheet_cell.

The position stops at the cell — one paragraph and one run stand for every position — so a path into a repeated cell still names the anchor, and round-trips. A test pins that boundary.

One line goes with it: a cell's style resolved at the payload's position, which for a repeat is the anchor's column, and table:default-cell-style-name is a column's to state. Not reachable today (SheetCell exposes no style(), and children reach it through the plain anchor id), so this is consistency rather than a fix.

Not xlsx

Sheet.cells there is one entry per real <c>, each with its own element and its own true position. Neither bug exists, so nothing is added to it.

Checks

  • 1582 tests, 1576 passed, 6 pre-existing skips. 4 are new. The reference HTML output is byte for byte unchanged.
  • clang-tidy clean; gcc-15 -Wall -Wextra -Werror over seven touched TUs.

Next

Splitting a repeated run on write, which makes Sheet::set_cell accept a repeated cell instead of refusing it. It needs the DOM row/cell split plus a parser entry point for re-registering the copies, and it is safe only because a handle re-resolves — hence this first.

Base automatically changed from feat/sheet-cell-write to main September 7, 2026 19:17
@andiwand
andiwand force-pushed the feat/sheet-repeat-identity branch 2 times, most recently from a5ef1c7 to 4c37fed Compare September 7, 2026 19:24
One element stands for every position a repeat covers, so an index cannot say
which of them a handle means: `SheetCell::position()` answered with the anchor
of the range, `Sheet::cell()` handed back the same handle throughout it, and
`DocumentPath` named a cell the caller had not asked for.

`sheet_cell(sheet, column, row)` now hands out `tag | ordinal | column | row`
for a repeat, and the registry decodes it against the sheet's cell index. A
cell standing for one position alone keeps its index, so nothing but a repeat
pays for the indirection and the store stays a flat array.

The decode is the shared `ElementRegistry::resolve_id`, identity for every
engine that does not shadow it. `RegistryElementAdapter` navigates through
`element_at`, so nothing else has to know, and `element_parent` drops the
position on its own - a sheet is stored as a plain index. Because the decode
goes through the index rather than the element it found, a handle follows the
index.

`extract_path` already spells a cell by position and `navigate_path` already
goes back through `sheet_cell`, so a path names the right cell untouched. The
position stops at the cell: one paragraph and one run stand for every position,
so a path into a repeated cell still names the anchor.

Cell style now resolves at the position the id carries rather than the
payload's, since the anchor sits in another column and
`table:default-cell-style-name` is a column's to state.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J325TWocZ4iXBjvKv2ZVZi
@andiwand
andiwand force-pushed the feat/sheet-repeat-identity branch from 4c37fed to 6a041fa Compare September 7, 2026 19:37
@andiwand
andiwand enabled auto-merge (squash) September 7, 2026 19:38
@andiwand
andiwand merged commit 2e21956 into main Sep 7, 2026
36 checks passed
@andiwand
andiwand deleted the feat/sheet-repeat-identity branch September 7, 2026 19:58
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.

1 participant