Fix 60384 and 60709: When shifting with merged regions we should correctly check if the region is moved along or needs to be removed because it is not fully kept via the shifting. This was broken by the fix for bug 59740, now additional unit tests ensure that it works better.

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1805518 13f79535-47bb-0310-9956-ffa450edef68

src/java/org/apache/poi/ss/usermodel/helpers/RowShifter.java

@@ -27,8 +27,6 @@ import org.apache.poi.ss.usermodel.Row;
 import org.apache.poi.ss.usermodel.Sheet;
 import org.apache.poi.ss.util.CellRangeAddress;
 import org.apache.poi.util.Internal;
-import org.apache.poi.util.POILogFactory;
-import org.apache.poi.util.POILogger;
 
 /**
  * Helper for shifting rows up or down
@@ -59,8 +57,9 @@ public abstract class RowShifter {
         for (int i = 0; i < size; i++) {
             CellRangeAddress merged = sheet.getMergedRegion(i);
 
-            // remove merged region that overlaps shifting
-            if (startRow + n <= merged.getFirstRow() && endRow + n >= merged.getLastRow()) {
+            // remove merged region that are replaced by the shifting,
+            // i.e. where the area includes something in the overwritten area
+            if(removalNeeded(merged, startRow, endRow, n)) {
                 removedIndices.add(i);
                 continue;
             }
@@ -94,6 +93,24 @@ public abstract class RowShifter {
         return shiftedRegions;
     }
 
+    private boolean removalNeeded(CellRangeAddress merged, int startRow, int endRow, int n) {
+        final int movedRows = endRow - startRow + 1;
+
+        // build a range of the rows that are overwritten, i.e. the target-area, but without
+        // rows that are moved along
+        final CellRangeAddress overwrite;
+        if(n > 0) {
+            // area is moved down => overwritten area is [endRow + n - movedRows, endRow + n]
+            overwrite = new CellRangeAddress(Math.max(endRow + 1, endRow + n - movedRows), endRow + n, 0, 0);
+        } else {
+            // area is moved up => overwritten area is [startRow + n, startRow + n + movedRows]
+            overwrite = new CellRangeAddress(startRow + n, Math.min(startRow - 1, startRow + n + movedRows), 0, 0);
+        }
+
+        // if the merged-region and the overwritten area intersect, we need to remove it
+        return merged.intersects(overwrite);
+    }
+
     /**
      * Updated named ranges
      */

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java

@@ -17,22 +17,7 @@
 
 package org.apache.poi.xssf.usermodel;
 
-import static org.apache.poi.POITestCase.skipTest;
-import static org.apache.poi.POITestCase.testPassesNow;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.fail;
-
-import java.io.IOException;
-
-import org.apache.poi.ss.usermodel.BaseTestSheetShiftRows;
-import org.apache.poi.ss.usermodel.Cell;
-import org.apache.poi.ss.usermodel.CellType;
-import org.apache.poi.ss.usermodel.Comment;
-import org.apache.poi.ss.usermodel.Row;
-import org.apache.poi.ss.usermodel.Sheet;
-import org.apache.poi.ss.usermodel.Workbook;
+import org.apache.poi.ss.usermodel.*;
 import org.apache.poi.ss.util.CellAddress;
 import org.apache.poi.ss.util.CellUtil;
 import org.apache.poi.util.IOUtils;
@@ -41,6 +26,12 @@ import org.apache.poi.xssf.XSSFTestDataSamples;
 import org.apache.xmlbeans.impl.values.XmlValueDisconnectedException;
 import org.junit.Test;
 
+import java.io.IOException;
+
+import static org.apache.poi.POITestCase.skipTest;
+import static org.apache.poi.POITestCase.testPassesNow;
+import static org.junit.Assert.*;
+
 public final class TestXSSFSheetShiftRows extends BaseTestSheetShiftRows {
 
     public TestXSSFSheetShiftRows(){
@@ -413,7 +404,6 @@ public final class TestXSSFSheetShiftRows extends BaseTestSheetShiftRows {
             skipTest(e);
         }
         
-
         workbook.close();
     }
 
@@ -486,4 +476,62 @@ public final class TestXSSFSheetShiftRows extends BaseTestSheetShiftRows {
         sheet.shiftRows(1, 2, 3);
         IOUtils.closeQuietly(wb);
     }
+
+    @Test
+    public void test60384() throws IOException {
+        XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("60384.xlsx");
+        XSSFSheet sheet = wb.getSheetAt(0);
+
+        assertEquals(2, sheet.getMergedRegions().size());
+        assertEquals(7, sheet.getMergedRegion(0).getFirstRow());
+        assertEquals(7, sheet.getMergedRegion(0).getLastRow());
+        assertEquals(8, sheet.getMergedRegion(1).getFirstRow());
+        assertEquals(8, sheet.getMergedRegion(1).getLastRow());
+
+        sheet.shiftRows(3, 8, 1);
+
+        // after shifting, the two named regions should still be there as they
+        // are fully inside the shifted area
+        assertEquals(2, sheet.getMergedRegions().size());
+        assertEquals(8, sheet.getMergedRegion(0).getFirstRow());
+        assertEquals(8, sheet.getMergedRegion(0).getLastRow());
+        assertEquals(9, sheet.getMergedRegion(1).getFirstRow());
+        assertEquals(9, sheet.getMergedRegion(1).getLastRow());
+
+        /*OutputStream out = new FileOutputStream("/tmp/60384.xlsx");
+        try {
+            wb.write(out);
+        } finally {
+            out.close();
+        }*/
+
+        wb.close();
+    }
+
+    @Test
+    public void test60709() throws IOException {
+        XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("60709.xlsx");
+        XSSFSheet sheet = wb.getSheetAt(0);
+
+        assertEquals(1, sheet.getMergedRegions().size());
+        assertEquals(2, sheet.getMergedRegion(0).getFirstRow());
+        assertEquals(2, sheet.getMergedRegion(0).getLastRow());
+
+        sheet.shiftRows(1, sheet.getLastRowNum()+1, -1, true, false);
+
+        // after shifting, the two named regions should still be there as they
+        // are fully inside the shifted area
+        assertEquals(1, sheet.getMergedRegions().size());
+        assertEquals(1, sheet.getMergedRegion(0).getFirstRow());
+        assertEquals(1, sheet.getMergedRegion(0).getLastRow());
+
+        /*OutputStream out = new FileOutputStream("/tmp/60709.xlsx");
+        try {
+            wb.write(out);
+        } finally {
+            out.close();
+        }*/
+
+        wb.close();
+    }
 }

src/testcases/org/apache/poi/ss/usermodel/BaseTestSheetShiftRows.java

@@ -502,8 +502,6 @@ public abstract class BaseTestSheetShiftRows {
      * Unified test for:
      * bug 46742: XSSFSheet.shiftRows should shift hyperlinks
      * bug 52903: HSSFSheet.shiftRows should shift hyperlinks
-     *
-     * @throws IOException
      */
     @Test
     public void testBug46742_52903_shiftHyperlinks() throws IOException {
@@ -642,7 +640,7 @@ public abstract class BaseTestSheetShiftRows {
     public void shiftMergedRowsToMergedRowsUp() throws IOException {
         Workbook wb = _testDataProvider.createWorkbook();
         Sheet sheet = wb.createSheet("test");
-        populateSheetCells(sheet);
+        populateSheetCells(sheet, 2);
 
 
         CellRangeAddress A1_E1 = new CellRangeAddress(0, 0, 0, 4);
@@ -661,9 +659,55 @@ public abstract class BaseTestSheetShiftRows {
         wb.close();
     }
 
-    private void populateSheetCells(Sheet sheet) {
+    @Test
+    public void shiftMergedRowsToMergedRowsOverlappingMergedRegion() throws IOException {
+        Workbook wb = _testDataProvider.createWorkbook();
+        Sheet sheet = wb.createSheet("test");
+        populateSheetCells(sheet, 10);
+
+        CellRangeAddress A1_E1 = new CellRangeAddress(0, 0, 0, 4);
+        CellRangeAddress A2_C2 = new CellRangeAddress(1, 7, 0, 2);
+
+        sheet.addMergedRegion(A1_E1);
+        sheet.addMergedRegion(A2_C2);
+
+        // A1:E1 should move to A5:E5
+        // A2:C2 should be removed
+        sheet.shiftRows(0, 0, 4);
+
+        assertEquals(1, sheet.getNumMergedRegions());
+        assertEquals(CellRangeAddress.valueOf("A5:E5"), sheet.getMergedRegion(0));
+
+        wb.close();
+    }
+
+    @Test
+    public void bug60384ShiftMergedRegion() throws IOException {
+        Workbook wb = _testDataProvider.createWorkbook();
+        Sheet sheet = wb.createSheet("test");
+        populateSheetCells(sheet, 9);
+
+
+        CellRangeAddress A8_E8 = new CellRangeAddress(7, 7, 0, 4);
+        CellRangeAddress A9_C9 = new CellRangeAddress(8, 8, 0, 2);
+
+        sheet.addMergedRegion(A8_E8);
+        sheet.addMergedRegion(A9_C9);
+
+        // A1:E1 should be removed
+        // A2:C2 will be A1:C1
+        sheet.shiftRows(3, sheet.getLastRowNum(), 1);
+
+        assertEquals(2, sheet.getNumMergedRegions());
+        assertEquals(CellRangeAddress.valueOf("A9:E9"), sheet.getMergedRegion(0));
+        assertEquals(CellRangeAddress.valueOf("A10:C10"), sheet.getMergedRegion(1));
+
+        wb.close();
+    }
+
+    private void populateSheetCells(Sheet sheet, int rowCount) {
         // populate sheet cells
-        for (int i = 0; i < 2; i++) {
+        for (int i = 0; i < rowCount; i++) {
             Row row = sheet.createRow(i);
             for (int j = 0; j < 5; j++) {
                 Cell cell = row.createCell(j);
@@ -678,7 +722,7 @@ public abstract class BaseTestSheetShiftRows {
         Sheet sheet = wb.createSheet("test");
 
         // populate sheet cells
-        populateSheetCells(sheet);
+        populateSheetCells(sheet, 2);
 
         CellRangeAddress A1_E1 = new CellRangeAddress(0, 0, 0, 4);
         CellRangeAddress A2_C2 = new CellRangeAddress(1, 1, 0, 2);