Bug 59740 - [PATCH] Sheet.shiftRows incorrectly shifts merged region on exists merged region
Summary: [PATCH] Sheet.shiftRows incorrectly shifts merged region on exists merged region
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 3.15-dev
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on: 60709 58443
Blocks:
  Show dependency tree
 
Reported: 2016-06-22 08:17 UTC by Daniil Lopatin
Modified: 2017-02-09 08:40 UTC (History)
0 users



Attachments
tests and patch (3.42 KB, patch)
2016-06-22 08:17 UTC, Daniil Lopatin
Details | Diff
patch, tests and updated javadoc (6.99 KB, patch)
2016-07-15 16:40 UTC, Daniil Lopatin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniil Lopatin 2016-06-22 08:17:47 UTC
Created attachment 33971 [details]
tests and patch

After Bug 58443 was implemented, on row shifting it's possible to get next error: "Cannot add merged region A1:F1 to sheet because it overlaps with an existing merged region (A1:G1)".

Steps to reproduce:
1. Create merged region A1:G1 in row 0
2. Create merged region A1:F1 in row 1
3. Shift row 1 to 0 position.

I've fixed it by deleting overlapped by shifting merged regions. Tests are provided. Please review and apply.
Comment 1 Javen O'Neal 2016-06-22 18:27:13 UTC
Correction:
(In reply to Danil Lopatin from comment #0)
> Steps to reproduce:
> 1. Create merged region A1:G1 in row 0
> 2. Create merged region *A2:F2* in row 1
> 3. Shift row 1 to 0 position.

Thanks for the patch and test.  Silently deleting merged regions seems reasonable to me as long as we document is behavior the least surprising behavior here?

* Silently deleting merged regions: would the merged regions in the source or destination row get deleted? Probably delete destination. We could certainly write this event to the POILogger for those who check it.
* Throwing an exception: any changes would need to be backed out or the check would need to be done before rows are removed
Comment 2 Javen O'Neal 2016-06-22 18:29:42 UTC
> Thanks for the patch and test.  Silently deleting merged regions seems
> reasonable to me as long as we document its behavior.

> What are people's thoughts on the least surprising
> behavior here?
Comment 3 Daniil Lopatin 2016-07-08 14:06:58 UTC
I hope this fix will be included in 3.15 version.
If you want, I could update documentation.
Comment 4 Javen O'Neal 2016-07-08 18:09:05 UTC
(In reply to Danil Lopatin from comment #3)
> If you want, I could update documentation.

Yes, please include a patch for the updated documentation, and I will commit the code changes and documentation together.
Comment 5 Daniil Lopatin 2016-07-15 16:40:33 UTC
Created attachment 34042 [details]
patch, tests and updated javadoc

I updated java documentation. Hope haven't missed some other documentations (like src/documentation). If so, please let me know. Thanks.
Comment 6 Dominik Stadler 2016-07-16 20:54:06 UTC
Applied via r1752997, thanks a lot for the patch and test-cases!
Comment 7 David Le Niniven 2017-02-06 10:24:36 UTC
The shiftRows method works as expected if we shift only one row at a time, but if we try to shift several rows in a single call, it removes all merged areas for the shifted rows.

For instance, if we have a Xlsx document with 2 merged cells A3:B3
Assuming lastRowNum = sheet.getLastRowNum();

Executing the following code will removed this merged region : 

sheet.shiftRows(1, lastRowNum+1, -1, true, false);

Whereas the following code will keep the shift the merged region into A2:B2 

for(int currentRow = 1; currentRow <=lastRowNum+1; currentRow++) {
    sheet.shiftRows(currentRow, currentRow, -1, true, false);
}
Comment 8 Javen O'Neal 2017-02-08 06:32:05 UTC
(In reply to David Le Niniven from comment #7)
> The shiftRows method works as expected if we shift only one row at a time,
> but if we try to shift several rows in a single call, it removes all merged
> areas for the shifted rows.

Please open a new bug, adding bug 59740 as a blocker for the new bug.