[Libreoffice] [Patch]Add method Copy to ScVbaWorksheets
Noel Power
nopower at novell.com
Fri Apr 8 11:01:23 PDT 2011
Hi Markus,
On 08/04/11 01:38, Markus Mohrhard wrote:
> Hello,
>
> here the patch for the EasyHack:
> http://wiki.documentfoundation.org/Development/Easy_Hacks#VBA_support_add_support_for_Worksheets.Copy
> or Bug: https://bugs.freedesktop.org/show_bug.cgi?id=34763 .
>
> It needs the patch from this afternoon to work correctly.
>
> Patch is under LGPLv3+/MPL.
>
Thanks for the patch, unfortunately there are a couple of problems with
it, I tried some examples and the results weren't as expected e.g.
starting with a workbook with the following sheets 'sheet1', 'sheet2'
and 'sheet3'
and trying a macro like:
sub test
worksheets.copy after:=worksheets(2)
end sub
results in the following sheets in the workbook 'sheet1', 'sheet2',
'sheet1_2', 'sheet2_2', 'sheet1_2_2', 'sheet3'
the expected result should be something like 'sheet1', 'sheet2',
'sheet1_2', 'sheet2_2', 'sheet3_2', 'sheet3'
and similarly running the following macro
sub test
worksheets.copy before:=worksheets(2)
end sub
results in a workbook with the following sheets 'sheet1', 'sheet1_2',
'sheet1_2_2', 'sheet1_2_2_2', 'sheet2', 'sheet3'
expected results would be 'sheet1', 'sheet1_2', 'sheet2_2', 'sheet3_2',
'sheet2','sheet3'
I think you are falling victim to the fact that you are modifying the
underlying container ( e.g. the sheets container ) whilst iterating over
it.
Also, although I appreciate the fact you moved some methods into the
excelvbahelper.cxx file in order to share code I don't really think
those methods belong there. Those methods are really local utility
functions and really couldn't be considered to be of general use.
Personally I am not such a stickler for such things and normally would
take the approach that sharing the code is more important that obeying
some lofty design idea. However in this case I see alot of duplication
in the Worksheets::Copy and the Worksheet::Copy, to my mind the
Worksheets::Copy should use ( or delegate to ) the Worksheet:::Copy (
and/or ) methods of the Worksheet object to achieve the desired result
and in this case this to me reinforces the idea we shouldn't
unnecessarily make public those helper methods.
So.. I am sorry first that I don't think the patch is suitable to commit
right now, also I am sorry that the so called easy hack has like alot of
things that seem initially easy thrown some unexpected problems. But
please don't despair, I spend some time having a look and thinking about
what we could do to reorganize things such that the Worksheets::Copy can
reuse the Worksheet object to do the business.
Here's what I propose
First I think for the normal case where you specify a 'Before' or
'After' parameter things should just work out pretty easily e.g. in
psuedo code
Worksheets::Copy( before, after )
{
if ( before.hasValue OR after.hasValue )
{
vector< excel::XWorksheet > xSheets;
// A) grab a local copy of the sheets in the sheet container
foreach sheet in sheets
xSheets.push_back( sheet )
next sheet
// B) prevent problems modifying the sheet container by working
with the copy
foreach sheet in XSheets
sheet.Copy( before, after )
next sheet
}
}
you will probably need to be a bit creative with some twisty logic to
ensure the order of the sheets in (A) is correct for when you want to
insert before/after
e.g. for workbook with sheets ( 'sheet1', 'sheet2', 'sheet3' ) and if
xSheets = { 'sheet1', 'sheet2', 'sheet3 } thenthe first example above
'Worksheets.Copy after:=Worksheets(2)
would end up with 'sheet1', 'sheet2', 'sheet2'_2, 'sheet3_2', 'sheet1_2'
whereas
Worksheets.Copy before:=Worksheets(2) will naturally work out as
expected e.g. 'sheet1', 'sheet1_2', 'sheet2_2', 'sheet3_2',
'sheet2','sheet3'
dealing with the case where if ( !before.hasValue AND !after.hasValue
promises ) to be trickier I think in this case we need to modify the
Worksheet object to have a method like
uno::Reference< ov::excel::XWorksheet >
ScVbaWorksheet::createSheetCopyInNewDoc();
The existing ScVbaWorksheet::Copy would then use that like
ScVbaWorksheet::Copy( before, after )
{
if ( ( before AND after ) are Empty ) then
createSheetCopyInNewDoc()
" "
" "
}
and the ScVbaWorksheets::Copy you will need to do something like
ScVbaWorksheets::Copy( before, after )
{
if ( ( before AND after ) are Empty ) then
{
// B) prevent problems modifying the sheet container by working
with the copy
foreach sheet in XSheets
if ( first sheet )
before = sheet->createSheetCopyInNewDoc()
else
sheet->copy( before, after )
next sheet
sheet.Copy( before, after )
next sheet
}
}
I did some mini experiments with parts of the above and it seems to work
out ( although I have some logic problems mostly due to lack of brains )
but I hope you get what I mean. I hope you are still interested to stick
at it and work through this and I would be happy to help you with that.
Anyway sorry for such a long mail and look forward to hearing from you
Noel
More information about the LibreOffice
mailing list