diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index d9c063455..01d4a1f70 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -35,6 +35,7 @@ + 47199 - Fixed PageSettingsBlock/Sheet to tolerate margin records after other non-PSB records 47069 - Fixed HSSFSheet#getFirstRowNum and HSSFSheet#getLastRowNum to return correct values after removal of all rows 47278 - Fixed XSSFCell to avoid generating xsi:nil entries in shared string table 47206 - Fixed XSSFCell to properly read inline strings diff --git a/src/java/org/apache/poi/hssf/model/Sheet.java b/src/java/org/apache/poi/hssf/model/Sheet.java index 0966d560e..34063aee1 100644 --- a/src/java/org/apache/poi/hssf/model/Sheet.java +++ b/src/java/org/apache/poi/hssf/model/Sheet.java @@ -57,7 +57,6 @@ import org.apache.poi.hssf.record.SaveRecalcRecord; import org.apache.poi.hssf.record.ScenarioProtectRecord; import org.apache.poi.hssf.record.SelectionRecord; import org.apache.poi.hssf.record.UncalcedRecord; -import org.apache.poi.hssf.record.UnknownRecord; import org.apache.poi.hssf.record.WSBoolRecord; import org.apache.poi.hssf.record.WindowTwoRecord; import org.apache.poi.hssf.record.aggregates.ChartSubstreamRecordAggregate; @@ -207,7 +206,7 @@ public final class Sheet implements Model { records.add(rra); //only add the aggregate once continue; } - + if (CustomViewSettingsRecordAggregate.isBeginRecord(recSid)) { // This happens three times in test sample file "29982.xls" // Also several times in bugzilla samples 46840-23373 and 46840-23374 @@ -217,28 +216,13 @@ public final class Sheet implements Model { if (PageSettingsBlock.isComponentRecord(recSid)) { if (_psBlock == null) { - // typical case - just one PSB (so far) + // first PSB record encountered - read all of them: _psBlock = new PageSettingsBlock(rs); records.add(_psBlock); - continue; + } else { + // one or more PSB records found after some intervening non-PSB records + _psBlock.addLateRecords(rs); } - if (recSid == UnknownRecord.HEADER_FOOTER_089C) { - // test samples: SharedFormulaTest.xls, ex44921-21902.xls, ex42570-20305.xls - _psBlock.addLateHeaderFooter(rs.getNext()); - continue; - } - // Some apps write PLS, WSBOOL, but PLS is part of - // This happens in the test sample file "NoGutsRecords.xls" and "WORKBOOK_in_capitals.xls" - // In this case the first PSB is two records back - int prevPsbIx = records.size()-2; - if (_psBlock != records.get(prevPsbIx) || !(records.get(prevPsbIx+1) instanceof WSBoolRecord)) { - // not quite the expected situation - throw new RuntimeException("two Page Settings Blocks found in the same sheet"); - } - records.remove(prevPsbIx); // WSBOOL will drop down one position. - PageSettingsBlock latePsb = new PageSettingsBlock(rs); - _psBlock = mergePSBs(_psBlock, latePsb); - records.add(_psBlock); continue; } @@ -247,7 +231,7 @@ public final class Sheet implements Model { _mergedCellsTable.read(rs); continue; } - + if (recSid == BOFRecord.sid) { ChartSubstreamRecordAggregate chartAgg = new ChartSubstreamRecordAggregate(rs); if (false) { @@ -372,34 +356,7 @@ public final class Sheet implements Model { recs.add(r); }}); } - /** - * Hack to recover from the situation where the page settings block has been split by - * an intervening {@link WSBoolRecord} - */ - private static PageSettingsBlock mergePSBs(PageSettingsBlock a, PageSettingsBlock b) { - List temp = new ArrayList(); - RecordTransferrer rt = new RecordTransferrer(temp); - a.visitContainedRecords(rt); - b.visitContainedRecords(rt); - RecordStream rs = new RecordStream(temp, 0); - PageSettingsBlock result = new PageSettingsBlock(rs); - if (rs.hasNext()) { - throw new RuntimeException("PageSettingsBlocks did not merge properly"); - } - return result; - } - private static final class RecordTransferrer implements RecordVisitor { - - private final List _destList; - - public RecordTransferrer(List destList) { - _destList = destList; - } - public void visitRecord(Record r) { - _destList.add(r); - } - } private static final class RecordCloner implements RecordVisitor { private final List _destList; @@ -1099,7 +1056,7 @@ public final class Sheet implements Model { */ public void setColumnWidth(int column, int width) { if(width > 255*256) throw new IllegalArgumentException("The maximum column width for an individual cell is 255 characters."); - + setColumn(column, null, new Integer(width), null, null, null); } diff --git a/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java b/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java index f906c7a99..98b589e67 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java +++ b/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java @@ -125,36 +125,47 @@ public final class PageSettingsBlock extends RecordAggregate { private boolean readARecord(RecordStream rs) { switch (rs.peekNextSid()) { case HorizontalPageBreakRecord.sid: + checkNotPresent(_rowBreaksRecord); _rowBreaksRecord = (PageBreakRecord) rs.getNext(); break; case VerticalPageBreakRecord.sid: + checkNotPresent(_columnBreaksRecord); _columnBreaksRecord = (PageBreakRecord) rs.getNext(); break; case HeaderRecord.sid: + checkNotPresent(_header); _header = (HeaderRecord) rs.getNext(); break; case FooterRecord.sid: + checkNotPresent(_footer); _footer = (FooterRecord) rs.getNext(); break; case HCenterRecord.sid: + checkNotPresent(_hCenter); _hCenter = (HCenterRecord) rs.getNext(); break; case VCenterRecord.sid: + checkNotPresent(_vCenter); _vCenter = (VCenterRecord) rs.getNext(); break; case LeftMarginRecord.sid: + checkNotPresent(_leftMargin); _leftMargin = (LeftMarginRecord) rs.getNext(); break; case RightMarginRecord.sid: + checkNotPresent(_rightMargin); _rightMargin = (RightMarginRecord) rs.getNext(); break; case TopMarginRecord.sid: + checkNotPresent(_topMargin); _topMargin = (TopMarginRecord) rs.getNext(); break; case BottomMarginRecord.sid: + checkNotPresent(_bottomMargin); _bottomMargin = (BottomMarginRecord) rs.getNext(); break; case UnknownRecord.PLS_004D: + checkNotPresent(_pls); _pls = rs.getNext(); while (rs.peekNextSid()==ContinueRecord.sid) { if (_plsContinues==null) { @@ -164,15 +175,19 @@ public final class PageSettingsBlock extends RecordAggregate { } break; case PrintSetupRecord.sid: + checkNotPresent(_printSetup); _printSetup = (PrintSetupRecord)rs.getNext(); break; case UnknownRecord.BITMAP_00E9: + checkNotPresent(_bitmap); _bitmap = rs.getNext(); break; case UnknownRecord.PRINTSIZE_0033: + checkNotPresent(_printSize); _printSize = rs.getNext(); break; case UnknownRecord.HEADER_FOOTER_089C: + checkNotPresent(_headerFooter); _headerFooter = rs.getNext(); break; default: @@ -182,6 +197,13 @@ public final class PageSettingsBlock extends RecordAggregate { return true; } + private void checkNotPresent(Record rec) { + if (rec != null) { + throw new RecordFormatException("Duplicate PageSettingsBlock record (sid=0x" + + Integer.toHexString(rec.getSid()) + ")"); + } + } + private PageBreakRecord getRowBreaksRecord() { if (_rowBreaksRecord == null) { _rowBreaksRecord = new HorizontalPageBreakRecord(); @@ -551,4 +573,40 @@ public final class PageSettingsBlock extends RecordAggregate { } _headerFooter = rec; } + + /** + * This method reads PageSettingsBlock records from the supplied RecordStream until the first + * non-PageSettingsBlock record is encountered. As each record is read, it is incorporated + * into this PageSettingsBlock. + *

+ * The latest Excel version seems to write the PageSettingsBlock uninterrupted. However there + * are several examples (that Excel reads OK) where these records are not written together: + *

    + *
  • HEADER_FOOTER(0x089C) after WINDOW2 - This record is new in 2007. Some apps + * seem to have scattered this record long after the PageSettingsBlock where it belongs + * test samples: SharedFormulaTest.xls, ex44921-21902.xls, ex42570-20305.xls
    • + *
  • PLS, WSBOOL, PageSettingsBlock - WSBOOL is not a PSB record. + * This happens in the test sample file "NoGutsRecords.xls" and "WORKBOOK_in_capitals.xls"
    • + *
  • Margins after DIMENSION - All of PSB should be before DIMENSION. (Bug-47199)
    • + *
+ * These were probably written by other applications (or earlier versions of Excel). It was + * decided to not write specific code for detecting each of these cases. POI now tolerates + * PageSettingsBlock records scattered all over the sheet record stream, and in any order, but + * does not allow duplicates of any of those records. + * + *

+ * Note - when POI writes out this PageSettingsBlock, the records will always be written + * in one consolidated block (in the standard ordering) regardless of how scattered the records + * were when they were originally read. + * + * @throws RecordFormatException if any PSB record encountered has the same type (sid) as + * a record that is already part of this PageSettingsBlock + */ + public void addLateRecords(RecordStream rs) { + while(true) { + if (!readARecord(rs)) { + break; + } + } + } } diff --git a/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java b/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java index 076bbfeb3..9380b6d2f 100644 --- a/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java +++ b/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java @@ -26,6 +26,7 @@ import org.apache.poi.hssf.HSSFTestDataSamples; import org.apache.poi.hssf.model.RecordStream; import org.apache.poi.hssf.model.Sheet; import org.apache.poi.hssf.record.BOFRecord; +import org.apache.poi.hssf.record.BottomMarginRecord; import org.apache.poi.hssf.record.DimensionsRecord; import org.apache.poi.hssf.record.EOFRecord; import org.apache.poi.hssf.record.FooterRecord; @@ -33,6 +34,7 @@ import org.apache.poi.hssf.record.HeaderRecord; import org.apache.poi.hssf.record.IndexRecord; import org.apache.poi.hssf.record.NumberRecord; import org.apache.poi.hssf.record.Record; +import org.apache.poi.hssf.record.RecordFormatException; import org.apache.poi.hssf.record.UnknownRecord; import org.apache.poi.hssf.record.WindowTwoRecord; import org.apache.poi.hssf.usermodel.HSSFPrintSetup; @@ -135,7 +137,7 @@ public final class TestPageSettingsBlock extends TestCase { Sheet sheet = Sheet.createSheet(rs); RecordCollector rv = new RecordCollector(); - sheet.visitContainedRecords(rv, rowIx); + sheet.visitContainedRecords(rv, 0); Record[] outRecs = rv.getRecords(); if (outRecs[4] == EOFRecord.instance) { throw new AssertionFailedError("Identified bug 46953 - EOF incorrectly appended to PSB"); @@ -151,6 +153,85 @@ public final class TestPageSettingsBlock extends TestCase { assertEquals(WindowTwoRecord.class, outRecs[6].getClass()); assertEquals(EOFRecord.instance, outRecs[7]); } + /** + * Bug 47199 was due to the margin records being located well after the initial PSB records. + * The example file supplied (attachment 23710) had three non-PSB record types + * between the PRINTSETUP record and first MARGIN record: + *

    + *
  • PRINTSETUP(0x00A1)
    • + *
  • DEFAULTCOLWIDTH(0x0055)
    • + *
  • COLINFO(0x007D)
    • + *
  • DIMENSIONS(0x0200)
    • + *
  • BottomMargin(0x0029)
    • + *
+ */ + public void testLateMargins_bug47199() { + + Record[] recs = { + BOFRecord.createSheetBOF(), + new HeaderRecord("&LSales Figures"), + new FooterRecord("&LJanuary"), + new DimensionsRecord(), + createBottomMargin(0.787F), + new WindowTwoRecord(), + EOFRecord.instance, + }; + RecordStream rs = new RecordStream(Arrays.asList(recs), 0); + + Sheet sheet; + try { + sheet = Sheet.createSheet(rs); + } catch (RuntimeException e) { + if (e.getMessage().equals("two Page Settings Blocks found in the same sheet")) { + throw new AssertionFailedError("Identified bug 47199a - failed to process late margings records"); + } + throw e; + } + + RecordCollector rv = new RecordCollector(); + sheet.visitContainedRecords(rv, 0); + Record[] outRecs = rv.getRecords(); + assertEquals(recs.length+1, outRecs.length); // +1 for index record + + assertEquals(BOFRecord.class, outRecs[0].getClass()); + assertEquals(IndexRecord.class, outRecs[1].getClass()); + assertEquals(HeaderRecord.class, outRecs[2].getClass()); + assertEquals(FooterRecord.class, outRecs[3].getClass()); + assertEquals(DimensionsRecord.class, outRecs[5].getClass()); + assertEquals(WindowTwoRecord.class, outRecs[6].getClass()); + assertEquals(EOFRecord.instance, outRecs[7]); + } + + private Record createBottomMargin(float value) { + BottomMarginRecord result = new BottomMarginRecord(); + result.setMargin(value); + return result; + } + + /** + * The PageSettingsBlock should not allow multiple copies of the same record. This extra assertion + * was added while fixing bug 47199. All existing POI test samples comply with this requirement. + */ + public void testDuplicatePSBRecord_bug47199() { + + // Hypothetical setup of PSB records which should cause POI to crash + Record[] recs = { + new HeaderRecord("&LSales Figures"), + new HeaderRecord("&LInventory"), + }; + RecordStream rs = new RecordStream(Arrays.asList(recs), 0); + + try { + new PageSettingsBlock(rs); + throw new AssertionFailedError("Identified bug 47199b - duplicate PSB records should not be allowed"); + } catch (RecordFormatException e) { + if (e.getMessage().equals("Duplicate PageSettingsBlock record (sid=0x14)")) { + // expected during successful test + } else { + throw new AssertionFailedError("Expected RecordFormatException due to duplicate PSB record"); + } + } + } private static UnknownRecord ur(int sid, String hexData) { return new UnknownRecord(sid, HexRead.readFromString(hexData));