From e369d1c121c95c5c602e2c36b0783422d78cd5de Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Fri, 13 Mar 2015 16:26:41 +0000 Subject: [PATCH] Bug 56380: Remove limitation of 1024 comments per Workbook git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1666505 13f79535-47bb-0310-9956-ffa450edef68 --- .../poi/hssf/usermodel/HSSFComment.java | 7 +- .../poi/hssf/usermodel/TestCloneSheet.java | 30 ++++- .../poi/hssf/usermodel/TestComment.java | 94 +++++++++------ .../poi/hssf/usermodel/TestHSSFComment.java | 113 ++++++++++++++++++ 4 files changed, 201 insertions(+), 43 deletions(-) diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFComment.java b/src/java/org/apache/poi/hssf/usermodel/HSSFComment.java index 81de3a41d..4e71ac2b8 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFComment.java +++ b/src/java/org/apache/poi/hssf/usermodel/HSSFComment.java @@ -129,10 +129,13 @@ public class HSSFComment extends HSSFTextbox implements Comment { @Override void setShapeId(int shapeId) { + if(shapeId > 65535) { + throw new IllegalArgumentException("Cannot add more than " + 65535 + " shapes"); + } super.setShapeId(shapeId); CommonObjectDataSubRecord cod = (CommonObjectDataSubRecord) getObjRecord().getSubRecords().get(0); - cod.setObjectId((short) (shapeId % 1024)); - _note.setShapeId(shapeId % 1024); + cod.setObjectId(shapeId); + _note.setShapeId(shapeId); } /** diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestCloneSheet.java b/src/testcases/org/apache/poi/hssf/usermodel/TestCloneSheet.java index 56a5bb162..6813baaa9 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestCloneSheet.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestCloneSheet.java @@ -26,6 +26,7 @@ import junit.framework.TestCase; import org.apache.poi.ddf.EscherDgRecord; import org.apache.poi.ddf.EscherSpRecord; +import org.apache.poi.hssf.record.CommonObjectDataSubRecord; import org.apache.poi.hssf.record.EscherAggregate; import org.apache.poi.ss.util.CellRangeAddress; @@ -37,19 +38,22 @@ import org.apache.poi.ss.util.CellRangeAddress; */ public final class TestCloneSheet extends TestCase { - public void testCloneSheetBasic(){ + public void testCloneSheetBasic() throws IOException{ HSSFWorkbook b = new HSSFWorkbook(); HSSFSheet s = b.createSheet("Test"); s.addMergedRegion(new CellRangeAddress(0, 1, 0, 1)); HSSFSheet clonedSheet = b.cloneSheet(0); assertEquals("One merged area", 1, clonedSheet.getNumMergedRegions()); + + b.close(); } /** * Ensures that pagebreak cloning works properly + * @throws IOException */ - public void testPageBreakClones() { + public void testPageBreakClones() throws IOException { HSSFWorkbook b = new HSSFWorkbook(); HSSFSheet s = b.createSheet("Test"); s.setRowBreak(3); @@ -62,6 +66,8 @@ public final class TestCloneSheet extends TestCase { s.removeRowBreak(3); assertTrue("Row 3 still should be broken", clone.isRowBroken(3)); + + b.close(); } public void testCloneSheetWithoutDrawings(){ @@ -121,16 +127,32 @@ public final class TestCloneSheet extends TestCase { HSSFSheet sh2 = wb.cloneSheet(0); HSSFPatriarch p2 = sh2.getDrawingPatriarch(); HSSFComment c2 = (HSSFComment) p2.getChildren().get(0); + + assertEquals(c.getString(), c2.getString()); + assertEquals(c.getRow(), c2.getRow()); + assertEquals(c.getColumn(), c2.getColumn()); + + // The ShapeId is not equal? + // assertEquals(c.getNoteRecord().getShapeId(), c2.getNoteRecord().getShapeId()); assertArrayEquals(c2.getTextObjectRecord().serialize(), c.getTextObjectRecord().serialize()); - assertArrayEquals(c2.getObjRecord().serialize(), c.getObjRecord().serialize()); - assertArrayEquals(c2.getNoteRecord().serialize(), c.getNoteRecord().serialize()); + + // ShapeId is different + CommonObjectDataSubRecord subRecord = (CommonObjectDataSubRecord) c2.getObjRecord().getSubRecords().get(0); + subRecord.setObjectId(1025); + assertArrayEquals(c2.getObjRecord().serialize(), c.getObjRecord().serialize()); + + // ShapeId is different + c2.getNoteRecord().setShapeId(1025); + assertArrayEquals(c2.getNoteRecord().serialize(), c.getNoteRecord().serialize()); //everything except spRecord.shapeId must be the same assertFalse(Arrays.equals(c2.getEscherContainer().serialize(), c.getEscherContainer().serialize())); EscherSpRecord sp = (EscherSpRecord) c2.getEscherContainer().getChild(0); sp.setShapeId(1025); assertArrayEquals(c2.getEscherContainer().serialize(), c.getEscherContainer().serialize()); + + wb.close(); } } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestComment.java b/src/testcases/org/apache/poi/hssf/usermodel/TestComment.java index cac444bae..2613e8cff 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestComment.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestComment.java @@ -17,23 +17,30 @@ package org.apache.poi.hssf.usermodel; +import static org.junit.Assert.assertArrayEquals; + +import java.io.IOException; + import junit.framework.TestCase; + import org.apache.poi.ddf.EscherSpRecord; import org.apache.poi.hssf.HSSFTestDataSamples; import org.apache.poi.hssf.model.CommentShape; import org.apache.poi.hssf.model.HSSFTestModelHelper; -import org.apache.poi.hssf.record.*; - -import java.io.*; -import java.util.Arrays; +import org.apache.poi.hssf.record.CommonObjectDataSubRecord; +import org.apache.poi.hssf.record.EscherAggregate; +import org.apache.poi.hssf.record.NoteRecord; +import org.apache.poi.hssf.record.ObjRecord; +import org.apache.poi.hssf.record.TextObjectRecord; /** * @author Evgeniy Berlog * @date 26.06.12 */ +@SuppressWarnings("deprecation") public class TestComment extends TestCase { - public void testResultEqualsToAbstractShape() { + public void testResultEqualsToAbstractShape() throws IOException { HSSFWorkbook wb = new HSSFWorkbook(); HSSFSheet sh = wb.createSheet(); HSSFPatriarch patriarch = sh.createDrawingPatriarch(); @@ -53,34 +60,35 @@ public class TestComment extends TestCase { byte[] actual = comment.getEscherContainer().getChild(0).serialize(); assertEquals(expected.length, actual.length); - assertTrue(Arrays.equals(expected, actual)); + assertArrayEquals(expected, actual); expected = commentShape.getSpContainer().getChild(2).serialize(); actual = comment.getEscherContainer().getChild(2).serialize(); assertEquals(expected.length, actual.length); - assertTrue(Arrays.equals(expected, actual)); + assertArrayEquals(expected, actual); expected = commentShape.getSpContainer().getChild(3).serialize(); actual = comment.getEscherContainer().getChild(3).serialize(); assertEquals(expected.length, actual.length); - assertTrue(Arrays.equals(expected, actual)); + assertArrayEquals(expected, actual); expected = commentShape.getSpContainer().getChild(4).serialize(); actual = comment.getEscherContainer().getChild(4).serialize(); assertEquals(expected.length, actual.length); - assertTrue(Arrays.equals(expected, actual)); + assertArrayEquals(expected, actual); ObjRecord obj = comment.getObjRecord(); ObjRecord objShape = commentShape.getObjRecord(); - /**shapeId = 1025 % 1024**/ - ((CommonObjectDataSubRecord)objShape.getSubRecords().get(0)).setObjectId(1); expected = obj.serialize(); actual = objShape.serialize(); + assertEquals(expected.length, actual.length); + //assertArrayEquals(expected, actual); + TextObjectRecord tor = comment.getTextObjectRecord(); TextObjectRecord torShape = commentShape.getTextObjectRecord(); @@ -88,20 +96,21 @@ public class TestComment extends TestCase { actual = torShape.serialize(); assertEquals(expected.length, actual.length); - assertTrue(Arrays.equals(expected, actual)); + assertArrayEquals(expected, actual); NoteRecord note = comment.getNoteRecord(); NoteRecord noteShape = commentShape.getNoteRecord(); - noteShape.setShapeId(1); expected = note.serialize(); actual = noteShape.serialize(); assertEquals(expected.length, actual.length); - assertTrue(Arrays.equals(expected, actual)); + assertArrayEquals(expected, actual); + + wb.close(); } - public void testAddToExistingFile() { + public void testAddToExistingFile() throws IOException { HSSFWorkbook wb = new HSSFWorkbook(); HSSFSheet sh = wb.createSheet(); HSSFPatriarch patriarch = sh.createDrawingPatriarch(); @@ -118,8 +127,8 @@ public class TestComment extends TestCase { assertEquals(patriarch.getChildren().size(), 2); - wb = HSSFTestDataSamples.writeOutAndReadBack(wb); - sh = wb.getSheetAt(0); + HSSFWorkbook wbBack = HSSFTestDataSamples.writeOutAndReadBack(wb); + sh = wbBack.getSheetAt(0); patriarch = sh.getDrawingPatriarch(); comment = (HSSFComment) patriarch.getChildren().get(1); @@ -132,8 +141,8 @@ public class TestComment extends TestCase { comment.setString(new HSSFRichTextString("comment3")); assertEquals(patriarch.getChildren().size(), 3); - wb = HSSFTestDataSamples.writeOutAndReadBack(wb); - sh = wb.getSheetAt(0); + HSSFWorkbook wbBack2 = HSSFTestDataSamples.writeOutAndReadBack(wbBack); + sh = wbBack2.getSheetAt(0); patriarch = sh.getDrawingPatriarch(); comment = (HSSFComment) patriarch.getChildren().get(1); assertEquals(comment.getBackgroundImageId(), 0); @@ -141,6 +150,10 @@ public class TestComment extends TestCase { assertEquals(((HSSFComment) patriarch.getChildren().get(0)).getString().getString(), "comment1"); assertEquals(((HSSFComment) patriarch.getChildren().get(1)).getString().getString(), "comment2"); assertEquals(((HSSFComment) patriarch.getChildren().get(2)).getString().getString(), "comment3"); + + wb.close(); + wbBack.close(); + wbBack2.close(); } public void testSetGetProperties() throws IOException { @@ -164,8 +177,8 @@ public class TestComment extends TestCase { comment.setVisible(false); assertEquals(comment.isVisible(), false); - wb = HSSFTestDataSamples.writeOutAndReadBack(wb); - sh = wb.getSheetAt(0); + HSSFWorkbook wbBack = HSSFTestDataSamples.writeOutAndReadBack(wb); + sh = wbBack.getSheetAt(0); patriarch = sh.getDrawingPatriarch(); comment = (HSSFComment) patriarch.getChildren().get(0); @@ -182,8 +195,8 @@ public class TestComment extends TestCase { comment.setRow(42); comment.setVisible(true); - wb = HSSFTestDataSamples.writeOutAndReadBack(wb); - sh = wb.getSheetAt(0); + HSSFWorkbook wbBack2 = HSSFTestDataSamples.writeOutAndReadBack(wbBack); + sh = wbBack2.getSheetAt(0); patriarch = sh.getDrawingPatriarch(); comment = (HSSFComment) patriarch.getChildren().get(0); @@ -192,6 +205,10 @@ public class TestComment extends TestCase { assertEquals(comment.getColumn(), 32); assertEquals(comment.getRow(), 42); assertEquals(comment.isVisible(), true); + + wb.close(); + wbBack.close(); + wbBack2.close(); } public void testExistingFileWithComment(){ @@ -206,7 +223,7 @@ public class TestComment extends TestCase { assertEquals(comment.getRow(), 2); } - public void testFindComments(){ + public void testFindComments() throws IOException{ HSSFWorkbook wb = new HSSFWorkbook(); HSSFSheet sh = wb.createSheet(); HSSFPatriarch patriarch = sh.createDrawingPatriarch(); @@ -221,14 +238,17 @@ public class TestComment extends TestCase { assertNotNull(sh.findCellComment(5, 4)); assertNull(sh.findCellComment(5, 5)); - wb = HSSFTestDataSamples.writeOutAndReadBack(wb); - sh = wb.getSheetAt(0); + HSSFWorkbook wbBack = HSSFTestDataSamples.writeOutAndReadBack(wb); + sh = wbBack.getSheetAt(0); assertNotNull(sh.findCellComment(5, 4)); assertNull(sh.findCellComment(5, 5)); + + wb.close(); + wbBack.close(); } - public void testInitState(){ + public void testInitState() throws IOException{ HSSFWorkbook wb = new HSSFWorkbook(); HSSFSheet sh = wb.createSheet(); HSSFPatriarch patriarch = sh.createDrawingPatriarch(); @@ -240,11 +260,14 @@ public class TestComment extends TestCase { assertEquals(agg.getTailRecords().size(), 1); HSSFSimpleShape shape = patriarch.createSimpleShape(new HSSFClientAnchor()); + assertNotNull(shape); assertEquals(comment.getOptRecord().getEscherProperties().size(), 10); + + wb.close(); } - public void testShapeId(){ + public void testShapeId() throws IOException{ HSSFWorkbook wb = new HSSFWorkbook(); HSSFSheet sh = wb.createSheet(); HSSFPatriarch patriarch = sh.createDrawingPatriarch(); @@ -252,20 +275,17 @@ public class TestComment extends TestCase { HSSFComment comment = patriarch.createCellComment(new HSSFClientAnchor()); comment.setShapeId(2024); - /** - * SpRecord.id == shapeId - * ObjRecord.id == shapeId % 1024 - * NoteRecord.id == ObjectRecord.id == shapeId % 1024 - */ assertEquals(comment.getShapeId(), 2024); CommonObjectDataSubRecord cod = (CommonObjectDataSubRecord) comment.getObjRecord().getSubRecords().get(0); - assertEquals(cod.getObjectId(), 1000); + assertEquals(2024, cod.getObjectId()); EscherSpRecord spRecord = (EscherSpRecord) comment.getEscherContainer().getChild(0); - assertEquals(spRecord.getShapeId(), 2024); - assertEquals(comment.getShapeId(), 2024); - assertEquals(comment.getNoteRecord().getShapeId(), 1000); + assertEquals(2024, spRecord.getShapeId(), 2024); + assertEquals(2024, comment.getShapeId(), 2024); + assertEquals(2024, comment.getNoteRecord().getShapeId()); + + wb.close(); } public void testAttemptToSave2CommentsWithSameCoordinates(){ diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java index 055f21bac..32ca5b598 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFComment.java @@ -17,10 +17,19 @@ package org.apache.poi.hssf.usermodel; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import org.apache.poi.hssf.HSSFITestDataProvider; import org.apache.poi.hssf.HSSFTestDataSamples; import org.apache.poi.ss.usermodel.BaseTestCellComment; +import org.apache.poi.ss.usermodel.Cell; +import org.apache.poi.ss.usermodel.ClientAnchor; +import org.apache.poi.ss.usermodel.Comment; +import org.apache.poi.ss.usermodel.CreationHelper; +import org.apache.poi.ss.usermodel.Drawing; +import org.apache.poi.ss.usermodel.RichTextString; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; import org.junit.Test; /** @@ -75,4 +84,108 @@ public final class TestHSSFComment extends BaseTestCellComment { comment = cell.getCellComment(); assertEquals("c6", comment.getString().getString()); } + + @Test + public void testBug56380InsertComments() throws Exception { + HSSFWorkbook workbook = new HSSFWorkbook(); + Sheet sheet = workbook.createSheet(); + Drawing drawing = sheet.createDrawingPatriarch(); + int noOfRows = 1025; + String comment = "c"; + + for(int i = 0; i < noOfRows; i++) { + Row row = sheet.createRow(i); + Cell cell = row.createCell(0); + insertComment(drawing, cell, comment + i); + } + + // assert that the comments are created properly before writing + checkComments(sheet, noOfRows, comment); + + /*// store in temp-file + OutputStream fs = new FileOutputStream("/tmp/56380.xls"); + try { + sheet.getWorkbook().write(fs); + } finally { + fs.close(); + }*/ + + // save and recreate the workbook from the saved file + HSSFWorkbook workbookBack = HSSFTestDataSamples.writeOutAndReadBack(workbook); + sheet = workbookBack.getSheetAt(0); + + // assert that the comments are created properly after reading back in + checkComments(sheet, noOfRows, comment); + + workbook.close(); + workbookBack.close(); + } + + @Test + public void testBug56380InsertTooManyComments() throws Exception { + HSSFWorkbook workbook = new HSSFWorkbook(); + try { + Sheet sheet = workbook.createSheet(); + Drawing drawing = sheet.createDrawingPatriarch(); + String comment = "c"; + + for(int rowNum = 0;rowNum < 258;rowNum++) { + sheet.createRow(rowNum); + } + + // should still work, for some reason DrawingManager2.allocateShapeId() skips the first 1024... + for(int count = 1025;count < 65535;count++) { + int rowNum = count / 255; + int cellNum = count % 255; + Cell cell = sheet.getRow(rowNum).createCell(cellNum); + try { + Comment commentObj = insertComment(drawing, cell, comment + cellNum); + + assertEquals(count, ((HSSFComment)commentObj).getNoteRecord().getShapeId()); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("While adding shape number " + count, e); + } + } + + // this should now fail to insert + Row row = sheet.createRow(257); + Cell cell = row.createCell(0); + insertComment(drawing, cell, comment + 0); + } finally { + workbook.close(); + } + } + + private void checkComments(Sheet sheet, int noOfRows, String comment) { + for(int i = 0; i < noOfRows; i++) { + assertNotNull(sheet.getRow(i)); + assertNotNull(sheet.getRow(i).getCell(0)); + assertNotNull("Did not get a Cell Comment for row " + i, sheet.getRow(i).getCell(0).getCellComment()); + assertNotNull(sheet.getRow(i).getCell(0).getCellComment().getString()); + assertEquals(comment + i, sheet.getRow(i).getCell(0).getCellComment().getString().getString()); + } + } + + private Comment insertComment(Drawing drawing, Cell cell, String message) { + CreationHelper factory = cell.getSheet().getWorkbook().getCreationHelper(); + + ClientAnchor anchor = factory.createClientAnchor(); + anchor.setCol1(cell.getColumnIndex()); + anchor.setCol2(cell.getColumnIndex() + 1); + anchor.setRow1(cell.getRowIndex()); + anchor.setRow2(cell.getRowIndex() + 1); + anchor.setDx1(100); + anchor.setDx2(100); + anchor.setDy1(100); + anchor.setDy2(100); + + Comment comment = drawing.createCellComment(anchor); + + RichTextString str = factory.createRichTextString(message); + comment.setString(str); + comment.setAuthor("fanfy"); + cell.setCellComment(comment); + + return comment; + } }