Re: [PATCH] fdo#50950 modify calc functions WEEKNUM(_ADD) to comply with ODFF1.2

Hi Winfried,

Long overdue, but better late than never..
This needed some detailed review, which is why I postponed it
originally.

On Friday, 2012-10-05 12:25:57 +0200, Winfried Donkers wrote:

> Attached patch modifies the calc functions WEEKNUM and WEEKNUM_ADD to
> respectively ISOWEEKNUM and WEEKNUM. These latter functions comply
> with ODFF1.2

Generally the patch works, but there are some things that need
consideration and more work.

While this

--- a/formula/source/core/resource/core_resource.src
+++ b/formula/source/core/resource/core_resource.src
@@ -317,7 +317,7 @@ Resource RID_STRLIST_FUNCTION_NAMES_ENGLISH_ODFF
-    String SC_OPCODE_WEEK { Text = "ISOWEEKNUM" ; };
+    String SC_OPCODE_ISOWEEKNUM { Text = "ISOWEEKNUM" ; };

changing only the identifier is fine, this

@@ -650,7 +650,7 @@ Resource RID_STRLIST_FUNCTION_NAMES_ENGLISH
-    String SC_OPCODE_WEEK { Text = "WEEKNUM" ; };
+    String SC_OPCODE_ISOWEEKNUM { Text = "ISOWEEKNUM" ; };

changing the string WEEKNUM to ISOWEEKNUM does not work, as these
resources are also used to save/load in ODF 1.0/1.1 and compile formulas
set via API. So saving in ODF 1.0/1.1 and reading in an older release
will fail and vice versa.

For connected reason, this

--- a/sc/source/core/tool/odffmap.cxx
+++ b/sc/source/core/tool/odffmap.cxx
@@ -35,7 +35,7 @@ ScCompiler::AddInMap ScCompiler::maAddInMap[] =
-    { "WEEKNUM", "WEEKNUM_ADD", false, "com.sun.star.sheet.addin.Analysis.getWeeknum", 
"COM.SUN.STAR.SHEET.ADDIN.ANALYS
+    { "WEEKNUM", "WEEKNUM", false, "com.sun.star.sheet.addin.Analysis.getWeeknum", 
"COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.G

would not work if the above change is not done as there would be two
WEEKNUM in the English map.

Not changing the strings on the other hand would leave them unchanged in
the "use English function names" case, which would be confusing for the
user as then we'd have ISOWEEKNUM and WEEKNUM vs. WEEKNUM and
WEEKNUM_ADD with WEEKNUM meaning two different things. I think we could
get around this by "patching" the generated symbol table for the English
case in the compiler, instead of duplicating
RID_STRLIST_FUNCTION_NAMES_ENGLISH, and add another field in AddInMap,
I could take a look later. For now just don't rename the functions.

Not renaming WEEKNUM_ADD to WEEKNUM also obsoletes this change

--- a/scaddins/source/analysis/analysishelper.cxx
+++ b/scaddins/source/analysis/analysishelper.cxx
@@ -48,7 +48,7 @@ const FuncDataBase pFuncDatas[] =
-    FUNCDATA( Weeknum,          DOUBLE,     INTPAR,     2,          FDCat_DateTime ),
+    FUNCDATA( Weeknum,          UNIQUE,     INTPAR,     2,          FDCat_DateTime ),


> The link to the help files has been broken, i.e. WEEKNUM now points to
> the (partially out of date) help text for WEEKNUM_ADD, but ISOWEEKNUM
> does not point to a help text.

The latter because of

--- a/sc/inc/helpids.h
+++ b/sc/inc/helpids.h
@@ -396,7 +396,7 @@
-#define HID_FUNC_KALENDERWOCHE                                  "SC_HID_FUNC_KALENDERWOCHE"
+#define HID_FUNC_ISOWEEKNUM                                     "SC_HID_FUNC_ISOWEEKNUM"

SC_HID_FUNC_KALENDERWOCHE is used in

helpcontent2/source/text/scalc/01/func_weeknum.xhp:<bookmark xml-lang="en-US" 
branch="hid/SC_HID_FUNC_KALENDERWOCHE" id="bm_id3158403" localize="false"/><paragraph 
role="heading" id="hd_id3159161" xml-lang="en-US" level="2" l10n="U" oldref="54"><variable 
id="weeknum"><link href="text/scalc/01/func_weeknum.xhp">WEEKNUM</link>

that should be adapted. The WEEKNUM_ADD help is in
helpcontent2/source/text/scalc/01/func_weeknumadd.xhp


> One important matter  is still open:
> When opening calc-documents that use the 'old' functions, they are not
> always computed correctly. The 'new' functions differ in paramter
> count.
> I do not know where to add code for backward compatibility when
> opening these documents, but with some help/hints I might be able to
> fix that.

There are three cases to be considered:

1. reading ODF 1.2 (ODFF)
2. writing ODF 1.0/1.1 (PODF)
3. reading ODF 1.0/1.1 (PODF)

1. Even if ODFF defines number of parameters and whether optional or
   not, older releases wrote the WEEKNUM internal function as ISOWEEKNUM
   with 2 parameters instead of 1.
   * could be checked in formula/source/core/api/FormulaCompiler.cxx
     FormulaCompiler::Factor()
     if (eOp == ocIsoWeeknum && mxSymbols->isODFF())
     and if it has 2 parameters put a token for the WEEKNUM Add-In
     instead. This would need some poking to mess around with the actual
     function token, a simple pFacToken->NewOpCode(...) would not do as
     a FormulaExternalToken is needed instead.

2. ISOWEEKNUM needs to be written as WEEKNUM again.
   * parameter could be added in formula/source/core/api/token.cxx
     MissingConvention::isRewriteNeeded()
     FormulaMissingContext::AddMoreArgs()
   * not having changed the function name in the resources should
     actually write the correct name, to be verified.

3. Very similar to #1, but instead the condition would be
   if (eOp == ocIsoWeeknum && mxSymbols->isPODF())
   So it may work using the same code with a condition
   if (eOp == ocIsoWeeknum && (mxSymbols->isODFF() || mxSymbols->isPODF()))

In a later step we could implement something that checks the second
parameter and if it is a literal Monday than switch to ISOWEEKNUM
instead of the WEEKNUM Add-In, but currently that's not a must.

I hope that helps, at least a little ;)

  Eike

-- 
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD
Support the FSFE, care about Free Software! https://fsfe.org/support/?erack

Attachment: pgpwymKzTCie0.pgp
Description: PGP signature