319790 - add a way to calculate the weeknumber, given a calIDateTime

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

We need a way to calculate the weeknumber from a given date. The code is currently in dateUtils.js, but that isn't easily accessible from xbl widgets or from possible c++ code. So it should be added to calIDateTime (or maybe we should add a calIDateTimeHelpers.idl for things like this somewhere in the future)

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

this patch should do the trick. Week calculation isn't as straightforward as you might think.
gekacheka, this patch isn't a direct conversion of the code in dateUtils.js. I thought you knew that code, that's why i cc-ed you. But i should have looked at cvs blame first (seems to be mikep's work). Still, feel free to check if the calculation looks reasonable.

Comment 2

[gekacheka]

Quick comments:

There should be some comment reference to the ISO 8601 standard.
(That's the basis for the calculations and corner cases.)

I'm not convinced the calIDateTime library should be dependent on the
calendar preference, or any preference.

[If it must be:
 * need comment to explain how the user's preference is
   expected to 'work' with the week numbering.  For example, is the
   invariant that the first thursday is in week 1?

 * First day of week is usually either Saturday (Islamic locales), Sunday
   (U.S., Jewish), or Monday (European locales), so it should 'work' with
   those, and preferably 'works' with any start day of week.
   http://www-306.ibm.com/software/globalization/topics/locales/calendar_intro.jsp

 * Maybe firstDayOfWeek should be an optional parameter, so it
   doesn't have to be decoded for every date in a loop.  
]

For the purpose of displaying week numbers in calendar views, the week
number generating function should be designed so it is easily
customized in extensions.  For example, a school might number weeks in
a semester (Week Aut12, Week Spr2), a business might number weeks in a
quarter (Q4W5).  Extensions can override javascript functions, but I
don't think they can override C++ methods directly.

One approach might be to define the week number function as a
method in the view base class so it is available to all the grid
views.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #2)
>  * First day of week is usually either Saturday (Islamic locales), Sunday
>    (U.S., Jewish), or Monday (European locales), so it should 'work' with
>    those, and preferably 'works' with any start day of week.

That's the pref that's being read. You can use it to set the first day of the week to whatever you want.

>  * Maybe firstDayOfWeek should be an optional parameter, so it
>    doesn't have to be decoded for every date in a loop.  

I'm not sure how much time decoding it takes. I think it's pretty fast to just get it from the preference service.
> 
> For the purpose of displaying week numbers in calendar views, the week
> number generating function should be designed so it is easily
> customized in extensions.
> 
> One approach might be to define the week number function as a
> method in the view base class so it is available to all the grid
> views.
> 
I don't think that's flexible enough. For example, I also need it for the html export i'm working on. Other might need it somewhere else. Passing in a javascript function to a c++ xpcom component won't work.

We could make it a seperate service, calIWeekNumberCalculator. (or a better name). Extensions can provide their own implementation of it.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Since this is purely presentational, I agree that it doesn't belong on calIDateTime.

(In reply to comment #3)
> 
> We could make it a seperate service, calIWeekNumberCalculator. (or a better
> name). Extensions can provide their own implementation of it.

I like this plan.  

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Although it seems that calIDateTime.idl already has a set of helpers which are likely to require similiar pref-dependencies or parameterization (startOfWeek, end ofWeek, startOfMonth, etc).  Hmmm.  Now I'm no longer sure what the best strategy is.

Comment 6

[Joey Minta]

(In reply to comment #5)
> Although it seems that calIDateTime.idl already has a set of helpers which are
> likely to require similiar pref-dependencies or parameterization (startOfWeek,
> end ofWeek, startOfMonth, etc).  Hmmm.  Now I'm no longer sure what the best
> strategy is.
> 

I've been relying on startOfWeek always returning 'Sunday' in the view code.  Then, the view code gets the weekStartOffset pref-value and adds that to the .day property.  My two cents: keep prefs/parameters out of calIDateTime and instead use them in a helper service.  At some point, we can rename .startOfWeek to something like .previousSunday.  I don't think anything related to startOfMonth can really change.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2

Updates the patch. There is a new interface now. Extensions could overrule the default implemenation, in order to have a different way of naming the weeks.
(The interface isn't used anywhere yet. That would be part 2)

Comment 8

[gekacheka]

Comment on attachment 207263 [details] [diff] [review]
patch v2

calWeekTitleService.js needs a comment referencing the "ISO 8601 standard".
(That's the basis for the correctness of the calculations and corner cases.)

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 207263 [details] [diff] [review]
patch v2

>+    // Calculate what day the first thursday of the year is: 5-firstday
>+    // This might be negative, so add 7, and then use modulo to get something
>+    // between 0 and 6.

This comment isn't really clear. I will modify it to:

Calculate what day the first thursday of the year is.
When the year start with a sunday, it's easy to check that the first
thursday will be the 5th of january. In this case, firstday = 0.
When the year starts on monday (firstday = 1), the first thirsday will be
the 4th, etc. So we get: firstThursday = 5 - firstday.
This might be negative, so add 7, and then use modulo to get a value
between 0 and 6.

>+    var firstThursday = ((5 - firstDay) + 7) % 7;

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 207263 [details] [diff] [review]
patch v2

Attempting to derive the commented formula myself from scratch has led me to think that it's not quite right because there are places were zero-based and one-based numbers are being intermixed.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: hand-derived version of a getFirstThursday function

Here's the function I came up with which I think is correct.  I spent a bunch more time than I wanted to, and while this bug is unfortunate, I don't think it should block 0.1, so I probably won't get time to make another review pass until after that ships.

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 209653 [details]
hand-derived version of a getFirstThursday function

>    var firstThursYearDayMod7 = firstThursWeekDay - startOfYear.weekday;
Filling-in firstThursWeekDay gives me:

 var firstThursYearDayMod7 = 4 - startOfYear.weekday;

This year, the year started on sunday, so startOfYear.weekday is 0 (see http://lxr.mozilla.org/seamonkey/source/calendar/base/public/calIDateTime.idl#158). So now firstThursYearDayMod7 is 4. But when jan 1 is sunday, jan 5 will be thursday. So it seems to me that this calculation will fail.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

> This year, the year started on sunday, so startOfYear.weekday is 0 (see
> http://lxr.mozilla.org/seamonkey/source/calendar/base/public/calIDateTime.idl#158).
> So now firstThursYearDayMod7 is 4. But when jan 1 is sunday, jan 5 will be
> thursday. So it seems to me that this calculation will fail.
> 

The day of the year is also zero-based, not one-based.  So 4 represents the fifth day of the year, i.e. January 5th.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v3

Updated patch, now should be more correct. Uses the code and comments from dmose's file.

Comment 15

[gekacheka]

(In reply to comment #3)
 
> We could make it a seperate service, calIWeekNumberCalculator. (or a better
> name). Extensions can provide their own implementation of it.

Can you point to somewhere that explains how to write an extension that overrides a service?  Or even better, upload an xpi of such an extension for week number, such as the simple week number, (yearDay % 7 + 1). 

Is such an extension removeable/disable-able, or does it overwrite the original implementation or its binding, say in /sunbird/components, so it cannot be removed/disabled?

(If it is not possible, then an alternative might be a service that checks for a preference that contains the source code of the javascript function to use, and if empty or not present, uses the 8601 definition.  Then an extension could simply define the preference on startup and undefine it on shutdown, so removing the extension would prevent it from being defined on next startup.)

Comment 16

[gekacheka]

Comment on attachment 209878 [details] [diff] [review]
patch v3


Found several inaccuracies in getWeekTitle.  Comments below, a suggested replacement will follow in next message, so you don't need to fix the comments.

>+calWeekTitleService.prototype.getWeekTitle =
>+function getWeekTitle(aDateTime) {
>+    /**
>+     * This implementation is based on the ISO 8601 standard. That standard
>+     * defines week one as the first week with at least 4 days. This is
>+     * equal to the week with the first thursday.

This is confusing because it is only true if the first day of the week is monday.  Need to explain ISO8601 also defines the first day of the week is monday.  

>+     * This implementation uses the second definition, because it allows
>+     * the user to set a different startday of the week (sunday instead
>+     * of monay is a common setting). If the first definition would be
>+     * used, all weeks could be off by one for certain years.

Not all days of all weeks, just the week number for some days of the week.  That's a problem if the week number is only calculated for the first day of the week.

>+     */
>+
>+    // Find the first thursday this year.    
>+    var startOfYear = aDateTime.startOfYear;
>+  
>+    // Thursday, expressed as the (zero-based) numerical day of the week 
>+    const firstThursWeekday = 4;

(don't really need 'first' as part of this name, it is the weekday for every thursday.)

>+
>+    // The first day of the year and the first Thursday of the year
>+    // are the same number of days apart, regardless of whether the days
>+    // are counted in relation to years or in relation to weeks.
>+    // We can express this using mod 7 math as follows:
>+    //
>+    // (firstThursYearDay - startOfYearDay) % 7 =
>+    //     firstThursWeekday % 7 - startOfYear.weekday % 7

Not accurate:
* '%' is remainder, not modulus.
* the subtraction also has to be performed modulo 7.

>+    //
>+    // Since startOfYearDay is zero in zero-based math, this simplifies to:
>+    //
>+    // firstThursYearDay % 7 = firstThursWeekday % 7 - startOfYear.weekday % 7
>+    // 
>+    // Furthermore, since the week day numbers are already supplied to us 
>+    // mod 7, no conversion is necessary there:
>+    var firstThursYearDayMod7 = firstThursWeekDay - startOfYear.weekday;
>+
>+    // the subtraction might have caused the result to be negative, so take
>+    // advantage of the fact that 
>+    //
>+    //     a (mod 7) + 7 = a (mod 7)

Not accurate, I think you mean
  (a + 7) mod 7 = a (mod 7)

>+    // 
>+    // to force it positive:
>+    firstThursYearDayMod7 = (firstThursYearDayMod7 + 7) % 7;
>+
>+    // The fact that the first Thursday of the year must happen in the first
>+    // 7 days of the year means that our answer is itself guaranteed to be 
>+    // the same number whether expressed mod 7 or non-modularly, so no 
>+    // conversion is necessary before returning.
>+    // Note that firstThursYearDayMod7 is 0-based, as .yearday requires
>+    
>+    var daysSinceFirstThursday = aDateTime.yearday - firstThursYearDayMod7;
>+
>+    // Calculate the number of days since the start of week one,
>+    // but take the users setting into account.
>+    var prefStartOfWeek;
>+    var prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefBranch);
>+    try {     
>+        format = prefBranch.getIntPref("calendar.week.start");

probably wanted prefStartOfWeek= here

>+    } catch(e) {
>+        // By default, weeks start on monday
>+        prefStartOfWeek = 1;
>+    }
>+
>+    var daysSinceStartOfWeekOne = daysSinceFirstThursday + firstThursWeekday - prefStartOfWeek;
>+
>+    // Special case for the first few days of the year.
>+    if (daysSinceStartOfWeekOne < 0) {
>+        // Same week number as the end of last year.
>+        var endOfLastYear = startOfYear.clone();
>+        endOfLastYear.day = 0;
>+        endOfLastYear.normalize();
>+        return this.getWeekTitle(endOfLastYear);
>+    }
>+
>+    // Round up
>+    var week = Math.ceil(daysSinceStartOfWeekOne / 7);

This erroneously produces week 0 for first day of week one, week 1 for first day of week 2, etc.

>+
>+    // Special case for the last few days of the year.
>+    // Make a correction for when the last week gets week nr 53 when it
>+    // is actually is the week in which the first thursday of next year
>+    // will happen.
>+    // The calculation first checks the number of days until the next
>+    // thursday: firstThursWeekday-weekday. Then it checks if this is the
>+    // next month (which is january of next year.)
>+    if (week == 53 &&
>+        aDateTime.month == 11 &&
>+        (aDateTime.day + (firstThursWeekday - aDateTime.weekday)) > 31) {

This subtraction doesn't take into account what day is the start day of the week.  

>+       week = 1;
>+    }
>+    
>+    return week;
>+};

Comment 17

[gekacheka]

Attachment: Compute week number from Thursday of current week.

Replacement for getWeekTitle.

This simplifies the code by computing the current week's Thursday, and computing the week number from that as  
  Math.floor(thursdayOfWeek.yearDay/7) + 1.  
This approach doesn't need any end-of-year special cases.

To help gain confidence in this approach, here is some test code I used in a JavaScript shell to verify the algorithm; it uses javascript dates.  But see the actual code in the attachment for explanatory comments about what it is doing.

// (TEST CODE TO VERIFY ALGORITHM USING JSDATES)
// week numbers should be same as thursday of week no matter what start day.


const SUN = 0, MON = 1, TUE = 2, WED = 3, THU = 4, FRI = 5, SAT = 6;

function weekNumber(jsDate, startWeekday) {
  if (startWeekday === undefined) startWeekday = MON;
  var daysSinceStartOfWeek = (jsDate.getDay() - startWeekday + 7) % 7; 
  var daysFromStartToThursdayOfWeek = (THU - startWeekday + 7) % 7;
  var thursdayOfWeek = new Date(jsDate);
  thursdayOfWeek.setDate
    (jsDate.getDate() - daysSinceStartOfWeek + daysFromStartToThursdayOfWeek);
  var weekNumber = Math.floor(yearDay(thursdayOfWeek)/7)+1;
  return weekNumber;
}

function yearDay(jsDate) {
  var jan1 = new Date(jsDate.getFullYear(), 0, 1);
  var msDiff = jsDate - jan1;
  return Math.floor(msDiff/(1000*60*60*24));
}

// test: print out weekdays for dates in Javascript-Shell
// verify weeknumber is same as thursday of week no matter what start day.
const WEEKDAYS = ["SU","MO","TU","WE","TH","FR","SA"];
for (var y = 1998; y <= 2005; y++) {
  for (var d = new Date(y, 11, 23); d.getMonth() != 0 || d.getDate() <= 8;
       d.setDate(d.getDate() + 1)) {
    var formattedDate = d.toLocaleFormat("%Y%m%d")+WEEKDAYS[d.getDay()];
    for (var startWeekday = SUN; startWeekday <= SAT; startWeekday++) { 
      var wk = String(weekNumber(d, startWeekday));
      if (wk.length == 1) wk = "0"+wk;
      formattedDate += " s="+WEEKDAYS[startWeekday]+":W"+wk;
    }
    // print is defined by Javascript-Shell
    print(formattedDate);
  }
  print("\n");
}

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 209967 [details]
Compute week number from Thursday of current week.

Could you morph this into a patch?

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #15)
> Can you point to somewhere that explains how to write an extension that
> overrides a service?

You just implement the interface in your extension, using the same contractid. It should get registerd after the default implementation, so it will override the default.
Removing the extension will remove the implementation, and the default will get used again.

> (If it is not possible, then an alternative might be a service that checks for
> a preference that contains the source code of the javascript function to use,

The current impl shows that it's not practical to put the code in a pref string. It's way too long.
If the component registration doesn't work, there are other ways we could use. But only if we really have to. For now, i just want to go with what we have.

Comment 20

[gekacheka]

(In reply to comment #18)
> (From update of attachment 209967 [details] [edit])
> Could you morph this into a patch?
> 
You can just substitute the attachment code for the getWeekTitle function in your code.  (I'm not set up to deal with the rest of the patch.)

(In reply to comment #19)
> (In reply to comment #15)
> > Can you point to somewhere that explains how to write an extension that
> > overrides a service?
> 
> You just implement the interface in your extension, using the same contractid.
> It should get registerd after the default implementation, so it will override
> the default.
> Removing the extension will remove the implementation, and the default will get
> used again.

Sounds plausible ... I'm not clear on how service registration works, so I was looking for a simple example.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v4

Updated patch.
Also includes a fix for the weekview, just because it was so easy :)

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: realt patch v4

Ok, should be the right patch now.

Comment 23

[Joey Minta]

Comment on attachment 210071 [details] [diff] [review]
realt patch v4

The overall algorithm for week-numbering looks pretty good.  Most of my comments are about implementation/documentation  issues.

+                        var weekformatter =
+                            Components.classes["@mozilla.org/calendar/weektitle-service;1"]
+                                      .getService(Components.interfaces.calIWeekTitleService);
Probably should be a const, not a var

+                        var weekTitle = 
+                            props.formatStringFromName('WeekTitle', [weekno], 1);
+                        nameArray.push(weekTitle);
I tend to think the 'Week' label should be included in the formatter.  For people who want to use something like Q1W4, the 'Week' title is unnecessary.  It's conceivable that someone may want to use a different label there altogether.  Are there good arguments for keeping it hardcoded here?

+    // The week number is the number of days since the start of week
+    // one, divided by 7, plus 1 (so the first week is W1, not W0).
"divided by 7" and rounded down.  It's not immediately clear that you're doing integer division in this comment.

+    const MONDAY = 1;
+    var startWeekday = MONDAY; // default to monday per ISO8601 standard.
If I recall, Sunbird/Lightning default to 0 (Sunday) in their preferences.  Having these two defaults differ seems like it is asking for trouble.

+    // To adjust, subtract the number of days to the start of the week, 
+    // and add the number of days to Thursday.  (The total adjustment
+    // is NOT modulo 7, it may be positive or negative.)
+    var thursdayOfWeek = aDateTime.clone();
+    thursdayOfWeek.day =
+        (aDateTime.day - daysSinceStartOfWeek + daysFromStartToThursdayOfWeek);
+    thursdayOfWeek.normalize();
I think this comment here needs to be expanded significantly, since it's where the majority of the work is going on ('adjust' is really vague. Adjust to what?).  Something like: "We're looking for the next Thursday that comes after the start of this week, where the start of the week is no longer always Sunday, but defined by the user's preference.  To do this subtract the number of days to the start of the week, and add the number of days from there to Thursday.  (The total adjustment is NOT modulo 7, it may be positive of negative depending on whether the Thursday for the week is before or after aDateTime.)"

My biggest concern here is that the name of the component is setting us up to have tons of fragmented components for other tasks like this.  I'm specifically thinking of some of the other functions in the dateFormatter in dateUtils.js.  Wouldn't this be better done as an initial start to a full calDateFormatUtils component?  On the one hand, that makes it harder for others to over-ride, but it keeps us from having tons of small services out there that need to be kept track of.  Opinions there?

If you're going to fix the week view, please also fix the multiweek view to use this.  That should be pretty trivial as well.

Comment 24

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: yet another patcg

This patch is my last version. I removed a bunch of comments. I decided i'm not going to write an introduction to basic math operators and date calculation.
This bug is about getting the framework into place. It's a pain to generate the patches, and i'm not going to change other things anymore. Nits about comments, using the code in other places and everything else belongs in a different bug. I really just want the framework in.

Comment 25

[gekacheka]

Comment on attachment 210806 [details] [diff] [review]
yet another patcg


>+    // The yearday number of the thursday this week
>+    var thisWeeksThursday = aDateTime.yearday - sinceStartOfWeek + startToThursday;
>+
>+    // For the first few days of the year, we might still be in week 52.
>+    if (thisWeeksThursday < 0)
>+        thisWeeksThursday += 52*7;

I think this optimization introduces a bug.
Some leap years have a week 53 (year starts and ends on a thursday) but this optimization will produce week 52 instead of week 53 for the first few days of a year after a 53 week year.

Comment 26

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #25)
> I think this optimization introduces a bug.
Let's fix that in a new bug.

Comment 27

[Joey Minta]

Comment on attachment 210806 [details] [diff] [review]
yet another patcg

+    const MONDAY = 1;
+    var startWeekday = MONDAY; // default to monday per ISO8601 standard.
+    try {     
+        var prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
+                                   .getService(Components.interfaces.nsIPrefBranch);
+        startWeekDay = prefBranch.getIntPref("calendar.week.start");
+    } catch (e) {}

I still really want this to be SUNDAY=0 before checkin, because it will cause problems with clean profiles.  We can change it back once preferences are fixed to work properly, but I don't forsee that happening prior to 0.3a2.

r=jminta with that and follow-ups filed for gekacheka's comment and using it in the multiweek view.  Please also get some docs up when you have time about how people can customize this.

Thanks for the perseverance.

Comment 28

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in. bug 326268 and bug 326270 filed.

Comment 29

[Joey Minta]

*** Bug 324841 has been marked as a duplicate of this bug. ***