Patch from Andrew Shirley from bug #49185 - Support for HSSFNames where the comment is stored in a NameCommentRecord

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

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 
     <changes>
         <release version="3.7-beta2" date="2010-??-??">
+           <action dev="POI-DEVELOPERS" type="add">49185 - Support for HSSFNames where the comment is stored in a NameCommentRecord</action>
            <action dev="POI-DEVELOPERS" type="fix">49599 - Correct writing of NoteRecord author text when switching between ASCII and Unicode</action>
            <action dev="POI-DEVELOPERS" type="fix">HWPF: Improve reading of auto-saved ("complex") documents</action>
            <action dev="POI-DEVELOPERS" type="add">Paragraph level as well as whole-file text extraction for Word 6/95 files through HWPF</action>

src/java/org/apache/poi/hssf/dev/BiffViewer.java

@@ -197,6 +197,7 @@ public final class BiffViewer {
 			case MulBlankRecord.sid:       return new MulBlankRecord(in);
 			case MulRKRecord.sid:          return new MulRKRecord(in);
 			case NameRecord.sid:           return new NameRecord(in);
+			case NameCommentRecord.sid:    return new NameCommentRecord(in);
 			case NoteRecord.sid:           return new NoteRecord(in);
 			case NumberRecord.sid:         return new NumberRecord(in);
 			case ObjRecord.sid:            return new ObjRecord(in);

src/java/org/apache/poi/hssf/model/InternalWorkbook.java

@@ -20,8 +20,11 @@ package org.apache.poi.hssf.model;
 import java.security.AccessControlException;
 import java.util.ArrayList;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
+import java.util.Map.Entry;
 
 import org.apache.poi.ddf.EscherBSERecord;
 import org.apache.poi.ddf.EscherBoolProperty;
@@ -57,6 +60,7 @@ import org.apache.poi.hssf.record.HyperlinkRecord;
 import org.apache.poi.hssf.record.InterfaceEndRecord;
 import org.apache.poi.hssf.record.InterfaceHdrRecord;
 import org.apache.poi.hssf.record.MMSRecord;
+import org.apache.poi.hssf.record.NameCommentRecord;
 import org.apache.poi.hssf.record.NameRecord;
 import org.apache.poi.hssf.record.PaletteRecord;
 import org.apache.poi.hssf.record.PasswordRecord;
@@ -165,6 +169,11 @@ public final class InternalWorkbook {
     private WriteAccessRecord writeAccess;
     private WriteProtectRecord writeProtect;
 
+    /**
+     * Hold the {@link NameCommentRecord}s indexed by the name of the {@link NameRecord} to which they apply.
+     */
+    private final Map<String, NameCommentRecord> commentRecords;
+
     private InternalWorkbook() {
     	records     = new WorkbookRecordList();
 
@@ -176,6 +185,7 @@ public final class InternalWorkbook {
 		maxformatid = -1;
 		uses1904datewindowing = false;
 		escherBSERecords = new ArrayList<EscherBSERecord>();
+		commentRecords = new LinkedHashMap<String, NameCommentRecord>();
     }
 
     /**
@@ -261,7 +271,7 @@ public final class InternalWorkbook {
                     // LinkTable can start with either of these
                     if (log.check( POILogger.DEBUG ))
                         log.log(DEBUG, "found SupBook record at " + k);
-                    retval.linkTable = new LinkTable(recs, k, retval.records);
+                    retval.linkTable = new LinkTable(recs, k, retval.records, retval.commentRecords);
                     k+=retval.linkTable.getRecordCount() - 1;
                     continue;
                 case FormatRecord.sid :
@@ -299,6 +309,13 @@ public final class InternalWorkbook {
                     if (log.check( POILogger.DEBUG ))
                         log.log(DEBUG, "found FileSharing at " + k);
                     retval.fileShare = (FileSharingRecord) rec;
+                    break;
+
+                case NameCommentRecord.sid:
+                    final NameCommentRecord ncr = (NameCommentRecord) rec;
+                    if (log.check( POILogger.DEBUG ))
+                        log.log(DEBUG, "found NameComment at " + k);
+                    retval.commentRecords.put(ncr.getNameText(), ncr);
                 default :
             }
             records.add(rec);
@@ -1823,6 +1840,14 @@ public final class InternalWorkbook {
         return linkTable.getNameRecord(index);
     }
 
+    /** gets the name comment record
+     * @param nameRecord name record who's comment is required.
+     * @return name comment record or <code>null</code> if there isn't one for the given name.
+     */
+    public NameCommentRecord getNameCommentRecord(final NameRecord nameRecord){
+        return commentRecords.get(nameRecord.getNameText());
+    }
+
     /** creates new name
      * @return new name record
      */
@@ -1880,6 +1905,22 @@ public final class InternalWorkbook {
         }
     }
 
+    /**
+     * If a {@link NameCommentRecord} is added or the name it references
+     *  is renamed, then this will update the lookup cache for it.
+     */
+    public void updateNameCommentRecordCache(final NameCommentRecord commentRecord) {
+       if(commentRecords.containsValue(commentRecord)) {
+          for(Entry<String,NameCommentRecord> entry : commentRecords.entrySet()) {
+             if(entry.getValue().equals(commentRecord)) {
+                commentRecords.remove(entry.getKey());
+                break;
+             }
+          }
+       }
+       commentRecords.put(commentRecord.getNameText(), commentRecord);
+    }
+
     /**
      * Returns a format index that matches the passed in format.  It does not tie into HSSFDataFormat.
      * @param format the format string

src/java/org/apache/poi/hssf/model/LinkTable.java

@@ -20,12 +20,14 @@ package org.apache.poi.hssf.model;
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 
 import org.apache.poi.hssf.record.CRNCountRecord;
 import org.apache.poi.hssf.record.CRNRecord;
 import org.apache.poi.hssf.record.CountryRecord;
 import org.apache.poi.hssf.record.ExternSheetRecord;
 import org.apache.poi.hssf.record.ExternalNameRecord;
+import org.apache.poi.hssf.record.NameCommentRecord;
 import org.apache.poi.hssf.record.NameRecord;
 import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.SupBookRecord;
@@ -149,7 +151,7 @@ final class LinkTable {
 	private final int _recordCount;
 	private final WorkbookRecordList _workbookRecordList; // TODO - would be nice to remove this
 
-	public LinkTable(List inputList, int startIndex, WorkbookRecordList workbookRecordList) {
+	public LinkTable(List inputList, int startIndex, WorkbookRecordList workbookRecordList, Map<String, NameCommentRecord> commentRecords) {
 
 		_workbookRecordList = workbookRecordList;
 		RecordStream rs = new RecordStream(inputList, startIndex);
@@ -176,11 +178,22 @@ final class LinkTable {
 		}
 
 		_definedNames = new ArrayList<NameRecord>();
-		// collect zero or more DEFINEDNAMEs id=0x18
-		while(rs.peekNextClass() == NameRecord.class) {
+		// collect zero or more DEFINEDNAMEs id=0x18,
+		//  with their comments if present
+		while(true) {
+		  Class nextClass = rs.peekNextClass();
+		  if (nextClass == NameRecord.class) {
 		    NameRecord nr = (NameRecord)rs.getNext();
 		    _definedNames.add(nr);
 		  }
+		  else if (nextClass == NameCommentRecord.class) {
+		    NameCommentRecord ncr = (NameCommentRecord)rs.getNext();
+		    commentRecords.put(ncr.getNameText(), ncr);
+		  }
+		  else {
+		    break;
+		  }
+		}
 
 		_recordCount = rs.getCountRead();
 		_workbookRecordList.getRecords().addAll(inputList.subList(startIndex, startIndex + _recordCount));

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

@@ -0,0 +1,150 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.hssf.record;
+
+import org.apache.poi.util.HexDump;
+import org.apache.poi.util.LittleEndianInput;
+import org.apache.poi.util.LittleEndianOutput;
+import org.apache.poi.util.StringUtil;
+
+/**
+ * Title: NAMECMT Record (0x0894)
+ * <p/>
+ * Description: Defines a comment associated with a specified name.
+ * <P>
+ * REFERENCE:
+ * <P>
+ *
+ * @author Andrew Shirley (aks at corefiling.co.uk)
+ */
+public final class NameCommentRecord extends StandardRecord {
+  public final static short sid = 0x0894;
+
+  private final short field_1_record_type;
+  private final short field_2_frt_cell_ref_flag;
+  private final long field_3_reserved;
+  //private short             field_4_name_length;
+  //private short             field_5_comment_length;
+  private String field_6_name_text;
+  private String field_7_comment_text;
+
+  public NameCommentRecord(final String name, final String comment) {
+    field_1_record_type = 0;
+    field_2_frt_cell_ref_flag = 0;
+    field_3_reserved = 0;
+    field_6_name_text = name;
+    field_7_comment_text = comment;
+  }
+
+  @Override
+  public void serialize(final LittleEndianOutput out) {
+    final int field_4_name_length = field_6_name_text.length();
+    final int field_5_comment_length = field_7_comment_text.length();
+
+    out.writeShort(field_1_record_type);
+    out.writeShort(field_2_frt_cell_ref_flag);
+    out.writeLong(field_3_reserved);
+    out.writeShort(field_4_name_length);
+    out.writeShort(field_5_comment_length);
+
+    out.writeByte(0);
+    out.write(field_6_name_text.getBytes());
+    out.writeByte(0);
+    out.write(field_7_comment_text.getBytes());
+  }
+
+  @Override
+  protected int getDataSize() {
+    return 18 // 4 shorts + 1 long + 2 spurious 'nul's
+         + field_6_name_text.length()
+         + field_7_comment_text.length();
+  }
+
+  /**
+   * @param ris the RecordInputstream to read the record from
+   */
+  public NameCommentRecord(final RecordInputStream ris) {
+    final LittleEndianInput in = ris;
+    field_1_record_type = in.readShort();
+    field_2_frt_cell_ref_flag = in.readShort();
+    field_3_reserved = in.readLong();
+    final int field_4_name_length = in.readShort();
+    final int field_5_comment_length = in.readShort();
+
+    in.readByte(); //spurious NUL
+    field_6_name_text = StringUtil.readCompressedUnicode(in, field_4_name_length);
+    in.readByte(); //spurious NUL
+    field_7_comment_text = StringUtil.readCompressedUnicode(in, field_5_comment_length);
+  }
+
+  /**
+   * return the non static version of the id for this record.
+   */
+  @Override
+  public short getSid() {
+    return sid;
+  }
+
+  @Override
+  public String toString() {
+    final StringBuffer sb = new StringBuffer();
+
+    sb.append("[NAMECMT]\n");
+    sb.append("    .record type            = ").append(HexDump.shortToHex(field_1_record_type)).append("\n");
+    sb.append("    .frt cell ref flag      = ").append(HexDump.byteToHex(field_2_frt_cell_ref_flag)).append("\n");
+    sb.append("    .reserved               = ").append(field_3_reserved).append("\n");
+    sb.append("    .name length            = ").append(field_6_name_text.length()).append("\n");
+    sb.append("    .comment length         = ").append(field_7_comment_text.length()).append("\n");
+    sb.append("    .name                   = ").append(field_6_name_text).append("\n");
+    sb.append("    .comment                = ").append(field_7_comment_text).append("\n");
+    sb.append("[/NAMECMT]\n");
+
+    return sb.toString();
+  }
+
+  /**
+   * @return the name of the NameRecord to which this comment applies.
+   */
+  public String getNameText() {
+    return field_6_name_text;
+  }
+  
+  /**
+   * Updates the name we're associated with, normally used
+   *  when renaming that Name
+   */
+  public void setNameText(String newName) {
+     field_6_name_text = newName;
+  }
+
+  /**
+   * @return the text of the comment.
+   */
+  public String getCommentText() {
+    return field_7_comment_text;
+  }
+  
+  public void setCommentText(String comment) {
+     field_7_comment_text = comment;
+  }
+
+  public short getRecordType() {
+    return field_1_record_type;
+  }
+
+}

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

@@ -173,6 +173,7 @@ public final class RecordFactory {
 		MulBlankRecord.class,
 		MulRKRecord.class,
 		NameRecord.class,
+		NameCommentRecord.class,
 		NoteRecord.class,
 		NumberRecord.class,
 		ObjectProtectRecord.class,

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

@@ -19,6 +19,7 @@ package org.apache.poi.hssf.usermodel;
 
 import org.apache.poi.hssf.model.HSSFFormulaParser;
 import org.apache.poi.hssf.model.InternalWorkbook;
+import org.apache.poi.hssf.record.NameCommentRecord;
 import org.apache.poi.hssf.record.NameRecord;
 import org.apache.poi.hssf.record.formula.Ptg;
 import org.apache.poi.ss.formula.FormulaType;
@@ -33,8 +34,10 @@ import org.apache.poi.ss.usermodel.Name;
 public final class HSSFName implements Name {
     private HSSFWorkbook _book;
     private NameRecord _definedNameRec;
+    private NameCommentRecord _commentRec;
 
-    /** Creates new HSSFName   - called by HSSFWorkbook to create a sheet from
+    /** 
+     * Creates new HSSFName   - called by HSSFWorkbook to create a name from
      * scratch.
      *
      * @see org.apache.poi.hssf.usermodel.HSSFWorkbook#createName()
@@ -42,8 +45,21 @@ public final class HSSFName implements Name {
      * @param book workbook object associated with the sheet.
      */
     /* package */ HSSFName(HSSFWorkbook book, NameRecord name) {
+      this(book, name, null);
+    }
+    /** 
+     * Creates new HSSFName   - called by HSSFWorkbook to create a name from
+     * scratch.
+     *
+     * @see org.apache.poi.hssf.usermodel.HSSFWorkbook#createName()
+     * @param name the Name Record
+     * @param comment the Name Comment Record, optional.
+     * @param book workbook object associated with the sheet.
+     */
+    /* package */ HSSFName(final HSSFWorkbook book, final NameRecord name, final NameCommentRecord comment) {
         _book = book;
         _definedNameRec = name;
+        _commentRec = comment;
     }
 
     /** Get the sheets name which this named range is referenced to
@@ -131,6 +147,13 @@ public final class HSSFName implements Name {
                 }
             }
         }
+        
+        // Update our comment, if there is one
+        if(_commentRec != null) {
+           String oldName = _commentRec.getNameText();
+           _commentRec.setNameText(nameName);
+           _book.getWorkbook().updateNameCommentRecordCache(_commentRec);
+        }
     }
 
     private static void validateName(String name){
@@ -230,7 +253,14 @@ public final class HSSFName implements Name {
      *
      * @return the user comment for this named range
      */
-    public String getComment(){
+    public String getComment() {
+        if(_commentRec != null) {
+           // Prefer the comment record if it has text in it
+           if(_commentRec.getCommentText() != null &&
+                 _commentRec.getCommentText().length() > 0) {
+              return _commentRec.getCommentText();
+           }
+        }
         return _definedNameRec.getDescriptionText();
     }
 
@@ -240,7 +270,12 @@ public final class HSSFName implements Name {
      * @param comment the user comment for this named range
      */
     public void setComment(String comment){
+        // Update the main record
         _definedNameRec.setDescriptionText(comment);
+        // If we have a comment record too, update that as well
+        if(_commentRec != null) {
+           _commentRec.setCommentText(comment);
+        }
     }
 
     /**

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

@@ -67,7 +67,6 @@ import org.apache.poi.hssf.util.CellReference;
 import org.apache.poi.poifs.filesystem.DirectoryNode;
 import org.apache.poi.poifs.filesystem.POIFSFileSystem;
 import org.apache.poi.ss.usermodel.CreationHelper;
-import org.apache.poi.ss.usermodel.PictureData;
 import org.apache.poi.ss.usermodel.Row.MissingCellPolicy;
 import org.apache.poi.ss.formula.FormulaType;
 import org.apache.poi.util.POILogFactory;
@@ -276,7 +275,8 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss
         }
 
         for (int i = 0 ; i < workbook.getNumNames() ; ++i){
-            HSSFName name = new HSSFName(this, workbook.getNameRecord(i));
+            NameRecord nameRecord = workbook.getNameRecord(i);
+            HSSFName name = new HSSFName(this, nameRecord, workbook.getNameCommentRecord(nameRecord));
             names.add(name);
         }
     }
@@ -970,7 +970,7 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss
 
         if (isNewRecord)
         {
-            HSSFName newName = new HSSFName(this, nameRecord);
+            HSSFName newName = new HSSFName(this, nameRecord, nameRecord.isBuiltInName() ? null : workbook.getNameCommentRecord(nameRecord));
             names.add(newName);
         }
 

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

@@ -18,12 +18,17 @@
 package org.apache.poi.hssf.model;
 
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 
 import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
+import org.apache.poi.hssf.record.NameCommentRecord;
+import org.apache.poi.hssf.record.NameRecord;
 import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.SSTRecord;
 import org.apache.poi.hssf.record.SupBookRecord;
@@ -138,7 +143,7 @@ public final class TestLinkTable extends TestCase {
 		
 		LinkTable lt;
 		try {
-			lt = new LinkTable(recList, 0, wrl);
+			lt = new LinkTable(recList, 0, wrl, Collections.<String, NameCommentRecord>emptyMap());
 		} catch (RuntimeException e) {
 			if (e.getMessage().equals("Expected an EXTERNSHEET record but got (org.apache.poi.hssf.record.SSTRecord)")) {
 				throw new AssertionFailedError("Identified bug 47001b");
@@ -148,4 +153,30 @@ public final class TestLinkTable extends TestCase {
 		}
 		assertNotNull(lt);
 	}	
+
+	/**
+	 *
+	 */
+	public void testNameCommentRecordBetweenNameRecords() {
+
+		final Record[] recs = {
+        new NameRecord(),
+        new NameCommentRecord("name1", "comment1"),
+        new NameRecord(),
+        new NameCommentRecord("name2", "comment2"),
+
+		};
+		final List<Record> recList = Arrays.asList(recs);
+		final WorkbookRecordList wrl = new WorkbookRecordList();
+		final Map<String, NameCommentRecord> commentRecords = new LinkedHashMap<String, NameCommentRecord>();
+
+		final LinkTable	lt = new LinkTable(recList, 0, wrl, commentRecords);
+		assertNotNull(lt);
+
+		assertEquals(2, commentRecords.size());
+    assertTrue(recs[1] == commentRecords.get("name1")); //== is intentionally not .equals()!
+    assertTrue(recs[3] == commentRecords.get("name2")); //== is intentionally not .equals()!
+
+    assertEquals(2, lt.getNumNames());
+	}
 }

src/testcases/org/apache/poi/hssf/record/TestNameCommentRecord.java

@@ -0,0 +1,42 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.hssf.record;
+
+import junit.framework.TestCase;
+
+import org.apache.poi.util.HexRead;
+
+/**
+ * Tests the NameCommentRecord serializes/deserializes correctly
+ *
+ * @author Andrew Shirley (aks at corefiling.co.uk)
+ */
+public final class TestNameCommentRecord extends TestCase {
+	public void testReserialize() {
+		final byte[] data = HexRead
+				.readFromString(""
+						+ "94 08 00 00 00 00 00 00 00 00 00 00 04 00 07 00 00 6E 61 6D 65 00 63 6F 6D 6D 65 6E 74]");
+		final RecordInputStream in = TestcaseRecordInputStream.create(NameCommentRecord.sid, data);
+		final NameCommentRecord ncr = new NameCommentRecord(in);
+		assertEquals(0x0894, ncr.getRecordType());
+		assertEquals("name", ncr.getNameText());
+		assertEquals("comment", ncr.getCommentText());
+		final byte[] data2 = ncr.serialize();
+		TestcaseRecordInputStream.confirmRecordEncoding(NameCommentRecord.sid, data, data2);
+	}
+}

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

@@ -1734,4 +1734,31 @@ if(1==2) {
        assertEquals(234.0, row.getCell(1).getNumericCellValue());
 }
     }
+    
+    /**
+     * Test for a file with NameRecord with NameCommentRecord comments
+     */
+    public void test49185() throws Exception {
+      HSSFWorkbook wb = openSample("49185.xls");
+      Name name = wb.getName("foobarName");
+      assertEquals("This is a comment", name.getComment());
+      
+      // Rename the name, comment comes with it
+      name.setNameName("ChangedName");
+      assertEquals("This is a comment", name.getComment());
+      
+      // Save and re-check
+      wb = writeOutAndReadBack(wb);
+      name = wb.getName("ChangedName");
+      assertEquals("This is a comment", name.getComment());
+      
+      // Now try to change it
+      name.setComment("Changed Comment");
+      assertEquals("Changed Comment", name.getComment());
+      
+      // Save and re-check
+      wb = writeOutAndReadBack(wb);
+      name = wb.getName("ChangedName");
+      assertEquals("Changed Comment", name.getComment());
+    }
 }