Bug 54920: do not set column and row separatedely, but use a reference

for newComment(), keep previous method as deprecated. Adjust all places
where newComment() is used and add unit test covering the bug.

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

src/ooxml/java/org/apache/poi/xssf/model/CommentsTable.java

@@ -126,9 +126,20 @@ public class CommentsTable extends POIXMLDocumentPart {
         return commentRefs.get(cellRef);
     }
 
+    /**
+     * This method is deprecated and should not be used any more as
+     * it overwrites the comment in Cell A1.
+     *
+     * @return
+     */
+    @Deprecated
     public CTComment newComment() {
+        return newComment("A1");
+    }
+    
+    public CTComment newComment(String ref) {
         CTComment ct = comments.getCommentList().addNewComment();
-        ct.setRef("A1");
+        ct.setRef(ref);
         ct.setAuthorId(0);
         
         if(commentRefs != null) {

src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFDrawing.java

@@ -33,6 +33,7 @@ import org.apache.poi.openxml4j.opc.PackageRelationship;
 import org.apache.poi.openxml4j.opc.TargetMode;
 import org.apache.poi.ss.usermodel.ClientAnchor;
 import org.apache.poi.ss.usermodel.Drawing;
+import org.apache.poi.ss.util.CellReference;
 import org.apache.poi.util.Internal;
 import org.apache.poi.xssf.model.CommentsTable;
 import org.apache.xmlbeans.XmlCursor;
@@ -298,9 +299,8 @@ public final class XSSFDrawing extends POIXMLDocumentPart implements Drawing {
                     ca.getCol2() + ", 0, " + ca.getRow2() + ", 0";
             vmlShape.getClientDataArray(0).setAnchorArray(0, position);
         }
-        XSSFComment shape = new XSSFComment(comments, comments.newComment(), vmlShape);
+        String ref = new CellReference(ca.getRow1(), ca.getCol1()).formatAsString();
-        shape.setColumn(ca.getCol1());
+		XSSFComment shape = new XSSFComment(comments, comments.newComment(ref), vmlShape);
-        shape.setRow(ca.getRow1());
         return shape;
     }
 

src/ooxml/testcases/org/apache/poi/xssf/model/TestCommentsTable.java

@@ -187,12 +187,9 @@ public class TestCommentsTable extends TestCase {
 
     public void testRemoveComment() throws Exception {
         CommentsTable sheetComments = new CommentsTable();
-        CTComment a1 = sheetComments.newComment();
+        CTComment a1 = sheetComments.newComment("A1");
-        a1.setRef("A1");
+        CTComment a2 = sheetComments.newComment("A2");
-        CTComment a2 = sheetComments.newComment();
+        CTComment a3 = sheetComments.newComment("A3");
-        a2.setRef("A2");
-        CTComment a3 = sheetComments.newComment();
-        a3.setRef("A3");
 
         assertSame(a1, sheetComments.getCTComment("A1"));
         assertSame(a2, sheetComments.getCTComment("A2"));
@@ -217,4 +214,76 @@ public class TestCommentsTable extends TestCase {
         assertNull(sheetComments.getCTComment("A2"));
         assertNull(sheetComments.getCTComment("A3"));
     }
+
+    public void testBug54920() {
+        final Workbook workbook = new XSSFWorkbook();
+        final Sheet sheet = workbook.createSheet("sheet01");
+        // create anchor
+        CreationHelper helper = sheet.getWorkbook().getCreationHelper();
+        ClientAnchor anchor = helper.createClientAnchor();
+
+        // place comment in A1
+        // NOTE - only occurs if a comment is placed in A1 first
+        Cell A1 = getCell(sheet, 0, 0);
+        //Cell A1 = getCell(sheet, 2, 2);
+        Drawing drawing = sheet.createDrawingPatriarch();
+        setComment(sheet, A1, drawing, "for A1", helper, anchor);
+        
+        // find comment in A1 before we set the comment in B2
+        Comment commentA1 = A1.getCellComment();
+        assertNotNull("Should still find the previous comment in A1, but had null", commentA1);
+        assertEquals("should find correct comment in A1, but had null: " + commentA1, "for A1", commentA1.getString().getString());
+        
+        // place comment in B2, according to Bug 54920 this removes the comment in A1!
+        Cell B2 = getCell(sheet, 1, 1);
+        setComment(sheet, B2, drawing, "for B2", helper, anchor);
+
+        // find comment in A1
+        Comment commentB2 = B2.getCellComment();
+        assertEquals("should find correct comment in B2, but had null: " + commentB2, "for B2", commentB2.getString().getString());
+        
+        // find comment in A1
+        commentA1 = A1.getCellComment();
+        assertNotNull("Should still find the previous comment in A1, but had null", commentA1);
+        assertEquals("should find correct comment in A1, but had null: " + commentA1, "for A1", commentA1.getString().getString());
+    }
+    
+    // Set the comment on a sheet
+    //
+    private static void setComment(Sheet sheet, Cell cell, Drawing drawing, String commentText, CreationHelper helper, ClientAnchor anchor) {
+        System.out.println("Setting col: " + cell.getColumnIndex() + " and row " + cell.getRowIndex());
+        anchor.setCol1(cell.getColumnIndex());
+        anchor.setCol2(cell.getColumnIndex());
+        anchor.setRow1(cell.getRowIndex());
+        anchor.setRow2(cell.getRowIndex());
+        
+        // get comment, or create if it does not exist
+        // NOTE - only occurs if getCellComment is called first
+        Comment comment = cell.getCellComment();
+        //Comment comment = null;
+        if (comment == null) {
+            comment = drawing.createCellComment(anchor);
+        }
+        comment.setAuthor("Test");
+        
+        // attach the comment to the cell
+        comment.setString(helper.createRichTextString(commentText));
+        cell.setCellComment(comment);
+    }
+    
+    // Get a cell, create as needed
+    //
+    private static Cell getCell(Sheet sheet, int rowIndex, int colIndex) {
+        Row row = sheet.getRow(rowIndex);
+        if (row == null) {
+            row = sheet.createRow(rowIndex);
+        }
+        
+        Cell cell = row.getCell(colIndex);
+        if (cell == null) {
+            cell = row.createCell(colIndex);
+        }
+        
+        return cell;
+    }
 }

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFComment.java

@@ -50,7 +50,7 @@ public final class TestXSSFComment extends BaseTestCellComment  {
         assertEquals(1, sheetComments.getCTComments().getAuthors().sizeOfAuthorArray());
         assertEquals(1, sheetComments.getNumberOfAuthors());
 
-        CTComment ctComment = sheetComments.newComment();
+        CTComment ctComment = sheetComments.newComment("A1");
         CTShape vmlShape = CTShape.Factory.newInstance();
 
         XSSFComment comment = new XSSFComment(sheetComments, ctComment, vmlShape);
@@ -64,7 +64,7 @@ public final class TestXSSFComment extends BaseTestCellComment  {
     public void testGetSetCol() {
         CommentsTable sheetComments = new CommentsTable();
         XSSFVMLDrawing vml = new XSSFVMLDrawing();
-        CTComment ctComment = sheetComments.newComment();
+        CTComment ctComment = sheetComments.newComment("A1");
         CTShape vmlShape = vml.newCommentShape();
 
         XSSFComment comment = new XSSFComment(sheetComments, ctComment, vmlShape);
@@ -82,7 +82,7 @@ public final class TestXSSFComment extends BaseTestCellComment  {
     public void testGetSetRow() {
         CommentsTable sheetComments = new CommentsTable();
         XSSFVMLDrawing vml = new XSSFVMLDrawing();
-        CTComment ctComment = sheetComments.newComment();
+        CTComment ctComment = sheetComments.newComment("A1");
         CTShape vmlShape = vml.newCommentShape();
 
         XSSFComment comment = new XSSFComment(sheetComments, ctComment, vmlShape);
@@ -150,7 +150,7 @@ public final class TestXSSFComment extends BaseTestCellComment  {
 
     public void testAuthor() {
         CommentsTable sheetComments = new CommentsTable();
-        CTComment ctComment = sheetComments.newComment();
+        CTComment ctComment = sheetComments.newComment("A1");
 
         assertEquals(1, sheetComments.getNumberOfAuthors());
         XSSFComment comment = new XSSFComment(sheetComments, ctComment, null);