Copilot commented on code in PR #971:
URL: https://github.com/apache/poi/pull/971#discussion_r3123296639
##########
poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFTable.java:
##########
@@ -168,24 +172,59 @@ public XWPFTable(CTTbl table, IBody part, boolean
initRow) {
createEmptyTable(table);
}
- for (CTRow row : table.getTrList()) {
- StringBuilder rowText = new StringBuilder();
- XWPFTableRow tabRow = new XWPFTableRow(row, this);
- tableRows.add(tabRow);
- for (CTTc cell : row.getTcList()) {
- for (CTP ctp : cell.getPList()) {
- XWPFParagraph p = new XWPFParagraph(ctp, part);
- if (rowText.length() > 0) {
- rowText.append('\t');
+ try (XmlCursor cursor = table.newCursor()) {
+ cursor.selectPath("./*");
+ while (cursor.toNextSelection()) {
+ XmlObject xmlObject = cursor.getObject();
+ if (xmlObject instanceof CTRow) {
+ processCTRow((CTRow)xmlObject);
+ }
+ else if (xmlObject instanceof CTSdtRow) {
+ List<CTRow> rows = new ArrayList<>();
+ collectCTRowsInnerSdtRow((CTSdtRow)xmlObject, rows);
+ for (CTRow row : rows)
+ {
+ processCTRow(row);
}
- rowText.append(p.getText());
}
}
Review Comment:
The constructor now adds CTRow instances extracted from CTSdtRow into
`tableRows`. Many mutation/access methods (e.g., `removeRow`,
`insertNewTableRow`, `addRow`, `getRow(int)`, `getNumberOfRows`) assume
`tableRows` indexes map 1:1 to `ctTbl`’s top-level `w:tr` array. With
SDT-derived rows included, `removeRow(pos)` will remove the wrong underlying
`w:tr` (or throw) and `getRow(int)`/`getNumberOfRows()` become inconsistent
with `getRows()`. Consider either (a) keeping `tableRows` aligned to top-level
`w:tr` only and handling SDT rows separately for text extraction, or (b)
refactoring row operations to remove/insert via the actual row element’s cursor
(or SDT wrapper) rather than `ctTbl.removeTr(pos)` and updating
`getRow/getNumberOfRows` semantics accordingly.
##########
poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFTable.java:
##########
@@ -168,24 +172,59 @@ public XWPFTable(CTTbl table, IBody part, boolean
initRow) {
createEmptyTable(table);
}
- for (CTRow row : table.getTrList()) {
- StringBuilder rowText = new StringBuilder();
- XWPFTableRow tabRow = new XWPFTableRow(row, this);
- tableRows.add(tabRow);
- for (CTTc cell : row.getTcList()) {
- for (CTP ctp : cell.getPList()) {
- XWPFParagraph p = new XWPFParagraph(ctp, part);
- if (rowText.length() > 0) {
- rowText.append('\t');
+ try (XmlCursor cursor = table.newCursor()) {
+ cursor.selectPath("./*");
+ while (cursor.toNextSelection()) {
+ XmlObject xmlObject = cursor.getObject();
+ if (xmlObject instanceof CTRow) {
+ processCTRow((CTRow)xmlObject);
+ }
+ else if (xmlObject instanceof CTSdtRow) {
+ List<CTRow> rows = new ArrayList<>();
+ collectCTRowsInnerSdtRow((CTSdtRow)xmlObject, rows);
+ for (CTRow row : rows)
+ {
+ processCTRow(row);
}
- rowText.append(p.getText());
}
}
- if (rowText.length() > 0) {
- this.text.append(rowText);
- this.text.append('\n');
+ }
+ }
+
+ private void processCTRow(CTRow row) {
+ StringBuilder rowText = new StringBuilder();
+ XWPFTableRow tableRow = new XWPFTableRow(row, this);
+ tableRows.add(tableRow);
+ for (CTTc cell : row.getTcList()) {
+ for (CTP ctp : cell.getPList()) {
+ XWPFParagraph p = new XWPFParagraph(ctp, part);
+ if (rowText.length() > 0) {
+ rowText.append('\t');
+ }
+ rowText.append(p.getText());
}
}
+ if (rowText.length() > 0) {
+ this.text.append(rowText);
+ this.text.append('\n');
+ }
+ }
+
+ private void collectCTRowsInnerSdtRow(CTSdtRow sdtRow, List<CTRow> rows) {
+ CTSdtContentRow sdtContent = sdtRow.getSdtContent();
+ if (sdtContent == null) {
+ return;
+ }
+
+ List<CTRow> rowsInnerSdtContent = sdtContent.getTrList();
+ if (!rowsInnerSdtContent.isEmpty()) {
+ rows.addAll(rowsInnerSdtContent);
+ return;
+ }
+
+ for (CTSdtRow innerSdt : sdtContent.getSdtList()) {
+ collectCTRowsInnerSdtRow(innerSdt, rows);
Review Comment:
`collectCTRowsInnerSdtRow` returns immediately when `sdtContent.getTrList()`
is non-empty, which skips any nested `w:sdt` elements that may appear alongside
`w:tr` within the same `CTSdtContentRow`. This will miss rows in mixed-content
SDTs and also loses document order between `tr` and nested `sdt`. Iterate
through the SDT content children in order (e.g., via an `XmlCursor` over
`sdtContent`) and collect both `CTRow` and nested `CTSdtRow` recursively
without the early return.
```suggestion
XmlCursor cursor = sdtContent.newCursor();
try {
if (!cursor.toFirstChild()) {
return;
}
do {
XmlObject child = cursor.getObject();
if (child instanceof CTRow) {
rows.add((CTRow) child);
} else if (child instanceof CTSdtRow) {
collectCTRowsInnerSdtRow((CTSdtRow) child, rows);
}
} while (cursor.toNextSibling());
} finally {
cursor.dispose();
```
##########
poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFTable.java:
##########
@@ -168,24 +172,59 @@ public XWPFTable(CTTbl table, IBody part, boolean
initRow) {
createEmptyTable(table);
}
Review Comment:
The "empty table" check only looks at `sizeOfTrArray() == 0`. A table that
contains only `w:sdt` row wrappers has no top-level `w:tr`, but it isn’t
actually empty; `createEmptyTable(table)` will inject an extra blank row when
using the default constructor (`initRow=true`). Consider extending the
condition to also check for the presence of SDT rows (e.g.,
`sizeOfSdtArray()==0`/`getSdtList().isEmpty()`), or otherwise detect whether
the table already contains any row-like content before creating a default row.
##########
poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFParagraph.java:
##########
@@ -164,6 +176,17 @@ private void buildRunsInOrderFromXml(XmlObject object) {
}
}
+ private void processCTRs(List<CTR> ctrs) {
+ if (ctrs == null) {
+ return;
+ }
+ for (CTR ctr : ctrs) {
+ if (ctr.getRPr() != null) {
Review Comment:
`processCTRs` adds `XWPFRun` objects for CTRs inside SDT content into the
paragraph’s `runs` list. Many editing APIs (`insertNewRun`,
`insertNewHyperlinkRun`, etc.) assume every entry in `runs` corresponds to a
CTR whose cursor is in the paragraph (or hyperlink/field) container; for CTRs
nested under `CTSdtContentRun`, `insertNewProvidedRun` will fail the
`isCursorInParagraph` check and return null for positions that point at these
SDT-derived runs. Either avoid adding nested SDT CTRs into `runs` (keep SDT
content accessible via `XWPFSDT`/`iruns`), or update the insertion/removal
logic and cursor positioning to support runs whose parent is SDT content so
that the `runs` list remains a valid edit surface.
```suggestion
private boolean isRunInParagraphEditContainer(CTR ctr) {
try (XmlCursor cursor = ctr.newCursor()) {
if (!cursor.toParent()) {
return false;
}
XmlObject parent = cursor.getObject();
return parent instanceof CTP
|| parent instanceof CTHyperlink
|| parent instanceof CTSimpleField;
}
}
private void processCTRs(List<CTR> ctrs) {
if (ctrs == null) {
return;
}
for (CTR ctr : ctrs) {
if (ctr.getRPr() != null && isRunInParagraphEditContainer(ctr)) {
```
##########
poi-ooxml/src/test/java/org/apache/poi/xwpf/usermodel/TestXWPFParagraph.java:
##########
@@ -44,6 +44,10 @@ Licensed to the Apache Software Foundation (ASF) under one
or more
import org.openxmlformats.schemas.wordprocessingml.x2006.main.CTPBdr;
import org.openxmlformats.schemas.wordprocessingml.x2006.main.CTPPr;
import org.openxmlformats.schemas.wordprocessingml.x2006.main.CTR;
+import org.openxmlformats.schemas.wordprocessingml.x2006.main.CTSdtBlock;
+import
org.openxmlformats.schemas.wordprocessingml.x2006.main.CTSdtContentBlock;
Review Comment:
These imports are unused in this test class (`CTSdtBlock`,
`CTSdtContentBlock`) and will cause compilation to fail with "unused import" in
Java. Remove them or add coverage that actually uses SDT blocks.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
