Skip to content

Expose document and table reader completeness facts - #62

Closed
pedroassumpcao wants to merge 15 commits into
tensorbee:mainfrom
Symbolic-ai:pedro/sym-5085_document_table_reader_facts
Closed

Expose document and table reader completeness facts#62
pedroassumpcao wants to merge 15 commits into
tensorbee:mainfrom
Symbolic-ai:pedro/sym-5085_document_table_reader_facts

Conversation

@pedroassumpcao

@pedroassumpcao pedroassumpcao commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • preserve previously dropped table, row, border, and legacy merge properties
  • expose document background and section completeness facts
  • expose fail-closed table, row, and cell reader facts
  • keep the existing ordered CellItemRef API and publicly re-export VMerge

Review follow-up

  • rebased onto current main
  • updated table-border test literals for the retained XML field
  • write gridBefore and gridAfter before later row properties in schema order
  • retain unmodeled table properties at their schema boundaries before tblPrChange
  • update the RTF diagnostic expectation now that hMerge is modeled
  • ready for maintainer F-ID integration

Validation

  • cargo test -p rdocx-oxml --lib
  • cargo test -p rdocx-layout --lib
  • focused rdocx integration test for table-cell property diagnostics
  • cargo clippy -p rdocx-oxml -p rdocx-layout -p rdocx --all-targets -- -D warnings
  • cargo fmt --all -- --check
  • python3 scripts/hash_harness.py --check

The local unfiltered rdocx run only reaches the existing environment-dependent word_and_powerpoint_chart_pixels_are_identical rasterizer oracle assertion. The failure does not exercise these reader changes.

@mantissaman mantissaman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for three merge blockers.\n\n- : the workspace no longer compiles because both literals omit the new field. Clippy, Test, and MSRV all fail with E0063.\n- : and are written after . In the sequence they precede , , , and . This emits schema-invalid row properties.\n- : all retained table-property XML is emitted after , even though is schema-final. An input such as plus therefore reopens as invalidly ordered XML. Retained children need schema-boundary positions, as the cell-property code already does.\n\nThis PR is also a draft, is based on the S60 main SHA, and has not gone through the F-ID sprint integration workflow.

@mantissaman
mantissaman dismissed their stale review August 31, 2026 16:54

Superseded immediately because shell quoting stripped technical identifiers from the body.

@mantissaman mantissaman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for three merge blockers.

  • crates/rdocx-layout/src/paginator.rs:5622: the workspace no longer compiles because both "CT_TblBorders" literals omit the new "extra_xml" field. Clippy, Test, and MSRV all fail with E0063.
  • crates/rdocx-oxml/src/table.rs:1013: "gridBefore" and "gridAfter" are written after "jc". In the "CT_TrPr" sequence they precede "cantSplit", "trHeight", "tblHeader", and "jc". This emits schema-invalid row properties.
  • crates/rdocx-oxml/src/table.rs:696: all retained table-property XML is emitted after "tblPrChange", even though "tblPrChange" is schema-final. An input such as "bidiVisual" plus "tblPrChange" therefore reopens as invalidly ordered XML. Retained children need schema-boundary positions, as the cell-property code already does.

This PR is also a draft, is based on the S60 main SHA, and has not gone through the F-ID sprint integration workflow.

@pedroassumpcao
pedroassumpcao force-pushed the pedro/sym-5085_document_table_reader_facts branch from 4970ffd to e8fd435 Compare August 31, 2026 17:48
@pedroassumpcao
pedroassumpcao marked this pull request as ready for review August 31, 2026 18:12
@pedroassumpcao

Copy link
Copy Markdown
Contributor Author

Addressed all review feedback in separate commits: ac5aff9 fixes the border test literals, 461b61f writes row grid offsets in schema order, e8fd435 preserves table properties at the schema boundary before tblPrChange, and 31f982c updates the modeled hMerge diagnostic expectation. The branch is ready for maintainer F-ID integration. Quick checks, Clippy, formatting, docs, bindings, WASM, Word fidelity, output stability, and release regressions pass; Test and MSRV are still running. Package oxml-layout has the same current-main font-inventory workflow mismatch described on the sibling PRs.

@mantissaman mantissaman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three earlier requests are fixed, but this revision still has four blocking correctness issues. 1. crates/rdocx-oxml/src/table.rs:929 and :1079 retain CT_TrPr extras without schema positions and emit them after revision markers, so retained row properties such as tblCellSpacing can become schema-invalid. 2. crates/rdocx-oxml/src/table.rs:968 parses gridBefore and gridAfter by local name only, allowing foreign namespace elements to acquire Word semantics. 3. crates/rdocx-oxml/src/table.rs:94 and :149 capture border children without owner namespace bindings, so prefixes declared on tblBorders or tcBorders can become unbound after write. 4. crates/rdocx/src/table.rs:797 omits header, grid_before, and grid_after from RowRef::has_formatting. Please add focused regressions for each case.

@pedroassumpcao

Copy link
Copy Markdown
Contributor Author

Addressed every requested change in separate commits:

  • f2b96e5 preserves retained row-property XML at its original schema boundary instead of appending it after modeled properties.
  • d65ce2f accepts gridBefore and gridAfter only from the WordprocessingML namespace while retaining foreign lookalikes.
  • 8db20ee carries owner namespace bindings into retained tblBorders and tcBorders children.
  • fa48a39 includes header rows and row grid offsets in RowRef::has_formatting().

Each behavior has a focused regression test. Formatting, clippy with warnings denied, the 49-entry hash harness, prose checks, and generated-skill sync pass. The Rust suite passes apart from two local-oracle checks unrelated to this PR: the pinned rasterizer and LibreOffice build differ from the versions installed on this machine. With those oracle cases excluded, 313 library tests and 129 integration tests pass.

@mantissaman mantissaman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The four previously requested changes are present at fa48a39, but two OOXML correctness blockers remain.

  1. crates/rdocx-oxml/src/table.rs:954-957, 1011, 1032, and 1754-1756 capture retained row-property XML without namespace bindings inherited from w:tr or w:trPr. Serialization rebuilds w:trPr at lines 1054-1056, so an extension prefix declared only on an owner can become unbound. Please propagate owner bindings into retained CT_TrPr children and add a serialize, reopen, and repeated-serialize regression.

  2. crates/rdocx-oxml/src/table.rs:997-1009 and 1018-1030 flatten malformed w:ins and w:del into one revision_xml collection, which is emitted after all typed markers at lines 1119-1124. A malformed insertion followed by a valid deletion can therefore be reordered after the deletion, violating the CT_TrPr sequence. Preserve malformed insertion and deletion nodes in their separate schema slots, with an ordering regression.

The substantive CI jobs pass. The Package oxml-layout failure is the known current-main inventory mismatch and is unrelated to these blockers.

@pedroassumpcao

Copy link
Copy Markdown
Contributor Author

Addressed both remaining OOXML blockers in separate commits:

  • db5a6e5 propagates namespace bindings declared on w:tr and w:trPr into retained row-property children. The regression serializes, reopens, and serializes again while checking bindings inherited from both owners.
  • 4f54a00 emits malformed insertion and deletion markers in their separate CT_TrPr schema slots, with a regression for malformed w:ins followed by a typed w:del.
  • d03ef4a keeps that schema-slot behavior while preserving the existing public revision_xml: Vec<Vec<u8>> shape after the combined workspace gate exposed a dependent-crate compatibility issue.

The focused and complete rdocx-oxml suites pass, rdocx-layout compiles against the preserved collection API, and workspace Clippy passes with warnings denied. Formatting and the 49-entry hash harness also pass. The combined integration branch is pinned at 70d3ee1da0c2ec600d020aa5c3eb3af860d300dc.

@mantissaman

Copy link
Copy Markdown
Contributor

Thank you, @pedroassumpcao. We reviewed the latest d03ef4a head and confirmed that it implements the requested row-owner namespace propagation and separate malformed insertion and deletion slots. We did not merge the live branch directly because the maintainer integration additionally uses expanded-name classification for those schema slots, preventing foreign same-local revision elements from acquiring Word semantics. The contribution outcome, public compatibility shape, and stronger namespace and schema-order hardening landed through F-X071 on main in merge commit 56bcdc1 and tag s62. Contributor credit is recorded in the delivery ledger. We are closing this PR as integrated with maintainer hardening.

@mantissaman mantissaman closed this Sep 1, 2026
@mantissaman

Copy link
Copy Markdown
Contributor

Thank you @pedroassumpcao. v0.12.0 includes the document, table, row-grid, border, formatting, and retained-property completeness outcome from this proposal through a reviewed hardened equivalent.

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