From c3cc69dc57db018a95d22beccfde24b2cc7191b7 Mon Sep 17 00:00:00 2001 From: Nick Burch Date: Fri, 7 Mar 2008 11:18:02 +0000 Subject: [PATCH] Patch from Josh from bug #43901 - Correctly update the internal last cell number when adding and removing cells (previously sometimes off-by-one) git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@634617 13f79535-47bb-0310-9956-ffa450edef68 --- src/documentation/content/xdocs/changes.xml | 1 + src/documentation/content/xdocs/status.xml | 1 + .../apache/poi/hssf/usermodel/HSSFRow.java | 155 ++++++++--------- .../apache/poi/hssf/usermodel/TestBugs.java | 23 --- .../poi/hssf/usermodel/TestHSSFRow.java | 160 +++++++++--------- 5 files changed, 164 insertions(+), 176 deletions(-) diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 99d81b0aec..bd4f7dab03 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -36,6 +36,7 @@ + 43901 - Correctly update the internal last cell number when adding and removing cells (previously sometimes off-by-one) 28231 - For apparently truncated files, which are somehow still valid, now issue a truncation warning but carry on, rather than giving an exception as before 44504 - Added initial support for recognising external functions like YEARFRAC and ISEVEN (using NameXPtg), via LinkTable support 44504 - Improvements to FormulaParser - operators, precedence, error literals, quotes in string literals, range checking on IntPtg, formulas with extra un-parsed stuff at the end, improved parse error handling diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 6fcbfee45c..62d0a8dceb 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -33,6 +33,7 @@ + 43901 - Correctly update the internal last cell number when adding and removing cells (previously sometimes off-by-one) 44504 - Added initial support for recognising external functions like YEARFRAC and ISEVEN (using NameXPtg), via LinkTable support 44504 - Improvements to FormulaParser - operators, precedence, error literals, quotes in string literals, range checking on IntPtg, formulas with extra un-parsed stuff at the end, improved parse error handling 44504 - Fixed number conversion inconsistencies in many functions, and improved RefEval diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFRow.java b/src/java/org/apache/poi/hssf/usermodel/HSSFRow.java index c759bcb46b..30bdc01f51 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFRow.java +++ b/src/java/org/apache/poi/hssf/usermodel/HSSFRow.java @@ -15,11 +15,6 @@ limitations under the License. ==================================================================== */ -/* - * HSSFRow.java - * - * Created on September 30, 2001, 3:44 PM - */ package org.apache.poi.hssf.usermodel; import java.util.Iterator; @@ -38,10 +33,7 @@ import org.apache.poi.hssf.record.RowRecord; * @author Andrew C. Oliver (acoliver at apache dot org) * @author Glen Stampoultzis (glens at apache.org) */ - -public class HSSFRow - implements Comparable -{ +public final class HSSFRow implements Comparable { // used for collections public final static int INITIAL_CAPACITY = 5; @@ -157,26 +149,31 @@ public class HSSFRow * @param cell to remove */ public void removeCell(HSSFCell cell) { - removeCell(cell, true); + if(cell == null) { + throw new IllegalArgumentException("cell must not be null"); + } + removeCell(cell, true); } private void removeCell(HSSFCell cell, boolean alsoRemoveRecords) { - if(alsoRemoveRecords) { - CellValueRecordInterface cval = cell.getCellValueRecord(); - sheet.removeValueRecord(getRowNum(), cval); - } - + short column=cell.getCellNum(); - if(cell!=null && column= cells.length || cell != cells[column]) { + throw new RuntimeException("Specified cell is not from this row"); } - if (cell.getCellNum() == row.getFirstCol()) - { + cells[column]=null; + + if(alsoRemoveRecords) { + CellValueRecordInterface cval = cell.getCellValueRecord(); + sheet.removeValueRecord(getRowNum(), cval); + } + + if (cell.getCellNum()+1 == row.getLastCol()) { + row.setLastCol((short) (findLastCell(row.getLastCol())+1)); + } + if (cell.getCellNum() == row.getFirstCol()) { row.setFirstCol(findFirstCell(row.getFirstCol())); } } @@ -234,7 +231,7 @@ public class HSSFRow * TODO - Should this really be public? */ protected int getOutlineLevel() { - return row.getOutlineLevel(); + return row.getOutlineLevel(); } /** @@ -244,55 +241,48 @@ public class HSSFRow * @param newColumn The new column number (0 based) */ public void moveCell(HSSFCell cell, short newColumn) { - // Ensure the destination is free - if(cells.length > newColumn && cells[newColumn] != null) { - throw new IllegalArgumentException("Asked to move cell to column " + newColumn + " but there's already a cell there"); - } - - // Check it's one of ours - if(! cells[cell.getCellNum()].equals(cell)) { - throw new IllegalArgumentException("Asked to move a cell, but it didn't belong to our row"); - } - - // Move the cell to the new position - // (Don't remove the records though) - removeCell(cell, false); - cell.updateCellNum(newColumn); - addCell(cell); + // Ensure the destination is free + if(cells.length > newColumn && cells[newColumn] != null) { + throw new IllegalArgumentException("Asked to move cell to column " + newColumn + " but there's already a cell there"); + } + + // Check it's one of ours + if(! cells[cell.getCellNum()].equals(cell)) { + throw new IllegalArgumentException("Asked to move a cell, but it didn't belong to our row"); + } + + // Move the cell to the new position + // (Don't remove the records though) + removeCell(cell, false); + cell.updateCellNum(newColumn); + addCell(cell); } /** * used internally to add a cell. */ - private void addCell(HSSFCell cell) - { - short column=cell.getCellNum(); - if (row.getFirstCol() == -1) - { - row.setFirstCol(column); - } - if (row.getLastCol() == -1) - { - row.setLastCol(column); - } + private void addCell(HSSFCell cell) { - if(column>=cells.length) - { - HSSFCell[] oldCells=cells; - int newSize=oldCells.length*2; - if(newSize=cells.length) { + HSSFCell[] oldCells=cells; + int newSize=oldCells.length*2; + if(newSize row.getLastCol()) - { - row.setLastCol(column); + + if (row.getLastCol() == -1 || column >= row.getLastCol()) { + row.setLastCol((short) (column+1)); // +1 -> for one past the last index } } @@ -324,16 +314,29 @@ public class HSSFRow } /** - * gets the number of the last cell contained in this row PLUS ONE. - * @return short representing the last logical cell in the row PLUS ONE, or -1 if the row does not contain any cells. + * Gets the index of the last cell contained in this row PLUS ONE. The result also + * happens to be the 1-based column number of the last cell. This value can be used as a + * standard upper bound when iterating over cells: + * 
 
+     * short minColIx = row.getFirstCellNum();
+     * short maxColIx = row.getLastCellNum();
+     * for(short colIx=minColIx; colIx<maxColIx; colIx++) {
+     *   HSSFCell cell = row.getCell(colIx);
+     *   if(cell == null) {
+     *     continue;
+     *   }
+     *   //... do something with cell
+     * }
+     *
+ * + * @return short representing the last logical cell in the row PLUS ONE, or -1 if the + * row does not contain any cells. */ - - public short getLastCellNum() - { - if (getPhysicalNumberOfCells() == 0) + public short getLastCellNum() { + if (getPhysicalNumberOfCells() == 0) { return -1; - else - return row.getLastCol(); + } + return row.getLastCol(); } @@ -493,8 +496,8 @@ public class HSSFRow } public Object next() { - if (!hasNext()) - throw new NoSuchElementException("At last element"); + if (!hasNext()) + throw new NoSuchElementException("At last element"); HSSFCell cell=cells[nextId]; thisId=nextId; findNext(); @@ -502,8 +505,8 @@ public class HSSFRow } public void remove() { - if (thisId == -1) - throw new IllegalStateException("remove() called before next()"); + if (thisId == -1) + throw new IllegalStateException("remove() called before next()"); cells[thisId]=null; } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestBugs.java b/src/testcases/org/apache/poi/hssf/usermodel/TestBugs.java index 0c48dfad04..04c729b3be 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestBugs.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestBugs.java @@ -1104,29 +1104,6 @@ extends TestCase { assertEquals(1, wb.getNumberOfSheets()); } - - /** - * POI is producing files with the wrong last-column - * number on the row - */ - public void BROKENtest43901() { - HSSFWorkbook book = new HSSFWorkbook(); - HSSFSheet sheet = book.createSheet("test"); - - // New row has last col -1 - HSSFRow row = sheet.createRow(0); - assertEquals(-1, row.getLastCellNum()); - if(row.getLastCellNum() == 0) { - fail("Identified bug 43901"); - } - - // Create two cells, will return one higher - // than that for the last number - row.createCell((short) 0); - assertEquals(1, row.getLastCellNum()); - row.createCell((short) 255); - assertEquals(256, row.getLastCellNum()); - } } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.java index 44c03cd20d..b6f22022c6 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.java @@ -1,4 +1,3 @@ - /* ==================================================================== Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with @@ -15,34 +14,22 @@ See the License for the specific language governing permissions and limitations under the License. ==================================================================== */ - package org.apache.poi.hssf.usermodel; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; + import junit.framework.TestCase; -import java.io.File; -import java.io.FileInputStream; -import java.io.FileOutputStream; - -import org.apache.poi.util.TempFile; - /** * Test HSSFRow is okay. * * @author Glen Stampoultzis (glens at apache.org) */ -public class TestHSSFRow - extends TestCase -{ - public TestHSSFRow(String s) - { - super(s); - } +public final class TestHSSFRow extends TestCase { - public void testLastAndFirstColumns() - throws Exception - { + public void testLastAndFirstColumns() { HSSFWorkbook workbook = new HSSFWorkbook(); HSSFSheet sheet = workbook.createSheet(); HSSFRow row = sheet.createRow((short) 0); @@ -51,127 +38,146 @@ public class TestHSSFRow row.createCell((short) 2); assertEquals(2, row.getFirstCellNum()); - assertEquals(2, row.getLastCellNum()); + assertEquals(3, row.getLastCellNum()); row.createCell((short) 1); assertEquals(1, row.getFirstCellNum()); - assertEquals(2, row.getLastCellNum()); - + assertEquals(3, row.getLastCellNum()); + // check the exact case reported in 'bug' 43901 - notice that the cellNum is '0' based row.createCell((short) 3); assertEquals(1, row.getFirstCellNum()); - assertEquals(3, row.getLastCellNum()); - + assertEquals(4, row.getLastCellNum()); } - public void testRemoveCell() - throws Exception - { + public void testRemoveCell() throws Exception { HSSFWorkbook workbook = new HSSFWorkbook(); HSSFSheet sheet = workbook.createSheet(); HSSFRow row = sheet.createRow((short) 0); assertEquals(-1, row.getLastCellNum()); assertEquals(-1, row.getFirstCellNum()); row.createCell((short) 1); - assertEquals(1, row.getLastCellNum()); + assertEquals(2, row.getLastCellNum()); assertEquals(1, row.getFirstCellNum()); row.createCell((short) 3); - assertEquals(3, row.getLastCellNum()); + assertEquals(4, row.getLastCellNum()); assertEquals(1, row.getFirstCellNum()); row.removeCell(row.getCell((short) 3)); - assertEquals(1, row.getLastCellNum()); + assertEquals(2, row.getLastCellNum()); assertEquals(1, row.getFirstCellNum()); row.removeCell(row.getCell((short) 1)); assertEquals(-1, row.getLastCellNum()); assertEquals(-1, row.getFirstCellNum()); - // check the row record actually writes it out as 0's + // all cells on this row have been removed + // so check the row record actually writes it out as 0's byte[] data = new byte[100]; row.getRowRecord().serialize(0, data); assertEquals(0, data[6]); assertEquals(0, data[8]); - File file = TempFile.createTempFile("XXX", "XLS"); - FileOutputStream stream = new FileOutputStream(file); - workbook.write(stream); - stream.close(); - FileInputStream inputStream = new FileInputStream(file); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + workbook.write(baos); + baos.close(); + ByteArrayInputStream inputStream = new ByteArrayInputStream(baos.toByteArray()); workbook = new HSSFWorkbook(inputStream); sheet = workbook.getSheetAt(0); - stream.close(); - file.delete(); + inputStream.close(); + assertEquals(-1, sheet.getRow((short) 0).getLastCellNum()); assertEquals(-1, sheet.getRow((short) 0).getFirstCellNum()); } - - public void testMoveCell() throws Exception { + + public void testMoveCell() { HSSFWorkbook workbook = new HSSFWorkbook(); HSSFSheet sheet = workbook.createSheet(); HSSFRow row = sheet.createRow((short) 0); HSSFRow rowB = sheet.createRow((short) 1); - + HSSFCell cellA2 = rowB.createCell((short)0); assertEquals(0, rowB.getFirstCellNum()); assertEquals(0, rowB.getFirstCellNum()); - + assertEquals(-1, row.getLastCellNum()); assertEquals(-1, row.getFirstCellNum()); HSSFCell cellB2 = row.createCell((short) 1); HSSFCell cellB3 = row.createCell((short) 2); HSSFCell cellB4 = row.createCell((short) 3); - + assertEquals(1, row.getFirstCellNum()); - assertEquals(3, row.getLastCellNum()); - + assertEquals(4, row.getLastCellNum()); + // Try to move to somewhere else that's used try { - row.moveCell(cellB2, (short)3); - fail(); - } catch(IllegalArgumentException e) {} - + row.moveCell(cellB2, (short)3); + fail("IllegalArgumentException should have been thrown"); + } catch(IllegalArgumentException e) { + // expected during successful test + } + // Try to move one off a different row try { - row.moveCell(cellA2, (short)3); - fail(); - } catch(IllegalArgumentException e) {} - + row.moveCell(cellA2, (short)3); + fail("IllegalArgumentException should have been thrown"); + } catch(IllegalArgumentException e) { + // expected during successful test + } + // Move somewhere spare assertNotNull(row.getCell((short)1)); - row.moveCell(cellB2, (short)5); + row.moveCell(cellB2, (short)5); assertNull(row.getCell((short)1)); assertNotNull(row.getCell((short)5)); - - assertEquals(5, cellB2.getCellNum()); + + assertEquals(5, cellB2.getCellNum()); assertEquals(2, row.getFirstCellNum()); - assertEquals(5, row.getLastCellNum()); - + assertEquals(6, row.getLastCellNum()); } - - public void testRowBounds() - throws Exception - { + + public void testRowBounds() { HSSFWorkbook workbook = new HSSFWorkbook(); HSSFSheet sheet = workbook.createSheet(); //Test low row bound - HSSFRow row = sheet.createRow( (short) 0); - //Test low row bound exception - boolean caughtException = false; + sheet.createRow( (short) 0); + //Test low row bound exception try { - row = sheet.createRow(-1); + sheet.createRow(-1); + fail("IndexOutOfBoundsException should have been thrown"); } catch (IndexOutOfBoundsException ex) { - caughtException = true; - } - assertTrue(caughtException); - //Test high row bound - row = sheet.createRow(65535); - //Test high row bound exception - caughtException = false; + // expected during successful test + } + + //Test high row bound + sheet.createRow(65535); + //Test high row bound exception try { - row = sheet.createRow(65536); + sheet.createRow(65536); + fail("IndexOutOfBoundsException should have been thrown"); } catch (IndexOutOfBoundsException ex) { - caughtException = true; - } - assertTrue(caughtException); + // expected during successful test + } + } + + /** + * Prior to patch 43901, POI was producing files with the wrong last-column + * number on the row + */ + public void testLastCellNumIsCorrectAfterAddCell_bug43901(){ + HSSFWorkbook book = new HSSFWorkbook(); + HSSFSheet sheet = book.createSheet("test"); + HSSFRow row = sheet.createRow(0); + + // New row has last col -1 + assertEquals(-1, row.getLastCellNum()); + if(row.getLastCellNum() == 0) { + fail("Identified bug 43901"); + } + + // Create two cells, will return one higher + // than that for the last number + row.createCell((short) 0); + assertEquals(1, row.getLastCellNum()); + row.createCell((short) 255); + assertEquals(256, row.getLastCellNum()); } - }