Re: [Libreoffice] [Patch]Add method Copy to ScVbaWorksheets

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