implemented creating comments in existing files, bugfixes

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

src/java/org/apache/poi/ddf/AbstractEscherOptRecord.java

@@ -16,10 +16,7 @@
 ==================================================================== */
 package org.apache.poi.ddf;
 
-import java.util.ArrayList;
+import java.util.*;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.List;
 
 import org.apache.poi.util.HexDump;
 import org.apache.poi.util.LittleEndian;
@@ -139,6 +136,24 @@ public abstract class AbstractEscherOptRecord extends EscherRecord
         } );
     }
 
+    /**
+     * Set an escher property. If a property with given propId already
+     exists it is replaced.
+     *
+     * @param value the property to set.
+     */
+    public void setEscherProperty(EscherProperty value){
+        for ( Iterator<EscherProperty> iterator =
+                      properties.iterator(); iterator.hasNext(); ) {
+            EscherProperty prop = iterator.next();
+            if (prop.getId() == value.getId()){
+                iterator.remove();
+            }
+        }
+        properties.add( value );
+        sortProperties();
+    }
+
     /**
      * Retrieve the string representation of this record.
      */

src/java/org/apache/poi/hssf/record/EscherAggregate.java

@@ -1152,4 +1152,15 @@ public final class EscherAggregate extends AbstractEscherHolderRecord {
     public List<Record> getTailRecords(){
         return Collections.unmodifiableList(tailRec);
     }
+
+    public NoteRecord getNoteRecordByObj(ObjRecord obj){
+        for (Record rec: tailRec){
+            NoteRecord note = (NoteRecord) rec;
+            CommonObjectDataSubRecord cod = (CommonObjectDataSubRecord) obj.getSubRecords().get(0);
+            if (note.getShapeId()  == cod.getObjectId()){
+                return note;
+            }
+        }
+        return null;
+    }
 }

src/java/org/apache/poi/hssf/usermodel/HSSFComment.java

@@ -16,8 +16,8 @@
 ==================================================================== */
 package org.apache.poi.hssf.usermodel;
 
-import org.apache.poi.hssf.record.NoteRecord;
+import org.apache.poi.ddf.EscherContainerRecord;
-import org.apache.poi.hssf.record.TextObjectRecord;
+import org.apache.poi.hssf.record.*;
 import org.apache.poi.ss.usermodel.Comment;
 import org.apache.poi.ss.usermodel.RichTextString;
 
@@ -28,59 +28,89 @@ import org.apache.poi.ss.usermodel.RichTextString;
  */
 public class HSSFComment extends HSSFTextbox implements Comment {
 
-	/*
+    /*
-	 * TODO - make HSSFComment more consistent when created vs read from file.
+      * TODO - make HSSFComment more consistent when created vs read from file.
-	 * Currently HSSFComment has two main forms (corresponding to the two constructors).   There
+      * Currently HSSFComment has two main forms (corresponding to the two constructors).   There
-	 * are certain operations that only work on comment objects in one of the forms (e.g. deleting
+      * are certain operations that only work on comment objects in one of the forms (e.g. deleting
-	 * comments).
+      * comments).
-	 * POI is also deficient in its management of RowRecord fields firstCol and lastCol.  Those 
+      * POI is also deficient in its management of RowRecord fields firstCol and lastCol.  Those
-	 * fields are supposed to take comments into account, but POI does not do this yet (feb 2009).
+      * fields are supposed to take comments into account, but POI does not do this yet (feb 2009).
-	 * It seems like HSSFRow should manage a collection of local HSSFComments 
+      * It seems like HSSFRow should manage a collection of local HSSFComments
-	 */
+      */
-	
+
     private boolean _visible;
     private int _row;
     private int _col;
     private String _author;
 
     private NoteRecord _note;
-    private TextObjectRecord _txo;
+
+    public HSSFComment(EscherContainerRecord spContainer, ObjRecord objRecord, TextObjectRecord textObjectRecord, NoteRecord _note) {
+        super(spContainer, objRecord, textObjectRecord);
+        this._note = _note;
+    }
 
     /**
      * Construct a new comment with the given parent and anchor.
      *
      * @param parent
-     * @param anchor  defines position of this anchor in the sheet
+     * @param anchor defines position of this anchor in the sheet
      */
     public HSSFComment(HSSFShape parent, HSSFAnchor anchor) {
         super(parent, anchor);
-        setShapeType(OBJECT_TYPE_COMMENT);
+        _note = createNoteRecord();
-
         //default color for comments
         setFillColor(0x08000050);
 
         //by default comments are hidden
-        _visible = false;
+        setVisible(false);
-
+        setAuthor("");
-        _author = "";
     }
 
     protected HSSFComment(NoteRecord note, TextObjectRecord txo) {
         this(null, new HSSFClientAnchor());
-        _txo = txo;
+        _textObjectRecord = txo;
         _note = note;
     }
 
+    @Override
+    protected ObjRecord createObjRecord() {
+        ObjRecord obj = new ObjRecord();
+        CommonObjectDataSubRecord c = new CommonObjectDataSubRecord();
+        c.setObjectType(OBJECT_TYPE_COMMENT);
+        c.setLocked(true);
+        c.setPrintable(true);
+        c.setAutofill(false);
+        c.setAutoline(true);
+
+        NoteStructureSubRecord u = new NoteStructureSubRecord();
+        EndSubRecord e = new EndSubRecord();
+        obj.addSubRecord(c);
+        obj.addSubRecord(u);
+        obj.addSubRecord(e);
+        return obj;
+    }
+
+    private NoteRecord createNoteRecord(){
+        NoteRecord note = new NoteRecord();
+        note.setFlags(NoteRecord.NOTE_VISIBLE);
+        note.setAuthor("");
+        return note;
+    }
+
+    @Override
+    void setShapeId(int shapeId) {
+        super.setShapeId(shapeId);
+        _note.setShapeId(shapeId-1024);
+    }
+
     /**
      * Returns whether this comment is visible.
      *
      * @param visible <code>true</code> if the comment is visible, <code>false</code> otherwise
      */
-    public void setVisible(boolean visible){
+    public void setVisible(boolean visible) {
-        if(_note != null) {
+        _note.setFlags(visible ? NoteRecord.NOTE_VISIBLE : NoteRecord.NOTE_HIDDEN);
-			_note.setFlags(visible ? NoteRecord.NOTE_VISIBLE : NoteRecord.NOTE_HIDDEN);
-		}
-        _visible = visible;
     }
 
     /**
@@ -89,7 +119,7 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @return <code>true</code> if the comment is visible, <code>false</code> otherwise
      */
     public boolean isVisible() {
-        return _visible;
+        return _note.getFlags() == NoteRecord.NOTE_VISIBLE;
     }
 
     /**
@@ -98,7 +128,7 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @return the 0-based row of the cell that contains the comment
      */
     public int getRow() {
-        return _row;
+        return _note.getRow();
     }
 
     /**
@@ -107,10 +137,7 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @param row the 0-based row of the cell that contains the comment
      */
     public void setRow(int row) {
-        if(_note != null) {
+        _note.setRow(row);
-			_note.setRow(row);
-        }
-        _row = row;
     }
 
     /**
@@ -118,8 +145,8 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      *
      * @return the 0-based column of the cell that contains the comment
      */
-    public int getColumn(){
+    public int getColumn() {
-        return _col;
+        return _note.getColumn();
     }
 
     /**
@@ -128,17 +155,15 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @param col the 0-based column of the cell that contains the comment
      */
     public void setColumn(int col) {
-        if(_note != null) {
+        _note.setColumn(col);
-		    _note.setColumn(col);
-        }
-        _col = col;
     }
+
     /**
      * @deprecated (Nov 2009) use {@link HSSFComment#setColumn(int)} }
      */
     @Deprecated
     public void setColumn(short col) {
-        setColumn((int)col);
+        setColumn((int) col);
     }
 
     /**
@@ -147,7 +172,7 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @return the name of the original author of the comment
      */
     public String getAuthor() {
-        return _author;
+        return _note.getAuthor();
     }
 
     /**
@@ -155,37 +180,35 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      *
      * @param author the name of the original author of the comment
      */
-    public void setAuthor(String author){
+    public void setAuthor(String author) {
-        if(_note != null) _note.setAuthor(author);
+        if (_note != null) _note.setAuthor(author);
-        this._author = author;
     }
-    
+
     /**
      * Sets the rich text string used by this comment.
      *
-     * @param string    Sets the rich text string used by this object.
+     * @param string Sets the rich text string used by this object.
      */
     public void setString(RichTextString string) {
         HSSFRichTextString hstring = (HSSFRichTextString) string;
         //if font is not set we must set the default one
-        if (hstring.numFormattingRuns() == 0) hstring.applyFont((short)0);
+        if (hstring.numFormattingRuns() == 0) hstring.applyFont((short) 0);
 
-        if (_txo != null) {
+        _textObjectRecord.setStr(hstring);
-            _txo.setStr(hstring);
-        }
         super.setString(string);
     }
-    
+
     /**
      * Returns the underlying Note record
      */
     protected NoteRecord getNoteRecord() {
-	    return _note;
+        return _note;
-	}
+    }
+
     /**
      * Returns the underlying Text record
      */
     public TextObjectRecord getTextObjectRecord() {
-	    return _txo;
+        return _textObjectRecord;
-	}
+    }
 }

src/java/org/apache/poi/hssf/usermodel/HSSFPatriarch.java

@@ -163,6 +163,7 @@ public final class HSSFPatriarch implements HSSFShapeContainer, Drawing {
         HSSFComment shape = new HSSFComment(null, anchor);
         shape.anchor = anchor;
         addShape(shape);
+        onCreate(shape);
         return shape;
     }
 

src/java/org/apache/poi/hssf/usermodel/HSSFPolygon.java

@@ -18,6 +18,7 @@
 package org.apache.poi.hssf.usermodel;
 
 import org.apache.poi.ddf.EscherContainerRecord;
+import org.apache.poi.ddf.EscherOptRecord;
 import org.apache.poi.hssf.record.ObjRecord;
 
 /**
@@ -38,12 +39,18 @@ public class HSSFPolygon
 
     @Override
     protected EscherContainerRecord createSpContainer() {
-        return null;  //To change body of implemented methods use File | Settings | File Templates.
+        EscherContainerRecord spContainer = new EscherContainerRecord();
+        spContainer.setRecordId( EscherContainerRecord.SP_CONTAINER );
+        spContainer.setOptions( (short) 0x000F );
+        EscherOptRecord optRecord = new EscherOptRecord();
+        optRecord.setRecordId(EscherOptRecord.RECORD_ID);
+        spContainer.addChildRecord(optRecord);
+        return spContainer;
     }
 
     @Override
     protected ObjRecord createObjRecord() {
-        return null;  //To change body of implemented methods use File | Settings | File Templates.
+        return null;
     }
 
     public int[] getXPoints()

src/java/org/apache/poi/hssf/usermodel/HSSFShape.java

@@ -53,8 +53,8 @@ public abstract class HSSFShape {
     HSSFAnchor anchor;
     HSSFPatriarch _patriarch;
 
-    protected EscherContainerRecord _escherContainer;
+    protected final EscherContainerRecord _escherContainer;
-    protected ObjRecord _objRecord;
+    protected final ObjRecord _objRecord;
     protected final EscherOptRecord _optRecord;
 
     public HSSFShape(EscherContainerRecord spContainer, ObjRecord objRecord) {
@@ -70,14 +70,11 @@ public abstract class HSSFShape {
     public HSSFShape(HSSFShape parent, HSSFAnchor anchor) {
         this.parent = parent;
         this.anchor = anchor;
-        this._escherContainer = new EscherContainerRecord();
+        this._escherContainer = createSpContainer();
-        _optRecord = new EscherOptRecord();
+        _optRecord = _escherContainer.getChildById(EscherOptRecord.RECORD_ID);
-        _optRecord.setRecordId( EscherOptRecord.RECORD_ID );
+        addStandardOptions(_optRecord);
-        _optRecord.addEscherProperty(new EscherSimpleProperty(EscherProperties.LINESTYLE__LINEDASHING, LINESTYLE_SOLID));
+        _objRecord = createObjRecord();
-        _optRecord.addEscherProperty(new EscherSimpleProperty(EscherProperties.LINESTYLE__LINEWIDTH, LINEWIDTH_DEFAULT));
+
-        _optRecord.addEscherProperty(new EscherRGBProperty(EscherProperties.FILL__FILLCOLOR, FILL__FILLCOLOR_DEFAULT));
-        _optRecord.addEscherProperty(new EscherRGBProperty(EscherProperties.LINESTYLE__COLOR, LINESTYLE__COLOR_DEFAULT));
-        _optRecord.addEscherProperty(new EscherBoolProperty(EscherProperties.FILL__NOFILLHITTEST, 0x0));
     }
 
     protected abstract EscherContainerRecord createSpContainer();
@@ -90,6 +87,14 @@ public abstract class HSSFShape {
         CommonObjectDataSubRecord cod = (CommonObjectDataSubRecord) _objRecord.getSubRecords().get(0);
         cod.setObjectId((short) (shapeId-1024));
     }
+
+    private void addStandardOptions(EscherOptRecord optRecord){
+        setPropertyValue(new EscherSimpleProperty(EscherProperties.LINESTYLE__LINEDASHING, LINESTYLE_SOLID));
+        setPropertyValue(new EscherSimpleProperty(EscherProperties.LINESTYLE__LINEWIDTH, LINEWIDTH_DEFAULT));
+        setPropertyValue(new EscherRGBProperty(EscherProperties.FILL__FILLCOLOR, FILL__FILLCOLOR_DEFAULT));
+        setPropertyValue(new EscherRGBProperty(EscherProperties.LINESTYLE__COLOR, LINESTYLE__COLOR_DEFAULT));
+        setPropertyValue(new EscherBoolProperty(EscherProperties.FILL__NOFILLHITTEST, 0x0));
+    }
     
     int getShapeId(){
         return ((EscherSpRecord)_escherContainer.getChildById(EscherSpRecord.RECORD_ID)).getShapeId();
@@ -270,19 +275,7 @@ public abstract class HSSFShape {
     }
 
     protected void setPropertyValue(EscherProperty property){
-        if (null == _optRecord.lookup(property.getId())){
+        _optRecord.setEscherProperty(property);
-            _optRecord.addEscherProperty(property);
-        } else {
-            int i=0;
-            for (EscherProperty prop: _optRecord.getEscherProperties()){
-                if (prop.getId() == property.getId()){
-                    _optRecord.getEscherProperties().remove(i);
-                    break;
-                }
-                i++;
-            }
-            _optRecord.addEscherProperty(property);
-        }
     }
 
     /**

src/java/org/apache/poi/hssf/usermodel/HSSFShapeFactory.java

@@ -127,6 +127,9 @@ public class HSSFShapeFactory {
                 case CommonObjectDataSubRecord.OBJECT_TYPE_TEXT:
                     shape = new HSSFTextbox(container, objRecord, txtRecord);
                     break;
+                case CommonObjectDataSubRecord.OBJECT_TYPE_COMMENT:
+                    shape = new HSSFComment(container, objRecord, txtRecord, agg.getNoteRecordByObj(objRecord));
+                    break;
                 default:
                     shape = new HSSFSimpleShape(container, objRecord);
             }

src/java/org/apache/poi/hssf/usermodel/HSSFShapeGroup.java

@@ -17,11 +17,7 @@
 
 package org.apache.poi.hssf.usermodel;
 
-import org.apache.poi.ddf.EscherChildAnchorRecord;
+import org.apache.poi.ddf.*;
-import org.apache.poi.ddf.EscherClientAnchorRecord;
-import org.apache.poi.ddf.EscherContainerRecord;
-import org.apache.poi.ddf.EscherRecord;
-import org.apache.poi.ddf.EscherSpgrRecord;
 import org.apache.poi.hssf.model.TextboxShape;
 import org.apache.poi.hssf.record.EscherAggregate;
 import org.apache.poi.hssf.record.ObjRecord;
@@ -76,7 +72,13 @@ public class HSSFShapeGroup
 
     @Override
     protected EscherContainerRecord createSpContainer() {
-        return null;  //To change body of implemented methods use File | Settings | File Templates.
+        EscherContainerRecord spContainer = new EscherContainerRecord();
+        spContainer.setRecordId( EscherContainerRecord.SP_CONTAINER );
+        spContainer.setOptions( (short) 0x000F );
+        EscherOptRecord optRecord = new EscherOptRecord();
+        optRecord.setRecordId(EscherOptRecord.RECORD_ID);
+        spContainer.addChildRecord(optRecord);
+        return spContainer;
     }
 
     @Override

src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java

@@ -2007,4 +2007,27 @@ public final class HSSFSheet implements org.apache.poi.ss.usermodel.Sheet {
         return new HSSFAutoFilter(this);
     }
 
+    protected HSSFComment findCellComment(int row, int column) {
+        return lookForComment(getDrawingPatriarch(), row, column);
+    }
+
+    private HSSFComment lookForComment(HSSFShapeContainer container, int row, int column){
+        for (Object object: container.getChildren()){
+            HSSFShape shape = (HSSFShape) object;
+            if (shape instanceof HSSFShapeGroup){
+                HSSFShape res =  lookForComment((HSSFShapeContainer) shape, row, column);
+                if (null != res){
+                    return (HSSFComment) res;
+                }
+                continue;
+            }
+            if (shape instanceof HSSFComment){
+                HSSFComment comment = (HSSFComment) shape;
+                if (comment.getColumn() == column && comment.getRow() == row){
+                    return comment;
+                }
+            }
+        }
+        return null;
+    }
 }

src/java/org/apache/poi/hssf/usermodel/HSSFSimpleShape.java

@@ -60,8 +60,6 @@ public class HSSFSimpleShape
     public final static short       OBJECT_TYPE_COMMENT            = 25;
 //    public final static short       OBJECT_TYPE_MICROSOFT_OFFICE_DRAWING = 30;
 
-    int shapeType = OBJECT_TYPE_LINE;
-
     private static final Map <Short, Short> objTypeToShapeType = new HashMap<Short, Short>();
 
     static {
@@ -77,8 +75,6 @@ public class HSSFSimpleShape
     public HSSFSimpleShape( HSSFShape parent, HSSFAnchor anchor)
     {
         super( parent, anchor );
-        _escherContainer = createSpContainer();
-        _objRecord = createObjRecord();
         setShapeType(OBJECT_TYPE_LINE);
     }
 
@@ -96,8 +92,11 @@ public class HSSFSimpleShape
         clientData.setRecordId( EscherClientDataRecord.RECORD_ID );
         clientData.setOptions( (short) 0x0000 );
 
+        EscherOptRecord optRecord = new EscherOptRecord();
+        optRecord.setRecordId( EscherOptRecord.RECORD_ID );
+
         spContainer.addChildRecord(sp);
-        spContainer.addChildRecord(_optRecord);
+        spContainer.addChildRecord(optRecord);
         spContainer.addChildRecord(anchor.getEscherAnchor());
         spContainer.addChildRecord(clientData);
         return spContainer;
@@ -159,6 +158,6 @@ public class HSSFSimpleShape
             System.out.println("Unknown shape type: "+shapeType);
             return;
         }
-        spRecord.setShapeType(objTypeToShapeType.get((short)shapeType));
+        spRecord.setShapeType(objTypeToShapeType.get((short) shapeType));
     }
 }

src/java/org/apache/poi/hssf/usermodel/HSSFTextbox.java

@@ -47,11 +47,7 @@ public class HSSFTextbox extends HSSFSimpleShape {
     public final static short VERTICAL_ALIGNMENT_JUSTIFY = 4;
     public final static short VERTICAL_ALIGNMENT_DISTRIBUTED = 7;
 
-
+    protected TextObjectRecord _textObjectRecord;
-    int marginLeft, marginRight, marginTop, marginBottom;
-    short halign, valign;
-
-    private TextObjectRecord _textObjectRecord;
 
     public HSSFTextbox(EscherContainerRecord spContainer, ObjRecord objRecord, TextObjectRecord textObjectRecord) {
         super(spContainer, objRecord);
@@ -113,15 +109,16 @@ public class HSSFTextbox extends HSSFSimpleShape {
         sp.setFlags(EscherSpRecord.FLAG_HAVEANCHOR | EscherSpRecord.FLAG_HASSHAPETYPE);
         opt.setRecordId(EscherOptRecord.RECORD_ID);
         opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTID, 0));
-        opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTLEFT, getMarginLeft()));
-        opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTRIGHT, getMarginRight()));
-        opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTBOTTOM, getMarginBottom()));
-        opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTTOP, getMarginTop()));
-
         opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__WRAPTEXT, 0));
         opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__ANCHORTEXT, 0));
         opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.GROUPSHAPE__PRINT, 0x00080000));
 
+        opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTLEFT, 0));
+        opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTRIGHT, 0));
+        opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTTOP, 0));
+        opt.addEscherProperty(new EscherSimpleProperty(EscherProperties.TEXT__TEXTBOTTOM, 0));
+
+
         EscherRecord anchor = getAnchor().getEscherAnchor();
         clientData.setRecordId(EscherClientDataRecord.RECORD_ID);
         clientData.setOptions((short) 0x0000);

src/testcases/org/apache/poi/hssf/model/HSSFTestModelHelper.java

@@ -1,5 +1,6 @@
 package org.apache.poi.hssf.model;
 
+import org.apache.poi.hssf.usermodel.HSSFComment;
 import org.apache.poi.hssf.usermodel.HSSFTextbox;
 
 /**
@@ -10,4 +11,8 @@ public class HSSFTestModelHelper {
     public static TextboxShape createTextboxShape(int shapeId, HSSFTextbox textbox){
         return new TextboxShape(textbox, shapeId);
     }
+
+    public static CommentShape createCommentShape(int shapeId, HSSFComment comment){
+        return new CommentShape(comment, shapeId);
+    }
 }

src/testcases/org/apache/poi/hssf/model/TestDrawingShapes.java

@@ -17,7 +17,7 @@ import static junit.framework.Assert.assertEquals;
  * @author Evgeniy Berlog
  * date: 12.06.12
  */
-public class TestDrawingShapes extends TestCase{
+public class TestDrawingShapes extends TestCase {
 
     /**
      * HSSFShape tree bust be built correctly
@@ -33,7 +33,7 @@ public class TestDrawingShapes extends TestCase{
      * ----shape
      * ----shape
      */
-    public void testDrawingGroups(){
+    public void testDrawingGroups() {
         HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("drawings.xls");
         HSSFSheet sheet = wb.getSheet("groups");
         HSSFPatriarch patriarch = sheet.getDrawingPatriarch();
@@ -47,7 +47,7 @@ public class TestDrawingShapes extends TestCase{
     }
 
     public void testHSSFShapeCompatibility() {
-        HSSFShape shape = new HSSFSimpleShape(null, new  HSSFClientAnchor());
+        HSSFShape shape = new HSSFSimpleShape(null, new HSSFClientAnchor());
         assertEquals(0x08000040, shape.getLineStyleColor());
         assertEquals(0x08000009, shape.getFillColor());
         assertEquals(HSSFShape.LINEWIDTH_DEFAULT, shape.getLineWidth());
@@ -61,22 +61,22 @@ public class TestDrawingShapes extends TestCase{
 
         assertEquals(7, opt.getEscherProperties().size());
         assertEquals(true,
-                ((EscherBoolProperty)opt.lookup(EscherProperties.TEXT__SIZE_TEXT_TO_FIT_SHAPE)).isTrue());
+                ((EscherBoolProperty) opt.lookup(EscherProperties.TEXT__SIZE_TEXT_TO_FIT_SHAPE)).isTrue());
         assertEquals(0x00000004,
-                ((EscherSimpleProperty)opt.lookup(EscherProperties.GEOMETRY__SHAPEPATH)).getPropertyValue());
+                ((EscherSimpleProperty) opt.lookup(EscherProperties.GEOMETRY__SHAPEPATH)).getPropertyValue());
         assertEquals(0x08000009,
-                ((EscherSimpleProperty)opt.lookup(EscherProperties.FILL__FILLCOLOR)).getPropertyValue());
+                ((EscherSimpleProperty) opt.lookup(EscherProperties.FILL__FILLCOLOR)).getPropertyValue());
         assertEquals(true,
-                ((EscherBoolProperty)opt.lookup(EscherProperties.FILL__NOFILLHITTEST)).isTrue());
+                ((EscherBoolProperty) opt.lookup(EscherProperties.FILL__NOFILLHITTEST)).isTrue());
         assertEquals(0x08000040,
-                ((EscherSimpleProperty)opt.lookup(EscherProperties.LINESTYLE__COLOR)).getPropertyValue());
+                ((EscherSimpleProperty) opt.lookup(EscherProperties.LINESTYLE__COLOR)).getPropertyValue());
         assertEquals(true,
-                ((EscherBoolProperty)opt.lookup(EscherProperties.LINESTYLE__NOLINEDRAWDASH)).isTrue());
+                ((EscherBoolProperty) opt.lookup(EscherProperties.LINESTYLE__NOLINEDRAWDASH)).isTrue());
         assertEquals(true,
-                ((EscherBoolProperty)opt.lookup(EscherProperties.GROUPSHAPE__PRINT)).isTrue());
+                ((EscherBoolProperty) opt.lookup(EscherProperties.GROUPSHAPE__PRINT)).isTrue());
     }
 
-    public void testDefaultPictureSettings(){
+    public void testDefaultPictureSettings() {
         HSSFPicture picture = new HSSFPicture(null, new HSSFClientAnchor());
         assertEquals(picture.getLineWidth(), HSSFShape.LINEWIDTH_DEFAULT);
         assertEquals(picture.getFillColor(), HSSFShape.FILL__FILLCOLOR_DEFAULT);
@@ -89,7 +89,7 @@ public class TestDrawingShapes extends TestCase{
     /**
      * No NullPointerException should appear
      */
-    public void testDefaultSettingsWithEmptyContainer(){
+    public void testDefaultSettingsWithEmptyContainer() {
         EscherContainerRecord container = new EscherContainerRecord();
         EscherOptRecord opt = new EscherOptRecord();
         opt.setRecordId(EscherOptRecord.RECORD_ID);
@@ -107,6 +107,7 @@ public class TestDrawingShapes extends TestCase{
         assertEquals(picture.isNoFill(), HSSFShape.NO_FILL_DEFAULT);
         assertEquals(picture.getPictureIndex(), -1);//not set yet
     }
+
     /**
      * create a rectangle, save the workbook, read back and verify that all shape properties are there
      */
@@ -116,7 +117,7 @@ public class TestDrawingShapes extends TestCase{
         HSSFSheet sheet = wb.createSheet();
 
         HSSFPatriarch drawing = sheet.createDrawingPatriarch();
-        HSSFClientAnchor anchor = new HSSFClientAnchor(10, 10, 200, 200, (short)2, 2, (short)15, 15);
+        HSSFClientAnchor anchor = new HSSFClientAnchor(10, 10, 200, 200, (short) 2, 2, (short) 15, 15);
         anchor.setAnchorType(2);
         assertEquals(anchor.getAnchorType(), 2);
 
@@ -139,7 +140,7 @@ public class TestDrawingShapes extends TestCase{
         assertEquals(1, drawing.getChildren().size());
 
         HSSFSimpleShape rectangle2 =
-                (HSSFSimpleShape)drawing.getChildren().get(0);
+                (HSSFSimpleShape) drawing.getChildren().get(0);
         assertEquals(HSSFSimpleShape.OBJECT_TYPE_RECTANGLE,
                 rectangle2.getShapeType());
         assertEquals(10000, rectangle2.getLineWidth());
@@ -163,7 +164,7 @@ public class TestDrawingShapes extends TestCase{
         sheet = wb.getSheetAt(0);
         drawing = sheet.getDrawingPatriarch();
         assertEquals(1, drawing.getChildren().size());
-        rectangle2 = (HSSFSimpleShape)drawing.getChildren().get(0);
+        rectangle2 = (HSSFSimpleShape) drawing.getChildren().get(0);
         assertEquals(HSSFSimpleShape.OBJECT_TYPE_RECTANGLE, rectangle2.getShapeType());
         assertEquals(77, rectangle2.getLineWidth());
         assertEquals(9, rectangle2.getLineStyle());
@@ -183,7 +184,7 @@ public class TestDrawingShapes extends TestCase{
         assertEquals(drawing.getChildren().size(), 2);
     }
 
-    public void testReadExistingImage(){
+    public void testReadExistingImage() {
         HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("drawings.xls");
         HSSFSheet sheet = wb.getSheet("pictures");
         HSSFPatriarch drawing = sheet.getDrawingPatriarch();
@@ -196,6 +197,9 @@ public class TestDrawingShapes extends TestCase{
         assertEquals(picture.getLineWidth(), HSSFShape.LINEWIDTH_DEFAULT);
         assertEquals(picture.getLineStyle(), HSSFShape.LINESTYLE_DEFAULT);
         assertEquals(picture.isNoFill(), true);
+
+        picture.setPictureIndex(2);
+        assertEquals(picture.getPictureIndex(), 2);
     }
 
 
@@ -206,20 +210,19 @@ public class TestDrawingShapes extends TestCase{
         HSSFPatriarch drawing = sheet.getDrawingPatriarch();
         assertEquals(1, drawing.getChildren().size());
 
-        for(HSSFShape shape : drawing.getChildren()){
+        HSSFSimpleShape shape = (HSSFSimpleShape) drawing.getChildren().get(0);
-            assertEquals(shape.isNoFill(), true);
+        assertEquals(shape.isNoFill(), true);
-            assertEquals(shape.getLineStyle(), HSSFShape.LINESTYLE_DASHDOTGEL);
+        assertEquals(shape.getLineStyle(), HSSFShape.LINESTYLE_DASHDOTGEL);
-            assertEquals(shape.getLineStyleColor(), 0x616161);
+        assertEquals(shape.getLineStyleColor(), 0x616161);
-            assertEquals(HexDump.toHex(shape.getFillColor()), shape.getFillColor(), 0x2CE03D);
+        assertEquals(HexDump.toHex(shape.getFillColor()), shape.getFillColor(), 0x2CE03D);
-            assertEquals(shape.getLineWidth(), HSSFShape.LINEWIDTH_ONE_PT*2);
+        assertEquals(shape.getLineWidth(), HSSFShape.LINEWIDTH_ONE_PT * 2);
-        }
     }
 
     public void testShapeIds() {
         HSSFWorkbook wb = new HSSFWorkbook();
         HSSFSheet sheet1 = wb.createSheet();
         HSSFPatriarch patriarch1 = sheet1.createDrawingPatriarch();
-        for(int i = 0; i < 2; i++) {
+        for (int i = 0; i < 2; i++) {
             patriarch1.createSimpleShape(new HSSFClientAnchor());
         }
 
@@ -240,15 +243,15 @@ public class TestDrawingShapes extends TestCase{
         assertEquals(3, spgrContainer.getChildRecords().size());
 
         EscherSpRecord sp0 =
-                ((EscherContainerRecord)spgrContainer.getChild(0)).getChildById(EscherSpRecord.RECORD_ID);
+                ((EscherContainerRecord) spgrContainer.getChild(0)).getChildById(EscherSpRecord.RECORD_ID);
         assertEquals(1024, sp0.getShapeId());
 
         EscherSpRecord sp1 =
-                ((EscherContainerRecord)spgrContainer.getChild(1)).getChildById(EscherSpRecord.RECORD_ID);
+                ((EscherContainerRecord) spgrContainer.getChild(1)).getChildById(EscherSpRecord.RECORD_ID);
         assertEquals(1025, sp1.getShapeId());
 
         EscherSpRecord sp2 =
-                ((EscherContainerRecord)spgrContainer.getChild(2)).getChildById(EscherSpRecord.RECORD_ID);
+                ((EscherContainerRecord) spgrContainer.getChild(2)).getChildById(EscherSpRecord.RECORD_ID);
         assertEquals(1026, sp2.getShapeId());
     }
 
@@ -256,7 +259,7 @@ public class TestDrawingShapes extends TestCase{
      * Test get new id for shapes from existing file
      * File already have for 1 shape on each sheet, because document must contain EscherDgRecord for each sheet
      */
-    public void testAllocateNewIds(){
+    public void testAllocateNewIds() {
         HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("empty.xls");
         HSSFSheet sheet = wb.getSheetAt(0);
         HSSFPatriarch patriarch = sheet.getDrawingPatriarch();
@@ -288,4 +291,37 @@ public class TestDrawingShapes extends TestCase{
         assertEquals(HSSFTestHelper.allocateNewShapeId(patriarch), 1027);
         assertEquals(HSSFTestHelper.allocateNewShapeId(patriarch), 1028);
     }
+
+    public void testOpt() throws Exception {
+        HSSFWorkbook wb = new HSSFWorkbook();
+
+        // create a sheet with a text box
+        HSSFSheet sheet = wb.createSheet();
+        HSSFPatriarch patriarch = sheet.createDrawingPatriarch();
+
+        HSSFTextbox textbox = patriarch.createTextbox(new HSSFClientAnchor());
+        EscherOptRecord opt1 = HSSFTestHelper.getOptRecord(textbox);
+        EscherOptRecord opt2 = textbox.getEscherContainer().getChildById(EscherOptRecord.RECORD_ID);
+        assertSame(opt1, opt2);
+    }
+    
+    public void testCorrectOrderInOptRecord(){
+        HSSFWorkbook wb = new HSSFWorkbook();
+        HSSFSheet sheet = wb.createSheet();
+        HSSFPatriarch patriarch = sheet.createDrawingPatriarch();
+
+        HSSFTextbox textbox = patriarch.createTextbox(new HSSFClientAnchor());
+        EscherOptRecord opt = HSSFTestHelper.getOptRecord(textbox);    
+        
+        String opt1Str = opt.toXml();
+
+        textbox.setFillColor(textbox.getFillColor());
+        assertEquals(opt1Str, textbox.getEscherContainer().getChildById(EscherOptRecord.RECORD_ID).toXml());
+        textbox.setLineStyle(textbox.getLineStyle());
+        assertEquals(opt1Str, textbox.getEscherContainer().getChildById(EscherOptRecord.RECORD_ID).toXml());
+        textbox.setLineWidth(textbox.getLineWidth());
+        assertEquals(opt1Str, textbox.getEscherContainer().getChildById(EscherOptRecord.RECORD_ID).toXml());
+        textbox.setLineStyleColor(textbox.getLineStyleColor());
+        assertEquals(opt1Str, textbox.getEscherContainer().getChildById(EscherOptRecord.RECORD_ID).toXml());
+    }
 }

src/testcases/org/apache/poi/hssf/usermodel/HSSFTestHelper.java

@@ -16,6 +16,7 @@
 ==================================================================== */
 
 package org.apache.poi.hssf.usermodel;
+import org.apache.poi.ddf.EscherOptRecord;
 import org.apache.poi.hssf.model.InternalSheet;
 import org.apache.poi.hssf.model.InternalWorkbook;
 import org.apache.poi.hssf.record.EscherAggregate;
@@ -47,4 +48,8 @@ public class HSSFTestHelper {
     public static int allocateNewShapeId(HSSFPatriarch patriarch){
         return patriarch.newShapeId();
     }
+
+    public static EscherOptRecord getOptRecord(HSSFShape shape){
+        return shape._optRecord;
+    }
 }

src/testcases/org/apache/poi/hssf/usermodel/TestComment.java

@@ -0,0 +1,241 @@
+package org.apache.poi.hssf.usermodel;
+
+import junit.framework.TestCase;
+import org.apache.poi.ddf.EscherSpRecord;
+import org.apache.poi.hssf.HSSFTestDataSamples;
+import org.apache.poi.hssf.model.CommentShape;
+import org.apache.poi.hssf.model.HSSFTestModelHelper;
+import org.apache.poi.hssf.record.CommonObjectDataSubRecord;
+import org.apache.poi.hssf.record.NoteRecord;
+import org.apache.poi.hssf.record.ObjRecord;
+import org.apache.poi.hssf.record.TextObjectRecord;
+
+import java.io.IOException;
+import java.util.Arrays;
+
+/**
+ * @author Evgeniy Berlog
+ * @date 26.06.12
+ */
+public class TestComment extends TestCase {
+
+    public void testResultEqualsToAbstractShape() {
+        HSSFWorkbook wb = new HSSFWorkbook();
+        HSSFSheet sh = wb.createSheet();
+        HSSFPatriarch patriarch = sh.createDrawingPatriarch();
+
+        HSSFComment comment = patriarch.createCellComment(new HSSFClientAnchor());
+        HSSFRow row = sh.createRow(0);
+        HSSFCell cell = row.createCell(0);
+        cell.setCellComment(comment);
+
+        CommentShape commentShape = HSSFTestModelHelper.createCommentShape(0, comment);
+
+        assertEquals(comment.getEscherContainer().getChildRecords().size(), 5);
+        assertEquals(commentShape.getSpContainer().getChildRecords().size(), 5);
+
+        //sp record
+        byte[] expected = commentShape.getSpContainer().getChild(0).serialize();
+        byte[] actual = comment.getEscherContainer().getChild(0).serialize();
+
+        assertEquals(expected.length, actual.length);
+        assertTrue(Arrays.equals(expected, actual));
+
+        expected = commentShape.getSpContainer().getChild(2).serialize();
+        actual = comment.getEscherContainer().getChild(2).serialize();
+
+        assertEquals(expected.length, actual.length);
+        assertTrue(Arrays.equals(expected, actual));
+
+        expected = commentShape.getSpContainer().getChild(3).serialize();
+        actual = comment.getEscherContainer().getChild(3).serialize();
+
+        assertEquals(expected.length, actual.length);
+        assertTrue(Arrays.equals(expected, actual));
+
+        expected = commentShape.getSpContainer().getChild(4).serialize();
+        actual = comment.getEscherContainer().getChild(4).serialize();
+
+        assertEquals(expected.length, actual.length);
+        assertTrue(Arrays.equals(expected, actual));
+
+        ObjRecord obj = comment.getObjRecord();
+        ObjRecord objShape = commentShape.getObjRecord();
+
+        expected = obj.serialize();
+        actual = objShape.serialize();
+
+        assertEquals(expected.length, actual.length);
+        assertTrue(Arrays.equals(expected, actual));
+
+        TextObjectRecord tor = comment.getTextObjectRecord();
+        TextObjectRecord torShape = commentShape.getTextObjectRecord();
+
+        expected = tor.serialize();
+        actual = torShape.serialize();
+
+        assertEquals(expected.length, actual.length);
+        assertTrue(Arrays.equals(expected, actual));
+
+        NoteRecord note = comment.getNoteRecord();
+        NoteRecord noteShape = commentShape.getNoteRecord();
+
+        expected = note.serialize();
+        actual = noteShape.serialize();
+
+        assertEquals(expected.length, actual.length);
+        assertTrue(Arrays.equals(expected, actual));
+    }
+
+    public void testAddToExistingFile() {
+        HSSFWorkbook wb = new HSSFWorkbook();
+        HSSFSheet sh = wb.createSheet();
+        HSSFPatriarch patriarch = sh.createDrawingPatriarch();
+
+        HSSFComment comment = patriarch.createCellComment(new HSSFClientAnchor());
+        comment.setString(new HSSFRichTextString("comment1"));
+        comment = patriarch.createCellComment(new HSSFClientAnchor());
+        comment.setString(new HSSFRichTextString("comment2"));
+
+        assertEquals(patriarch.getChildren().size(), 2);
+
+        wb = HSSFTestDataSamples.writeOutAndReadBack(wb);
+        sh = wb.getSheetAt(0);
+        patriarch = sh.getDrawingPatriarch();
+
+        assertEquals(patriarch.getChildren().size(), 2);
+        comment = patriarch.createCellComment(new HSSFClientAnchor());
+        comment.setString(new HSSFRichTextString("comment3"));
+
+        assertEquals(patriarch.getChildren().size(), 3);
+        wb = HSSFTestDataSamples.writeOutAndReadBack(wb);
+        sh = wb.getSheetAt(0);
+        patriarch = sh.getDrawingPatriarch();
+        assertEquals(patriarch.getChildren().size(), 3);
+        assertEquals(((HSSFComment) patriarch.getChildren().get(0)).getString().getString(), "comment1");
+        assertEquals(((HSSFComment) patriarch.getChildren().get(1)).getString().getString(), "comment2");
+        assertEquals(((HSSFComment) patriarch.getChildren().get(2)).getString().getString(), "comment3");
+    }
+
+    public void testSetGetProperties() throws IOException {
+        HSSFWorkbook wb = new HSSFWorkbook();
+        HSSFSheet sh = wb.createSheet();
+        HSSFPatriarch patriarch = sh.createDrawingPatriarch();
+
+        HSSFComment comment = patriarch.createCellComment(new HSSFClientAnchor());
+        comment.setString(new HSSFRichTextString("comment1"));
+        assertEquals(comment.getString().getString(), "comment1");
+
+        comment.setAuthor("poi");
+        assertEquals(comment.getAuthor(), "poi");
+
+        comment.setColumn(3);
+        assertEquals(comment.getColumn(), 3);
+
+        comment.setRow(4);
+        assertEquals(comment.getRow(), 4);
+
+        comment.setVisible(false);
+        assertEquals(comment.isVisible(), false);
+
+        wb = HSSFTestDataSamples.writeOutAndReadBack(wb);
+        sh = wb.getSheetAt(0);
+        patriarch = sh.getDrawingPatriarch();
+
+        comment = (HSSFComment) patriarch.getChildren().get(0);
+
+        assertEquals(comment.getString().getString(), "comment1");
+        assertEquals("poi", comment.getAuthor());
+        assertEquals(comment.getColumn(), 3);
+        assertEquals(comment.getRow(), 4);
+        assertEquals(comment.isVisible(), false);
+
+        comment.setString(new HSSFRichTextString("comment12"));
+        comment.setAuthor("poi2");
+        comment.setColumn(32);
+        comment.setRow(42);
+        comment.setVisible(true);
+
+        wb = HSSFTestDataSamples.writeOutAndReadBack(wb);
+        sh = wb.getSheetAt(0);
+        patriarch = sh.getDrawingPatriarch();
+        comment = (HSSFComment) patriarch.getChildren().get(0);
+
+        assertEquals(comment.getString().getString(), "comment12");
+        assertEquals("poi2", comment.getAuthor());
+        assertEquals(comment.getColumn(), 32);
+        assertEquals(comment.getRow(), 42);
+        assertEquals(comment.isVisible(), true);
+    }
+
+    public void testExistingFileWithComment(){
+        HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("drawings.xls");
+        HSSFSheet sheet = wb.getSheet("comments");
+        HSSFPatriarch drawing = sheet.getDrawingPatriarch();
+        assertEquals(1, drawing.getChildren().size());
+        HSSFComment comment = (HSSFComment) drawing.getChildren().get(0);
+        assertEquals(comment.getAuthor(), "evgeniy");
+        assertEquals(comment.getString().getString(), "evgeniy:\npoi test");
+        assertEquals(comment.getColumn(), 1);
+        assertEquals(comment.getRow(), 2);
+    }
+
+    public void testFindComments(){
+        HSSFWorkbook wb = new HSSFWorkbook();
+        HSSFSheet sh = wb.createSheet();
+        HSSFPatriarch patriarch = sh.createDrawingPatriarch();
+
+        HSSFComment comment = patriarch.createCellComment(new HSSFClientAnchor());
+        HSSFRow row = sh.createRow(5);
+        HSSFCell cell = row.createCell(4);
+        cell.setCellComment(comment);
+
+        HSSFTestModelHelper.createCommentShape(0, comment);
+
+        assertNotNull(sh.findCellComment(5, 4));
+        assertNull(sh.findCellComment(5, 5));
+
+        wb = HSSFTestDataSamples.writeOutAndReadBack(wb);
+        sh = wb.getSheetAt(0);
+
+        assertNotNull(sh.findCellComment(5, 4));
+        assertNull(sh.findCellComment(5, 5));
+    }
+
+    public void testInitState(){
+        HSSFWorkbook wb = new HSSFWorkbook();
+        HSSFSheet sh = wb.createSheet();
+        HSSFPatriarch patriarch = sh.createDrawingPatriarch();
+
+        HSSFComment comment = patriarch.createCellComment(new HSSFClientAnchor());
+
+        HSSFSimpleShape shape = patriarch.createSimpleShape(new HSSFClientAnchor());
+
+        //5 properties of HSSFShape + 8 of HSSFTextbox
+        assertEquals(comment._optRecord.getEscherProperties().size(), 13);
+    }
+
+    public void testShapeId(){
+        HSSFWorkbook wb = new HSSFWorkbook();
+        HSSFSheet sh = wb.createSheet();
+        HSSFPatriarch patriarch = sh.createDrawingPatriarch();
+
+        HSSFComment comment = patriarch.createCellComment(new HSSFClientAnchor());
+
+        comment.setShapeId(2024);
+        /**
+         * SpRecord.id == shapeId
+         * ObjRecord.id == shapeId - 1024
+         * NoteRecord.id == ObjectRecord.id == shapeId - 1024
+         */
+
+        assertEquals(comment.getShapeId(), 2024);
+
+        CommonObjectDataSubRecord cod = (CommonObjectDataSubRecord) comment.getObjRecord().getSubRecords().get(0);
+        assertEquals(cod.getObjectId(), 1000);
+        EscherSpRecord spRecord = (EscherSpRecord) comment.getEscherContainer().getChild(0);
+        assertEquals(spRecord.getShapeId(), 2024);
+        assertEquals(comment.getShapeId(), 2024);
+        assertEquals(comment.getNoteRecord().getShapeId(), 1000);
+    }
+}

src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java

@@ -32,7 +32,7 @@ public final class TestHSSFComment extends BaseTestCellComment {
     }
 
     public void testDefaultShapeType() {
-        HSSFComment comment = new HSSFComment((HSSFShape)null, (HSSFAnchor)null);
+        HSSFComment comment = new HSSFComment((HSSFShape)null, new HSSFClientAnchor());
         assertEquals(HSSFSimpleShape.OBJECT_TYPE_COMMENT, comment.getShapeType());
     }