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

classic Classic list List threaded Threaded
14 messages Options
Winfried Donkers Winfried Donkers
Reply | Threaded
Open this post in threaded view
|

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

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

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.

 

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.

 

 

 

Winfried

 


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

0001-fdo-50950-modify-calc-functions-WEEKNUM-_ADD-to-comp.patch (27K) Download Attachment
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

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

Hi Winfried,

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.

Just to tell you that I didn't forget about this patch, I'll come back
to you with details later, maybe only after LibOCon.

Thanks
  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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment
Winfried Donkers Winfried Donkers
Reply | Threaded
Open this post in threaded view
|

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

> Attached patch modifies the calc functions WEEKNUM and
> WEEKNUM_ADD to respectively ISOWEEKNUM and WEEKNUM.

I noted that the patch for fdo50950  has been pushed to master whilst 'git pul --rebase'-ing this morning.
However i did not see any mail that it has been checked.
Is it correct that the patch has been pushed to master?


Winfried
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

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

Hi Winfried,

On Monday, 2012-10-22 09:27:35 +0200, Winfried Donkers wrote:

> I noted that the patch for fdo50950  has been pushed to master whilst 'git pul --rebase'-ing this morning.
> However i did not see any mail that it has been checked.
> Is it correct that the patch has been pushed to master?

I do not see anything related in master, probably it is just in your
local branch?

  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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment
Winfried Donkers Winfried Donkers
Reply | Threaded
Open this post in threaded view
|

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

Hi Eike,

> > Is it correct that the patch has been pushed to master?
> I do not see anything related in master, probably it is just in your local
> branch?

You are quite right, of course :)
I apologise for disturbing you and quickly return to my investigating/experimenting with IFERROR().

Winfried
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Winfried Donkers Winfried Donkers
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Eike Rathke-2
Hi Eike,

Would it be an idea if I submit the patch via gerrit? Not to hurry you, but to prevent it from getting burried accidentally ;)
(same goes for IFERROR() and IFNA() patches)

By the way, any suggestions/preferences for the next ODFF1.2 formulas to start working on?


Winfried

> -----Oorspronkelijk bericht-----
> Van: Eike Rathke [mailto:[hidden email]]
> Verzonden: maandag 15 oktober 2012 14:54
> Aan: Winfried Donkers
> CC: [hidden email]
> Onderwerp: Re: [PATCH] fdo#50950 modify calc functions WEEKNUM(_ADD)
> to comply with ODFF1.2
>
> Hi Winfried,
>
> 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.
>
> Just to tell you that I didn't forget about this patch, I'll come back to you with
> details later, maybe only after LibOCon.
>
> Thanks
>   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
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Winfried Donkers
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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Winfried Donkers
Hi Winfried,

On Tuesday, 2012-11-13 11:08:07 +0100, Winfried Donkers wrote:

> Would it be an idea if I submit the patch via gerrit? Not to hurry you, but to prevent it from getting burried accidentally ;)

It's just buried in my mail folder ;-)
Now that I wrote the review we don't need it in gerrit (anymore).

> (same goes for IFERROR() and IFNA() patches)

If you have something ready for those you can submit it to gerrit,
that's fine.

> By the way, any suggestions/preferences for the next ODFF1.2 formulas to start working on?

NUMBERVALUE would be good, to have a locale-independent (as opposed to
VALUE) function to convert text to numeric values. Note that some
fragments are already in the code base, but not implemented yet:
grep -r NUMBERVALUE formula/{inc,source} sc/{inc,source}

Btw, in sc/source/ui/src/scfuncs.src the function lacks the parameter
for group separators.

  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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment
Winfried Donkers Winfried Donkers
Reply | Threaded
Open this post in threaded view
|

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

Hi Eike.

> Now that I wrote the review we don't need it in gerrit (anymore).

I look forward to your comments and probably will modify the code accordingly ;)

> > (same goes for IFERROR() and IFNA() patches)
> If you have something ready for those you can submit it to gerrit, that's fine.

I will, later today.

> > By the way, any suggestions/preferences for the next ODFF1.2 formulas to
> start working on?
> NUMBERVALUE would be good ...

Incidentally, I was looking at the same function :)
I  created bug 57180 for NUMBERVALUE and will start working on it.

Winfried
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
m.a.riosv m.a.riosv
Reply | Threaded
Open this post in threaded view
|

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

Hi Winfried,

a common question in ML, Ask and forums, is about the functions:

SUMIFS 6.16.63,  AVERAGEIFS 6.18.6,  COUNTIFS 6.13.10
(OpenDocument-v1.2-part2)

People often do not understand very well how to use SUMPRODUCT() or matrix formulas, and with them, is more complicate the use of regular expressions.

I do not use MS in anyway but I think this functions are there, and if I not in a mistake, also will be introduced in AOO.

I think they could be very helpful, and good for a best interoperability.

Miguel Ángel.

Winfried Donkers Winfried Donkers
Reply | Threaded
Open this post in threaded view
|

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

Hi Miguel,

> SUMIFS 6.16.63,  AVERAGEIFS 6.18.6,  COUNTIFS 6.13.10

The functions you name wait for license clearance to integrate the OOo code for these functions in LibO.
fdo 41214 is about SUMIFS, AVERAGEIF(S) and COUNTIFS, you can read about all the details there.

Winfried

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Winfried Donkers Winfried Donkers
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Eike Rathke-2
Hi Eike,

I'm currently putting your review into a better patch:

-renaming the functions: I reverted some lines as you proposed.

> 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.
That link wasn't broken, the xhp files were not included in the patch. I guess it is beyond the scope for git and that I should use ./g instead to include it. Am I correct here?

> 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)
This will need some study from me, poking is not my strongest capabilty.

I will put the latest diff file as an attachment in bug 50950, as I'm afraid the 'poking' will take some time.

Winfried
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

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

Hi Winfried,

On Tuesday, 2012-11-20 13:24:06 +0100, Winfried Donkers wrote:

> > 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.
> That link wasn't broken, the xhp files were not included in the patch.
> I guess it is beyond the scope for git and that I should use ./g
> instead to include it. Am I correct here?

Hm.. the dreaded submodules.. ;)  Please see
http://wiki.documentfoundation.org/Development/Submodules
and especially
http://wiki.documentfoundation.org/Development/Submodules#What_is_the_impact_for_developers_working_in_the_auxiliary_repositories_.3F


  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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment
Winfried Donkers Winfried Donkers
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Eike Rathke-2
Hi Eike,

> > 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)
> [...]

I'm afraid I'm lost here. I can't find my way in/around FormulaCompiler::Factor().
To put it simple: with respect to calc-formulas I understand what the user sees and I (often) understand what's happening in ScInterpreter. But the bit inbetween as rather foggy for me. And to be honest, I doubt if a thorough study of that area will help me much.
So I've got another three cases to consider :-/
1. Give up fdo50950 - I don't like this option, not vanity but I would the functions itself to be available in LibO
3. Get some help from co-developers who are familiar with this kind of poking - that would be nice, but is out of my control
2. Struggle on in the hope of magically getting there - this will be the default as I don't like case 1

I will see that the submodules are included in the patch sofar that I attached to the bug itself and concentrate on other bugs (like IFERROR/IFNA/NUMBERVALUE).

Winfried

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice