Skip to content

Expose numbering and effective formatting reader facts - #63

Closed
pedroassumpcao wants to merge 11 commits into
tensorbee:mainfrom
Symbolic-ai:pedro/sym-5086_numbering_effective_formatting
Closed

Expose numbering and effective formatting reader facts#63
pedroassumpcao wants to merge 11 commits into
tensorbee:mainfrom
Symbolic-ai:pedro/sym-5086_numbering_effective_formatting

Conversation

@pedroassumpcao

@pedroassumpcao pedroassumpcao commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • model standard numbering identifiers, templates, and paragraph-style links
  • preserve producer-defined numbering suffix and alignment values as unmodeled XML
  • expose a read-only numbering-level projection with exact known, none, and producer-defined formats
  • expose start, suffix, marker text, alignment, style, and presentation facts
  • resolve concrete paragraph and body-run properties through their applicable inheritance layers

Review follow-up

  • rebased onto current main
  • keep numbering-level run properties scoped to marker presentation
  • stop merging marker formatting into ordinary paragraph body runs
  • retain marker-presentation reporting through the numbering-level reader API
  • ready for maintainer F-ID integration

Validation

  • cargo test -p rdocx-oxml --lib
  • focused rdocx numbering and effective-formatting tests
  • cargo clippy -p rdocx-oxml -p rdocx --all-targets -- -D warnings
  • cargo fmt --all -- --check
  • python3 scripts/hash_harness.py --check
  • CI Test and MSRV jobs pass

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 one formatting correctness defect.\n\n- : formats the numbering marker, not every text run in the numbered paragraph. Merging it into makes ordinary paragraph text inherit marker-only bold, font, color, and similar properties. The repository's existing EPUB diagnostics already describe this field as "list marker run styling". Keep marker presentation separate from effective body-run formatting and change the regression test that currently expects marker bold on the text run.\n\nCI is otherwise green apart from the known package-inventory mismatch, but this semantic defect blocks integration. The 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 one formatting correctness defect.

  • crates/rdocx/src/document.rs:3773: "CT_Lvl::rpr" formats the numbering marker, not every text run in the numbered paragraph. Merging it into "effective_run_properties" makes ordinary paragraph text inherit marker-only bold, font, color, and similar properties. The repository's existing EPUB diagnostics already describe this field as "list marker run styling". Keep marker presentation separate from effective body-run formatting and change the regression test that currently expects marker bold on the text run.

CI is otherwise green apart from the known package-inventory mismatch, but this semantic defect blocks integration. The 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-5086_numbering_effective_formatting branch from 64e07f8 to 4714da4 Compare August 31, 2026 17:50
@pedroassumpcao
pedroassumpcao marked this pull request as ready for review August 31, 2026 18:11
@pedroassumpcao

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in commit 4714da4: effective numbering marker formatting no longer leaks into body runs. The regression test pins marker color and boldness separately from run formatting. The branch is ready for maintainer F-ID integration. Test, MSRV, Clippy, formatting, bindings, WASM, Word fidelity, and release-regression checks pass. Package oxml-layout remains red only because of the current-main font-inventory workflow mismatch.

@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 earlier marker-run feedback is fixed, but two correctness issues remain. 1. crates/rdocx/src/document.rs:3722-3738 resolves numbering-level paragraph properties before applying direct paragraph numbering, so direct numId selection, override, and numId 0 can use the wrong level properties or none. Add regressions for all three paths and resolve the effective numbering source first. 2. crates/rdocx/src/document.rs:2847-2884 documents has_unmodeled_properties as including retained definition facts, but ignores modeled yet unexposed fields such as nsid, tmpl, and multi_level_type. Include those facts or narrow the documented contract, with a regression.

@pedroassumpcao

Copy link
Copy Markdown
Contributor Author

Addressed both requested changes in separate commits:

  • a469633 resolves the paragraph's final direct numId and ilvl before selecting numbering-level paragraph properties. Regression coverage includes direct numbering, overriding inherited numbering, and numId=0 cancellation.
  • 1ec6b99 reports unexposed abstract-numbering metadata (nsid, tmpl, and multiLevelType) plus retained raw level pPr/rPr as unmodeled facts.

The complete rdocx and rdocx-oxml test suites pass with the two known machine-specific oracle cases excluded. Clippy with warnings denied, formatting, the 49-entry hash harness, prose checks, and generated-skill sync all pass.

@pedroassumpcao

Copy link
Copy Markdown
Contributor Author

One integration-safety follow-up is now included as 60bc663.

The previous implementation chose the review's “include modeled yet unexposed fields” option. Symbolic's downstream tests showed that this makes every ordinary generated list unmodeled because add_list_definition intentionally emits nsid and tmpl. I therefore switched to the review's alternative: the API contract is now explicitly limited to retained XML and attributes, including raw extended level pPr/rPr, while modeled producer metadata is not classified as unmodeled. The regression distinguishes those two cases.

This preserves the reader's conservative signal for retained raw data without rejecting normal numbered lists downstream.

@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 previously requested direct-numbering overlay and narrowed unmodeled-fact contract are present at 60bc663. One effective-numbering blocker remains.

crates/rdocx/src/document.rs:3717-3733 passes only the explicit paragraph style ID to numbering_level_for_style, although crates/rdocx/src/style.rs:131-137 applies the default paragraph style when no explicit style exists. Because numbering_level_for_style at document.rs:3798-3807 requires a style ID, a paragraph with absent or unstyled pPr that inherits numId from the default style cannot match w:lvl/w:pStyle, infer ilvl, or merge the selected level pPr. Please derive one explicit-or-default effective style ID and use it consistently for both style resolution and numbering-level association, with regressions for absent pPr and pPr without pStyle.

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

@pedroassumpcao

Copy link
Copy Markdown
Contributor Author

Addressed the remaining effective-numbering blocker in commit 7eaf4f4.

effective_paragraph_properties now derives one explicit-or-default paragraph style ID and uses it for both style resolution and w:lvl/w:pStyle association. The regression covers both requested cases: an absent pPr, and a present pPr without pStyle.

The integration-safety correction in 77ee9bd also limits has_unmodeled_properties to extra XML and attributes, excluding raw preservation overlays that ordinary generated list markers use.

The complete rdocx suite passes with only the documented machine-specific oracle cases excluded, 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 7eaf4f4 head, including the default-paragraph-style numbering association. The maintainer integration adopts the reviewed numbering and effective-formatting facts and the default-style correction, while retaining conservative reporting for producer raw pPr and rPr sidecars. We intentionally did not adopt the later broad exclusion of those sidecars because it creates false negatives for retained unmodeled content. The hardened outcome landed through F-X071 on main in merge commit 56bcdc1 and tag s62, with contributor credit recorded. We are closing this PR as integrated with maintainer hardening rather than merging the live head directly.

@mantissaman mantissaman closed this Sep 1, 2026
@mantissaman

Copy link
Copy Markdown
Contributor

Thank you @pedroassumpcao. v0.12.0 includes the numbering identity, level metadata, and effective paragraph and run formatting 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