Bug 56017

Summary: Shifting rows with comment result - Unreadable content error and comment deletion
Product: POI Reporter: yevsjunk
Component: XSSFAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: major CC: merlusha
Priority: P2    
Version: 3.11-FINAL   
Target Milestone: ---   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 55814, 57828    
Attachments: Test file - before running the code
Broken excel 2010 file
Proposed patch for comments above shifted rows

Description yevsjunk 2014-01-16 10:04:58 UTC
If I try to shiftrows that contains comments nad open excel document in Excel I get a popup warning Message "Unreadable content..." that says that comments were deleted.

How to reproduce:
Create new Excel 2007 Document, in first cell (A1) add comment (any text).

Run code:

FileInputStream inputStream = null;
FileOutputStream outputStream = null;
XSSFWorkbook book = null;

try {
	inputStream = new FileInputStream("c://1//Test.xlsx");
	book = new XSSFWorkbook(inputStream);

	XSSFSheet sheet = book.getSheetAt(0);

	sheet.shiftRows(0, 1, 1);

	outputStream = new FileOutputStream("c://1//Test.xlsx");
	book.write(outputStream);
	outputStream.close();

}
Comment 1 yevsjunk 2014-01-16 10:27:21 UTC
Created attachment 31215 [details]
Test file - before running the code
Comment 2 Dominik Stadler 2014-02-02 10:40:48 UTC
I have added a test-case in r1563587 and r1563588 which runs this, I fixed a problem with comment-refs which only affected the in-memory state of comments, as I don't have a Windows-machine with Excel at hand to verify/reproduce the problem, so I am not sure if it is fixed in latest trunk or not...
Comment 3 yevsjunk 2014-02-06 18:49:46 UTC
Thanks, I will test it latter.
Meanwhile, I made some work around in shiftRows method  of XSSFSheet...

Anyway, will the next POI release include the fix to the issue?
Comment 4 j.brauge 2014-04-23 08:04:39 UTC
This bug persists in 3.10 final release and 3.11 dev.
Is there a target release for a fix ?
Best regards.
Comment 5 happyhuman 2014-06-05 01:54:44 UTC
In: org.apache.poi.xssf.usermodel.XSSFSheet

I believe line:
2451            if(rownum < startRow || rownum > endRow) continue;

Should be above:
2439            if(sheetComments != null){


The exception I was getting started here:
2445                        ref = new CellReference(rownum + n, ref.getCol());

Looks as though it's trying to update the comments table with the new cell references after the rows are shifted, but the iterator is over all rows and the check for the shifted range is after this block. Therefore, an attempt could be made for a negative row value. Fixed the issue for me anyway and hope it helps.
Comment 6 happyhuman 2014-06-05 01:58:30 UTC
In: org.apache.poi.xssf.usermodel.XSSFSheet

I believe line:
2451            if(rownum < startRow || rownum > endRow) continue;

Should be above:
2439            if(sheetComments != null){


The exception I was getting started here:
2445                        ref = new CellReference(rownum + n, ref.getCol());

Looks as though it's trying to update the comments table with the new cell references after the rows are shifted, but the iterator is over all rows and the check for the shifted range is after this block. Therefore, an attempt could be made for a negative row value. I would assume if you do not get an exception the workbook may corrupt. Fixed the issue for me anyway and hope it helps.
Comment 7 m_and_m 2014-10-17 14:35:47 UTC
In the shiftRows method when it adjusts comments/rowheight it does all rows(!) in the current sheet. It doesn't always cause exception or error when opening xlsx, but always makes worse all of comments.
For example:
if I call it: sheet.shiftRows(3, 4, 20, true, false);
then it shift all comments on 0, 1, and 2 rows too instead of leave in peace.

It's a simply, but annoying bug. Please fix it asap!
Comment 8 Mikalai Zhudro 2015-02-04 13:44:39 UTC
Created attachment 32431 [details]
Broken excel 2010 file
Comment 9 Mikalai Zhudro 2015-02-04 14:51:50 UTC
I was able to reproduce this bug with Excel 2010. (poi 3.11 Final)
My further investigation shows that problem lies in following code:

XSSFSheet.java:2567

if(sheetComments != null){
                //TODO shift Note's anchor in the associated /xl/drawing/vmlDrawings#.vml
                CTCommentList lst = sheetComments.getCTComments().getCommentList();
                for (CTComment comment : lst.getCommentArray()) {
                    String oldRef = comment.getRef();
                    CellReference ref = new CellReference(oldRef);
                    if(ref.getRow() == rownum){
                        ref = new CellReference(rownum + n, ref.getCol());
                        comment.setRef(ref.formatAsString());
                        sheetComments.referenceUpdated(oldRef, comment);
                    }
                }
            }
I looked at /xl/drawing/vmlDrawings#.vml and note's anchor was not shifted.
So I updated this file manually and was able to open excel file without problem.
Can somebody implement this TODO??
Comment 10 Mikalai Zhudro 2015-02-24 14:20:13 UTC
I believe we should add above:
if(sheetComments != null){

  if(n>0 & rownum < startRow)  continue;
  if(n<0 & rownum < startRow+n) continue;

This solves problem only if comments are above shifted rows.
However if comments are below shifted rows the problem is present.
CellReference for all comments which are below shifted rows are set up to the last row.
Attached you can find patch with included fix for
comments which are above shifted rows.
Comment 11 Mikalai Zhudro 2015-02-24 14:21:35 UTC
Created attachment 32517 [details]
Proposed patch for comments above shifted rows
Comment 12 Dominik Stadler 2015-03-15 20:56:09 UTC
I gave this a try and ended up changing the implementation of XSSFSheet.shiftRows() quite a bit, see r1666843. 

The current code did not handle a few things well, e.g. removing comments in overwritten rows and cases where comments are moved around. Moving the comments while still iterating caused cases where comments where moved multiple times, messing up things.

Also XSSFComment.setRow() was prone to a xmlbeans bug when resetting row-nunmbers. 

A few new test-cases now verify the most common cases, please report new bugs with sample code if there are still cases that are handled incorrectly.