]> source.dussan.org Git - poi.git/commitdiff
Bugzilla 47199 - Fixed PageSettingsBlock/Sheet to tolerate margin records after other...
authorJosh Micich <josh@apache.org>
Mon, 1 Jun 2009 18:41:01 +0000 (18:41 +0000)
committerJosh Micich <josh@apache.org>
Mon, 1 Jun 2009 18:41:01 +0000 (18:41 +0000)
git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@780774 13f79535-47bb-0310-9956-ffa450edef68

src/documentation/content/xdocs/status.xml
src/java/org/apache/poi/hssf/model/Sheet.java
src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java
src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java

index d9c063455ddc7946bb1b06e9107eee3f0c63ad4b..01d4a1f706eb8086fae59959945dd79e2adc4dcb 100644 (file)
@@ -35,6 +35,7 @@
         <release version="3.5-beta7" date="2009-??-??">
         </release>
         <release version="3.5-beta6" date="2009-06-11">
+           <action dev="POI-DEVELOPERS" type="fix">47199 - Fixed PageSettingsBlock/Sheet to tolerate margin records after other non-PSB records</action>
            <action dev="POI-DEVELOPERS" type="fix">47069 - Fixed HSSFSheet#getFirstRowNum and HSSFSheet#getLastRowNum to return correct values after removal of all rows</action>
            <action dev="POI-DEVELOPERS" type="fix">47278 - Fixed XSSFCell to avoid generating xsi:nil entries in shared string table</action>
            <action dev="POI-DEVELOPERS" type="fix">47206 - Fixed XSSFCell to properly read inline strings</action>
index 0966d560eb1efee665e18452c3b8382c308b6059..34063aee1d274dadb6b814b22b3e782363f48c9a 100644 (file)
@@ -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;
-                }
-                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, <psb> but PLS is part of <psb>
-                // 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");
+                } else {
+                    // one or more PSB records found after some intervening non-PSB records
+                    _psBlock.addLateRecords(rs);
                 }
-                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<Record> temp = new ArrayList<Record>();
-        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<Record> _destList;
 
-        public RecordTransferrer(List<Record> destList) {
-            _destList = destList;
-        }
-        public void visitRecord(Record r) {
-            _destList.add(r);
-        }
-    }
     private static final class RecordCloner implements RecordVisitor {
 
         private final List<RecordBase> _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);
     }
 
index f906c7a9986a4a65f8cfc5a7d85654f7173e438b..98b589e67e4855b24f258a894a7cc130b3a4465b 100644 (file)
@@ -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.
+        * <p/>
+        * 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:
+        * <ul>
+        * <li><b>HEADER_FOOTER(0x089C) after WINDOW2</b> - 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</li>
+        * <li><b>PLS, WSBOOL, PageSettingsBlock</b> - WSBOOL is not a PSB record. 
+        * This happens in the test sample file "NoGutsRecords.xls" and "WORKBOOK_in_capitals.xls"</li>
+        * <li><b>Margins after DIMENSION</b> - All of PSB should be before DIMENSION. (Bug-47199)</li>
+        * </ul>
+        * 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.
+        * 
+        * <p/>
+        * <b>Note</b> - 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;
+                       }
+               }
+       }
 }
index 076bbfeb3726600779ca69949ded0570ff29d444..9380b6d2f8e9924029bf1eb8d474388af8f03d60 100644 (file)
@@ -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:
+        * <ul>
+        * <li>PRINTSETUP(0x00A1)</li>
+        * <li>DEFAULTCOLWIDTH(0x0055)</li>
+        * <li>COLINFO(0x007D)</li>
+        * <li>DIMENSIONS(0x0200)</li>
+        * <li>BottomMargin(0x0029)</li>
+        * </ul>
+        */
+       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));