From aac8563019dc8727919c9d887f4241ddb45935cd Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Thu, 5 Feb 2009 07:39:57 +0000 Subject: [PATCH] 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 --- src/documentation/content/xdocs/changes.xml | 1 + src/documentation/content/xdocs/status.xml | 1 + src/java/org/apache/poi/hssf/model/Sheet.java | 26 +- .../apache/poi/hssf/record/NoteRecord.java | 338 +++++++++--------- .../org/apache/poi/hssf/record/RowRecord.java | 119 +++--- .../apache/poi/hssf/usermodel/HSSFCell.java | 5 +- .../poi/hssf/usermodel/HSSFComment.java | 93 +++-- .../apache/poi/hssf/usermodel/HSSFRow.java | 202 ++++++----- .../apache/poi/hssf/usermodel/HSSFSheet.java | 27 +- .../poi/hssf/usermodel/TestHSSFComment.java | 32 +- .../poi/hssf/usermodel/TestHSSFRow.java | 50 ++- 11 files changed, 484 insertions(+), 410 deletions(-) diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index eb6c43861..292f202f8 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 46654 - HSSFRow/RowRecord to properly update cell boundary indexes 46643 - Fixed formula parser to encode range operator with tMemFunc 46647 - Fixed COUNTIF NE operator and other special cases involving type conversion 46635 - Added a method to remove slides diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 45ebb7b03..d4f168efe 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 46654 - HSSFRow/RowRecord to properly update cell boundary indexes 46643 - Fixed formula parser to encode range operator with tMemFunc 46647 - Fixed COUNTIF NE operator and other special cases involving type conversion 46635 - Added a method to remove slides diff --git a/src/java/org/apache/poi/hssf/model/Sheet.java b/src/java/org/apache/poi/hssf/model/Sheet.java index 548d5ad78..5e528f659 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 null, typically empty array + */ + public NoteRecord[] getNoteRecords() { + List temp = new ArrayList(); + 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 eac7596e9..f24ea0b4a 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 NoteRecord 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 - } + /** + * Construct a new NoteRecord 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; - } + /** + * @return id of this record. + */ + public short getSid() { + return sid; + } - /** - * Read the record data from the supplied RecordInputStream - */ - 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); - } + /** + * Read the record data from the supplied RecordInputStream + */ + 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()); - } - } + 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); - } + 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(); + /** + * 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(); - } + 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; - } + /** + * 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(short row){ - field_1_row = 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 short getColumn(){ - return field_2_col; - } + /** + * 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(short col){ - field_2_col = 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 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; - } + /** + * 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 short getShapeId() { + return field_4_shapeid; + } - /** - * Object id for OBJ record that contains the comment - */ - public void setShapeId(short id){ - field_4_shapeid = id; - } + /** + * 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 + * + * @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; - } + /** + * 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 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 5f084a231..a2f1e22de 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(); @@ -86,12 +84,23 @@ public final class RowRecord extends StandardRecord { field_8_xf_index = in.readShort(); } + /** + * 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 109b95d37..7ad80298c 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 null 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 txshapesByShapeId = new HashMap(); for (Iterator 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 3d28fc1e2..e74026043 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; + /* + * 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 = null; - private TextObjectRecord txo = null; + 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 true if the comment is visible, false 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 true if the comment is visible, false 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 ebd83a86b..f8848f074 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 PLUS ONE. The result also + * Gets the index of the last cell contained in this row PLUS ONE. 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: - * 
 
+     * 
      * short minColIx = row.getFirstCellNum();
      * short maxColIx = row.getLastCellNum();
      * for(short colIx=minColIx; colIx<maxColIx; colIx++) {
@@ -397,15 +406,15 @@ public final class HSSFRow implements Comparable, Row {
      *   //... do something with cell
      * }
      * 

-     * 
+     *
      * @return short representing the last logical cell in the row PLUS ONE, 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 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 {
       int thisId=-1;
       int nextId=-1;
-      
+
       public CellIterator()
       {
         findNext();
@@ -605,7 +613,7 @@ public final class HSSFRow implements Comparable, Row {
         return nextId 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 9525a181a..7014cb514 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 8e7864fad..444eef108 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());
     }
-
 }