Skip to content

Adjusting XSLFDrawing's slide shape tree scan in constructor to prevent shape ID warnings. - #1187

Merged
pjfanning merged 2 commits into
apache:trunkfrom
dbunner2:fix/xslf-group-shape-id-warning
Jul 28, 2026
Merged

Adjusting XSLFDrawing's slide shape tree scan in constructor to prevent shape ID warnings.#1187
pjfanning merged 2 commits into
apache:trunkfrom
dbunner2:fix/xslf-group-shape-id-warning

Conversation

@dbunner2

Copy link
Copy Markdown
Contributor

Creating a shape directly on an XSLFGroupShape produces a misleading "shape id * has been already used" warning for every shape already on the slide, even though those IDs are valid. This happens because the XSLFDrawing constructor always scans the entire slide and re-registers every shape ID, regardless of whether it's creating the top-level drawing for the slide or a drawing for a nested group.

That extra scan isn't necessary for group drawings. By the time a group drawing is created (through createGroup() or getShapes()), the top-level drawing has already recursively scanned the entire slide, including all nested groups. Scanning again just re-registers the same IDs and causes the false warnings.

This change limits the scan to the top-level drawing only. It also adds a regression test that captures the logged output and verifies no warnings are produced for the reported scenario. Reverting the fix causes the test to fail with the original warnings. I also ran the full org.apache.poi.xslf.* test suite (231 tests) with no regressions.

Fixes #924.

…event it from reregistering every shape id it found. This will restrict the scan to only run when construting the sheet's own top-level drawing. Fixes issue apache#924.
Comment thread poi-ooxml/src/main/java/org/apache/poi/xslf/usermodel/XSLFDrawing.java Outdated
Comment thread poi-ooxml/src/test/java/org/apache/poi/xslf/usermodel/TestXSLFGroupShape.java Outdated
Comment thread poi-ooxml/src/test/java9/module-info.java Outdated
@dbunner2
dbunner2 requested a review from pjfanning July 20, 2026 17:04
@dbunner2

Copy link
Copy Markdown
Contributor Author

Hi @pjfanning - Anything else I can add to this while I have some time? Just let me know.

@pjfanning
pjfanning merged commit 368f94a into apache:trunk Jul 28, 2026
4 checks passed
@pjfanning

Copy link
Copy Markdown
Member

Merged. I still don't like using mockito here but I don't have time to come up with a different approach. In future, I'm likely to just block PRs with mockito testing. This is a lib where everything should be testable already and we can always expose state via package private getters to expose more state that can be verified.

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.

Bug: shape id <shapeId> has been already used warning when using XSLFGroupShape on XSLFSlide

3 participants