Bug 15050 - [PATCH]HSSFWorkbook#cloneSheet() throws RuntimeException
Summary: [PATCH]HSSFWorkbook#cloneSheet() throws RuntimeException
Alias: None
Product: POI
Classification: Unclassified
Component: HSSF (show other bugs)
Version: 2.0-dev
Hardware: Other other
: P3 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
: 14487 17474 17540 (view as bug list)
Depends on:
Reported: 2002-12-04 05:27 UTC by Toshiaki Kamoshida
Modified: 2004-11-16 19:05 UTC (History)
3 users (show)

To Fix simply is,Record implements Cloneable. (823 bytes, patch)
2002-12-04 05:30 UTC, Toshiaki Kamoshida
Details | Diff
This is minimum testcase to "find" this problem,but not enough for this patch... (565 bytes, text/plain)
2002-12-12 10:13 UTC, Toshiaki Kamoshida
Added clone Method for org.apache.poi.hssf.record.aggregates.FormulaRecordAggregate (613 bytes, patch)
2003-03-20 10:20 UTC, Henning Boeger
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.