Date: prev next · Thread: first prev next last
2011 Archives by date, by thread · List index 

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@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@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@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@novell.com>

Context

Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.