| Summary: | [PATCH]HSSFWorkbook#cloneSheet() throws RuntimeException | ||
|---|---|---|---|
| Product: | POI | Reporter: | Toshiaki Kamoshida <kamoshida.toshiaki> |
| Component: | HSSF | Assignee: | POI Developers List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | avernet, dennis, ndz43wv02 |
| Priority: | P3 | ||
| Version: | 2.0-dev | ||
| Target Milestone: | --- | ||
| Hardware: | Other | ||
| OS: | other | ||
| Attachments: |
To Fix simply is,Record implements Cloneable.
This is minimum testcase to "find" this problem,but not enough for this patch... Added clone Method for org.apache.poi.hssf.record.aggregates.FormulaRecordAggregate |
||
|
Description
Toshiaki Kamoshida
2002-12-04 05:27:11 UTC
Created attachment 4036 [details]
To Fix simply is,Record implements Cloneable.
Cloneable is very convenience. If one subclass's instance has only primitive or immutable object's reference fields,it is no need to override clone() method,each fields are automatically copied on native method at java.lang.Object,and there is no other bad effects. 1. Can u please supply a simple testcase for this error? 2. Anybody has any objections to my applying this? I tend to agree that Record should implement cloneable, but am not very conversant with the cloneSheet implementation (All the more reason why there should be a test case!!) Created attachment 4140 [details]
This is minimum testcase to "find" this problem,but not enough for this patch...
When I set a breakpoint at org.apache.poi.hssf.record.Record#clone(), and execute cloneSheet() by using "My" Excel file on a debuggar,I found there is no implementation of clone() at thease classes.If the target sheet has one of them,cloneSheet() will fail. 1. org.apache.poi.hssf.record.MargeCellsRecord 2. org.apache.poi.hssf.record.ColumnInfoRecord 3. org.apache.poi.hssf.record.BottomMarginRecord 4. org.apache.poi.hssf.record.LeftMarginRecord 5. org.apache.poi.hssf.record.RightMarginRecord 6. org.apache.poi.hssf.record.TopMarginRecord Maybe some other classes are same,I don't know. I feel,the testcase for this patch should use a sheet that has at least one instance of all each Record's subclass.But I can't create it easily. Please someone make it:) thanks. thats what i wanted .. to be sure i am not solving the wrong problem. I tend to agree that getting Record to implement cloneable should be a simple fix without too many side effects. I'll wait a while to see if Jason (who implemented most of the cloning stuff) has any alternate ideas, and then go ahead (hopefully with some unit tests) This is a duplicate of #14487, which I created. However, this one now has more info, so if you want to mark #14487 as the duplicate, feel free. OK I originally wrote the cloneSheet implementation. The reason for not implementing clonable was that at the time I did not know which records were sheet based and which are workbook based. As such i wanted to know which records i still needed to implement. Instead of making Record implement Clonable it may be better to fix the records that dont have their own clone method. Initially there was some debate whether a shallow clone is appropriate (ie by implementing Clonable), and the suggestion from the list (Andy?) was to perform deep clones, which is how it is done. Jason Oh I forgot to mention, I did initially implement shallow clones and that worked fine. If my memory serves me its only the aggregate records that need to be deep cloned. I can implement the extra Records listed in this bug report next week if you want, or we can just implement Clonable and hope for the best. Jason *** Bug 14487 has been marked as a duplicate of this bug. *** I'll only be able to look at it next weekend, so if you can do something next week, go ahead. Shawn does this work? *** Bug 17474 has been marked as a duplicate of this bug. *** *** Bug 17540 has been marked as a duplicate of this bug. *** fixed, afaik, with all records mentioned herein. This class needs a clone() Method too: org.apache.poi.hssf.record.aggregates.FormulaRecordAggregate Created attachment 5441 [details]
Added clone Method for org.apache.poi.hssf.record.aggregates.FormulaRecordAggregate
Modified version comitted (made it deeper clone). Thanks Henning. Pls cross check. |