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

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?


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




Thanks for your help.
/Albert

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.