Re: [Libreoffice] [PATCH] Bug 33293 - [EasyHack] Make starting count of worksheets configurable

Hi Albert,

On Tue, 2011-05-31 at 00:10 +0200, Albert Thuswaldner wrote:
> Hi all,
> Submitting a patch for  Bug 33293 for review. Please let me know if
> you want me to improve some parts of it.

So, I did test your patch on the master branch.  I have some feedback.

First off, I'm pretty sure you forgot to include your change in cui,
since I had to add the following change to get the option page to show
up.

--- a/cui/source/options/treeopt.src
+++ b/cui/source/options/treeopt.src
@@ -237,6 +237,7 @@ Resource RID_OFADLG_OPTIONS_TREE_PAGES
         {
             < "%PRODUCTNAME Calc" ; 0; > ;
             < "General" ;                      SID_SC_TP_LAYOUT                        ;> ;
+            < "Initialize" ;                   RID_SC_TP_INIT                  ;> ;
             < "View" ;                 SID_SC_TP_CONTENT                       ;> ;
             < "International" ;                RID_OFA_TP_INTERNATIONAL        ;> ;
             < "Calculate" ;                    SID_SC_TP_CALC                          ;> ;

Now, once the option page is shown, it does work great. :-)  I change
the number of sheets in the Options page, click OK, create a new
spreadsheet document, and the new document has the number of sheets as
specified in the Options dialog.

However, you didn't change the code that stores the options into the
user configuration, so once you exit the application the value goes back
to 3.  This needs to be fixed.  The good news is that you are almost
there.  You just need to add new option nodes in
officecfg/registry/schema/org/openoffice/Office/Calc.xcs and use that
configuration path to load and save the values in the ScDocCfg class.

My other nitpick is to prefer static_cast over C-style cast e.g. instead
of 

SetInitSheet( (sal_Int16) nIntVal );

let's do

SetInitSheet( static_cast<sal_Int16>(nIntVal) );

etc.

One last thing is regarding the following code block:

    // Get the customized initial tab count...

    // ... from option dialog.
    const ScDocOptions& rDocOpt = SC_MOD()->GetDocOptions();
    SCTAB nInitTabCount = rDocOpt.GetInitSheet();

    // ... by VBA API.
    const ScAppOptions& rAppOpt = SC_MOD()->GetAppOptions();
    SCTAB nNewTabCount = rAppOpt.GetTabCountInNewSpreadsheet();
    if ( nNewTabCount >= 1 && nNewTabCount <= MAXTAB )
    {
        nInitTabCount = nNewTabCount;
    }
    for (SCTAB i=1; i<nInitTabCount; i++)
        pDoc->MakeTable(i,false);

in ScTabViewShell::Construct(), which I find a bit awkward as the doc
options and the app options (presumably for the VBA API) fight each
other in a bit weird way.  I would like to have this cleaned up a bit,
but we can do this cleanup later after integrating your patch, since
this may become a bit tricky depending on what VBA API requires.

So, let's fix the configuration loading and saving, then let's get this
piece in.  Thanks a lot for your work. :-)

Kohei