diff --git a/src/java/org/apache/poi/hssf/model/Sheet.java b/src/java/org/apache/poi/hssf/model/Sheet.java index b5db343d8e..e059cce27e 100644 --- a/src/java/org/apache/poi/hssf/model/Sheet.java +++ b/src/java/org/apache/poi/hssf/model/Sheet.java @@ -68,6 +68,7 @@ import org.apache.poi.hssf.record.aggregates.MergedCellsTable; import org.apache.poi.hssf.record.aggregates.PageSettingsBlock; import org.apache.poi.hssf.record.aggregates.RecordAggregate; import org.apache.poi.hssf.record.aggregates.RowRecordsAggregate; +import org.apache.poi.hssf.record.aggregates.RecordAggregate.PositionTrackingVisitor; import org.apache.poi.hssf.record.aggregates.RecordAggregate.RecordVisitor; import org.apache.poi.hssf.util.CellRangeAddress; import org.apache.poi.hssf.util.PaneInformation; @@ -105,7 +106,6 @@ public final class Sheet implements Model { private static POILogger log = POILogFactory.getLogger(Sheet.class); protected ArrayList records = null; - int preoffset = 0; // offset of the sheet in a new file protected int dimsloc = -1; // TODO - remove dimsloc protected PrintGridlinesRecord printGridlines = null; protected GridsetRecord gridset = null; @@ -148,7 +148,7 @@ public final class Sheet implements Model { * @see #createSheet(List,int,int) */ public Sheet() { - _mergedCellsTable = new MergedCellsTable(); + _mergedCellsTable = new MergedCellsTable(); } /** @@ -245,13 +245,18 @@ public final class Sheet implements Model { } if (rec.getSid() == MergeCellsRecord.sid) { - // when the MergedCellsTable is found in the right place, we expect those records to be contiguous + // when the MergedCellsTable is found in the right place, we expect those records to be contiguous RecordStream rs = new RecordStream(inRecs, k); retval._mergedCellsTable.read(rs); k += rs.getCountRead()-1; continue; } - + if (rec.getSid() == UncalcedRecord.sid) { + // don't add UncalcedRecord to the list + retval._isUncalced = true; // this flag is enough + continue; + } + if (rec.getSid() == BOFRecord.sid) { bofEofNestingLevel++; @@ -269,9 +274,6 @@ public final class Sheet implements Model { break; } } - else if (rec.getSid() == UncalcedRecord.sid) { - retval._isUncalced = true; - } else if (rec.getSid() == DimensionsRecord.sid) { // Make a columns aggregate if one hasn't ready been created. @@ -585,61 +587,23 @@ public final class Sheet implements Model { log.log(POILogger.DEBUG, "Sheet.setDimensions exiting"); } - /** - * Set the preoffset when using DBCELL records (currently unused) - this is - * the position of this sheet within the whole file. - * - * @param offset the offset of the sheet's BOF within the file. - */ + public void visitContainedRecords(RecordVisitor rv, int offset) { - public void setPreOffset(int offset) - { - this.preoffset = offset; - } - - /** - * get the preoffset when using DBCELL records (currently unused) - this is - * the position of this sheet within the whole file. - * - * @return offset the offset of the sheet's BOF within the file. - */ - - public int getPreOffset() - { - return preoffset; - } - - /** - * Serializes all records in the sheet into one big byte array. Use this to write - * the sheet out. - * - * @param offset to begin write at - * @param data array containing the binary representation of the records in this sheet - * - */ - - public int serialize(int offset, byte [] data) - { - if (log.check( POILogger.DEBUG )) - log.log(POILogger.DEBUG, "Sheet.serialize using offsets"); - - int pos = offset; + PositionTrackingVisitor ptv = new PositionTrackingVisitor(rv, offset); + boolean haveSerializedIndex = false; for (int k = 0; k < records.size(); k++) { RecordBase record = (RecordBase) records.get(k); - // Don't write out UncalcedRecord entries, as - // we handle those specially just below - if (record instanceof UncalcedRecord) { - continue; + if (record instanceof RecordAggregate) { + RecordAggregate agg = (RecordAggregate) record; + agg.visitContainedRecords(ptv); + } else { + ptv.visitRecord((Record) record); } - // Once the rows have been found in the list of records, start - // writing out the blocked row information. This includes the DBCell references - pos += record.serialize(pos, data); - // If the BOF record was just serialized then add the IndexRecord if (record instanceof BOFRecord) { if (!haveSerializedIndex) { @@ -649,50 +613,42 @@ public final class Sheet implements Model { // If there are diagrams, they have their own BOFRecords, // and one shouldn't go in after that! if (_isUncalced) { - UncalcedRecord rec = new UncalcedRecord(); - pos += rec.serialize(pos, data); + ptv.visitRecord(new UncalcedRecord()); } //Can there be more than one BOF for a sheet? If not then we can //remove this guard. So be safe it is left here. if (_rowsAggregate != null) { - pos += serializeIndexRecord(k, pos, data); + // find forward distance to first RowRecord + int initRecsSize = getSizeOfInitialSheetRecords(k); + int currentPos = ptv.getPosition(); + ptv.visitRecord(_rowsAggregate.createIndexRecord(currentPos, initRecsSize)); } } } } - if (log.check( POILogger.DEBUG )) { - log.log(POILogger.DEBUG, "Sheet.serialize returning "); - } - return pos-offset; } - /** - * @param indexRecordOffset also happens to be the end of the BOF record - * @return the size of the serialized INDEX record + * 'initial sheet records' are between INDEX and the 'Row Blocks' + * @param bofRecordIndex index of record after which INDEX record is to be placed + * @return count of bytes from end of INDEX record to first ROW record. */ - private int serializeIndexRecord(int bofRecordIndex, int indexRecordOffset, byte[] data) { + private int getSizeOfInitialSheetRecords(int bofRecordIndex) { - // 'initial sheet records' are between INDEX and first ROW record. - int sizeOfInitialSheetRecords = 0; + int result = 0; // start just after BOF record (INDEX is not present in this list) for (int j = bofRecordIndex + 1; j < records.size(); j++) { - RecordBase tmpRec = ((RecordBase) records.get(j)); - if (tmpRec instanceof UncalcedRecord) { - continue; - } + RecordBase tmpRec = (RecordBase) records.get(j); if (tmpRec instanceof RowRecordsAggregate) { break; } - sizeOfInitialSheetRecords += tmpRec.getRecordSize(); + result += tmpRec.getRecordSize(); } if (_isUncalced) { - sizeOfInitialSheetRecords += UncalcedRecord.getStaticRecordSize(); + result += UncalcedRecord.getStaticRecordSize(); } - IndexRecord index = _rowsAggregate.createIndexRecord(indexRecordOffset, sizeOfInitialSheetRecords); - return index.serialize(indexRecordOffset, data); + return result; } - /** * Create a row record. (does not add it to the records contained in this sheet) */ @@ -1351,32 +1307,6 @@ public final class Sheet implements Model { } } - /** - * @return the serialized size of this sheet - */ - public int getSize() { - int retval = 0; - - for ( int k = 0; k < records.size(); k++) { - RecordBase record = (RecordBase) records.get(k); - if (record instanceof UncalcedRecord) { - // skip the UncalcedRecord if present, it's only encoded if the isUncalced flag is set - continue; - } - retval += record.getRecordSize(); - } - // add space for IndexRecord if needed - if (_rowsAggregate != null) { - // rowsAggregate knows how to make the index record - retval += IndexRecord.getRecordSizeForBlockCount(_rowsAggregate.getRowBlockCount()); - } - // Add space for UncalcedRecord - if (_isUncalced) { - retval += UncalcedRecord.getStaticRecordSize(); - } - return retval; - } - public List getRecords() { return records; diff --git a/src/java/org/apache/poi/hssf/record/aggregates/RecordAggregate.java b/src/java/org/apache/poi/hssf/record/aggregates/RecordAggregate.java index f9cbcd7770..9ea9d61e99 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/RecordAggregate.java +++ b/src/java/org/apache/poi/hssf/record/aggregates/RecordAggregate.java @@ -36,10 +36,16 @@ public abstract class RecordAggregate extends RecordBase { protected final void fillFields(RecordInputStream in) { throw new RuntimeException("Should not be called"); } - public final short getSid() { + public final short getSid() { throw new RuntimeException("Should not be called"); - } + } + /** + * Visit each of the atomic BIFF records contained in this {@link RecordAggregate} in the order + * that they should be written to file. Implementors may or may not return the actual + * {@link Record}s being used to manage POI's internal implementation. Callers should not + * assume either way, and therefore only attempt to modify those {@link Record}s after cloning + */ public abstract void visitContainedRecords(RecordVisitor rv); public final int serialize(int offset, byte[] data) { @@ -94,4 +100,27 @@ public abstract class RecordAggregate extends RecordBase { _totalSize += r.getRecordSize(); } } + /** + * A wrapper for {@link RecordVisitor} which accumulates the sizes of all + * records visited. + */ + public static final class PositionTrackingVisitor implements RecordVisitor { + private final RecordVisitor _rv; + private int _position; + + public PositionTrackingVisitor(RecordVisitor rv, int initialPosition) { + _rv = rv; + _position = initialPosition; + } + public void visitRecord(Record r) { + _position += r.getRecordSize(); + _rv.visitRecord(r); + } + public void setPosition(int position) { + _position = position; + } + public int getPosition() { + return _position; + } + } } diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java b/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java index 1126e56c1c..29b3fc4938 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java +++ b/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java @@ -51,6 +51,7 @@ import org.apache.poi.hssf.record.RecordFactory; import org.apache.poi.hssf.record.SSTRecord; import org.apache.poi.hssf.record.UnicodeString; import org.apache.poi.hssf.record.UnknownRecord; +import org.apache.poi.hssf.record.aggregates.RecordAggregate.RecordVisitor; import org.apache.poi.hssf.record.formula.Area3DPtg; import org.apache.poi.hssf.record.formula.MemFuncPtg; import org.apache.poi.hssf.record.formula.NameXPtg; @@ -108,7 +109,7 @@ public class HSSFWorkbook extends POIDocument */ private ArrayList names; - + /** * this holds the HSSFFont objects attached to this workbook. * We only create these from the low level records as required. @@ -128,7 +129,7 @@ public class HSSFWorkbook extends POIDocument * someplace else. */ private HSSFDataFormat formatter; - + /** * The policy to apply in the event of missing or * blank cells when fetching from a row. @@ -379,7 +380,7 @@ public class HSSFWorkbook extends POIDocument /** * Sets the policy on what to do when * getting missing or blank cells from a row. - * This will then apply to all calls to + * This will then apply to all calls to * {@link HSSFRow.getCell()}. See * {@link MissingCellPolicy} */ @@ -402,17 +403,17 @@ public class HSSFWorkbook extends POIDocument private void validateSheetIndex(int index) { int lastSheetIx = _sheets.size() - 1; if (index < 0 || index > lastSheetIx) { - throw new IllegalArgumentException("Sheet index (" + throw new IllegalArgumentException("Sheet index (" + index +") is out of range (0.." + lastSheetIx + ")"); } } - + /** * Selects a single sheet. This may be different to - * the 'active' sheet (which is the sheet with focus). + * the 'active' sheet (which is the sheet with focus). */ public void setSelectedTab(int index) { - + validateSheetIndex(index); int nSheets = _sheets.size(); for (int i=0; i - * - * Care must be taken if the removed sheet is the currently active or only selected sheet in - * the workbook. There are a few situations when Excel must have a selection and/or active + * + * Care must be taken if the removed sheet is the currently active or only selected sheet in + * the workbook. There are a few situations when Excel must have a selection and/or active * sheet. (For example when printing - see Bug 40414).
- * + * * This method makes sure that if the removed sheet was active, another sheet will become * active in its place. Furthermore, if the removed sheet was the only selected sheet, another - * sheet will become selected. The newly active/selected sheet will have the same index, or + * sheet will become selected. The newly active/selected sheet will have the same index, or * one less if the removed sheet was the last in the workbook. - * + * * @param index of the sheet (0-based) */ public void removeSheetAt(int index) { @@ -1017,7 +1018,7 @@ public class HSSFWorkbook extends POIDocument if(fontindex == Short.MAX_VALUE){ throw new IllegalArgumentException("Maximum number of fonts was exceeded"); } - + // Ask getFontAt() to build it for us, // so it gets properly cached return getFontAt(fontindex); @@ -1033,7 +1034,7 @@ public class HSSFWorkbook extends POIDocument for (short i=0; i<=getNumberOfFonts(); i++) { // Remember - there is no 4! if(i == 4) continue; - + HSSFFont hssfFont = getFontAt(i); if (hssfFont.getBoldweight() == boldWeight && hssfFont.getColor() == color @@ -1083,7 +1084,7 @@ public class HSSFWorkbook extends POIDocument return retval; } - + /** * Reset the fonts cache, causing all new calls * to getFontAt() to create new objects. @@ -1173,6 +1174,37 @@ public class HSSFWorkbook extends POIDocument //poifs.writeFilesystem(stream); } + /** + * Totals the sizes of all sheet records and eventually serializes them + */ + private static final class SheetRecordCollector implements RecordVisitor { + + private List _list; + private int _totalSize; + + public SheetRecordCollector() { + _totalSize = 0; + _list = new ArrayList(128); + } + public int getTotalSize() { + return _totalSize; + } + public void visitRecord(Record r) { + _list.add(r); + _totalSize+=r.getRecordSize(); + } + public int serialize(int offset, byte[] data) { + int result = 0; + int nRecs = _list.size(); + for(int i=0; iUncalcedRecord was present.

@@ -429,13 +445,13 @@ public final class TestSheet extends TestCase { records.add(createWindow2Record()); records.add(EOFRecord.instance); Sheet sheet = Sheet.createSheet(records, 0, 0); - - int estimatedSize = sheet.getSize(); - int serializedSize = sheet.serialize(0, new byte[estimatedSize]); - if (serializedSize != estimatedSize) { - throw new AssertionFailedError("Identified bug 45066 b"); - } - assertEquals(90, serializedSize); + + // The original bug was due to different logic for collecting records for sizing and + // serialization. The code has since been refactored into a single method for visiting + // all contained records. Now this test is much less interesting + SizeCheckingRecordVisitor scrv = new SizeCheckingRecordVisitor(); + sheet.visitContainedRecords(scrv, 0); + assertEquals(90, scrv.getTotalSize()); } /** @@ -479,31 +495,31 @@ public final class TestSheet extends TestCase { * That value is found on the IndexRecord. */ private static int getDbCellRecordPos(Sheet sheet) { - int size = sheet.getSize(); - byte[] data = new byte[size]; - sheet.serialize(0, data); MyIndexRecordListener myIndexListener = new MyIndexRecordListener(); - EventRecordFactory erf = new EventRecordFactory(myIndexListener, new short[] { IndexRecord.sid, }); - erf.processRecords(new ByteArrayInputStream(data)); + sheet.visitContainedRecords(myIndexListener, 0); IndexRecord indexRecord = myIndexListener.getIndexRecord(); int dbCellRecordPos = indexRecord.getDbcellAt(0); return dbCellRecordPos; } - private static final class MyIndexRecordListener implements ERFListener { + private static final class MyIndexRecordListener implements RecordVisitor { private IndexRecord _indexRecord; public MyIndexRecordListener() { // no-arg constructor } - public boolean processRecord(Record rec) { - _indexRecord = (IndexRecord)rec; - return true; - } public IndexRecord getIndexRecord() { return _indexRecord; } + public void visitRecord(Record r) { + if (r instanceof IndexRecord) { + if (_indexRecord != null) { + throw new RuntimeException("too many index records"); + } + _indexRecord = (IndexRecord)r; + } + } } /** diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestWorkbook.java b/src/testcases/org/apache/poi/hssf/usermodel/TestWorkbook.java index 416d4127a0..a585b8b686 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestWorkbook.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestWorkbook.java @@ -17,23 +17,19 @@ package org.apache.poi.hssf.usermodel; -import java.io.ByteArrayInputStream; import java.io.File; import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; -import java.util.Iterator; import junit.framework.TestCase; import org.apache.poi.hssf.HSSFTestDataSamples; -import org.apache.poi.hssf.eventmodel.ERFListener; -import org.apache.poi.hssf.eventmodel.EventRecordFactory; import org.apache.poi.hssf.model.Workbook; import org.apache.poi.hssf.record.BackupRecord; import org.apache.poi.hssf.record.LabelSSTRecord; import org.apache.poi.hssf.record.Record; -import org.apache.poi.hssf.record.aggregates.RowRecordsAggregate; +import org.apache.poi.hssf.record.aggregates.RecordAggregate.RecordVisitor; import org.apache.poi.hssf.util.CellRangeAddress; import org.apache.poi.poifs.filesystem.POIFSFileSystem; import org.apache.poi.util.TempFile; @@ -72,10 +68,7 @@ public final class TestWorkbook extends TestCase { * HSSFSheet last row or first row is incorrect.

* */ - - public void testWriteSheetSimple() - throws IOException - { + public void testWriteSheetSimple() throws IOException { File file = TempFile.createTempFile("testWriteSheetSimple", ".xls"); FileOutputStream out = new FileOutputStream(file); @@ -84,13 +77,10 @@ public final class TestWorkbook extends TestCase { HSSFRow r = null; HSSFCell c = null; - for (short rownum = ( short ) 0; rownum < 100; rownum++) - { + for (int rownum = 0; rownum < 100; rownum++) { r = s.createRow(rownum); - // r.setRowNum(( short ) rownum); - for (short cellnum = ( short ) 0; cellnum < 50; cellnum += 2) - { + for (int cellnum = 0; cellnum < 50; cellnum += 2) { c = r.createCell(cellnum); c.setCellValue(rownum * 10000 + cellnum + ((( double ) rownum / 1000) @@ -104,8 +94,6 @@ public final class TestWorkbook extends TestCase { sanityChecker.checkHSSFWorkbook(wb); assertEquals("LAST ROW == 99", 99, s.getLastRowNum()); assertEquals("FIRST ROW == 0", 0, s.getFirstRowNum()); - - // assert((s.getLastRowNum() == 99)); } /** @@ -130,13 +118,10 @@ public final class TestWorkbook extends TestCase { HSSFRow r = null; HSSFCell c = null; - for (short rownum = ( short ) 0; rownum < 100; rownum++) - { + for (int rownum = 0; rownum < 100; rownum++) { r = s.createRow(rownum); - // r.setRowNum(( short ) rownum); - for (short cellnum = ( short ) 0; cellnum < 50; cellnum += 2) - { + for (int cellnum = 0; cellnum < 50; cellnum += 2) { c = r.createCell(cellnum); c.setCellValue(rownum * 10000 + cellnum + ((( double ) rownum / 1000) @@ -145,13 +130,11 @@ public final class TestWorkbook extends TestCase { c.setCellValue(new HSSFRichTextString("TEST")); } } - for (short rownum = ( short ) 0; rownum < 25; rownum++) - { + for (int rownum = 0; rownum < 25; rownum++) { r = s.getRow(rownum); s.removeRow(r); } - for (short rownum = ( short ) 75; rownum < 100; rownum++) - { + for (int rownum = 75; rownum < 100; rownum++) { r = s.getRow(rownum); s.removeRow(r); } @@ -428,12 +411,10 @@ public final class TestWorkbook extends TestCase { HSSFWorkbook wb = new HSSFWorkbook(); HSSFSheet s = wb.createSheet(); - for (short rownum = ( short ) 0; rownum < 100; rownum++) - { + for (int rownum = 0; rownum < 100; rownum++) { HSSFRow r = s.createRow(rownum); - for (short cellnum = ( short ) 0; cellnum < 50; cellnum += 2) - { + for (int cellnum = 0; cellnum < 50; cellnum += 2) { HSSFCell c = r.createCell(cellnum); c.setCellValue(rownum * 10000 + cellnum + ((( double ) rownum / 1000) @@ -465,13 +446,10 @@ public final class TestWorkbook extends TestCase { /** * Test the backup field gets set as expected. */ - - public void testBackupRecord() - throws Exception - { - HSSFWorkbook wb = new HSSFWorkbook(); - wb.createSheet(); - Workbook workbook = wb.getWorkbook(); + public void testBackupRecord() { + HSSFWorkbook wb = new HSSFWorkbook(); + wb.createSheet(); + Workbook workbook = wb.getWorkbook(); BackupRecord record = workbook.getBackupRecord(); assertEquals(0, record.getBackup()); @@ -479,7 +457,7 @@ public final class TestWorkbook extends TestCase { assertEquals(1, record.getBackup()); } - private static final class RecordCounter implements ERFListener { + private static final class RecordCounter implements RecordVisitor { private int _count; public RecordCounter() { @@ -488,9 +466,10 @@ public final class TestWorkbook extends TestCase { public int getCount() { return _count; } - public boolean processRecord(Record rec) { - _count++; - return true; + public void visitRecord(Record r) { + if (r instanceof LabelSSTRecord) { + _count++; + } } } @@ -499,9 +478,7 @@ public final class TestWorkbook extends TestCase { * * We need to make sure only one LabelSSTRecord is produced. */ - public void testRepeatingBug() - throws Exception - { + public void testRepeatingBug() { HSSFWorkbook workbook = new HSSFWorkbook(); HSSFSheet sheet = workbook.createSheet("Design Variants"); HSSFRow row = sheet.createRow(2); @@ -510,12 +487,8 @@ public final class TestWorkbook extends TestCase { cell.setCellValue(new HSSFRichTextString("Class")); cell = row.createCell(2); - byte[] data = new byte[sheet.getSheet().getSize()]; - sheet.getSheet().serialize(0, data); RecordCounter rc = new RecordCounter(); - EventRecordFactory erf = new EventRecordFactory(rc, new short[] { LabelSSTRecord.sid, }); - erf.processRecords(new ByteArrayInputStream(data)); - + sheet.getSheet().visitContainedRecords(rc, 0); assertEquals(1, rc.getCount()); }