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

@@ -33,6 +33,7 @@
 
     <changes>
         <release version="3.5-beta7" date="2009-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">47415 - Fixed PageSettingsBlock to allow multiple PLS records</action>
            <action dev="POI-DEVELOPERS" type="fix">47412 - Fixed concurrency issue with EscherProperties.initProps()</action>
            <action dev="POI-DEVELOPERS" type="fix">47143 - Fixed OOM in HSSFWorkbook#getAllPictures when reading .xls files containing metafiles</action>
            <action dev="POI-DEVELOPERS" type="add">Added implementation for ISNA()</action>

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;

src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java

@@ -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;
@@ -50,6 +49,44 @@ import org.apache.poi.hssf.record.VerticalPageBreakRecord;
  * @author Josh Micich
  */
 public final class PageSettingsBlock extends RecordAggregate {
+
+	/**
+	 * PLS is potentially a <i>continued</i> 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.<br/>
+		 * 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<ContinueRecord> temp = new ArrayList<ContinueRecord>();
+				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;
@@ -62,20 +99,14 @@ public final class PageSettingsBlock extends RecordAggregate {
 	private RightMarginRecord _rightMargin;
 	private TopMarginRecord _topMargin;
 	private BottomMarginRecord _bottomMargin;
-	private Record _pls;
+	private final List<PLSAggregate> _plsRecords;
-	/**
-	 * holds any continue records found after the PLS record.<br/>
-	 * 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<ContinueRecord> _plsContinues;
 	private PrintSetupRecord _printSetup;
 	private Record _bitmap;
 	private Record _headerFooter;
 	private Record _printSize;
 
 	public PageSettingsBlock(RecordStream rs) {
+		_plsRecords = new ArrayList<PLSAggregate>();
 		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<PLSAggregate>();
 		_rowBreaksRecord = new HorizontalPageBreakRecord();
 		_columnBreaksRecord = new VerticalPageBreakRecord();
 		_header = new HeaderRecord("");
@@ -165,14 +197,7 @@ public final class PageSettingsBlock extends RecordAggregate {
 				_bottomMargin = (BottomMarginRecord) rs.getNext();
 				break;
 			case UnknownRecord.PLS_004D:
-				checkNotPresent(_pls);
+				_plsRecords.add(new PLSAggregate(rs));
-				_pls = rs.getNext();
-				while (rs.peekNextSid()==ContinueRecord.sid) {
-					if (_plsContinues==null) {
-						_plsContinues = new LinkedList<ContinueRecord>();
-					}
-					_plsContinues.add((ContinueRecord)rs.getNext());
-				}
 				break;
 			case PrintSetupRecord.sid:
 				checkNotPresent(_printSetup);
@@ -260,11 +285,8 @@ public final class PageSettingsBlock extends RecordAggregate {
 		visitIfPresent(_rightMargin, rv);
 		visitIfPresent(_topMargin, rv);
 		visitIfPresent(_bottomMargin, rv);
-		visitIfPresent(_pls, rv);
+		for (RecordAggregate pls : _plsRecords) {
-		if (_plsContinues != null) {
+			pls.visitContainedRecords(rv);
-			for (ContinueRecord cr : _plsContinues) {
-				visitIfPresent(cr, rv);
-			}
 		}
 		visitIfPresent(_printSetup, rv);
 		visitIfPresent(_bitmap, rv);
@@ -573,7 +595,6 @@ public final class PageSettingsBlock extends RecordAggregate {
 	/**
 	 * 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) {

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;
@@ -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() {
 
@@ -273,4 +273,46 @@ public final class TestPageSettingsBlock extends TestCase {
 		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.</p>
+	 *
+	 * 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));
+	}
 }