Fix for bug introduced in r745976. EscherContainerRecord shouldn't hand out it's...

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@746018 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/src/java/org/apache/poi/ddf/EscherContainerRecord.java b/src/java/org/apache/poi/ddf/EscherContainerRecord.java
index fe78fbd6543ef63817ed0f7d9316bd2c681c0790..f1785304b4fb28144f01032b6f7387da3f9084c0 100644 (file)
--- a/src/java/org/apache/poi/ddf/EscherContainerRecord.java
+++ b/src/java/org/apache/poi/ddf/EscherContainerRecord.java
@@ -34,8 +34,7 @@ import java.io.PrintWriter;
  *
  * @author Glen Stampoultzis
  */
-public class EscherContainerRecord extends EscherRecord
-{
+public final class EscherContainerRecord extends EscherRecord {
     public static final short DGG_CONTAINER    = (short)0xF000;
     public static final short BSTORE_CONTAINER = (short)0xF001;
     public static final short DG_CONTAINER     = (short)0xF002;
@@ -57,7 +56,7 @@ public class EscherContainerRecord extends EscherRecord
             bytesWritten += childBytesWritten;
             offset += childBytesWritten;
             bytesRemaining -= childBytesWritten;
-            getChildRecords().add( child );
+            addChildRecord(child);
             if (offset >= data.length && bytesRemaining > 0)
             {
                 System.out.println("WARNING: " + bytesRemaining + " bytes remaining but no space left");
@@ -73,16 +72,16 @@ public class EscherContainerRecord extends EscherRecord
         LittleEndian.putShort(data, offset, getOptions());
         LittleEndian.putShort(data, offset+2, getRecordId());
         int remainingBytes = 0;
-        for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); )
-        {
-            EscherRecord r = (EscherRecord) iterator.next();
+        Iterator<EscherRecord> iterator = _childRecords.iterator();
+        while (iterator.hasNext()) {
+            EscherRecord r = iterator.next();
             remainingBytes += r.getRecordSize();
         }
         LittleEndian.putInt(data, offset+4, remainingBytes);
         int pos = offset+8;
-        for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); )
-        {
-            EscherRecord r = (EscherRecord) iterator.next();
+        iterator = _childRecords.iterator();
+        while (iterator.hasNext()) {
+            EscherRecord r = iterator.next();
             pos += r.serialize(pos, data, listener );
         }
 
@@ -90,12 +89,11 @@ public class EscherContainerRecord extends EscherRecord
         return pos - offset;
     }
 
-    public int getRecordSize()
-    {
+    public int getRecordSize() {
         int childRecordsSize = 0;
-        for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); )
-        {
-            EscherRecord r = (EscherRecord) iterator.next();
+        Iterator<EscherRecord> iterator = _childRecords.iterator();
+        while (iterator.hasNext()) {
+            EscherRecord r = iterator.next();
             childRecordsSize += r.getRecordSize();
         }
         return 8 + childRecordsSize;
@@ -106,46 +104,51 @@ public class EscherContainerRecord extends EscherRecord
      *  given recordId?
      */
     public boolean hasChildOfType(short recordId) {
-        for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); )
-        {
-            EscherRecord r = (EscherRecord) iterator.next();
+        Iterator<EscherRecord> iterator = _childRecords.iterator();
+        while (iterator.hasNext()) {
+            EscherRecord r = iterator.next();
             if(r.getRecordId() == recordId) {
                 return true;
             }
         }
         return false;
     }
-
+    
     /**
-     * Returns a list of all the child (escher) records
-     *  of the container.
+     * @return a copy of the list of all the child records of the container.
      */
     public List<EscherRecord> getChildRecords() {
-        return _childRecords;
+        return new ArrayList<EscherRecord>(_childRecords);
     }
-    
+    /**
+     * replaces the internal child list with the contents of the supplied <tt>childRecords</tt>
+     */
+    public void setChildRecords(List<EscherRecord> childRecords) {
+       if (childRecords == _childRecords) {
+               throw new IllegalStateException("Child records private data member has escaped");
+       }
+        _childRecords.clear();
+        _childRecords.addAll(childRecords);
+    }
+
+   
     /**
      * Returns all of our children which are also
      *  EscherContainers (may be 0, 1, or vary rarely
      *   2 or 3)
      */
-    public List getChildContainers() {
-        List containers = new ArrayList();
-        for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); )
-        {
-            EscherRecord r = (EscherRecord) iterator.next();
+    public List<EscherContainerRecord> getChildContainers() {
+        List<EscherContainerRecord> containers = new ArrayList<EscherContainerRecord>();
+        Iterator<EscherRecord> iterator = _childRecords.iterator();
+        while (iterator.hasNext()) {
+            EscherRecord r = iterator.next();
             if(r instanceof EscherContainerRecord) {
-               containers.add(r);
+               containers.add((EscherContainerRecord) r);
             }
         }
         return containers;
     }
 
-    public void setChildRecords(List<EscherRecord> childRecords) {
-        _childRecords.clear();
-        _childRecords.addAll(childRecords);
-    }
-
     public String getRecordName() {
         switch (getRecordId()) {
             case DGG_CONTAINER:
@@ -165,13 +168,12 @@ public class EscherContainerRecord extends EscherRecord
         }
     }
 
-    public void display( PrintWriter w, int indent )
-    {
-        super.display( w, indent );
-        for (Iterator iterator = _childRecords.iterator(); iterator.hasNext();)
+    public void display(PrintWriter w, int indent) {
+        super.display(w, indent);
+        for (Iterator<EscherRecord> iterator = _childRecords.iterator(); iterator.hasNext();)
         {
-            EscherRecord escherRecord = (EscherRecord) iterator.next();
-            escherRecord.display( w, indent + 1 );
+            EscherRecord escherRecord = iterator.next();
+            escherRecord.display(w, indent + 1);
         }
     }
 
@@ -188,16 +190,15 @@ public class EscherContainerRecord extends EscherRecord
         String nl = System.getProperty( "line.separator" );
 
         StringBuffer children = new StringBuffer();
-        if ( getChildRecords().size() > 0 )
-        {
+        if (_childRecords.size() > 0) {
             children.append( "  children: " + nl );
             
             int count = 0;
-            for ( Iterator iterator = getChildRecords().iterator(); iterator.hasNext(); )
+            for ( Iterator<EscherRecord> iterator = _childRecords.iterator(); iterator.hasNext(); )
             {
                String newIndent = indent + "   ";
                
-                EscherRecord record = (EscherRecord) iterator.next();
+                EscherRecord record = iterator.next();
                 children.append(newIndent + "Child " + count + ":" + nl);
                 
                 if(record instanceof EscherContainerRecord) {
@@ -215,18 +216,17 @@ public class EscherContainerRecord extends EscherRecord
             indent + "  isContainer: " + isContainerRecord() + nl +
             indent + "  options: 0x" + HexDump.toHex( getOptions() ) + nl +
             indent + "  recordId: 0x" + HexDump.toHex( getRecordId() ) + nl +
-            indent + "  numchildren: " + getChildRecords().size() + nl +
+            indent + "  numchildren: " + _childRecords.size() + nl +
             indent + children.toString();
 
     }
 
-    public EscherSpRecord getChildById( short recordId )
-    {
-        for ( Iterator iterator = _childRecords.iterator(); iterator.hasNext(); )
-        {
-            EscherRecord escherRecord = (EscherRecord) iterator.next();
-            if (escherRecord.getRecordId() == recordId)
-                return (EscherSpRecord) escherRecord;
+    public EscherSpRecord getChildById(short recordId) {
+        Iterator<EscherRecord> iterator = _childRecords.iterator();
+        while (iterator.hasNext()) {
+            EscherRecord r = iterator.next();
+            if (r.getRecordId() == recordId)
+                return (EscherSpRecord) r;
         }
         return null;
     }
@@ -236,15 +236,15 @@ public class EscherContainerRecord extends EscherRecord
      *
      * @param out - list to store found records
      */
-    public void getRecordsById(short recordId, List out){
-        for(Iterator it = _childRecords.iterator(); it.hasNext();) {
-            Object er = it.next();
-            EscherRecord r = (EscherRecord)er;
+    public void getRecordsById(short recordId, List<EscherRecord> out){
+        Iterator<EscherRecord> iterator = _childRecords.iterator();
+        while (iterator.hasNext()) {
+            EscherRecord r = iterator.next();
             if(r instanceof EscherContainerRecord) {
                 EscherContainerRecord c = (EscherContainerRecord)r;
                 c.getRecordsById(recordId, out );
             } else if (r.getRecordId() == recordId){
-                out.add(er);
+                out.add(r);
             }
         }
     }

diff --git a/src/scratchpad/src/org/apache/poi/hslf/model/MovieShape.java b/src/scratchpad/src/org/apache/poi/hslf/model/MovieShape.java
index 152433d729afdbb3c2d11f76a955c9e5422954cc..ff4cac0a55074f5a1c2fb3c8db002db9f5d09493 100755 (executable)
--- a/src/scratchpad/src/org/apache/poi/hslf/model/MovieShape.java
+++ b/src/scratchpad/src/org/apache/poi/hslf/model/MovieShape.java
@@ -83,7 +83,7 @@ public final class MovieShape extends Picture {
 \r
         EscherClientDataRecord cldata = new EscherClientDataRecord();\r
         cldata.setOptions((short)0xF);\r
-        _escherContainer.getChildRecords().add(cldata);\r
+        _escherContainer.addChildRecord(cldata);\r
 \r
         OEShapeAtom oe = new OEShapeAtom();\r
         InteractiveInfo info = new InteractiveInfo();\r

diff --git a/src/testcases/org/apache/poi/ddf/TestEscherContainerRecord.java b/src/testcases/org/apache/poi/ddf/TestEscherContainerRecord.java
index 490a3d50ca5ea1a7ebd7f9db75c817126d6238c9..8d12b1c51674489a9512c319b5e9bd628e9586d1 100644 (file)
--- a/src/testcases/org/apache/poi/ddf/TestEscherContainerRecord.java
+++ b/src/testcases/org/apache/poi/ddf/TestEscherContainerRecord.java
@@ -1,4 +1,3 @@
-
 /* ====================================================================
    Licensed to the Apache Software Foundation (ASF) under one or more
    contributor license agreements.  See the NOTICE file distributed with
@@ -20,14 +19,17 @@ package org.apache.poi.ddf;
 
 import java.io.File;
 import java.io.FileInputStream;
+import java.util.List;
 
 import junit.framework.TestCase;
 import org.apache.poi.util.HexRead;
 import org.apache.poi.util.HexDump;
 import org.apache.poi.util.IOUtils;
 
-public class TestEscherContainerRecord extends TestCase
-{
+/**
+ * Tests for {@link EscherContainerRecord}
+ */
+public final class TestEscherContainerRecord extends TestCase {
        private String ESCHER_DATA_PATH;
        
        protected void setUp() {
@@ -129,17 +131,18 @@ public class TestEscherContainerRecord extends TestCase
                    "  properties:" + nl;
         assertEquals( expected, r.toString() );
     }
+    
+    private static final class DummyEscherRecord extends EscherRecord {
+       public DummyEscherRecord() { }
+        public int fillFields( byte[] data, int offset, EscherRecordFactory recordFactory ) { return 0; }
+        public int serialize( int offset, byte[] data, EscherSerializationListener listener ) { return 0; }
+        public int getRecordSize() { return 10; }
+        public String getRecordName() { return ""; }
+    }
 
     public void testGetRecordSize() {
         EscherContainerRecord r = new EscherContainerRecord();
-        r.addChildRecord(new EscherRecord()
-        {
-            public int fillFields( byte[] data, int offset, EscherRecordFactory recordFactory ) { return 0; }
-            public int serialize( int offset, byte[] data, EscherSerializationListener listener ) { return 0; }
-            public int getRecordSize() { return 10; }
-            public String getRecordName() { return ""; }
-        } );
-
+        r.addChildRecord(new DummyEscherRecord());
         assertEquals(18, r.getRecordSize());
     }
 
@@ -158,4 +161,36 @@ public class TestEscherContainerRecord extends TestCase
        EscherContainerRecord record = new EscherContainerRecord();
        record.fillFields(data, 0, new DefaultEscherRecordFactory());
     }
+    
+    /**
+     * Ensure {@link EscherContainerRecord} doesn't spill its guts everywhere
+     */
+    public void testChildren() {
+       EscherContainerRecord ecr = new EscherContainerRecord();
+       List<EscherRecord> children0 = ecr.getChildRecords();
+       assertEquals(0, children0.size());
+
+       EscherRecord chA = new DummyEscherRecord();
+       EscherRecord chB = new DummyEscherRecord();
+       EscherRecord chC = new DummyEscherRecord();
+       
+       ecr.addChildRecord(chA);
+       ecr.addChildRecord(chB);
+       children0.add(chC);
+       
+       List<EscherRecord> children1 = ecr.getChildRecords();
+       assertTrue(children0 !=  children1);
+       assertEquals(2, children1.size());
+       assertEquals(chA, children1.get(0));
+       assertEquals(chB, children1.get(1));
+       
+       assertEquals(1, children0.size()); // first copy unchanged
+       
+       ecr.setChildRecords(children0);
+       ecr.addChildRecord(chA);
+       List<EscherRecord> children2 = ecr.getChildRecords();
+       assertEquals(2, children2.size());
+       assertEquals(chC, children2.get(0));
+       assertEquals(chA, children2.get(1));
+       }
 }