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

[Kohei Yoshida]

Hi Albert,

On Wed, 2011-06-01 at 15:29 +0200, Albert Thuswaldner wrote:
> Hi Kohei and Marcus,
> Thanks for your input.
> 
> On Tue, May 31, 2011 at 01:38, Markus Mohrhard
> <markus.mohrhard at googlemail.com> wrote:
> > Hello Albert,
> >
> > just two quick comments.
> >
> > We are trying to get away from using sal_Int16 for sheet numbers and use
> > SCTAB instead, so it would be nice if you could change this.
> 
> Ok, I 'll change that.
> 
> > And I think we can't/shouldn't create a document without a sheet (but Kohei
> > may prove me wrong). And did you check that no negative numbers/characters
> > are inserted?
> 
> Ok, I will see how to implement some checks on the user input.
> 
> On Tue, May 31, 2011 at 17:11, Kohei Yoshida <kyoshida at novell.com> wrote:
> > The only thing I might say is that, I'm not too sure about the page
> > being called "Initialize".  There may be a better naming for an option
> > page like this....  "Defaults" maybe?  I don't know.
> >
> 
> Well I'm not sure either what to call it. I just used "Initalize" for
> the lack of something else.
>  I guess it depends on what other options that might be added to this
> category in the future.
> What about "Start-up"?
> 
> Whatever name is chosen, it should be reflected in the handler class
> name as well, right? So I would rename ScTpInitOptions to something
> which match the config tab name.
> 
> On Wed, Jun 1, 2011 at 04:29, Kohei Yoshida <kyoshida at novell.com> wrote:
> > 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                          ;> ;
> 
> Oh, that's wierd, I made the change, but somehow it was not included
> in the patch.
> 
> > 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.
> 
> Ok, overlooked that part. I'm quite amazed by how many places in the
> code that requires a change just to implement this small feature. :)
> 
> > 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.
> 
> Yes, I'll fix that.
> 
> > 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.
> 
> I'm not all to happy with that part either. It is not optimal but I
> think it fits with the use case, i.e:
> You set the number of sheet tabs via the option dialog, and also you
> can _temporary_ set the number of sheet tabs via the VBA API.
> 
> This is how it's going to be used in the end, right?

Nah.  Nothing says that the value set from the VBA API has to be
temporary.  The comment only says that this functionality is currently
only used from the VBA API.  That doesn't mean it should stay that way.

Anyway, I didn't mean to say that this is something you need to fix; on
the contrary this is something we should be looking into after we merge
your patch in.  I need to check with Noel about the VBA API too (he is
in CC).  So, let's not worry about this for now. :-)

> are, as I understand it, used by the vba scripting api to _temporary_
> set/get the number of sheets. But
> using these methods will not save the the number of sheets to the
> configuration file. I did a failled implementation of that, see the
> attached files (further up in hte thread):
> 
> tpinit.cxx.appoptio
> tpinit.hxx.appoptio
> 
> I imagne that you don't want to store the number of sheets as an
> option when yoy use the VBA API. Thats why I implemeted it in
> ScDocOptions without knowing the differance between them as you write
> about below. So what i have implemeted is not the correct approach,
> but merging it into ScAppOptions might not be such a good idea either.

I failed to see though, why changing the number of sheets from VBA API
should not be stored in the configuration.  After all, when you change
configuration settings using the Basic API (or UNO API for that matter),
they will stick.  The same should apply to VBA API.

Anyway, I didn't mean to put this burden on your shoulder. So we'll be
happy to take care of that after the fact.  I was just mentioning this
to make us aware of what needs to be done.

So, let's address Markus' comment and my comment regarding the change in
officecfg, then let's merge your patch in first.  We'll worry about the
issue with the VBA API after that.

Thanks a lot,

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc
<kyoshida at novell.com>