Bug 54920

Summary: [PATCH] XSSFDrawing.createCellComment causes CommentsTable to lose reference to comment in cell A1
Product: POI Reporter: ericleecarroll <ericleecarroll>
Component: XSSFAssignee: POI Developers List <dev>
Severity: normal CC: dominik.stadler
Priority: P2    
Version: 3.9-FINAL   
Target Milestone: ---   
Hardware: PC   
OS: Mac OS X 10.4   
Bug Depends on:    
Bug Blocks: 58175    
Attachments: Unit Test
Updated unit-test which shows the problem more clearly, i.e. comment in A1 is there before and gone after comment in B2 is set
Proposed fix for

Description ericleecarroll 2013-05-02 16:56:59 UTC
Created attachment 30250 [details]
Unit Test

XSSFDrawing.createCellComment calls CommentsTable.newComment, which creates a temporary "A1" cell reference, and then updates the cell reference with the proper row and column.  But that temporary "A1" is placed in the commentRefs map, and can overwrite a legitimate comment in "A1".

I've attached a unit test that shows the issue.  There are a pair of 'NOTES' in the source that show the required steps to reproduce.
Comment 1 Dominik Stadler 2013-05-25 14:19:51 UTC
Created attachment 30322 [details]
Updated unit-test which shows the problem more clearly, i.e. comment in A1 is there before and gone after comment in B2 is set
Comment 2 Dominik Stadler 2013-05-25 14:25:41 UTC
Created attachment 30323 [details]
Proposed fix for

CommentsTable is dangerous as it overwrites comments in A1 because it uses this column as default reference in newComment(). 

This proposed patch changes this so a reference is always passed to newComment() and thus can be set immediately. This also avoids having to adjust the shape later on in the setCol()/setRow() columns.

Adjusted tests and the original test in this bug run fine then.

I am not sure if this breaks API compatibility, is newComment() exported to the outside? We could add a second newComment(String ref) to fix this in a compatible way, but this keeps a method which is dangerous to use anyway...

Another option to fix this would be to use a reference which does not exist in normal sheets, e.g. something like A0, but I did not look into this yet, any thoughts?
Comment 3 Nick Burch 2013-06-18 22:37:14 UTC
We normally try to maintain binary compatibility, unless there's a very good reason

If some situations will work fine with the current method signature, then I'd suggest we @deprecate it, add a note about the new one, and change all POI code to use the new call

If the old method signature will never work, then remove it, add the new one, and add something to the release notes.
Comment 4 Dominik Stadler 2013-06-23 21:34:55 UTC
Fixed with revision 1495894, we now deprecated the newComment() method without arguments and added a new one which receives the target reference for the comment as parameter. All uses in POI itself are adjusted accordingly.