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

[Albert Thuswaldner]

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?

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

On Wed, Jun 1, 2011 at 04:59, Kohei Yoshida <kyoshida at novell.com> wrote:
> On Tue, 2011-05-31 at 22:29 -0400, Kohei Yoshida wrote:
>> 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.
>
> After looking around this a bit, I think it makes sense to
>
> 1) merge these two options storing the same thing, and
> 2) have it in ScAppOptions, not ScDocOptions.

I'm not sure about this.  As I wrote in a reply further up in the
thread, the methods in ScAppOptions:

SetTabCountInNewSpreadsheet
GetTabCountInNewSpreadsheet

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.

> Option values in ScDocOptions appear to be stored with the document as
> well as with the configuration, whereas those in ScAppOptions are stored
> with the configuration only.  Since it makes no sense to store the new
> document settings with each document, it probably should belong to
> ScAppOptions.
>
> This also means that I've misplaced several options in ScDocOptions as
> well.... The formula and compatibility options are not stored with the
> document, so they should really be in ScAppOptions.  I need to fix that
> later.... :-P
>
> Kohei
>
> --
> Kohei Yoshida, LibreOffice hacker, Calc
> <kyoshida at novell.com>
>
>

Thanks for your help.
/Albert