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

Fri, 08 Apr 2011 11:01:41 -0700 

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







_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Reply via email to