From af131d4ceeee18c471acb821761778d54c08c392 Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Wed, 24 Jun 2009 19:45:44 +0000 Subject: [PATCH] Bugzilla 47415 - Fixed PageSettingsBlock to allow multiple PLS records git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@788157 13f79535-47bb-0310-9956-ffa450edef68 --- src/documentation/content/xdocs/status.xml | 1 + .../apache/poi/hssf/record/UnknownRecord.java | 3 + .../record/aggregates/PageSettingsBlock.java | 99 +++++++++++-------- .../aggregates/TestPageSettingsBlock.java | 66 ++++++++++--- 4 files changed, 118 insertions(+), 51 deletions(-) diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 77d0ecabe..34fe02d10 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -33,6 +33,7 @@ + 47415 - Fixed PageSettingsBlock to allow multiple PLS records 47412 - Fixed concurrency issue with EscherProperties.initProps() 47143 - Fixed OOM in HSSFWorkbook#getAllPictures when reading .xls files containing metafiles Added implementation for ISNA() diff --git a/src/java/org/apache/poi/hssf/record/UnknownRecord.java b/src/java/org/apache/poi/hssf/record/UnknownRecord.java index 607eb917c..750cc5dab 100644 --- a/src/java/org/apache/poi/hssf/record/UnknownRecord.java +++ b/src/java/org/apache/poi/hssf/record/UnknownRecord.java @@ -42,6 +42,9 @@ public final class UnknownRecord extends StandardRecord { * The few POI test samples with this record have data { 0x03, 0x00 }. */ public static final int PRINTSIZE_0033 = 0x0033; + /** + * Environment-Specific Print Record + */ public static final int PLS_004D = 0x004D; public static final int SHEETPR_0081 = 0x0081; public static final int SORT_0090 = 0x0090; 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 1b9228bec..097eff112 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java +++ b/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java @@ -6,7 +6,7 @@ (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at - http://www.apache.org/licenses/LICENSE-2.0 + http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, @@ -19,7 +19,6 @@ package org.apache.poi.hssf.record.aggregates; import java.util.ArrayList; import java.util.Iterator; -import java.util.LinkedList; import java.util.List; import org.apache.poi.hssf.model.RecordStream; @@ -44,14 +43,52 @@ import org.apache.poi.hssf.record.VerticalPageBreakRecord; /** * Groups the page settings records for a worksheet.

- * + * * See OOO excelfileformat.pdf sec 4.4 'Page Settings Block' - * + * * @author Josh Micich */ public final class PageSettingsBlock extends RecordAggregate { - // Every one of these component records is optional - // (The whole PageSettingsBlock may not be present) + + /** + * PLS is potentially a continued record, but is currently uninterpreted by POI + */ + private static final class PLSAggregate extends RecordAggregate { + private static final ContinueRecord[] EMPTY_CONTINUE_RECORD_ARRAY = { }; + private final Record _pls; + /** + * holds any continue records found after the PLS record.
+ * This would not be required if PLS was properly interpreted. + * Currently, PLS is an {@link UnknownRecord} and does not automatically + * include any trailing {@link ContinueRecord}s. + */ + private ContinueRecord[] _plsContinues; + + public PLSAggregate(RecordStream rs) { + _pls = rs.getNext(); + if (rs.peekNextSid()==ContinueRecord.sid) { + List temp = new ArrayList(); + while (rs.peekNextSid()==ContinueRecord.sid) { + temp.add((ContinueRecord)rs.getNext()); + } + _plsContinues = new ContinueRecord[temp.size()]; + temp.toArray(_plsContinues); + } else { + _plsContinues = EMPTY_CONTINUE_RECORD_ARRAY; + } + } + + @Override + public void visitContainedRecords(RecordVisitor rv) { + rv.visitRecord(_pls); + for (int i = 0; i < _plsContinues.length; i++) { + rv.visitRecord(_plsContinues[i]); + } + } + } + + // Every one of these component records is optional + // (The whole PageSettingsBlock may not be present) private PageBreakRecord _rowBreaksRecord; private PageBreakRecord _columnBreaksRecord; private HeaderRecord _header; @@ -62,20 +99,14 @@ public final class PageSettingsBlock extends RecordAggregate { private RightMarginRecord _rightMargin; private TopMarginRecord _topMargin; private BottomMarginRecord _bottomMargin; - private Record _pls; - /** - * holds any continue records found after the PLS record.
- * This would not be required if PLS was properly interpreted. - * Currently, PLS is an {@link UnknownRecord} and does not automatically - * include any trailing {@link ContinueRecord}s. - */ - private List _plsContinues; + private final List _plsRecords; private PrintSetupRecord _printSetup; private Record _bitmap; private Record _headerFooter; private Record _printSize; public PageSettingsBlock(RecordStream rs) { + _plsRecords = new ArrayList(); while(true) { if (!readARecord(rs)) { break; @@ -87,6 +118,7 @@ public final class PageSettingsBlock extends RecordAggregate { * Creates a PageSettingsBlock with default settings */ public PageSettingsBlock() { + _plsRecords = new ArrayList(); _rowBreaksRecord = new HorizontalPageBreakRecord(); _columnBreaksRecord = new VerticalPageBreakRecord(); _header = new HeaderRecord(""); @@ -97,7 +129,7 @@ public final class PageSettingsBlock extends RecordAggregate { } /** - * @return true if the specified Record sid is one belonging to the + * @return true if the specified Record sid is one belonging to the * 'Page Settings Block'. */ public static boolean isComponentRecord(int sid) { @@ -115,8 +147,8 @@ public final class PageSettingsBlock extends RecordAggregate { case UnknownRecord.PLS_004D: case PrintSetupRecord.sid: case UnknownRecord.BITMAP_00E9: - case UnknownRecord.PRINTSIZE_0033: - case UnknownRecord.HEADER_FOOTER_089C: // extra header/footer settings supported by Excel 2007 + case UnknownRecord.PRINTSIZE_0033: + case UnknownRecord.HEADER_FOOTER_089C: // extra header/footer settings supported by Excel 2007 return true; } return false; @@ -165,14 +197,7 @@ public final class PageSettingsBlock extends RecordAggregate { _bottomMargin = (BottomMarginRecord) rs.getNext(); break; case UnknownRecord.PLS_004D: - checkNotPresent(_pls); - _pls = rs.getNext(); - while (rs.peekNextSid()==ContinueRecord.sid) { - if (_plsContinues==null) { - _plsContinues = new LinkedList(); - } - _plsContinues.add((ContinueRecord)rs.getNext()); - } + _plsRecords.add(new PLSAggregate(rs)); break; case PrintSetupRecord.sid: checkNotPresent(_printSetup); @@ -243,7 +268,7 @@ public final class PageSettingsBlock extends RecordAggregate { visitIfPresent(_rowBreaksRecord, rv); visitIfPresent(_columnBreaksRecord, rv); - // Write out empty header / footer records if these are missing + // Write out empty header / footer records if these are missing if (_header == null) { rv.visitRecord(new HeaderRecord("")); } else { @@ -260,11 +285,8 @@ public final class PageSettingsBlock extends RecordAggregate { visitIfPresent(_rightMargin, rv); visitIfPresent(_topMargin, rv); visitIfPresent(_bottomMargin, rv); - visitIfPresent(_pls, rv); - if (_plsContinues != null) { - for (ContinueRecord cr : _plsContinues) { - visitIfPresent(cr, rv); - } + for (RecordAggregate pls : _plsRecords) { + pls.visitContainedRecords(rv); } visitIfPresent(_printSetup, rv); visitIfPresent(_bitmap, rv); @@ -569,11 +591,10 @@ public final class PageSettingsBlock extends RecordAggregate { public HCenterRecord getHCenter() { return _hCenter; } - + /** * HEADERFOOTER is new in 2007. Some apps seem to have scattered this record long after * the {@link PageSettingsBlock} where it belongs. - * */ public void addLateHeaderFooter(Record rec) { if (_headerFooter != null) { @@ -596,21 +617,21 @@ public final class PageSettingsBlock extends RecordAggregate { *

  • 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. + *
  • 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 + * 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 + * 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) { 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 f3730efd7..e3a01b476 100644 --- a/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java +++ b/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java @@ -27,6 +27,7 @@ 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.ContinueRecord; import org.apache.poi.hssf.record.DimensionsRecord; import org.apache.poi.hssf.record.EOFRecord; import org.apache.poi.hssf.record.FooterRecord; @@ -47,19 +48,19 @@ import org.apache.poi.util.HexRead; /** * Tess for {@link PageSettingsBlock} - * + * * @author Dmitriy Kumshayev */ public final class TestPageSettingsBlock extends TestCase { - + public void testPrintSetup_bug46548() { - - // PageSettingBlock in this file contains PLS (sid=x004D) record - // followed by ContinueRecord (sid=x003C) + + // PageSettingBlock in this file contains PLS (sid=x004D) record + // followed by ContinueRecord (sid=x003C) HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("ex46548-23133.xls"); HSSFSheet sheet = wb.getSheetAt(0); HSSFPrintSetup ps = sheet.getPrintSetup(); - + try { ps.getCopies(); } catch (NullPointerException e) { @@ -71,7 +72,6 @@ public final class TestPageSettingsBlock extends TestCase { /** * Bug 46840 occurred because POI failed to recognise HEADERFOOTER as part of the * {@link PageSettingsBlock}. - * */ public void testHeaderFooter_bug46840() { @@ -157,7 +157,7 @@ public final class TestPageSettingsBlock extends TestCase { } /** * 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 + * The example file supplied (attachment 23710) had three non-PSB record types * between the PRINTSETUP record and first MARGIN record: *

      *
    • PRINTSETUP(0x00A1)
      • @@ -238,7 +238,7 @@ public final class TestPageSettingsBlock extends TestCase { private static UnknownRecord ur(int sid, String hexData) { return new UnknownRecord(sid, HexRead.readFromString(hexData)); } - + /** * Excel tolerates missing header / footer records, but adds them (empty) in when re-saving. * This is not critical functionality but it has been decided to keep POI consistent with @@ -257,7 +257,7 @@ public final class TestPageSettingsBlock extends TestCase { RecordCollector rc = new RecordCollector(); psb.visitContainedRecords(rc); Record[] outRecs = rc.getRecords(); - + if (outRecs.length == 2) { throw new AssertionFailedError("PageSettingsBlock didn't add missing header/footer records"); } @@ -266,11 +266,53 @@ public final class TestPageSettingsBlock extends TestCase { assertEquals(FooterRecord.class, outRecs[1].getClass()); assertEquals(HCenterRecord.class, outRecs[2].getClass()); assertEquals(VCenterRecord.class, outRecs[3].getClass()); - - // make sure the added header / footer records are empty + + // make sure the added header / footer records are empty HeaderRecord hr = (HeaderRecord) outRecs[0]; assertEquals("", hr.getText()); FooterRecord fr = (FooterRecord) outRecs[1]; assertEquals("", fr.getText()); } + + /** + * Apparently it's OK to have more than one PLS record. + * Attachment 23866 from bug 47415 had a PageSettingsBlock with two PLS records. This file + * seems to open OK in Excel(2007) but both PLS records are removed (perhaps because the + * specified printers were not available on the testing machine). Since the example file does + * not upset Excel, POI will preserve multiple PLS records.

      + * + * As of June 2009, PLS is still uninterpreted by POI + */ + public void testDuplicatePLS_bug47415() { + Record plsA = ur(UnknownRecord.PLS_004D, "BA AD F0 0D"); + Record plsB = ur(UnknownRecord.PLS_004D, "DE AD BE EF"); + Record contB1 = new ContinueRecord(HexRead.readFromString("FE ED")); + Record contB2 = new ContinueRecord(HexRead.readFromString("FA CE")); + Record[] recs = { + new HeaderRecord("&LSales Figures"), + new FooterRecord("&LInventory"), + new HCenterRecord(), + new VCenterRecord(), + plsA, + plsB, contB1, contB2, // make sure continuing PLS is still OK + }; + RecordStream rs = new RecordStream(Arrays.asList(recs), 0); + PageSettingsBlock psb; + try { + psb = new PageSettingsBlock(rs); + } catch (RecordFormatException e) { + if ("Duplicate PageSettingsBlock record (sid=0x4d)".equals(e.getMessage())) { + throw new AssertionFailedError("Identified bug 47415"); + } + throw e; + } + + // serialize the PSB to see what records come out + RecordCollector rc = new RecordCollector(); + psb.visitContainedRecords(rc); + Record[] outRecs = rc.getRecords(); + + // records were assembled in standard order, so this simple check is OK + assertTrue(Arrays.equals(recs, outRecs)); + } }