Hi Kohei and Marcus,
Thanks for your input.
On Tue, May 31, 2011 at 01:38, Markus Mohrhard
<markus.mohrh...@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 <kyosh...@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 <kyosh...@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 <kyosh...@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
> <kyosh...@novell.com>
>
>
Thanks for your help.
/Albert
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
