The current UI functions WEEKNUM and WEEKNUM_ADD dor not strictly comply with ODFF1.2 functions ISOWEEKNUM and WEEKNUM respectively. I plan to correct this as suggested by Eike) by: 1 enhance UI WEEKNUM_ADD to support all ODF WEEKNUM modes 2 in the UI rename WEEKNUM to ISOWEEKNUM 3 change ISOWEEKNUM to accept only one parameter 4 during import's formula compile step for ISOWEEKNUM check if a second argument is provided a if so and if it is a constant value !=1 strip the argument (the Monday case that was and is real ISO 8601) b if it is a constant value ==1 map the function to WEEKNUM * there's a slight chance that a user wanted exactly the behavior "ISO but Sunday" resulting from the current undocumented implementation details, which isn't supported by ODF WEEKNUM definition, but I think that should be a very rare case, if at all, and is neglectable c if it is not a constant (i.e. computed) argument do nothing and let the interpreter complain about the second parameter A possible fifth step could be to rename UI WEEKNUM_ADD to WEEKNUM, with the result that UI and ODF use the same function names. This bug is a sub-bug of bug 50488
Created attachment 63254 [details] [review] patch sofar, not yet ready for submitting attached diff file contains the functions WEEKNUM and ISOWEEKNUM (100% ODFF1.2 compliant). The old WEEKNUM_ADD UI-function has been removed from analysis module. Still missing is code to automatically convert old-style saved WEEKNUM or ISOWEEKNUM use in files. Possibly something must be added to state whether MS Office or Lotus recognise WEEKNUM and ISOWEEKNUM (ODFF1.2).
Created attachment 63255 [details] test document for WEEKNUM and ISOWEEKNUM calc document contains calls of WEEKNUM with all possible mode options and calls of ISOWEEKNUM, for 7 22-day periods (each period a different weekday on january 1)
Comment on attachment 63254 [details] [review] patch sofar, not yet ready for submitting Review of attachment 63254 [details] [review]: ----------------------------------------------------------------- Overall, please retain the old Add-In getWeeknum() functionality for ODF 1.1 import/export and Excel interoperability. Add the new Mode arguments from ODFF. In binary file format Excel expects WEEKNUM to be flagged as Add-In, so simply removing it from the corresponding tables will not do. We'd need another mechanism to treat this then. So at the moment we'd end up with 3 functions (UI names), WEEKNUM, WEEKNUM_ADD and ISOWEEKNUM. WEEKNUM and WEEKNUM_ADD would be saved as WEEKNUM in ODFF, and read from ODFF as internal WEEKNUM. For how to map the internal WEEKNUM to the Add-In WEEKNUM_ADD during Excel export we'd have to see. When solved we then could also map the Add-In to internal WEEKNUM during Excel import and get rid of WEEKNUM_ADD, or map ODF WEEKNUM as Add-In WEEKNUM. However, as lined out earlier on the mailing list I think easiest would be to just have the Add-In WEEKNUM_ADD implementation, remove the internal WEEKNUM implementation, rename WEEKNUM_ADD to WEEKNUM and add the internal ISOWEEKNUM implementation, so effectively we'd have 2 functions. When reading ODF where ISOWEEKNUM has 2 parameters (as could have been saved with the current WEEKNUM implementation) map it to Add-In WEEKNUM if the second parameter is a constant 1. Remove the parameter if it is a constant other than 1 and keep function as ISOWEEKNUM. I'll go into detail about that on the mailing list. For some detailed nitpicks see the diff's annotations. ::: formula/inc/formula/compiler.hrc @@ +401,4 @@ > #define SC_OPCODE_BITLSHIFT 399 > #define SC_OPCODE_GET_DATEDIF 400 > #define SC_OPCODE_XOR 401 > +#define SC_OPCODE_ISOWEEKNUM 402 As ISOWEEKNUM has exactly 1 parameter that should go before SC_OPCODE_STOP_1_PAR instead. ::: sc/inc/helpids.h @@ -444,4 @@ > #define HID_FUNC_JAHR "SC_HID_FUNC_JAHR" > #define HID_FUNC_TAGE "SC_HID_FUNC_TAGE" > #define HID_FUNC_DATEDIF "SC_HID_FUNC_DATEDIF" > -#define HID_FUNC_KALENDERWOCHE "SC_HID_FUNC_KALENDERWOCHE" Renaming a HID isn't such a good idea, help content depends on it. Better to just add the new HID. Or if you do rename IDs adapt also the references under helpcontent2/source/text/scalc/ However, when replacing the internal WEEKNUM with ISOWEEKNUM it would be good to remove HID_FUNC_KALENDERWOCHE and add HID_FUNC_ISOWEEKNUM and adapt helpcontent2 IDs and help text. ::: sc/source/core/tool/addinhelpid.cxx @@ -138,4 @@ > { "getTbilleq" , HID_AAI_FUNC_TBILLEQ }, > { "getTbillprice" , HID_AAI_FUNC_TBILLPRICE }, > { "getTbillyield" , HID_AAI_FUNC_TBILLYIELD }, > - { "getWeeknum" , HID_AAI_FUNC_WEEKNUM }, Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability. ::: sc/source/core/tool/interpr2.cxx @@ +229,4 @@ > void ScInterpreter::ScGetWeekOfYear() > { > RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "sc", "er", "ScInterpreter::ScGetWeekOfYear" ); > if ( MustHaveParamCount( GetByte(), 2 ) ) In ODFF the second parameter to WEEKNUM is optional. ::: sc/source/core/tool/odffmap.cxx @@ -44,4 @@ > { "WORKDAY", "WORKDAY", false, "com.sun.star.sheet.addin.Analysis.getWorkday", "COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.GETWORKDAY" }, > { "YEARFRAC", "YEARFRAC", false, "com.sun.star.sheet.addin.Analysis.getYearfrac", "COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.GETYEARFRAC" }, > { "EDATE", "EDATE", false, "com.sun.star.sheet.addin.Analysis.getEdate", "COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.GETEDATE" }, > - { "WEEKNUM", "WEEKNUM_ADD", false, "com.sun.star.sheet.addin.Analysis.getWeeknum", "COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.GETWEEKNUM" }, Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability. ::: sc/source/filter/oox/formulabase.cxx @@ -629,4 @@ > { "ODDFPRICE", "ODDFPRICE", 462, NOID, 8, 9, V, { RR }, FUNCFLAG_EXTERNAL }, > { "ODDFYIELD", "ODDFYIELD", 463, NOID, 8, 9, V, { RR }, FUNCFLAG_EXTERNAL }, > { "RANDBETWEEN", "RANDBETWEEN", 464, NOID, 2, 2, V, { RR }, FUNCFLAG_VOLATILE | FUNCFLAG_EXTERNAL }, > - { "WEEKNUM", "WEEKNUM", 465, NOID, 1, 2, V, { RR }, FUNCFLAG_EXTERNAL }, Do not remove for proper Excel interoperability. @@ -750,4 @@ > { "GAUSS", 0, NOID, NOID, 1, 1, V, { VR }, FUNCFLAG_MACROCALLODF }, > { "IFNA", 0, NOID, NOID, 2, 2, V, { VR, RO }, FUNCFLAG_MACROCALLODF }, > { "ISFORMULA", 0, NOID, NOID, 1, 1, V, { RO }, FUNCFLAG_MACROCALLODF }, > - { "ISOWEEKNUM", 0, NOID, NOID, 1, 2, V, { VR }, FUNCFLAG_MACROCALLODF }, Do not remove here, Excel does not know ISOWEEKNUM yet. ::: sc/source/ui/src/scfuncs.src @@ +1047,4 @@ > }; > String 5 // Description of Parameter 2 > { > + Text [ en-US ] = "Indicates the first day of the week (1 or 11 = Sunday, 2 or 12 = Monday, 13 - 17 = Tuesday - Saturday. Week 1 is the week that contains January 1. Value 21 or 150 calculate according to ISO8601: first day of week is Monday and week 1 is the week where the first Thursday of the year occurs)." ; Check if this lengthy text can be really displayed in the Function Wizard. I doubt it, especially when taking translations into account that form even longer texts. Longer explanations better go into help text. @@ +1054,5 @@ > + Resource SC_OPCODE_ISOWEEKNUM > + { > + String 1 // Description > + { > + Text [ en-US ] = "Calculates the ISO8601 calender week for the given date. ISO 8601 defines the calendar week as a time interval of seven calendar days starting with a Monday, and the first calendar week of a year as the one that includes the first Thursday of that year." ; Check if this lengthy text can be really displayed in the Function Wizard. I doubt it, especially when taking translations into account that form even longer texts. Longer explanations better go into help text. ::: sc/util/hidother.src @@ -114,4 @@ > hidspecial HID_FUNC_JAHR { HelpID = HID_FUNC_JAHR; }; > hidspecial HID_FUNC_TAGE { HelpID = HID_FUNC_TAGE; }; > hidspecial HID_FUNC_DATEDIF { HelpID = HID_FUNC_DATEDIF; }; > -hidspecial HID_FUNC_KALENDERWOCHE { HelpID = HID_FUNC_KALENDERWOCHE; }; Do not rename HIDs, or if you do adapt also the references under helpcontent2/source/text/scalc/ ::: scaddins/idl/com/sun/star/sheet/addin/XAnalysis.idl @@ -70,5 @@ > - long getWeeknum( > - [in] com::sun::star::beans::XPropertySet xOptions, > - [in] long nStartDate, [in] long nMode ) > - raises( com::sun::star::lang::IllegalArgumentException ); > - Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability. ::: scaddins/source/analysis/analysis.cxx @@ -608,5 @@ > - > - return ( nDate - nFirstInYear + ( ( nMode == 1 )? ( nFirstDayInYear + 1 ) % 7 : nFirstDayInYear ) ) / 7 + 1; > -} > - > - Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability. Instead, implement full ODFF WEEKNUM compliance here. ::: scaddins/source/analysis/analysis.hxx @@ -126,4 @@ > virtual sal_Int32 SAL_CALL getWorkday( constREFXPS&, sal_Int32 nStartDate, sal_Int32 nDays, const ANY& aHDay ) THROWDEF_RTE_IAE; > virtual double SAL_CALL getYearfrac( constREFXPS&, sal_Int32 nStartDate, sal_Int32 nEndDate, const ANY& aMode ) THROWDEF_RTE_IAE; > virtual sal_Int32 SAL_CALL getEdate( constREFXPS&, sal_Int32 nStartDate, sal_Int32 nMonths ) THROWDEF_RTE_IAE; > - virtual sal_Int32 SAL_CALL getWeeknum( constREFXPS&, sal_Int32 nStartDate, sal_Int32 nMode ) THROWDEF_RTE_IAE; Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability. ::: scaddins/source/analysis/analysis.src @@ -159,5 @@ > - { > - Text [ en-US ] = "A number from 1 to 3 that specifies the day with which a week begins"; > - }; > - }; > - Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability. ::: scaddins/source/analysis/analysis_deffuncnames.src @@ -64,5 @@ > - < "KALENDERWOCHE"; >; > - < "WEEKNUM"; >; > - }; > - }; > - Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability. ::: scaddins/source/analysis/analysis_funcnames.src @@ -48,5 @@ > - String ANALYSIS_FUNCNAME_Weeknum > - { > - Text [ en-US ] = "WEEKNUM"; > - }; > - Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability. ::: scaddins/source/analysis/analysishelper.cxx @@ -57,4 @@ > FUNCDATA( Workday, UNIQUE, INTPAR, 3, FDCat_DateTime ), > FUNCDATA( Yearfrac, UNIQUE, INTPAR, 3, FDCat_DateTime ), > FUNCDATA( Edate, UNIQUE, INTPAR, 2, FDCat_DateTime ), > - FUNCDATA( Weeknum, DOUBLE, INTPAR, 2, FDCat_DateTime ), Do not remove the function from the Add-In, we need it to read/write ODF 1.1 and read older documents and for Excel interoperability.
Created attachment 64112 [details] [review] patch sofar, not ready to be committed Attached diff file has all the comments Eike gave in his splinter review processed. -A strange phenomenon occurs: neither formula WEEKNUM nor WEEKNUM_ADD is not recognised is core/sc/qa/unit/ucalc.cxx and when running, UI formula WEEKNUM is not known, but WEEKNUM_ADD is. I grepped what I could and search in opengrok, but I can't find any instance of the text WEEKNUM_ADD in my code. What am I doing wrong? -processing of saved old-style use of WEEKNUM/WEEKNUM_ADD has not yet been put into the code.
Created attachment 64113 [details] [review] patch sofar, not ready to be committed The diff file still contained one error, this diff file builds.
Comment on attachment 64113 [details] [review] patch sofar, not ready to be committed Review of attachment 64113 [details] [review]: ----------------------------------------------------------------- ::: formula/inc/formula/compiler.hrc @@ +356,4 @@ > #define SC_OPCODE_TABLE_OP 362 > #define SC_OPCODE_BETA_DIST 363 > #define SC_OPCODE_BETA_INV 364 > +#define SC_OPCODE_WEEKNUM 365 /* miscellaneous */ As WEEKNUM is no internal function anymore remove the opcode definition here. ::: formula/source/core/resource/core_resource.src @@ +317,4 @@ > String SC_OPCODE_TABLE_OP { Text = "MULTIPLE.OPERATIONS" ; }; > String SC_OPCODE_BETA_DIST { Text = "BETADIST" ; }; > String SC_OPCODE_BETA_INV { Text = "BETAINV" ; }; > + String SC_OPCODE_WEEKNUM { Text = "WEEKNUM" ; }; This does not work, the internal WEEKNUM function is removed in favor of the Add-In function, but defining WEEKNUM here as internal will not look for the Add-In anymore during compile. @@ +651,4 @@ > String SC_OPCODE_TABLE_OP { Text = "TABLE" ; }; > String SC_OPCODE_BETA_DIST { Text = "BETADIST" ; }; > String SC_OPCODE_BETA_INV { Text = "BETAINV" ; }; > + String SC_OPCODE_WEEKNUM { Text = "WEEKNUM" ; }; This does not work, the internal WEEKNUM function is removed in favor of the Add-In function, but defining WEEKNUM here as internal will not look for the Add-In anymore during compile. @@ +1800,2 @@ > { > Text [ en-US ] = "WEEKNUM" ; This does not work, the internal WEEKNUM function is removed in favor of the Add-In function, but defining WEEKNUM here as internal will not look for the Add-In anymore during compile. ::: sc/qa/unit/ucalc.cxx @@ +3354,5 @@ > "TIMEVALUE", > "TODAY", > "WEEKDAY", > +//50950 temporary measure > +// "WEEKNUM", Yes, this needs to be removed. ::: sc/source/core/tool/odffmap.cxx @@ +35,4 @@ > { "WORKDAY", "WORKDAY", false, "com.sun.star.sheet.addin.Analysis.getWorkday", "COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.GETWORKDAY" }, > { "YEARFRAC", "YEARFRAC", false, "com.sun.star.sheet.addin.Analysis.getYearfrac", "COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.GETYEARFRAC" }, > { "EDATE", "EDATE", false, "com.sun.star.sheet.addin.Analysis.getEdate", "COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.GETEDATE" }, > + { "WEEKNUM", "WEEKNUM", false, "com.sun.star.sheet.addin.Analysis.getWeeknum", "COM.SUN.STAR.SHEET.ADDIN.ANALYSIS.GETWEEKNUM" }, Note: renaming WEEKNUM_ADD to WEEKNUM also needs to unset the bDouble flag in the function's data to not have the "_ADD" appended anymore. So in scaddins/source/analysis/analysishelper.cxx const FuncDataBase pFuncDatas[] for Weeknum change DOUBLE to UNIQUE ::: sc/source/filter/oox/formulabase.cxx @@ +666,4 @@ > { "COUNTBLANK", "COUNTBLANK", 347, 347, 1, 1, V, { RO }, 0 }, > { "ISPMT", "ISPMT", 350, 350, 4, 4, V, { VR }, 0 }, > { "DATEDIF", "DATEDIF", 351, 351, 3, 3, V, { VR }, 0 }, > + { "ISOWEEKNUM", "ISOWEEKNUM", 355, 355, 1, 1, V, { VR }, 0 }, Excel doesn't know ISOWEEKNUM, inventing it here does not help ;-) ::: sc/source/ui/src/scfuncs.src @@ +1023,4 @@ > { > String 1 // Description > { > + Text [ en-US ] = "Calculates the ISO8601 calender week for the given date: first day of week is Monday and week 1 is the week where the first Thursday of the year occurs)." ; Just nitpicking: it's ISO 8601 instead of ISO8601 ::: scaddins/source/analysis/analysis.cxx @@ +592,1 @@ > sal_Int32 SAL_CALL AnalysisAddIn::getWeeknum( constREFXPS& xOpt, sal_Int32 nDate, sal_Int32 nMode ) THROWDEF_RTE_IAE As nMode will be optional it now needs to be const ANY& rMode To evaluate the ANY that then should be sal_Int32 nMode = aAnyConv.getInt32( xOpt, rMode, 1 ); @@ +623,5 @@ > + break; > + default : > + //QUESTION: what happens if the optional argument nMode is not entered by user? > + //incorrect argument; we should cope with that properly; right now we'll just return -1 > + return -1; ODFF defines the default of mode if omitted to 1 (see also the ANY evaluation in previous annotation), any value not defined in ODFF should result in an error, i.e. throw lang::IllegalArgumentException() ::: scaddins/source/analysis/analysis.src @@ +146,2 @@ > { > + Text [ en-US ] = "Indicates the first day of the week (1, 11=Sunday, 2, 12=Monday, 13-17=Tuesday-Saturday. 21 or 150 calculate according to ISO8601)." ; Nitpick: ISO 8601
Created attachment 64281 [details] [review] patch sofar, not ready to be committed I corrected as in review of previous attachment; all works fine now, so that I now will be starting on the work to properly interpret old-style documents.
submitted patch via dev list. Bacwards compatibility (reading documents with the old-style functions) is still to be fixed, But I don't know where to put code for that.
Created attachment 70309 [details] patch sofar, not intended for submitting Eike's review of Nov 16 processed, except for the ODF 1.0/1.1 - 1.2 interoperability.
Status is not 'assigned' as the last work on the patch (interoperability with older version) is beyond my possiblities. Either someone else could finish the patch or someone must give ample code pointers and hints so that I can add the needed code.
Adding self to CC if not already on
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.