| Summary: | [PATCH] XSSFDrawing.createCellComment causes CommentsTable to lose reference to comment in cell A1 | ||
|---|---|---|---|
| Product: | POI | Reporter: | ericleecarroll <ericleecarroll> |
| Component: | XSSF | Assignee: | POI Developers List <dev> |
| Status: | RESOLVED FIXED | ||
| 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 |
||
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
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?
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. 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. |
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.