Fix for bug 46654 - HSSFRow/RowRecord needs to properly update cell boundary indexes

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

11 files changed, 502 insertions, 428 deletions

diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml
index eb6c43861a..292f202f80 100644
--- a/src/documentation/content/xdocs/changes.xml
+++ b/src/documentation/content/xdocs/changes.xml
@@ -37,6 +37,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.5-beta5" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46654 - HSSFRow/RowRecord to properly update cell boundary indexes</action>
            <action dev="POI-DEVELOPERS" type="fix">46643 - Fixed formula parser to encode range operator with tMemFunc</action>
            <action dev="POI-DEVELOPERS" type="fix">46647 - Fixed COUNTIF NE operator and other special cases involving type conversion</action>
            <action dev="POI-DEVELOPERS" type="add">46635 - Added a method to remove slides</action>
diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml
index 45ebb7b03a..d4f168efe5 100644
--- a/src/documentation/content/xdocs/status.xml
+++ b/src/documentation/content/xdocs/status.xml
@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.5-beta5" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46654 - HSSFRow/RowRecord to properly update cell boundary indexes</action>
            <action dev="POI-DEVELOPERS" type="fix">46643 - Fixed formula parser to encode range operator with tMemFunc</action>
            <action dev="POI-DEVELOPERS" type="fix">46647 - Fixed COUNTIF NE operator and other special cases involving type conversion</action>
            <action dev="POI-DEVELOPERS" type="add">46635 - Added a method to remove slides</action>
diff --git a/src/java/org/apache/poi/hssf/model/Sheet.java b/src/java/org/apache/poi/hssf/model/Sheet.java
index 548d5ad781..5e528f659c 100644
--- a/src/java/org/apache/poi/hssf/model/Sheet.java
+++ b/src/java/org/apache/poi/hssf/model/Sheet.java
@@ -40,6 +40,7 @@ import org.apache.poi.hssf.record.GutsRecord;
 import org.apache.poi.hssf.record.IndexRecord;
 import org.apache.poi.hssf.record.IterationRecord;
 import org.apache.poi.hssf.record.MergeCellsRecord;
+import org.apache.poi.hssf.record.NoteRecord;
 import org.apache.poi.hssf.record.ObjRecord;
 import org.apache.poi.hssf.record.ObjectProtectRecord;
 import org.apache.poi.hssf.record.PaneRecord;
@@ -215,7 +216,7 @@ public final class Sheet implements Model {
                 } else {
                     if (bofEofNestingLevel == 2) {
                         // It's normal for a chart to have its own PageSettingsBlock
-                        // Fall through and add psb here, because chart records 
+                        // Fall through and add psb here, because chart records
                         // are stored loose among the sheet records.
                         // this latest psb does not clash with _psBlock
                     } else if (windowTwo != null) {
@@ -342,13 +343,13 @@ public final class Sheet implements Model {
             // Excel seems to always write the DIMENSION record, but tolerates when it is not present
             // in all cases Excel (2007) adds the missing DIMENSION record
             if (rra == null) {
-                // bug 46206 alludes to files which skip the DIMENSION record 
+                // bug 46206 alludes to files which skip the DIMENSION record
                 // when there are no row/cell records.
                 // Not clear which application wrote these files.
                 rra = new RowRecordsAggregate();
             } else {
                 log.log(POILogger.WARN, "DIMENSION record not found even though row/cells present");
-                // Not sure if any tools write files like this, but Excel reads them OK  
+                // Not sure if any tools write files like this, but Excel reads them OK
             }
             dimsloc = findFirstRecordLocBySid(WindowTwoRecord.sid);
             _dimensions = rra.createDimensions();
@@ -1795,4 +1796,23 @@ public final class Sheet implements Model {
         }
         return _dataValidityTable;
     }
+    /**
+     * Get the {@link NoteRecord}s (related to cell comments) for this sheet
+     * @return never <code>null</code>, typically empty array
+     */
+    public NoteRecord[] getNoteRecords() {
+        List<NoteRecord> temp = new ArrayList<NoteRecord>();
+        for(int i=records.size()-1; i>=0; i--) {
+            RecordBase rec = records.get(i);
+            if (rec instanceof NoteRecord) {
+                temp.add((NoteRecord) rec);
+            }
+        }
+        if (temp.size() < 1) {
+            return NoteRecord.EMPTY_ARRAY;
+        }
+        NoteRecord[] result = new NoteRecord[temp.size()];
+        temp.toArray(result);
+        return result;
+    }
 }
diff --git a/src/java/org/apache/poi/hssf/record/NoteRecord.java b/src/java/org/apache/poi/hssf/record/NoteRecord.java
index eac7596e9d..f24ea0b4ae 100644
--- a/src/java/org/apache/poi/hssf/record/NoteRecord.java
+++ b/src/java/org/apache/poi/hssf/record/NoteRecord.java
@@ -26,22 +26,24 @@ import org.apache.poi.util.StringUtil;
  * @author Yegor Kozlov
  */
 public final class NoteRecord extends StandardRecord {
-    public final static short sid = 0x001C;
+	public final static short sid = 0x001C;
 
-    /**
-     * Flag indicating that the comment is hidden (default)
-     */
-    public final static short NOTE_HIDDEN = 0x0;
+	public static final NoteRecord[] EMPTY_ARRAY = { };
 
-    /**
-     * Flag indicating that the comment is visible
-     */
-    public final static short NOTE_VISIBLE = 0x2;
+	/**
+	 * Flag indicating that the comment is hidden (default)
+	 */
+	public final static short NOTE_HIDDEN = 0x0;
+
+	/**
+	 * Flag indicating that the comment is visible
+	 */
+	public final static short NOTE_VISIBLE = 0x2;
 
 	private static final Byte DEFAULT_PADDING = new Byte((byte)0);
 
-    private short field_1_row;
-	private short field_2_col;
+	private int field_1_row;
+	private int field_2_col;
 	private short field_3_flags;
 	private short field_4_shapeid;
 	private boolean field_5_hasMultibyte;
@@ -54,180 +56,180 @@ public final class NoteRecord extends StandardRecord {
 	 */
 	private Byte field_7_padding;
 
-    /**
-     * Construct a new <code>NoteRecord</code> and
-     * fill its data with the default values
-     */
-    public NoteRecord() {
-        field_6_author = "";
-        field_3_flags = 0;
-        field_7_padding = DEFAULT_PADDING; // seems to be always present regardless of author text
-    }
-
-    /**
-     * @return id of this record.
-     */
-    public short getSid() {
-        return sid;
-    }
-
-    /**
-     * Read the record data from the supplied <code>RecordInputStream</code>
-     */
-    public NoteRecord(RecordInputStream in) {
-        field_1_row = in.readShort();
-        field_2_col = in.readShort();
-        field_3_flags = in.readShort();
-        field_4_shapeid = in.readShort();
-        int length = in.readShort();
-        field_5_hasMultibyte = in.readByte() != 0x00;
-        if (field_5_hasMultibyte) {
-        	field_6_author = StringUtil.readUnicodeLE(in, length);
-        } else {
-        	field_6_author = StringUtil.readCompressedUnicode(in, length);
-        }
+	/**
+	 * Construct a new <code>NoteRecord</code> and
+	 * fill its data with the default values
+	 */
+	public NoteRecord() {
+		field_6_author = "";
+		field_3_flags = 0;
+		field_7_padding = DEFAULT_PADDING; // seems to be always present regardless of author text
+	}
+
+	/**
+	 * @return id of this record.
+	 */
+	public short getSid() {
+		return sid;
+	}
+
+	/**
+	 * Read the record data from the supplied <code>RecordInputStream</code>
+	 */
+	public NoteRecord(RecordInputStream in) {
+		field_1_row = in.readShort();
+		field_2_col = in.readShort();
+		field_3_flags = in.readShort();
+		field_4_shapeid = in.readShort();
+		int length = in.readShort();
+		field_5_hasMultibyte = in.readByte() != 0x00;
+		if (field_5_hasMultibyte) {
+			field_6_author = StringUtil.readUnicodeLE(in, length);
+		} else {
+			field_6_author = StringUtil.readCompressedUnicode(in, length);
+		}
  		if (in.available() == 1) {
 			field_7_padding = new Byte(in.readByte());
-        }
-    }
-
-    public void serialize(LittleEndianOutput out) {
-        out.writeShort(field_1_row);
-        out.writeShort(field_2_col);
-        out.writeShort(field_3_flags);
-        out.writeShort(field_4_shapeid);
-        out.writeShort(field_6_author.length());
-        out.writeByte(field_5_hasMultibyte ? 0x01 : 0x00);
-        if (field_5_hasMultibyte) {
-        	StringUtil.putUnicodeLE(field_6_author, out);
-        } else {
-        	StringUtil.putCompressedUnicode(field_6_author, out);
-        }
-        if (field_7_padding != null) {
-        	out.writeByte(field_7_padding.intValue());
-        }
-    }
-
-    protected int getDataSize() {
-        return 11 // 5 shorts + 1 byte
-        	+ field_6_author.length() * (field_5_hasMultibyte ? 2 : 1)
-        	+ (field_7_padding == null ? 0 : 1);
-    }
-
-    /**
-     * Convert this record to string.
-     * Used by BiffViewer and other utilities.
-     */
-    public String toString() {
-        StringBuffer buffer = new StringBuffer();
-
-        buffer.append("[NOTE]\n");
-        buffer.append("    .row    = ").append(field_1_row).append("\n");
-        buffer.append("    .col    = ").append(field_2_col).append("\n");
-        buffer.append("    .flags  = ").append(field_3_flags).append("\n");
-        buffer.append("    .shapeid= ").append(field_4_shapeid).append("\n");
-        buffer.append("    .author = ").append(field_6_author).append("\n");
-        buffer.append("[/NOTE]\n");
-        return buffer.toString();
-    }
-
-    /**
-     * Return the row that contains the comment
-     *
-     * @return the row that contains the comment
-     */
-    public short getRow(){
-        return field_1_row;
-    }
-
-    /**
-     * Specify the row that contains the comment
-     *
-     * @param row the row that contains the comment
-     */
-    public void setRow(short row){
-        field_1_row = row;
-    }
-
-    /**
-     * Return the column that contains the comment
-     *
-     * @return the column that contains the comment
-     */
-    public short getColumn(){
-        return field_2_col;
-    }
-
-    /**
-     * Specify the column that contains the comment
-     *
-     * @param col the column that contains the comment
-     */
-    public void setColumn(short col){
-        field_2_col = col;
-    }
-
-    /**
-     * Options flags.
-     *
-     * @return the options flag
-     * @see #NOTE_VISIBLE
-     * @see #NOTE_HIDDEN
-     */
-    public short getFlags(){
-        return field_3_flags;
-    }
-
-    /**
-     * Options flag
-     *
-     * @param flags the options flag
-     * @see #NOTE_VISIBLE
-     * @see #NOTE_HIDDEN
-     */
-    public void setFlags(short flags){
-        field_3_flags = flags;
-    }
-
-    /**
-     * Object id for OBJ record that contains the comment
-     */
-    public short getShapeId(){
-        return field_4_shapeid;
-    }
-
-    /**
-     * Object id for OBJ record that contains the comment
-     */
-    public void setShapeId(short id){
-        field_4_shapeid = id;
-    }
-
-    /**
-     * Name of the original comment author
-     *
-     * @return the name of the original author of the comment
-     */
-    public String getAuthor(){
-        return field_6_author;
-    }
-
-    /**
-     * Name of the original comment author
-     *
-     * @param author the name of the original author of the comment
-     */
-    public void setAuthor(String author){
-        field_6_author = author;
-    }
-
-    public Object clone() {
-        NoteRecord rec = new NoteRecord();
-        rec.field_1_row = field_1_row;
-        rec.field_2_col = field_2_col;
-        rec.field_3_flags = field_3_flags;
-        rec.field_4_shapeid = field_4_shapeid;
-        rec.field_6_author = field_6_author;
-        return rec;
-    }
+		}
+	}
+
+	public void serialize(LittleEndianOutput out) {
+		out.writeShort(field_1_row);
+		out.writeShort(field_2_col);
+		out.writeShort(field_3_flags);
+		out.writeShort(field_4_shapeid);
+		out.writeShort(field_6_author.length());
+		out.writeByte(field_5_hasMultibyte ? 0x01 : 0x00);
+		if (field_5_hasMultibyte) {
+			StringUtil.putUnicodeLE(field_6_author, out);
+		} else {
+			StringUtil.putCompressedUnicode(field_6_author, out);
+		}
+		if (field_7_padding != null) {
+			out.writeByte(field_7_padding.intValue());
+		}
+	}
+
+	protected int getDataSize() {
+		return 11 // 5 shorts + 1 byte
+			+ field_6_author.length() * (field_5_hasMultibyte ? 2 : 1)
+			+ (field_7_padding == null ? 0 : 1);
+	}
+
+	/**
+	 * Convert this record to string.
+	 * Used by BiffViewer and other utilities.
+	 */
+	public String toString() {
+		StringBuffer buffer = new StringBuffer();
+
+		buffer.append("[NOTE]\n");
+		buffer.append("    .row    = ").append(field_1_row).append("\n");
+		buffer.append("    .col    = ").append(field_2_col).append("\n");
+		buffer.append("    .flags  = ").append(field_3_flags).append("\n");
+		buffer.append("    .shapeid= ").append(field_4_shapeid).append("\n");
+		buffer.append("    .author = ").append(field_6_author).append("\n");
+		buffer.append("[/NOTE]\n");
+		return buffer.toString();
+	}
+
+	/**
+	 * Return the row that contains the comment
+	 *
+	 * @return the row that contains the comment
+	 */
+	public int getRow() {
+		return field_1_row;
+	}
+
+	/**
+	 * Specify the row that contains the comment
+	 *
+	 * @param row the row that contains the comment
+	 */
+	public void setRow(int row) {
+		field_1_row = row;
+	}
+
+	/**
+	 * Return the column that contains the comment
+	 *
+	 * @return the column that contains the comment
+	 */
+	public int getColumn() {
+		return field_2_col;
+	}
+
+	/**
+	 * Specify the column that contains the comment
+	 *
+	 * @param col the column that contains the comment
+	 */
+	public void setColumn(int col) {
+		field_2_col = col;
+	}
+
+	/**
+	 * Options flags.
+	 *
+	 * @return the options flag
+	 * @see #NOTE_VISIBLE
+	 * @see #NOTE_HIDDEN
+	 */
+	public short getFlags() {
+		return field_3_flags;
+	}
+
+	/**
+	 * Options flag
+	 *
+	 * @param flags the options flag
+	 * @see #NOTE_VISIBLE
+	 * @see #NOTE_HIDDEN
+	 */
+	public void setFlags(short flags) {
+		field_3_flags = flags;
+	}
+
+	/**
+	 * Object id for OBJ record that contains the comment
+	 */
+	public short getShapeId() {
+		return field_4_shapeid;
+	}
+
+	/**
+	 * Object id for OBJ record that contains the comment
+	 */
+	public void setShapeId(short id) {
+		field_4_shapeid = id;
+	}
+
+	/**
+	 * Name of the original comment author
+	 *
+	 * @return the name of the original author of the comment
+	 */
+	public String getAuthor() {
+		return field_6_author;
+	}
+
+	/**
+	 * Name of the original comment author
+	 *
+	 * @param author the name of the original author of the comment
+	 */
+	public void setAuthor(String author) {
+		field_6_author = author;
+	}
+
+	public Object clone() {
+		NoteRecord rec = new NoteRecord();
+		rec.field_1_row = field_1_row;
+		rec.field_2_col = field_2_col;
+		rec.field_3_flags = field_3_flags;
+		rec.field_4_shapeid = field_4_shapeid;
+		rec.field_6_author = field_6_author;
+		return rec;
+	}
 }
diff --git a/src/java/org/apache/poi/hssf/record/RowRecord.java b/src/java/org/apache/poi/hssf/record/RowRecord.java
index 5f084a2318..a2f1e22def 100644
--- a/src/java/org/apache/poi/hssf/record/RowRecord.java
+++ b/src/java/org/apache/poi/hssf/record/RowRecord.java
@@ -43,16 +43,16 @@ public final class RowRecord extends StandardRecord {
      */
     public final static int MAX_ROW_NUMBER = 65535;
     
-    private int               field_1_row_number;
-    private short             field_2_first_col;
-    private short             field_3_last_col;   // plus 1
-    private short             field_4_height;
-    private short             field_5_optimize;   // hint field for gui, can/should be set to zero
+    private int field_1_row_number;
+    private int field_2_first_col;
+    private int field_3_last_col; // plus 1
+    private short field_4_height;
+    private short field_5_optimize; // hint field for gui, can/should be set to zero
 
     // for generated sheets.
-    private short             field_6_reserved;
+    private short field_6_reserved;
     /** 16 bit options flags */
-    private int             field_7_option_flags;
+    private int field_7_option_flags;
     private static final BitField          outlineLevel  = BitFieldFactory.getInstance(0x07);
 
     // bit 3 reserved
@@ -60,22 +60,20 @@ public final class RowRecord extends StandardRecord {
     private static final BitField          zeroHeight    = BitFieldFactory.getInstance(0x20);
     private static final BitField          badFontHeight = BitFieldFactory.getInstance(0x40);
     private static final BitField          formatted     = BitFieldFactory.getInstance(0x80);
-    private short             field_8_xf_index;   // only if isFormatted
+    private short field_8_xf_index;   // only if isFormatted
 
     public RowRecord(int rowNumber) {
         field_1_row_number = rowNumber;
-        field_2_first_col = -1;
-        field_3_last_col = -1;
         field_4_height = (short)0xFF;
         field_5_optimize = ( short ) 0;
         field_6_reserved = ( short ) 0;
         field_7_option_flags = OPTION_BITS_ALWAYS_SET; // seems necessary for outlining
 
         field_8_xf_index = ( short ) 0xf;
+        setEmpty();
     }
 
-    public RowRecord(RecordInputStream in)
-    {
+    public RowRecord(RecordInputStream in) {
         field_1_row_number   = in.readUShort();
         field_2_first_col    = in.readShort();
         field_3_last_col     = in.readShort();
@@ -87,11 +85,22 @@ public final class RowRecord extends StandardRecord {
     }
 
     /**
+     * Updates the firstCol and lastCol fields to the reserved value (-1) 
+     * to signify that this row is empty
+     */
+    public void setEmpty() {
+        field_2_first_col = 0;
+        field_3_last_col = 0;
+    }
+    public boolean isEmpty() {
+        return (field_2_first_col | field_3_last_col) == 0;
+    }
+    
+    /**
      * set the logical row number for this row (0 based index)
      * @param row - the row number
      */
-    public void setRowNumber(int row)
-    {
+    public void setRowNumber(int row) {
         field_1_row_number = row;
     }
 
@@ -99,17 +108,14 @@ public final class RowRecord extends StandardRecord {
      * set the logical col number for the first cell this row (0 based index)
      * @param col - the col number
      */
-    public void setFirstCol(short col)
-    {
+    public void setFirstCol(int col) {
         field_2_first_col = col;
     }
 
     /**
-     * set the logical col number for the last cell this row (0 based index)
-     * @param col - the col number
+     * @param col - one past the zero-based index to the last cell in this row
      */
-    public void setLastCol(short col)
-    {
+    public void setLastCol(int col) {
         field_3_last_col = col;
     }
 
@@ -117,8 +123,7 @@ public final class RowRecord extends StandardRecord {
      * set the height of the row
      * @param height of the row
      */
-    public void setHeight(short height)
-    {
+    public void setHeight(short height) {
         field_4_height = height;
     }
 
@@ -126,8 +131,7 @@ public final class RowRecord extends StandardRecord {
      * set whether to optimize or not (set to 0)
      * @param optimize (set to 0)
      */
-    public void setOptimize(short optimize)
-    {
+    public void setOptimize(short optimize) {
         field_5_optimize = optimize;
     }
 
@@ -137,8 +141,7 @@ public final class RowRecord extends StandardRecord {
      * set the outline level of this row
      * @param ol - the outline level
      */
-    public void setOutlineLevel(short ol)
-    {
+    public void setOutlineLevel(short ol) {
         field_7_option_flags = outlineLevel.setValue(field_7_option_flags, ol);
     }
 
@@ -146,8 +149,7 @@ public final class RowRecord extends StandardRecord {
      * set whether or not to collapse this row
      * @param c - collapse or not
      */
-    public void setColapsed(boolean c)
-    {
+    public void setColapsed(boolean c) {
         field_7_option_flags = colapsed.setBoolean(field_7_option_flags, c);
     }
 
@@ -155,8 +157,7 @@ public final class RowRecord extends StandardRecord {
      * set whether or not to display this row with 0 height
      * @param z  height is zero or not.
      */
-    public void setZeroHeight(boolean z)
-    {
+    public void setZeroHeight(boolean z) {
         field_7_option_flags = zeroHeight.setBoolean(field_7_option_flags, z);
     }
 
@@ -164,8 +165,7 @@ public final class RowRecord extends StandardRecord {
      * set whether the font and row height are not compatible
      * @param  f  true if they aren't compatible (damn not logic)
      */
-    public void setBadFontHeight(boolean f)
-    {
+    public void setBadFontHeight(boolean f) {
         field_7_option_flags = badFontHeight.setBoolean(field_7_option_flags, f);
     }
 
@@ -173,8 +173,7 @@ public final class RowRecord extends StandardRecord {
      * set whether the row has been formatted (even if its got all blank cells)
      * @param f  formatted or not
      */
-    public void setFormatted(boolean f)
-    {
+    public void setFormatted(boolean f) {
         field_7_option_flags = formatted.setBoolean(field_7_option_flags, f);
     }
 
@@ -185,8 +184,7 @@ public final class RowRecord extends StandardRecord {
      * @see org.apache.poi.hssf.record.ExtendedFormatRecord
      * @param index to the XF record
      */
-    public void setXFIndex(short index)
-    {
+    public void setXFIndex(short index) {
         field_8_xf_index = index;
     }
 
@@ -194,8 +192,7 @@ public final class RowRecord extends StandardRecord {
      * get the logical row number for this row (0 based index)
      * @return row - the row number
      */
-    public int getRowNumber()
-    {
+    public int getRowNumber() {
         return field_1_row_number;
     }
 
@@ -203,17 +200,15 @@ public final class RowRecord extends StandardRecord {
      * get the logical col number for the first cell this row (0 based index)
      * @return col - the col number
      */
-    public short getFirstCol()
-    {
+    public int getFirstCol() {
         return field_2_first_col;
     }
 
     /**
-     * get the logical col number for the last cell this row plus one (0 based index)
-     * @return col - the last col number + 1
+     * get the logical col number for the last cell this row (0 based index), plus one 
+     * @return col - the last col index + 1
      */
-    public short getLastCol()
-    {
+    public int getLastCol() {
         return field_3_last_col;
     }
 
@@ -221,8 +216,7 @@ public final class RowRecord extends StandardRecord {
      * get the height of the row
      * @return height of the row
      */
-    public short getHeight()
-    {
+    public short getHeight() {
         return field_4_height;
     }
 
@@ -230,8 +224,7 @@ public final class RowRecord extends StandardRecord {
      * get whether to optimize or not (set to 0)
      * @return optimize (set to 0)
      */
-    public short getOptimize()
-    {
+    public short getOptimize() {
         return field_5_optimize;
     }
 
@@ -240,8 +233,7 @@ public final class RowRecord extends StandardRecord {
      * method)
      * @return options - the bitmask
      */
-    public short getOptionFlags()
-    {
+    public short getOptionFlags() {
         return (short)field_7_option_flags;
     }
 
@@ -252,8 +244,7 @@ public final class RowRecord extends StandardRecord {
      * @return ol - the outline level
      * @see #getOptionFlags()
      */
-    public short getOutlineLevel()
-    {
+    public short getOutlineLevel() {
         return (short)outlineLevel.getValue(field_7_option_flags);
     }
 
@@ -262,8 +253,7 @@ public final class RowRecord extends StandardRecord {
      * @return c - colapse or not
      * @see #getOptionFlags()
      */
-    public boolean getColapsed()
-    {
+    public boolean getColapsed() {
         return (colapsed.isSet(field_7_option_flags));
     }
 
@@ -272,8 +262,7 @@ public final class RowRecord extends StandardRecord {
      * @return - z height is zero or not.
      * @see #getOptionFlags()
      */
-    public boolean getZeroHeight()
-    {
+    public boolean getZeroHeight() {
         return zeroHeight.isSet(field_7_option_flags);
     }
 
@@ -282,9 +271,7 @@ public final class RowRecord extends StandardRecord {
      * @return - f -true if they aren't compatible (damn not logic)
      * @see #getOptionFlags()
      */
-
-    public boolean getBadFontHeight()
-    {
+    public boolean getBadFontHeight() {
         return badFontHeight.isSet(field_7_option_flags);
     }
 
@@ -293,9 +280,7 @@ public final class RowRecord extends StandardRecord {
      * @return formatted or not
      * @see #getOptionFlags()
      */
-
-    public boolean getFormatted()
-    {
+    public boolean getFormatted() {
         return formatted.isSet(field_7_option_flags);
     }
 
@@ -306,14 +291,11 @@ public final class RowRecord extends StandardRecord {
      * @see org.apache.poi.hssf.record.ExtendedFormatRecord
      * @return index to the XF record or bogus value (undefined) if isn't formatted
      */
-
-    public short getXFIndex()
-    {
+    public short getXFIndex() {
         return field_8_xf_index;
     }
 
-    public String toString()
-    {
+    public String toString() {
         StringBuffer sb = new StringBuffer();
 
         sb.append("[ROW]\n");
@@ -350,8 +332,7 @@ public final class RowRecord extends StandardRecord {
         return ENCODED_SIZE - 4;
     }
 
-    public short getSid()
-    {
+    public short getSid() {
         return sid;
     }
 
diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFCell.java b/src/java/org/apache/poi/hssf/usermodel/HSSFCell.java
index 109b95d373..7ad80298cb 100644
--- a/src/java/org/apache/poi/hssf/usermodel/HSSFCell.java
+++ b/src/java/org/apache/poi/hssf/usermodel/HSSFCell.java
@@ -1056,6 +1056,7 @@ public class HSSFCell implements Cell {
      * @return cell comment or <code>null</code> if not found
      */
     protected static HSSFComment findCellComment(Sheet sheet, int row, int column){
+        // TODO - optimise this code by searching backwards, find NoteRecord first, quit if not found. Find one TXO by id
         HSSFComment comment = null;
         HashMap<Integer, TextObjectRecord> txshapesByShapeId = new HashMap<Integer, TextObjectRecord>(); 
         for (Iterator<RecordBase> it = sheet.getRecords().iterator(); it.hasNext(); ) {
@@ -1066,7 +1067,7 @@ public class HSSFCell implements Cell {
                    TextObjectRecord txo = txshapesByShapeId.get(new Integer(note.getShapeId()));
                    comment = new HSSFComment(note, txo);
                    comment.setRow(note.getRow());
-                   comment.setColumn(note.getColumn());
+                   comment.setColumn((short)note.getColumn());
                    comment.setAuthor(note.getAuthor());
                    comment.setVisible(note.getFlags() == NoteRecord.NOTE_VISIBLE);
                    comment.setString(txo.getStr());
@@ -1074,7 +1075,7 @@ public class HSSFCell implements Cell {
                }
            } else if (rec instanceof ObjRecord){
                ObjRecord obj = (ObjRecord)rec;
-               SubRecord sub = (SubRecord)obj.getSubRecords().get(0);
+               SubRecord sub = obj.getSubRecords().get(0);
                if (sub instanceof CommonObjectDataSubRecord){
                    CommonObjectDataSubRecord cmo = (CommonObjectDataSubRecord)sub;
                    if (cmo.getObjectType() == CommonObjectDataSubRecord.OBJECT_TYPE_COMMENT){
diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFComment.java b/src/java/org/apache/poi/hssf/usermodel/HSSFComment.java
index 3d28fc1e25..e74026043c 100644
--- a/src/java/org/apache/poi/hssf/usermodel/HSSFComment.java
+++ b/src/java/org/apache/poi/hssf/usermodel/HSSFComment.java
@@ -28,12 +28,23 @@ import org.apache.poi.ss.usermodel.RichTextString;
  */
 public class HSSFComment extends HSSFTextbox implements Comment {
 
-    private boolean visible;
-    private short col, row;
-    private String author;
-
-    private NoteRecord note = null;
-    private TextObjectRecord txo = null;
+	/*
+	 * TODO - make HSSFComment more consistent when created vs read from file.
+	 * 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
+	 * comments).
+	 * 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).
+	 * 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;
 
     /**
      * Construct a new comment with the given parent and anchor.
@@ -41,25 +52,23 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @param parent
      * @param anchor  defines position of this anchor in the sheet
      */
-    public HSSFComment( HSSFShape parent, HSSFAnchor anchor )
-    {
-        super( parent, anchor );
+    public HSSFComment(HSSFShape parent, HSSFAnchor anchor) {
+        super(parent, anchor);
         setShapeType(OBJECT_TYPE_COMMENT);
 
         //default color for comments
         fillColor = 0x08000050;
 
         //by default comments are hidden
-        visible = false;
+        _visible = false;
 
-        author = "";
+        _author = "";
     }
 
-    protected HSSFComment( NoteRecord note, TextObjectRecord txo )
-    {
-        this( (HSSFShape)null, (HSSFAnchor)null );
-        this.txo = txo;
-        this.note = note;
+    protected HSSFComment(NoteRecord note, TextObjectRecord txo) {
+        this((HSSFShape) null, (HSSFAnchor) null);
+        _txo = txo;
+        _note = note;
     }
 
     /**
@@ -68,8 +77,10 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @param visible <code>true</code> if the comment is visible, <code>false</code> otherwise
      */
     public void setVisible(boolean visible){
-        if(note != null) note.setFlags(visible ? NoteRecord.NOTE_VISIBLE : NoteRecord.NOTE_HIDDEN);
-        this.visible = visible;
+        if(_note != null) {
+			_note.setFlags(visible ? NoteRecord.NOTE_VISIBLE : NoteRecord.NOTE_HIDDEN);
+		}
+        _visible = visible;
     }
 
     /**
@@ -77,8 +88,8 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      *
      * @return <code>true</code> if the comment is visible, <code>false</code> otherwise
      */
-    public boolean isVisible(){
-        return this.visible;
+    public boolean isVisible() {
+        return _visible;
     }
 
     /**
@@ -86,8 +97,8 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      *
      * @return the 0-based row of the cell that contains the comment
      */
-    public int getRow(){
-        return row;
+    public int getRow() {
+        return _row;
     }
 
     /**
@@ -95,9 +106,11 @@ 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((short)row);
-        this.row = (short)row;
+    public void setRow(int row) {
+        if(_note != null) {
+			_note.setRow(row);
+        }
+        _row = row;
     }
 
     /**
@@ -106,7 +119,7 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @return the 0-based column of the cell that contains the comment
      */
     public int getColumn(){
-        return col;
+        return _col;
     }
 
     /**
@@ -114,9 +127,11 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      *
      * @param col the 0-based column of the cell that contains the comment
      */
-    public void setColumn(short col){
-        if(note != null) note.setColumn(col);
-        this.col = col;
+    public void setColumn(short col) {
+        if(_note != null) {
+		    _note.setColumn(col);
+        }
+        _col = col;
     }
 
     /**
@@ -124,8 +139,8 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      *
      * @return the name of the original author of the comment
      */
-    public String getAuthor(){
-        return author;
+    public String getAuthor() {
+        return _author;
     }
 
     /**
@@ -134,8 +149,8 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      * @param author the name of the original author of the comment
      */
     public void setAuthor(String author){
-        if(note != null) note.setAuthor(author);
-        this.author = author;
+        if(_note != null) _note.setAuthor(author);
+        this._author = author;
     }
     
     /**
@@ -143,13 +158,13 @@ public class HSSFComment extends HSSFTextbox implements Comment {
      *
      * @param string    Sets the rich text string used by this object.
      */
-    public void setString( RichTextString string )  {
+    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 (txo != null) {
-            txo.setStr(hstring);
+        if (_txo != null) {
+            _txo.setStr(hstring);
         }
         super.setString(string);
     }
@@ -157,9 +172,13 @@ public class HSSFComment extends HSSFTextbox implements Comment {
     /**
      * Returns the underlying Note record
      */
-    protected NoteRecord getNoteRecord() { return note; }
+    protected NoteRecord getNoteRecord() {
+	    return _note;
+	}
     /**
      * Returns the underlying Text record
      */
-    protected TextObjectRecord getTextObjectRecord() { return txo; }
+    protected TextObjectRecord getTextObjectRecord() {
+	    return _txo;
+	}
 }
diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFRow.java b/src/java/org/apache/poi/hssf/usermodel/HSSFRow.java
index ebd83a86b3..f8848f0741 100644
--- a/src/java/org/apache/poi/hssf/usermodel/HSSFRow.java
+++ b/src/java/org/apache/poi/hssf/usermodel/HSSFRow.java
@@ -30,11 +30,11 @@ import org.apache.poi.ss.usermodel.Row;
  * High level representation of a row of a spreadsheet.
  *
  * Only rows that have cells should be added to a Sheet.
- * @version 1.0-pre
+ *
  * @author  Andrew C. Oliver (acoliver at apache dot org)
  * @author Glen Stampoultzis (glens at apache.org)
  */
-public final class HSSFRow implements Comparable, Row {
+public final class HSSFRow implements Row {
 
     // used for collections
     public final static int INITIAL_CAPACITY = 5;
@@ -65,14 +65,8 @@ public final class HSSFRow implements Comparable, Row {
      * @param rowNum the row number of this row (0 based)
      * @see org.apache.poi.hssf.usermodel.HSSFSheet#createRow(int)
      */
-    HSSFRow(HSSFWorkbook book, HSSFSheet sheet, int rowNum)
-    {
-        this.rowNum = rowNum;
-        this.book = book;
-        this.sheet = sheet;
-        row = new RowRecord(rowNum);
-
-        setRowNum(rowNum);
+    HSSFRow(HSSFWorkbook book, HSSFSheet sheet, int rowNum) {
+        this(book, sheet, new RowRecord(rowNum));
     }
 
     /**
@@ -84,13 +78,15 @@ public final class HSSFRow implements Comparable, Row {
      * @param record the low level api object this row should represent
      * @see org.apache.poi.hssf.usermodel.HSSFSheet#createRow(int)
      */
-    HSSFRow(HSSFWorkbook book, HSSFSheet sheet, RowRecord record)
-    {
+    HSSFRow(HSSFWorkbook book, HSSFSheet sheet, RowRecord record) {
         this.book = book;
         this.sheet = sheet;
         row = record;
-
         setRowNum(record.getRowNumber());
+        // Don't trust colIx boundaries as read by other apps
+        // set the RowRecord empty for the moment
+        record.setEmpty();
+        // subsequent calls to createCellFromRecord() will update the colIx boundaries properly
     }
 
     /**
@@ -136,10 +132,10 @@ public final class HSSFRow implements Comparable, Row {
      */
     public HSSFCell createCell(int columnIndex, int type)
     {
-    	short shortCellNum = (short)columnIndex;
-    	if(columnIndex > 0x7FFF) {
-    		shortCellNum = (short)(0xffff - columnIndex);
-    	}
+        short shortCellNum = (short)columnIndex;
+        if(columnIndex > 0x7FFF) {
+            shortCellNum = (short)(0xffff - columnIndex);
+        }
 
         HSSFCell cell = new HSSFCell(book, sheet, getRowNum(), shortCellNum, type);
         addCell(cell);
@@ -158,7 +154,7 @@ public final class HSSFRow implements Comparable, Row {
         removeCell((HSSFCell)cell, true);
     }
     private void removeCell(HSSFCell cell, boolean alsoRemoveRecords) {
-        
+
         int column=cell.getColumnIndex();
         if(column < 0) {
             throw new RuntimeException("Negative cell indexes not allowed");
@@ -167,20 +163,19 @@ public final class HSSFRow implements Comparable, Row {
             throw new RuntimeException("Specified cell is not from this row");
         }
         cells[column]=null;
-        
+
         if(alsoRemoveRecords) {
             CellValueRecordInterface cval = cell.getCellValueRecord();
             sheet.getSheet().removeValueRecord(getRowNum(), cval);
         }
-        
         if (cell.getColumnIndex()+1 == row.getLastCol()) {
-            row.setLastCol((short) (findLastCell(row.getLastCol())+1));
+            row.setLastCol(calculateNewLastCellPlusOne(row.getLastCol()));
         }
         if (cell.getColumnIndex() == row.getFirstCol()) {
-            row.setFirstCol(findFirstCell(row.getFirstCol()));
+            row.setFirstCol(calculateNewFirstCell(row.getFirstCol()));
         }
     }
-    
+
     /**
      * Removes all the cells from the row, and their
      *  records too.
@@ -200,27 +195,40 @@ public final class HSSFRow implements Comparable, Row {
      * @param cell low level cell to create the high level representation from
      * @return HSSFCell representing the low level record passed in
      */
-    protected HSSFCell createCellFromRecord(CellValueRecordInterface cell) {
+    HSSFCell createCellFromRecord(CellValueRecordInterface cell) {
         HSSFCell hcell = new HSSFCell(book, sheet, cell);
 
         addCell(hcell);
+        int colIx = cell.getColumn();
+        if (row.isEmpty()) {
+            row.setFirstCol(colIx);
+            row.setLastCol(colIx + 1);
+        } else {
+            if (colIx < row.getFirstCol()) {
+                row.setFirstCol(colIx);
+            } else if (colIx > row.getLastCol()) {
+                row.setLastCol(colIx + 1);
+            } else {
+                // added cell is within first and last cells
+            }
+        }
+        // TODO - RowRecord column boundaries need to be updated for cell comments too
         return hcell;
     }
 
     /**
      * set the row number of this row.
-     * @param rowNum  the row number (0-based)
+     * @param rowIndex  the row number (0-based)
      * @throws IndexOutOfBoundsException if the row number is not within the range 0-65535.
      */
-    public void setRowNum(int rowNum) {
-        if ((rowNum < 0) || (rowNum > RowRecord.MAX_ROW_NUMBER)) {
-          throw new IllegalArgumentException("Invalid row number (" + rowNum 
+    public void setRowNum(int rowIndex) {
+        if ((rowIndex < 0) || (rowIndex > RowRecord.MAX_ROW_NUMBER)) {
+          throw new IllegalArgumentException("Invalid row number (" + rowIndex
                   + ") outside allowable range (0.." + RowRecord.MAX_ROW_NUMBER + ")");
         }
-        this.rowNum = rowNum;
-        if (row != null)
-        {
-            row.setRowNumber(rowNum);   // used only for KEY comparison (HSSFRow)
+        rowNum = rowIndex;
+        if (row != null) {
+            row.setRowNumber(rowIndex);   // used only for KEY comparison (HSSFRow)
         }
     }
 
@@ -232,7 +240,7 @@ public final class HSSFRow implements Comparable, Row {
     {
         return rowNum;
     }
-    
+
     /**
      * Returns the HSSFSheet this row belongs to
      *
@@ -252,7 +260,7 @@ public final class HSSFRow implements Comparable, Row {
     protected int getOutlineLevel() {
         return row.getOutlineLevel();
     }
-    
+
     /**
      * Moves the supplied cell to a new column, which
      *  must not already have a cell there!
@@ -264,12 +272,12 @@ public final class HSSFRow implements Comparable, Row {
         if(cells.length > newColumn && cells[newColumn] != null) {
             throw new IllegalArgumentException("Asked to move cell to column " + newColumn + " but there's already a cell there");
         }
-        
+
         // Check it's one of ours
         if(! cells[cell.getColumnIndex()].equals(cell)) {
             throw new IllegalArgumentException("Asked to move a cell, but it didn't belong to our row");
         }
-        
+
         // Move the cell to the new position
         // (Don't remove the records though)
         removeCell(cell, false);
@@ -294,14 +302,14 @@ public final class HSSFRow implements Comparable, Row {
             System.arraycopy(oldCells,0,cells,0,oldCells.length);
         }
         cells[column]=cell;
-        
+
         // fix up firstCol and lastCol indexes
-        if (row.getFirstCol() == -1 || column < row.getFirstCol()) {
+        if (row.isEmpty() || column < row.getFirstCol()) {
             row.setFirstCol((short)column);
         }
-        
-        if (row.getLastCol() == -1 || column >= row.getLastCol()) {
-            row.setLastCol((short) (column+1)); // +1 -> for one past the last index 
+
+        if (row.isEmpty() || column >= row.getLastCol()) {
+            row.setLastCol((short) (column+1)); // +1 -> for one past the last index
         }
     }
 
@@ -311,14 +319,16 @@ public final class HSSFRow implements Comparable, Row {
      *  you get a null.
      * This is the basic call, with no policies applied
      *
-     * @param cellnum  0 based column number
+     * @param cellIndex  0 based column number
      * @return HSSFCell representing that column or null if undefined.
      */
-    private HSSFCell retrieveCell(int cellnum) {
-        if(cellnum<0||cellnum>=cells.length) return null;
-        return cells[cellnum];
+    private HSSFCell retrieveCell(int cellIndex) {
+        if(cellIndex<0||cellIndex>=cells.length) {
+            return null;
+        }
+        return cells[cellIndex];
     }
-    
+
     /**
      * @deprecated (Aug 2008) use {@link #getCell(int)}
      */
@@ -326,7 +336,7 @@ public final class HSSFRow implements Comparable, Row {
         int ushortCellNum = cellnum & 0x0000FFFF; // avoid sign extension
         return getCell(ushortCellNum);
     }
-    
+
     /**
      * Get the hssfcell representing a given column (logical cell)
      *  0-based.  If you ask for a cell that is not defined then
@@ -339,7 +349,7 @@ public final class HSSFRow implements Comparable, Row {
     public HSSFCell getCell(int cellnum) {
         return getCell(cellnum, book.getMissingCellPolicy());
     }
-    
+
     /**
      * Get the hssfcell representing a given column (logical cell)
      *  0-based.  If you ask for a cell that is not defined, then
@@ -374,19 +384,18 @@ public final class HSSFRow implements Comparable, Row {
      * get the number of the first cell contained in this row.
      * @return short representing the first logical cell in the row, or -1 if the row does not contain any cells.
      */
-    public short getFirstCellNum()
-    {
-        if (getPhysicalNumberOfCells() == 0)
+    public short getFirstCellNum() {
+        if (row.isEmpty()) {
             return -1;
-        else
-            return row.getFirstCol();
+        }
+        return (short) row.getFirstCol();
     }
 
     /**
-     * Gets the index of the last cell contained in this row <b>PLUS ONE</b>. The result also 
+     * Gets the index of the last cell contained in this row <b>PLUS ONE</b>. The result also
      * happens to be the 1-based column number of the last cell.  This value can be used as a
      * standard upper bound when iterating over cells:
-     * <pre> 
+     * <pre>
      * short minColIx = row.getFirstCellNum();
      * short maxColIx = row.getLastCellNum();
      * for(short colIx=minColIx; colIx&lt;maxColIx; colIx++) {
@@ -397,15 +406,15 @@ public final class HSSFRow implements Comparable, Row {
      *   //... do something with cell
      * }
      * </pre>
-     * 
+     *
      * @return short representing the last logical cell in the row <b>PLUS ONE</b>, or -1 if the
      *  row does not contain any cells.
      */
     public short getLastCellNum() {
-        if (getPhysicalNumberOfCells() == 0) {
+        if (row.isEmpty()) {
             return -1;
         }
-        return row.getLastCol();
+        return (short) row.getLastCol();
     }
 
 
@@ -446,7 +455,7 @@ public final class HSSFRow implements Comparable, Row {
     public void setZeroHeight(boolean zHeight) {
         row.setZeroHeight(zHeight);
     }
-  
+
     /**
      * get whether or not to display this row with 0 height
      * @return - zHeight height is zero or not.
@@ -478,7 +487,7 @@ public final class HSSFRow implements Comparable, Row {
         short height = row.getHeight();
 
         //The low-order 15 bits contain the row height.
-        //The 0x8000 bit indicates that the row is standard height (optional) 
+        //The 0x8000 bit indicates that the row is standard height (optional)
         if ((height & 0x8000) != 0) height = sheet.getSheet().getDefaultRowHeight();
         else height &= 0x7FFF;
 
@@ -508,46 +517,46 @@ public final class HSSFRow implements Comparable, Row {
     }
 
     /**
-     * used internally to refresh the "last cell" when the last cell is removed.
+     * used internally to refresh the "last cell plus one" when the last cell is removed.
+     * @return 0 when row contains no cells
      */
+    private int calculateNewLastCellPlusOne(int lastcell) {
+        int cellIx = lastcell - 1;
+        HSSFCell r = retrieveCell(cellIx);
 
-    private short findLastCell(short lastcell)
-    {
-        short cellnum = (short) (lastcell - 1);
-        HSSFCell r = getCell(cellnum);
-
-        while (r == null && cellnum >= 0)
-        {
-            r = getCell(--cellnum);
+        while (r == null) {
+            if (cellIx < 0) {
+                return 0;
+            }
+            r = retrieveCell(--cellIx);
         }
-        return cellnum;
+        return cellIx+1;
     }
 
     /**
      * used internally to refresh the "first cell" when the first cell is removed.
+     * @return 0 when row contains no cells (also when first cell is occupied)
      */
+    private int calculateNewFirstCell(int firstcell) {
+        int cellIx = firstcell + 1;
+        HSSFCell r = retrieveCell(cellIx);
 
-    private short findFirstCell(short firstcell)
-    {
-        short cellnum = (short) (firstcell + 1);
-        HSSFCell r = getCell(cellnum);
-
-        while (r == null && cellnum <= getLastCellNum())
-        {
-            r = getCell(++cellnum);
+        while (r == null) {
+            if (cellIx <= cells.length) {
+                return 0;
+            }
+            r = retrieveCell(++cellIx);
         }
-        if (cellnum > getLastCellNum())
-            return -1;
-        return cellnum;
+        return cellIx;
     }
-    
+
     /**
      * Is this row formatted? Most aren't, but some rows
      *  do have whole-row styles. For those that do, you
      *  can get the formatting from {@link #getRowStyle()}
      */
     public boolean isFormatted() {
-    	return row.getFormatted();
+        return row.getFormatted();
     }
     /**
      * Returns the whole-row cell styles. Most rows won't
@@ -555,7 +564,7 @@ public final class HSSFRow implements Comparable, Row {
      *  {@link #isFormatted()} to check first.
      */
     public HSSFCellStyle getRowStyle() {
-    	if(!isFormatted()) { return null; }
+        if(!isFormatted()) { return null; }
         short styleIndex = row.getXFIndex();
         ExtendedFormatRecord xf = book.getWorkbook().getExFormatAt(styleIndex);
         return new HSSFCellStyle(styleIndex, xf, book);
@@ -564,19 +573,19 @@ public final class HSSFRow implements Comparable, Row {
      * Applies a whole-row cell styling to the row.
      */
     public void setRowStyle(HSSFCellStyle style) {
-    	row.setFormatted(true);
-    	row.setXFIndex(style.getIndex());
+        row.setFormatted(true);
+        row.setXFIndex(style.getIndex());
     }
 
     /**
-     * @return cell iterator of the physically defined cells. 
+     * @return cell iterator of the physically defined cells.
      * Note that the 4th element might well not be cell 4, as the iterator
      *  will not return un-defined (null) cells.
      * Call getCellNum() on the returned cells to know which cell they are.
-     * As this only ever works on physically defined cells, 
+     * As this only ever works on physically defined cells,
      *  the {@link org.apache.poi.ss.usermodel.Row.MissingCellPolicy} has no effect.
      */
-    public Iterator cellIterator()
+    public Iterator<Cell> cellIterator()
     {
       return new CellIterator();
     }
@@ -584,18 +593,17 @@ public final class HSSFRow implements Comparable, Row {
      * Alias for {@link #cellIterator} to allow
      *  foreach loops
      */
-    public Iterator iterator() { 
+    public Iterator iterator() {
        return cellIterator();
     }
-    
+
     /**
      * An iterator over the (physical) cells in the row.
      */
-    private class CellIterator implements Iterator
-    {
+    private class CellIterator implements Iterator<Cell> {
       int thisId=-1;
       int nextId=-1;
-      
+
       public CellIterator()
       {
         findNext();
@@ -605,7 +613,7 @@ public final class HSSFRow implements Comparable, Row {
         return nextId<cells.length;
       }
 
-      public Object next() {
+      public Cell next() {
           if (!hasNext())
               throw new NoSuchElementException("At last element");
         HSSFCell cell=cells[nextId];
@@ -619,7 +627,7 @@ public final class HSSFRow implements Comparable, Row {
               throw new IllegalStateException("remove() called before next()");
         cells[thisId]=null;
       }
-      
+
       private void findNext()
       {
         int i=nextId+1;
@@ -629,7 +637,7 @@ public final class HSSFRow implements Comparable, Row {
         }
         nextId=i;
       }
-      
+
     }
 
     public int compareTo(Object obj)
diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java b/src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java
index 30556bd554..af51ce654e 100644
--- a/src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java
+++ b/src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java
@@ -37,6 +37,7 @@ import org.apache.poi.hssf.record.CellValueRecordInterface;
 import org.apache.poi.hssf.record.DVRecord;
 import org.apache.poi.hssf.record.EscherAggregate;
 import org.apache.poi.hssf.record.ExtendedFormatRecord;
+import org.apache.poi.hssf.record.NoteRecord;
 import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.RowRecord;
 import org.apache.poi.hssf.record.SCLRecord;
@@ -45,10 +46,11 @@ import org.apache.poi.hssf.record.WindowTwoRecord;
 import org.apache.poi.hssf.record.aggregates.DataValidityTable;
 import org.apache.poi.hssf.record.formula.FormulaShifter;
 import org.apache.poi.hssf.util.PaneInformation;
+import org.apache.poi.hssf.util.Region;
+import org.apache.poi.ss.usermodel.Cell;
 import org.apache.poi.ss.usermodel.CellStyle;
 import org.apache.poi.ss.usermodel.Row;
 import org.apache.poi.ss.util.CellRangeAddress;
-import org.apache.poi.hssf.util.Region;
 import org.apache.poi.util.POILogFactory;
 import org.apache.poi.util.POILogger;
 
@@ -1160,6 +1162,12 @@ public final class HSSFSheet implements org.apache.poi.ss.usermodel.Sheet {
             s = endRow;
             inc = -1;
         }
+        NoteRecord[] noteRecs;
+        if (moveComments) {
+            noteRecs = sheet.getNoteRecords(); 
+        } else {
+            noteRecs = NoteRecord.EMPTY_ARRAY;
+        }
 
         shiftMerged(startRow, endRow, n, true);
         sheet.getPageSettings().shiftRowBreaks(startRow, endRow, n);
@@ -1182,12 +1190,6 @@ public final class HSSFSheet implements org.apache.poi.ss.usermodel.Sheet {
             //  be done for the now empty destination row
             if (row == null) continue; // Nothing to do for this row
 
-            // Fetch the first and last columns of the
-            //  row now, so we still have them to hand
-            //  once we start removing cells
-            short firstCol = row.getFirstCellNum();
-            short lastCol = row.getLastCellNum();
-
             // Fix up row heights if required
             if (copyRowHeight) {
                 row2Replace.setHeight(row.getHeight());
@@ -1198,7 +1200,7 @@ public final class HSSFSheet implements org.apache.poi.ss.usermodel.Sheet {
 
             // Copy each cell from the source row to
             //  the destination row
-            for(Iterator cells = row.cellIterator(); cells.hasNext(); ) {
+            for(Iterator<Cell> cells = row.cellIterator(); cells.hasNext(); ) {
                 HSSFCell cell = (HSSFCell)cells.next();
                 row.removeCell( cell );
                 CellValueRecordInterface cellRecord = cell.getCellValueRecord();
@@ -1219,8 +1221,13 @@ public final class HSSFSheet implements org.apache.poi.ss.usermodel.Sheet {
             //  destination row. Note that comments can
             //  exist for cells which are null
             if(moveComments) {
-                for( short col = firstCol; col <= lastCol; col++ ) {
-                    HSSFComment comment = getCellComment(rowNum, col);
+                // This code would get simpler if NoteRecords could be organised by HSSFRow.
+                for(int i=noteRecs.length-1; i>=0; i--) {
+                    NoteRecord nr = noteRecs[i];
+                    if (nr.getRow() != rowNum) {
+                        continue;
+                    }
+                    HSSFComment comment = getCellComment(rowNum, nr.getColumn());
                     if (comment != null) {
                        comment.setRow(rowNum + n);
                     }
diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
index 9525a181a1..7014cb5149 100644
--- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
+++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java
@@ -16,9 +16,11 @@
 ==================================================================== */
 package org.apache.poi.hssf.usermodel;
 
-import junit.framework.TestCase;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
 
-import java.io.*;
+import junit.framework.TestCase;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
 
@@ -37,7 +39,7 @@ public final class TestHSSFComment extends TestCase {
         String commentText = "We can set comments in POI";
         String commentAuthor = "Apache Software Foundation";
         int cellRow = 3;
-        short cellColumn = 1;
+        int cellColumn = 1;
 
         HSSFWorkbook wb = new HSSFWorkbook();
 
@@ -55,6 +57,10 @@ public final class TestHSSFComment extends TestCase {
         comment.setString(string1);
         comment.setAuthor(commentAuthor);
         cell.setCellComment(comment);
+        if (false) {
+            // TODO - the following line should break this test, but it doesn't
+            cell.removeCellComment();
+        }
 
         //verify our settings
         assertEquals(HSSFSimpleShape.OBJECT_TYPE_COMMENT, comment.getShapeType());
@@ -79,11 +85,11 @@ public final class TestHSSFComment extends TestCase {
         assertEquals(commentText, comment.getString().getString());
         assertEquals(cellRow, comment.getRow());
         assertEquals(cellColumn, comment.getColumn());
-        
-        
+
+
         // Change slightly, and re-test
         comment.setString(new HSSFRichTextString("New Comment Text"));
-        
+
         out = new ByteArrayOutputStream();
         wb.write(out);
         out.close();
@@ -131,7 +137,7 @@ public final class TestHSSFComment extends TestCase {
 
              assertEquals(HSSFSimpleShape.OBJECT_TYPE_COMMENT, comment.getShapeType());
              assertEquals("Yegor Kozlov", comment.getAuthor());
-             assertFalse("cells in the second column have not empyy notes", 
+             assertFalse("cells in the second column have not empyy notes",
                      "".equals(comment.getString().getString()));
              assertEquals(rownum, comment.getRow());
              assertEquals(cell.getColumnIndex(), comment.getColumn());
@@ -176,24 +182,24 @@ public final class TestHSSFComment extends TestCase {
         }
 
      }
-    
+
     public void testDeleteComments() throws Exception {
         HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("SimpleWithComments.xls");
         HSSFSheet sheet = wb.getSheetAt(0);
-        
+
         // Zap from rows 1 and 3
         assertNotNull(sheet.getRow(0).getCell(1).getCellComment());
         assertNotNull(sheet.getRow(1).getCell(1).getCellComment());
         assertNotNull(sheet.getRow(2).getCell(1).getCellComment());
-        
+
         sheet.getRow(0).getCell(1).removeCellComment();
         sheet.getRow(2).getCell(1).setCellComment(null);
-        
+
         // Check gone so far
         assertNull(sheet.getRow(0).getCell(1).getCellComment());
         assertNotNull(sheet.getRow(1).getCell(1).getCellComment());
         assertNull(sheet.getRow(2).getCell(1).getCellComment());
-        
+
         // Save and re-load
         ByteArrayOutputStream out = new ByteArrayOutputStream();
         wb.write(out);
@@ -204,7 +210,7 @@ public final class TestHSSFComment extends TestCase {
         assertNull(sheet.getRow(0).getCell(1).getCellComment());
         assertNotNull(sheet.getRow(1).getCell(1).getCellComment());
         assertNull(sheet.getRow(2).getCell(1).getCellComment());
-        
+
 //        FileOutputStream fout = new FileOutputStream("/tmp/c.xls");
 //        wb.write(fout);
 //        fout.close();
diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.java
index 8e7864fad2..444eef108e 100644
--- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.java
+++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.java
@@ -17,9 +17,12 @@
 
 package org.apache.poi.hssf.usermodel;
 
+import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
+import org.apache.poi.hssf.record.BlankRecord;
+import org.apache.poi.hssf.record.RowRecord;
 
 /**
  * Test HSSFRow is okay.
@@ -48,6 +51,32 @@ public final class TestHSSFRow extends TestCase {
         assertEquals(1, row.getFirstCellNum());
         assertEquals(4, row.getLastCellNum());
     }
+    public void testLastAndFirstColumns_bug46654() {
+        int ROW_IX = 10;
+        int COL_IX = 3;
+        HSSFWorkbook workbook = new HSSFWorkbook();
+        HSSFSheet sheet = workbook.createSheet("Sheet1");
+        RowRecord rowRec = new RowRecord(ROW_IX);
+        rowRec.setFirstCol((short)2);
+        rowRec.setLastCol((short)5);
+
+        BlankRecord br = new BlankRecord();
+        br.setRow(ROW_IX);
+		br.setColumn((short)COL_IX);
+
+        sheet.getSheet().addValueRecord(ROW_IX, br);
+        HSSFRow row = new HSSFRow(workbook, sheet, rowRec);
+        HSSFCell cell = row.createCellFromRecord(br);
+
+        if (row.getFirstCellNum() == 2 && row.getLastCellNum() == 5) {
+            throw new AssertionFailedError("Identified bug 46654a");
+        }
+        assertEquals(COL_IX, row.getFirstCellNum());
+        assertEquals(COL_IX + 1, row.getLastCellNum());
+        row.removeCell(cell);
+        assertEquals(-1, row.getFirstCellNum());
+        assertEquals(-1, row.getLastCellNum());
+    }
 
     /**
      * Make sure that there is no cross-talk between rows especially with getFirstCellNum and getLastCellNum
@@ -204,11 +233,11 @@ public final class TestHSSFRow extends TestCase {
         row.createCell(255);
         assertEquals(256, row.getLastCellNum());
     }
-    
+
     /**
      * Tests for the missing/blank cell policy stuff
      */
-    public void testGetCellPolicy() throws Exception {
+    public void testGetCellPolicy() {
         HSSFWorkbook book = new HSSFWorkbook();
         HSSFSheet sheet = book.createSheet("test");
         HSSFRow row = sheet.createRow(0);
@@ -223,7 +252,7 @@ public final class TestHSSFRow extends TestCase {
         row.createCell(1).setCellValue(3.2);
         row.createCell(4, HSSFCell.CELL_TYPE_BLANK);
         row.createCell(5).setCellValue(4);
-        
+
         // First up, no policy given, uses default
         assertEquals(HSSFCell.CELL_TYPE_STRING,  row.getCell(0).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(1).getCellType());
@@ -231,7 +260,7 @@ public final class TestHSSFRow extends TestCase {
         assertEquals(null, row.getCell(3));
         assertEquals(HSSFCell.CELL_TYPE_BLANK,   row.getCell(4).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(5).getCellType());
-        
+
         // RETURN_NULL_AND_BLANK - same as default
         assertEquals(HSSFCell.CELL_TYPE_STRING,  row.getCell(0, HSSFRow.RETURN_NULL_AND_BLANK).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(1, HSSFRow.RETURN_NULL_AND_BLANK).getCellType());
@@ -239,7 +268,7 @@ public final class TestHSSFRow extends TestCase {
         assertEquals(null, row.getCell(3, HSSFRow.RETURN_NULL_AND_BLANK));
         assertEquals(HSSFCell.CELL_TYPE_BLANK,   row.getCell(4, HSSFRow.RETURN_NULL_AND_BLANK).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(5, HSSFRow.RETURN_NULL_AND_BLANK).getCellType());
-        
+
         // RETURN_BLANK_AS_NULL - nearly the same
         assertEquals(HSSFCell.CELL_TYPE_STRING,  row.getCell(0, HSSFRow.RETURN_BLANK_AS_NULL).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(1, HSSFRow.RETURN_BLANK_AS_NULL).getCellType());
@@ -247,7 +276,7 @@ public final class TestHSSFRow extends TestCase {
         assertEquals(null, row.getCell(3, HSSFRow.RETURN_BLANK_AS_NULL));
         assertEquals(null, row.getCell(4, HSSFRow.RETURN_BLANK_AS_NULL));
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(5, HSSFRow.RETURN_BLANK_AS_NULL).getCellType());
-        
+
         // CREATE_NULL_AS_BLANK - creates as needed
         assertEquals(HSSFCell.CELL_TYPE_STRING,  row.getCell(0, HSSFRow.CREATE_NULL_AS_BLANK).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(1, HSSFRow.CREATE_NULL_AS_BLANK).getCellType());
@@ -255,7 +284,7 @@ public final class TestHSSFRow extends TestCase {
         assertEquals(HSSFCell.CELL_TYPE_BLANK,   row.getCell(3, HSSFRow.CREATE_NULL_AS_BLANK).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_BLANK,   row.getCell(4, HSSFRow.CREATE_NULL_AS_BLANK).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(5, HSSFRow.CREATE_NULL_AS_BLANK).getCellType());
-        
+
         // Check created ones get the right column
         assertEquals(0, row.getCell(0, HSSFRow.CREATE_NULL_AS_BLANK).getColumnIndex());
         assertEquals(1, row.getCell(1, HSSFRow.CREATE_NULL_AS_BLANK).getColumnIndex());
@@ -263,12 +292,12 @@ public final class TestHSSFRow extends TestCase {
         assertEquals(3, row.getCell(3, HSSFRow.CREATE_NULL_AS_BLANK).getColumnIndex());
         assertEquals(4, row.getCell(4, HSSFRow.CREATE_NULL_AS_BLANK).getColumnIndex());
         assertEquals(5, row.getCell(5, HSSFRow.CREATE_NULL_AS_BLANK).getColumnIndex());
-        
-        
+
+
         // Now change the cell policy on the workbook, check
         //  that that is now used if no policy given
         book.setMissingCellPolicy(HSSFRow.RETURN_BLANK_AS_NULL);
-        
+
         assertEquals(HSSFCell.CELL_TYPE_STRING,  row.getCell(0).getCellType());
         assertEquals(HSSFCell.CELL_TYPE_NUMERIC, row.getCell(1).getCellType());
         assertEquals(null, row.getCell(2));
@@ -300,5 +329,4 @@ public final class TestHSSFRow extends TestCase {
         row2 = sheet.getRow(1);
         assertEquals(400, row2.getHeight());
     }
-
 }