Bug 57171 and 57163: Adjust the active sheet in setSheetOrder() and removeSheet() for both HSSF and XSSF

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

src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java

@@ -480,6 +480,21 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss
 
         workbook.updateNamesAfterCellShift(shifter);
 
+        // adjust active sheet if necessary
+        int active = getActiveSheetIndex();
+        if(active == oldSheetIndex) {
+            // moved sheet was the active one
+            setActiveSheet(pos);
+        } else if ((active < oldSheetIndex && active < pos) ||
+                (active > oldSheetIndex && active > pos)) {
+            // not affected
+        } else if (pos > oldSheetIndex) {
+            // moved sheet was below before and is above now => active is one less
+            setActiveSheet(active-1);
+        } else {
+            // remaining case: moved sheet was higher than active before and is lower now => active is one more
+            setActiveSheet(active+1);
+        }
     }
 
     private void validateSheetIndex(int index) {
@@ -937,7 +952,6 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss
      */
     public void removeSheetAt(int index) {
         validateSheetIndex(index);
-        boolean wasActive = getSheetAt(index).isActive();
         boolean wasSelected = getSheetAt(index).isSelected();
 
         _sheets.remove(index);
@@ -954,9 +968,6 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss
         if (newSheetIndex >= nSheets) {
             newSheetIndex = nSheets-1;
         }
-        if (wasActive) {
-            setActiveSheet(newSheetIndex);
-        }
 
         if (wasSelected) {
             boolean someOtherSheetIsStillSelected = false;
@@ -970,6 +981,16 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss
                 setSelectedTab(newSheetIndex);
             }
         }
+
+        // adjust active sheet
+        int active = getActiveSheetIndex();
+        if(active == index) {
+            // removed sheet was the active one, reset active sheet if there is still one left now
+            setActiveSheet(newSheetIndex);
+        } else if (active > index) {
+            // removed sheet was below the active one => active is one less now
+            setActiveSheet(active-1);
+        }
     }
 
     /**

src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java

@@ -727,7 +727,9 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Iterable<X
         CTSheet sheet = addSheet(sheetname);
 
         int sheetNumber = 1;
-        for(XSSFSheet sh : sheets) sheetNumber = (int)Math.max(sh.sheet.getSheetId() + 1, sheetNumber);
+        for(XSSFSheet sh : sheets) {
+            sheetNumber = (int)Math.max(sh.sheet.getSheetId() + 1, sheetNumber);
+        }
 
         XSSFSheet wrapper = (XSSFSheet)createRelationship(XSSFRelation.WORKSHEET, XSSFFactory.getInstance(), sheetNumber);
         wrapper.sheet = sheet;
@@ -1072,6 +1074,27 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Iterable<X
         XSSFSheet sheet = getSheetAt(index);
         removeRelation(sheet);
         sheets.remove(index);
+
+        // only set new sheet if there are still some left
+        if(sheets.size() == 0) {
+            return;
+        }
+
+        // the index of the closest remaining sheet to the one just deleted
+        int newSheetIndex = index;
+        if (newSheetIndex >= sheets.size()) {
+            newSheetIndex = sheets.size()-1;
+        }
+
+        // adjust active sheet
+        int active = getActiveSheetIndex();
+        if(active == index) {
+            // removed sheet was the active one, reset active sheet if there is still one left now
+            setActiveSheet(newSheetIndex);
+        } else if (active > index) {
+            // removed sheet was below the active one => active is one less now
+            setActiveSheet(active-1);
+        }
     }
 
     /**
@@ -1374,6 +1397,7 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Iterable<X
     public void setSheetOrder(String sheetname, int pos) {
         int idx = getSheetIndex(sheetname);
         sheets.add(pos, sheets.remove(idx));
+
         // Reorder CTSheets
         CTSheets ct = workbook.getSheets();
         XmlObject cts = ct.getSheetArray(idx).copy();
@@ -1386,6 +1410,22 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Iterable<X
         for(int i=0; i < sheetArray.length; i++) {
             sheets.get(i).sheet = sheetArray[i];
         }
+
+        // adjust active sheet if necessary
+        int active = getActiveSheetIndex();
+        if(active == idx) {
+            // moved sheet was the active one
+            setActiveSheet(pos);
+        } else if ((active < idx && active < pos) ||
+                (active > idx && active > pos)) {
+            // not affected
+        } else if (pos > idx) {
+            // moved sheet was below before and is above now => active is one less
+            setActiveSheet(active-1);
+        } else {
+            // remaining case: moved sheet was higher than active before and is lower now => active is one more
+            setActiveSheet(active+1);
+        }
     }
 
     /**

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

@@ -29,7 +29,6 @@ import org.apache.poi.ss.util.CellRangeAddress;
 import org.apache.poi.ss.util.CellUtil;
 import org.apache.poi.xssf.XSSFITestDataProvider;
 import org.apache.poi.xssf.XSSFTestDataSamples;
-import org.junit.Test;
 
 /**
  * @author Yegor Kozlov
@@ -190,13 +189,167 @@ public final class TestXSSFSheetShiftRows extends BaseTestSheetShiftRows {
         assertEquals("Amdocs:\ntest\n", comment.getString().getString());
 	}
 
-	@Test
+	public void testBug55280() throws IOException {
-	public void testBug55280() {
         Workbook w = new XSSFWorkbook();
+        try {
             Sheet s = w.createSheet();
             for (int row = 0; row < 5000; ++row)
                 s.addMergedRegion(new CellRangeAddress(row, row, 0, 3));
 
             s.shiftRows(0, 4999, 1);        // takes a long time...
+        } finally {
+            w.close();
+        }
+	}
+
+    public void test57171() throws Exception {
+	    Workbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx");
+        assertEquals(5, wb.getActiveSheetIndex());
+        removeAllSheetsBut(5, wb); // 5 is the active / selected sheet
+        assertEquals(0, wb.getActiveSheetIndex());
+
+        Workbook wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb);
+        assertEquals(0, wbRead.getActiveSheetIndex());
+
+        wbRead.removeSheetAt(0);
+        assertEquals(0, wbRead.getActiveSheetIndex());
+
+        //wb.write(new FileOutputStream("/tmp/57171.xls"));
+    }
+
+    public void test57163() throws IOException {
+        Workbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx");
+        assertEquals(5, wb.getActiveSheetIndex());
+        wb.removeSheetAt(0);
+        assertEquals(4, wb.getActiveSheetIndex());
+
+        //wb.write(new FileOutputStream("/tmp/57163.xls"));
+    }
+
+    public void testSetSheetOrderAndAdjustActiveSheet() throws Exception {
+        Workbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx");
+        
+        assertEquals(5, wb.getActiveSheetIndex());
+
+        // move the sheets around in all possible combinations to check that the active sheet
+        // is set correctly in all cases
+        wb.setSheetOrder(wb.getSheetName(5), 4);
+        assertEquals(4, wb.getActiveSheetIndex());
+        
+        wb.setSheetOrder(wb.getSheetName(5), 5);
+        assertEquals(4, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(3), 5);
+        assertEquals(3, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(4), 5);
+        assertEquals(3, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(2), 2);
+        assertEquals(3, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(2), 1);
+        assertEquals(3, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(3), 5);
+        assertEquals(5, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(0), 5);
+        assertEquals(4, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(0), 5);
+        assertEquals(3, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(0), 5);
+        assertEquals(2, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(0), 5);
+        assertEquals(1, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(0), 5);
+        assertEquals(0, wb.getActiveSheetIndex());
+
+        wb.setSheetOrder(wb.getSheetName(0), 5);
+        assertEquals(5, wb.getActiveSheetIndex());
+    }   
+
+    public void testRemoveSheetAndAdjustActiveSheet() throws Exception {
+        Workbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx");
+        
+        assertEquals(5, wb.getActiveSheetIndex());
+        
+        wb.removeSheetAt(0);
+        assertEquals(4, wb.getActiveSheetIndex());
+        
+        wb.setActiveSheet(3);
+        assertEquals(3, wb.getActiveSheetIndex());
+        
+        wb.removeSheetAt(4);
+        assertEquals(3, wb.getActiveSheetIndex());
+
+        wb.removeSheetAt(3);
+        assertEquals(2, wb.getActiveSheetIndex());
+
+        wb.removeSheetAt(0);
+        assertEquals(1, wb.getActiveSheetIndex());
+
+        wb.removeSheetAt(1);
+        assertEquals(0, wb.getActiveSheetIndex());
+
+        wb.removeSheetAt(0);
+        assertEquals(0, wb.getActiveSheetIndex());
+
+        try {
+            wb.removeSheetAt(0);
+            fail("Should catch exception as no more sheets are there");
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+        assertEquals(0, wb.getActiveSheetIndex());
+        
+        wb.createSheet();
+        assertEquals(0, wb.getActiveSheetIndex());
+        
+        wb.removeSheetAt(0);
+        assertEquals(0, wb.getActiveSheetIndex());
+    }
+
+    // TODO: enable when bug 57165 is fixed
+    public void disabled_test57165() throws IOException {
+        Workbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx");
+        assertEquals(5, wb.getActiveSheetIndex());
+        removeAllSheetsBut(3, wb);
+        assertEquals(0, wb.getActiveSheetIndex());
+        wb.createSheet("New Sheet1");
+        assertEquals(0, wb.getActiveSheetIndex());
+        wb.cloneSheet(0); // Throws exception here
+        wb.setSheetName(1, "New Sheet");
+        assertEquals(0, wb.getActiveSheetIndex());
+
+        //wb.write(new FileOutputStream("/tmp/57165.xls"));
+    }
+
+//    public void test57165b() throws IOException {
+//        Workbook wb = new XSSFWorkbook();
+//        try {
+//            wb.createSheet("New Sheet 1");
+//            wb.createSheet("New Sheet 2");
+//        } finally {
+//            wb.close();
+//        }
+//    }
+
+    private static void removeAllSheetsBut(int sheetIndex, Workbook wb)
+    {
+        int sheetNb = wb.getNumberOfSheets();
+        // Move this sheet at the first position
+        wb.setSheetOrder(wb.getSheetName(sheetIndex), 0);
+        // Must make this sheet active (otherwise, for XLSX, Excel might protest that active sheet no longer exists)
+        // I think POI should automatically handle this case when deleting sheets...
+//      wb.setActiveSheet(0);
+        for (int sn = sheetNb - 1; sn > 0; sn--)
+        {
+            wb.removeSheetAt(sn);
+        }
     }
 }

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

@@ -192,20 +192,45 @@ public abstract class BaseTestWorkbook {
         workbook.createSheet("sheet2");
         workbook.createSheet("sheet3");
         assertEquals(3, workbook.getNumberOfSheets());
+
+        assertEquals(0, workbook.getActiveSheetIndex());
+
         workbook.removeSheetAt(1);
         assertEquals(2, workbook.getNumberOfSheets());
         assertEquals("sheet3", workbook.getSheetName(1));
+        assertEquals(0, workbook.getActiveSheetIndex());
+
         workbook.removeSheetAt(0);
         assertEquals(1, workbook.getNumberOfSheets());
         assertEquals("sheet3", workbook.getSheetName(0));
+        assertEquals(0, workbook.getActiveSheetIndex());
+
         workbook.removeSheetAt(0);
         assertEquals(0, workbook.getNumberOfSheets());
+        assertEquals(0, workbook.getActiveSheetIndex());
 
         //re-create the sheets
         workbook.createSheet("sheet1");
         workbook.createSheet("sheet2");
         workbook.createSheet("sheet3");
-        assertEquals(3, workbook.getNumberOfSheets());
+        workbook.createSheet("sheet4");
+        assertEquals(4, workbook.getNumberOfSheets());
+
+        assertEquals(0, workbook.getActiveSheetIndex());
+        workbook.setActiveSheet(2);
+        assertEquals(2, workbook.getActiveSheetIndex());
+
+        workbook.removeSheetAt(2);
+        assertEquals(2, workbook.getActiveSheetIndex());
+
+        workbook.removeSheetAt(1);
+        assertEquals(1, workbook.getActiveSheetIndex());
+
+        workbook.removeSheetAt(0);
+        assertEquals(0, workbook.getActiveSheetIndex());
+
+        workbook.removeSheetAt(0);
+        assertEquals(0, workbook.getActiveSheetIndex());
     }
 
     @Test
@@ -287,11 +312,18 @@ public abstract class BaseTestWorkbook {
         assertEquals(8, wb.getSheetIndex("Sheet 8"));
         assertEquals(9, wb.getSheetIndex("Sheet 9"));
 
+        // check active sheet
+        assertEquals(0, wb.getActiveSheetIndex());
+        
         // Change
         wb.setSheetOrder("Sheet 6", 0);
+        assertEquals(1, wb.getActiveSheetIndex());
         wb.setSheetOrder("Sheet 3", 7);
         wb.setSheetOrder("Sheet 1", 9);
         
+        // now the first sheet is at index 1
+        assertEquals(1, wb.getActiveSheetIndex());
+
         // Check they're currently right
         assertEquals(0, wb.getSheetIndex("Sheet 6"));
         assertEquals(1, wb.getSheetIndex("Sheet 0"));
@@ -317,6 +349,8 @@ public abstract class BaseTestWorkbook {
         assertEquals(8, wbr.getSheetIndex("Sheet 9"));
         assertEquals(9, wbr.getSheetIndex("Sheet 1"));
 
+        assertEquals(1, wb.getActiveSheetIndex());
+        
         // Now get the index by the sheet, not the name
         for(int i=0; i<10; i++) {
         	Sheet s = wbr.getSheetAt(i);