Threaded Comments & Review — issue #25 (comments + replies render in PowerPoint)#55
Merged
Merged
Conversation
Implements all 8 sub-features additively, built from the OOXML spec (no upstream prior art): - SF1 CommentAuthors (legacy integer-id) + modern GUID-keyed authors - SF2 legacy <p:cm> + modern <p188:cm> oxml/part classes (2018/8/main) - SF3 Slide.comments add/remove/iter (lazy modern part create+relate) - SF4 Comment.replies threaded chains - SF5 author/text/created_at(tz-aware)/anchor_position accessors - SF6 Comment.resolve()/resolved (modern; legacy raises, documented) - SF7 Shape.comments per-shape anchored filter - SF8 legacy<->modern coexistence on round-trip (both enumerable) DEFECT caught by Interceptor visual verification (trinity was green): modern <p188:cm> referenced GUID authorIds while only a legacy integer-id commentAuthors.xml existed -> dangling refs -> PowerPoint repair dialog. Fixed: modern /ppt/authors.xml (application/vnd.ms-powerpoint.authors+xml, GUID-keyed <p188:author>, presentation->authors threadedCommentAuthors rel). providerId made RequiredAttribute so it always serializes (PowerPoint always writes it). Tests: +97 pytest (tests/test_issue25_comments.py, 43 issue-25), +14 behave. Trinity: pytest 3751, ruff clean, behave 1062, 0 failed. uat_issue25.py 17/17 (script-QA). Post-fix PowerPoint Review-pane visual deferred to maintainer UAT per repo §6a (env wall on re-open; residual risk disclosed — structural reasoning produced one false-green on this part). No push/PR pre-signoff. Refs #25 Closes scanny#487 scanny#538 scanny#756
Cato cross-vendor audit flagged `anchorShapeId` as an out-of-schema
attribute on <p188:cm> (2018/8/main defines only id/authorId/created/
status) — a plausible second PowerPoint repair-dialog trigger that the
deferred post-fix visual would have hidden.
Anchor now stored in <p188:extLst>/<p188:ext uri="{fork URI}"> — the
OOXML-sanctioned extension mechanism conformant consumers MUST ignore
on unknown URI. SF7 per-shape filter + round-trip preserved
(anchored comment round-trips True). <p188:cm> now carries only
schema attributes.
Trinity green: pytest 3751, ruff clean, behave 1062, 0 failed.
Refs #25
…UAT) Maintainer opened uat_issue25_comments.pptx in PowerPoint: file opened clean (no repair) but ZERO comments displayed. Root cause: the add path builds the body via get_or_add_txBody() (a bare <p188:txBody/>) then sets text via _set_txBody_text, which only PRESERVED a pre-existing <a:bodyPr> and never created one. <p188:txBody> is a DrawingML a:CT_TextBody whose schema REQUIRES <a:bodyPr> as first child; PowerPoint silently drops comments with a bodyPr-less txBody — no repair dialog, the comment is just absent. The entire green trinity + 17/17 UAT could not see this; only maintainer visual review did. Fix: _set_txBody_text now GUARANTEES a leading <a:bodyPr/> (inserts it when absent) for both comments and replies. Added DescribeThreadedCommentBodyPrRegression (2 tests) asserting every txBody carries <a:bodyPr> before <a:p>, so the silent-drop class is now test-caught. uat_issue25.py OUT path moved under ./uat/ per CLAUDE.md §6a update. Trinity: pytest 3753, ruff clean, behave 1062, 0 failed. Refs #25
… threads now render (issue #25) Threaded comments were invisible in PowerPoint's Comments pane despite a green trinity and clean script UAT. Root-caused by diffing our OOXML against a threaded comment PowerPoint for Mac itself authored+saved (ground truth, captured this session) — three string axes and one element were inferred wrong in Waves 1-2: - comment-part content type: was application/vnd.ms-powerpoint.threadedComments+xml -> application/vnd.ms-powerpoint.comments+xml - slide -> comment-part reltype: was .../2018/10/relationships/threadedComment -> .../2018/10/relationships/comments - presentation -> authors reltype: was .../2018/10/relationships/threadedCommentAuthors -> .../2018/10/relationships/authors - every <p188:cm> now emits its required first child <pc:sldMkLst><pc:docMk/><pc:sldMk cId="0" sldId="{p:sldId}"/></pc:sldMkLst> (ns .../2013/main/command) — the slide binding PowerPoint needs; omitting it (or the fork-private extLst anchor) leaves the comment unbound. PowerPoint resolves the comment part by the slide relationship type; an unrecognized reltype meant the part was never loaded -> empty pane. Files: opc/constants.py (3 values; symbols unchanged so the PartFactory registry + producer move in lockstep — Cato-audited), oxml/ns.py (+pc), oxml/comments.py (sldMkLst ZeroOrOne + set_slide_marker), comments.py (Comments.add binds the slide). 6 regression assertions in DescribeThreadedCommentPowerPointContract lock all four axes (red->green proven by stashing src) plus set_slide_marker idempotency. Verification: pytest 3759 passed; ruff check + format clean; behave 1062 scenarios / 3215 steps 0 failed. Regenerated UAT deck opened in real PowerPoint via Interceptor — Comments pane renders 3 threads, correct author/text, resolved status persisted (uat/FIXED_comments_rendering.png vs uat/REPRO_no_comments_pane.png). Cato cross-vendor audit: pass. KNOWN-INCOMPLETE: PowerPoint still drops every <p188:reply> (replies do not render; only parent comments do). This is a logically independent reply-level contract defect, deliberately NOT inference-fixed — it needs its own PowerPoint-authored <p188:reply> ground-truth diff. Tracked as a scoped follow-up in the session ISA. UAT script runs clean — pending maintainer visual signoff in PowerPoint/Keynote. No push/PR per repo §6a. Refs #25.
…— replies now render (issue #25) Follow-up to ca4da3f. After the four parent-binding fixes, PowerPoint rendered parent comment threads but SILENTLY DROPPED every reply (`interceptor macos find "Merging"` → 0; only empty Reply boxes). The advisor (Rule 2) correctly flagged reply-rendering as a logically independent concern none of the four parent fixes touched. Root cause is authoritative, not inference: the [MS-PPTX] CT_Comment schema (learn.microsoft.com/openspecs MS-PPTX, 2018/8/main) defines <xsd:complexType name="CT_Comment"><xsd:sequence> <xsd:group ref="EG_CommentAnchor"/> <!-- pc:sldMkLst --> <xsd:element name="pos" minOccurs="0"/> <xsd:element name="replyLst" type="CT_CommentReplyList" minOccurs="0"/> <xsd:group ref="EG_CommentProperties"/> <!-- txBody, extLst --> </xsd:sequence></xsd:complexType> replyLst MUST precede txBody. Our CT_ThreadedComment ZeroOrOne successors emitted txBody first, replyLst after — out of sequence, so PowerPoint validated the parent and discarded the misplaced replyLst. The reply element itself (CT_CommentReply = EG_CommentProperties only) was already correct; this was purely a child-ordering defect. Fix: reorder the successor tuples in src/pptx/oxml/comments.py so the emitted order is pc:sldMkLst, [pos], p188:replyLst, p188:txBody, [p188:extLst] — matching the schema sequence exactly. No API change. Two new regression assertions lock it (it_places_replyLst_before_txBody_per_ms_pptx_sequence, it_keeps_reply_txBody_with_bodyPr_after_reorder); red->green proven by stashing oxml/comments.py (the order test fails pre-fix). Verification: pytest 3761 passed; ruff check All checks passed; ruff format clean; behave 1062 scenarios / 3215 steps 0 failed. Regenerated UAT deck: replyLst-before-txBody True, reply texts present. Opened in real PowerPoint via Interceptor — AX tree now exposes the reply node "Comment from Alex Reviewer. Merging now." plus a "View 1 more reply" control (PowerPoint parsed the full reply list); pre-fix that count was zero. uat/FIXED_replies_rendering.png. Issue #25 now fully resolves: parent comments AND replies render in PowerPoint's Comments pane. UAT script runs clean — pending maintainer visual signoff in PowerPoint/Keynote. No push/PR per repo §6a. Refs #25.
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.
Closes #25.
Modern PowerPoint threaded comments (
<p188:cm>, 2018/8/main) — fulladd/read/reply/resolve API plus the OOXML wiring PowerPoint actually
requires. Maintainer visual signoff received (PowerPoint Mac: parent
threads and replies render in the Comments pane).
What was broken
Five PowerPoint-only defects, all invisible to the test trinity (only a
real PowerPoint open caught them):
<p188:txBody>missing required<a:bodyPr>(a7c6cb9).…ms-powerpoint.threadedComments+xml→ must be…ms-powerpoint.comments+xml.…/2018/10/relationships/threadedComment→…/comments.…/threadedCommentAuthors→…/authors.<p188:cm>missing its required<pc:sldMkLst>slide-binding moniker (ns…/2013/main/command).<p188:replyLst>emitted after<p188:txBody>—[MS-PPTX]CT_Commentis a strict sequenceanchor → pos? → replyLst? → txBody; out-of-order replyLst made PowerPoint render the parent and silently drop every reply.#2–5 root-caused by diffing a comment PowerPoint itself authored; #6 from
the authoritative Microsoft Open-Specs
CT_CommentXSD. Constant valueschanged in
opc/constants.py(symbol names kept so the PartFactoryregistry and producer move in lockstep);
pcnamespace added; childordering fixed in
oxml/comments.py.Verification
python3 -m pytest tests/ -q→ 3761 passedpython3 -m ruff check src tests→ All checks passed!python3 -m behave features/ --no-color→ 1062 scenarios, 0 failed; 3215 steps, 0 failedDescribeThreadedCommentPowerPointContract(8 assertions) locks all six axes incl. the[MS-PPTX]sequence; red→green proven by stashing the serializer.UAT script (
uat/, gitignored) runs clean; maintainer visuallyconfirmed in PowerPoint.