Bug 15050

Summary: [PATCH]HSSFWorkbook#cloneSheet() throws RuntimeException
Product: POI Reporter: Toshiaki Kamoshida <kamoshida.toshiaki>
Component: HSSFAssignee: POI Developers List <dev>
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

When I tried to clone a sheet by calling HSSFWorkbook#cloneSheet(int), 
HSSFWorkbook#cloneSheet(int) throws RuntimeException from 
org.apache.poi.hssf.record.Record#clone(),because some subclasses of 
org.apache.poi.hssf.record.Record has no individual implementation of clone() 
Comment 1 Toshiaki Kamoshida 2002-12-04 05:30:00 UTC
Created attachment 4036 [details]
To Fix simply is,Record implements Cloneable.
Comment 2 Toshiaki Kamoshida 2002-12-04 05:45:42 UTC
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.
Comment 3 Avik Sengupta 2002-12-12 08:43:59 UTC
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!!)
Comment 4 Toshiaki Kamoshida 2002-12-12 10:13:15 UTC
Created attachment 4140 [details]
This is minimum testcase to "find" this problem,but not enough for this patch...
Comment 5 Toshiaki Kamoshida 2002-12-12 10:51:01 UTC
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:)
Comment 6 Avik Sengupta 2002-12-12 12:49:36 UTC
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)
Comment 7 Dennis Doubleday 2002-12-12 14:17:09 UTC
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.
Comment 8 Jason Height 2002-12-12 20:41:20 UTC
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.

Comment 9 Jason Height 2002-12-12 20:53:16 UTC
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.

Comment 10 Avik Sengupta 2002-12-13 04:49:15 UTC
*** Bug 14487 has been marked as a duplicate of this bug. ***
Comment 11 Avik Sengupta 2002-12-13 04:53:15 UTC
I'll only be able to look at it next weekend, so if you can do something next
week, go ahead. 
Comment 12 Andy Oliver 2002-12-27 04:58:06 UTC
Shawn does this work?
Comment 13 Avik Sengupta 2003-02-27 13:32:54 UTC
*** Bug 17474 has been marked as a duplicate of this bug. ***
Comment 14 Avik Sengupta 2003-03-01 14:30:15 UTC
*** Bug 17540 has been marked as a duplicate of this bug. ***
Comment 15 Avik Sengupta 2003-03-01 14:32:09 UTC
fixed, afaik, with all records mentioned herein. 
Comment 16 Henning Boeger 2003-03-05 16:51:44 UTC
This class needs a clone() Method too:

Comment 17 Henning Boeger 2003-03-20 10:20:56 UTC
Created attachment 5441 [details]
Added clone Method for org.apache.poi.hssf.record.aggregates.FormulaRecordAggregate
Comment 18 Avik Sengupta 2003-03-20 20:03:51 UTC
Modified version comitted (made it deeper clone). Thanks Henning. 
Pls cross check.