ASF Bugzilla – Attachment 33329 Details for
Bug 56454
XSSFSheet.shiftRows(...) and HSSFSheet.shiftRows(...) incorrectly handle merged regions that do not contain column 0.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Progress towards a fix
bug56454.patch (text/plain), 13.98 KB, created by
Javen O'Neal
on 2015-12-06 21:18:18 UTC
(
hide
)
Description:
Progress towards a fix
Filename:
MIME Type:
Creator:
Javen O'Neal
Created:
2015-12-06 21:18:18 UTC
Size:
13.98 KB
patch
obsolete
>Index: src/ooxml/java/org/apache/poi/xssf/usermodel/helpers/XSSFRowShifter.java >=================================================================== >--- src/ooxml/java/org/apache/poi/xssf/usermodel/helpers/XSSFRowShifter.java (revision 1718218) >+++ src/ooxml/java/org/apache/poi/xssf/usermodel/helpers/XSSFRowShifter.java (working copy) >@@ -18,9 +18,12 @@ > package org.apache.poi.xssf.usermodel.helpers; > > import java.util.ArrayList; >-import java.util.HashSet; >+import java.util.Collection; >+import java.util.Collections; > import java.util.List; >-import java.util.Set; >+import java.util.Map; >+import java.util.Map.Entry; >+import java.util.TreeMap; > > import org.apache.poi.ss.formula.FormulaParseException; > import org.apache.poi.ss.formula.FormulaParser; >@@ -71,59 +74,118 @@ > * @return an array of affected cell regions > */ > public List<CellRangeAddress> shiftMerged(int startRow, int endRow, int n) { >- List<CellRangeAddress> shiftedRegions = new ArrayList<CellRangeAddress>(); >- Set<Integer> removedIndices = new HashSet<Integer>(); >+ Map<Integer, CellRangeAddress> modifiedRegions = new TreeMap<Integer, CellRangeAddress>(); >+ List<CellRangeAddress> removedRegions = new ArrayList<CellRangeAddress>(); >+ Collection<Integer> modifiedIndices = new ArrayList<Integer>(); //overwritten >+ >+ if (n == 0) { >+ return Collections.emptyList(); >+ } >+ >+ int destStartRow = startRow + n; >+ int destEndRow = endRow + n; >+ > //move merged regions completely if they fall within the new region boundaries when they are shifted >- int size = sheet.getNumMergedRegions(); >- for (int i = 0; i < size; i++) { >+ final int numMergedRegions = sheet.getNumMergedRegions(); >+ for (int i = 0; i < numMergedRegions; i++) { > CellRangeAddress merged = sheet.getMergedRegion(i); >- >- boolean inStart = (merged.getFirstRow() >= startRow || merged.getLastRow() >= startRow); >- boolean inEnd = (merged.getFirstRow() <= endRow || merged.getLastRow() <= endRow); >- >- //don't check if it's not within the shifted area >- if (!inStart || !inEnd) { >+ >+ // Behavior: Unaffected >+ // Condition: if shift source and destination rows do not overlap with merged region >+ boolean mergedRegionNotInSourceRows = (endRow < merged.getFirstRow() || merged.getLastRow() < startRow); >+ boolean mergedRegionNotInDestRows = (destEndRow < merged.getFirstRow() || merged.getLastRow() < destStartRow); >+ if (mergedRegionNotInSourceRows && mergedRegionNotInDestRows) { > continue; > } >- >- //only shift if the region outside the shifted rows is not merged too >- if (!containsCell(merged, startRow - 1, 0) && !containsCell(merged, endRow + 1, 0)) { >+ // Condition: or if source and destination rows fully inside merged region >+ boolean sourceRowsInMergedRegion = (merged.getFirstRow() < startRow && endRow < merged.getLastRow()); >+ boolean destRowsInMergedRegion = (merged.getFirstRow() < destStartRow && destEndRow < merged.getLastRow()); >+ if (sourceRowsInMergedRegion && destRowsInMergedRegion) { >+ continue; >+ } >+ >+ // CellRangeAddresses cannot be modified in place. >+ // At this point, any CellRangeAddress will be modified or removed. >+ modifiedIndices.add(i); >+ >+ // Behavior: Merged region gets deleted >+ // Condition: if merged region is outside shift source rows but fully or partially contained within shift destination rows >+ // Condition: if destination rows contain merged regions, merged regions would be shrunk or broken up. Remove them if there's any conflict. >+ boolean mergedRegionInDestRows = (destStartRow <= merged.getLastRow() && merged.getFirstRow() <= destEndRow); >+ if (mergedRegionInDestRows) { >+ removedRegions.add(i, merged); >+ continue; >+ } >+ >+ // At this point, any CellRangeAddress will be modified. >+ modifiedRegions.put(i, merged); >+ >+ // Behavior: Entire merged region gets shifted down >+ // Condition: if shift source rows fully contain the merged region >+ if (startRow <= merged.getFirstRow() && merged.getLastRow() <= endRow) { > merged.setFirstRow(merged.getFirstRow() + n); > merged.setLastRow(merged.getLastRow() + n); >- //have to remove/add it back >- shiftedRegions.add(merged); >- removedIndices.add(i); >+ continue; > } >+ >+ // Behavior: Merged region gets elongated >+ // Condition: if startRow is inside the merged region and not the first row of the region (which would result in a shift) and n>0 >+ if (n > 0) { >+ boolean startRowInMergedRegion = merged.getFirstRow() < startRow && startRow <= merged.getLastRow(); >+ boolean endRowAfterMergedRegion = merged.getLastRow() <= endRow; >+ if (startRowInMergedRegion && endRowAfterMergedRegion) { >+ merged.setLastRow(merged.getLastRow() + n); >+ continue; >+ } >+ } >+ else { >+ // Condition: if endRow is inside the merged region and not the last row of the region (which would result in a shift) and n>0 >+ boolean endRowInMergedRegion = merged.getFirstRow() <= endRow && endRow < merged.getLastRow(); >+ boolean startRowBeforeMergedRegion = startRow <= merged.getFirstRow(); >+ if (endRowInMergedRegion && startRowBeforeMergedRegion) { >+ merged.setFirstRow(merged.getFirstRow() + n); >+ continue; >+ } >+ } >+ >+ // Behavior: Merged region gets shrunk >+ // Condition: startRow is moved down or endRow is moved up >+ boolean mergedRegionShrunkFromTop = (startRow == merged.getFirstRow() && merged.getLastRow() <= destEndRow); >+ if (mergedRegionShrunkFromTop) { >+ merged.setFirstRow(destStartRow); >+ continue; >+ } >+ boolean mergedRegionShrunkFromBottom = (destStartRow <= merged.getLastRow() && merged.getLastRow() == endRow); >+ if (mergedRegionShrunkFromBottom) { >+ merged.setLastRow(destEndRow); >+ continue; >+ } > } > >- if(!removedIndices.isEmpty()) { >- sheet.removeMergedRegions(removedIndices); >+ // probably need to replace all merged regions atomically and only verify if there are >+ // overlapping merged regions after all replacements and removals are done, then either >+ // roll back all changes and throw an exception if there are overlapping merged regions >+ // or delete the merged regions that are invalidated due to the shift >+ for (Entry<Integer, CellRangeAddress> entry : modifiedRegions.entrySet()) { >+ sheet.replaceMergedRegion(entry.getKey(), entry.getValue()); > } >+ >+ // Remove all merged regions that were modified or deleted >+ if(!modifiedIndices.isEmpty()) { >+ sheet.removeMergedRegions(modifiedIndices); >+ } > >- //read so it doesn't get shifted again >- for (CellRangeAddress region : shiftedRegions) { >+ /*// Add back merged regions that were modified >+ for (CellRangeAddress region : modifiedRegions) { > sheet.addMergedRegion(region); >- } >- return shiftedRegions; >+ }*/ >+ // Include deleted regions in the return value >+ List<CellRangeAddress> affectedRegions = new ArrayList<CellRangeAddress>(modifiedRegions.values()); >+ affectedRegions.addAll(removedRegions); >+ return affectedRegions; > } > > /** >- * Check if the row and column are in the specified cell range >- * >- * @param cr the cell range to check in >- * @param rowIx the row to check >- * @param colIx the column to check >- * @return true if the range contains the cell [rowIx,colIx] >- */ >- private static boolean containsCell(CellRangeAddress cr, int rowIx, int colIx) { >- if (cr.getFirstRow() <= rowIx && cr.getLastRow() >= rowIx >- && cr.getFirstColumn() <= colIx && cr.getLastColumn() >= colIx) { >- return true; >- } >- return false; >- } >- >- /** > * Updated named ranges > */ > public void updateNamedRanges(FormulaShifter shifter) { >Index: src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java >=================================================================== >--- src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java (revision 1718218) >+++ src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java (working copy) >@@ -396,6 +396,24 @@ > } > } > } >+ >+ /** >+ * Verify that candidate region does not intersect with an existing, non-ignored merged region in this sheet >+ * >+ * @param candidateRegion >+ * @param indexToIgnore the index of the merged region to ignore >+ * @throws IllegalStateException if candidate region intersects an existing, non-ignored merged region in this sheet >+ */ >+ private void validateMergedRegions(CellRangeAddress candidateRegion, int indexToIgnore) { >+ List<CellRangeAddress> mergedRegions = getMergedRegions(); >+ mergedRegions.remove(indexToIgnore); >+ for (final CellRangeAddress existingRegion : getMergedRegions()) { >+ if (existingRegion.intersects(candidateRegion)) { >+ throw new IllegalStateException("Cannot add merged region " + candidateRegion.formatAsString() + >+ " to sheet because it overlaps with an existing merged region (" + existingRegion.formatAsString() + ")."); >+ } >+ } >+ } > > /** > * Adjusts the column width to fit the contents. >@@ -1154,6 +1172,8 @@ > * Returns the merged region at the specified index. If you want multiple > * regions, it is faster to call {@link #getMergedRegions()} than to call > * this each time. >+ * Modifying the returned merged region will not affect the merged regions in the sheet. >+ * FIXME: Perhaps this should return an UnmodifiableCellRangeAddress to eliminate confusion? > * > * @return the merged region at the specified index > */ >@@ -1751,7 +1771,7 @@ > worksheet.unsetMergeCells(); > } > } >- >+ > /** > * Removes a number of merged regions of cells (hence letting them free) > * >@@ -1769,7 +1789,7 @@ > List<CTMergeCell> newMergeCells = new ArrayList<CTMergeCell>(ctMergeCells.sizeOfMergeCellArray()); > > int idx = 0; >- for (CTMergeCell mc : ctMergeCells.getMergeCellArray()) { >+ for (CTMergeCell mc : ctMergeCells.getMergeCellList()) { > if (!indices.contains(idx++)) newMergeCells.add(mc); > } > >@@ -1780,6 +1800,39 @@ > ctMergeCells.setMergeCellArray(newMergeCells.toArray(newMergeCellsArray)); > } > } >+ >+ /** >+ * Replace a merged region with a different merged region. >+ * This is faster than calling <code>removeMergedRegion(int)</code> >+ * and <code>addMergedRegion(CellRangeAddress)</code> >+ * >+ * @param index of the region to unmerge >+ * @param >+ */ >+ //@Override >+ public void replaceMergedRegion(int index, CellRangeAddress region) { >+ region.validate(SpreadsheetVersion.EXCEL2007); >+ >+ // throw IllegalStateException if the argument CellRangeAddress intersects with >+ // a multi-cell array formula defined in this sheet >+ validateArrayFormulas(region); >+ >+ if (!worksheet.isSetMergeCells()) { >+ throw new IllegalStateException("Worksheet has no merged cells to replace."); >+ } >+ >+ CTMergeCells ctMergeCells = worksheet.getMergeCells(); >+ int size = ctMergeCells.sizeOfMergeCellArray(); >+ assert(0 <= index && index < size); >+ CTMergeCell ctMergeCell = ctMergeCells.getMergeCellArray(index); >+ >+ // Throw IllegalStateException if the argument CellRangeAddress intersects with >+ // a merged region already in this sheet >+ validateMergedRegions(region, index); >+ >+ String ref = region.formatAsString(); >+ ctMergeCell.setRef(ref); >+ } > > /** > * Remove a row from this sheet. All cells contained in the row are removed as well >Index: src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java >=================================================================== >--- src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java (revision 1718218) >+++ src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java (working copy) >@@ -1385,8 +1385,8 @@ > } > > //only shift if the region outside the shifted rows is not merged too >- if (!SheetUtil.containsCell(merged, startRow - 1, 0) && >- !SheetUtil.containsCell(merged, endRow + 1, 0)) { >+ if (!merged.containsRow(startRow - 1) && >+ !merged.containsRow(endRow + 1)) { > merged.setFirstRow(merged.getFirstRow() + n); > merged.setLastRow(merged.getLastRow() + n); > //have to remove/add it back >Index: src/java/org/apache/poi/ss/util/SheetUtil.java >=================================================================== >--- src/java/org/apache/poi/ss/util/SheetUtil.java (revision 1718221) >+++ src/java/org/apache/poi/ss/util/SheetUtil.java (working copy) >@@ -325,6 +325,7 @@ > * @param rowIx the row to check > * @param colIx the column to check > * @return true if the range contains the cell [rowIx, colIx] >+ * @deprecated 3.14beta2. Use {@link CellRangeAddressBase#isInRange(int, int)} instead. > */ > public static boolean containsCell(CellRangeAddress cr, int rowIx, int colIx) { > return cr.isInRange(rowIx, colIx);
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 56454
:
31556
|
33328
|
33329
|
33330