diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java b/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java index da741d4..4b37d9e 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java +++ b/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); + } } /** diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java index 119f775..88581d9 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java @@ -727,7 +727,9 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Iterable= 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 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); + } } /** diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java index d3729e0..e949dd1 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java +++ b/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() { + public void testBug55280() throws IOException { Workbook w = new XSSFWorkbook(); - Sheet s = w.createSheet(); - for (int row = 0; row < 5000; ++row) - s.addMergedRegion(new CellRangeAddress(row, row, 0, 3)); + 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... + 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); + } + } } diff --git a/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java b/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java index ce3b787..1d1184a 100644 --- a/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java +++ b/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,10 +312,17 @@ 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")); @@ -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); diff --git a/test-data/spreadsheet/57171_57163_57165.xlsx b/test-data/spreadsheet/57171_57163_57165.xlsx new file mode 100644 index 0000000..5d94a47 Binary files /dev/null and b/test-data/spreadsheet/57171_57163_57165.xlsx differ