|Summary:||[PATCH] XSSFDrawing.createCellComment causes CommentsTable to lose reference to comment in cell A1|
|Component:||XSSF||Assignee:||POI Developers List <dev>|
|OS:||Mac OS X 10.4|
|Bug Depends on:|
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.