GNOME Bugzilla – Bug 620125
Calendar calculates the week number wrong
Last modified: 2010-06-20 22:09:17 UTC
In some instances the current implementation of the calendar week number functionality shows the wrong week number. For examples see the testing script attached to bug #603532: https://bugzilla.gnome.org/attachment.cgi?id=154081 I will attach a patch implementing the algorithm found here: http://en.wikipedia.org/wiki/ISO_week_date#Calculating_the_week_number_of_a_given_date
Created attachment 162345 [details] [review] Correctly calculate week numbers for calendar Implements the algorithm specified here: http://en.wikipedia.org/wiki/ISO_week_date#Calculating_the_week_number_of_a_given_date
Mmmmh, glibc indeed gives the same week numbers as your test script. It doesn't use the algorithm mentioned on the wikipedia page though, possibly for good reason: (In reply to comment #1) > Implements the algorithm specified here: > http://en.wikipedia.org/wiki/ISO_week_date#Calculating_the_week_number_of_a_given_date Quote from that page: "See the Discussion page for accurate algorithms. The algorithm below does not work." Can you try to implement one of the algorithms mentioned on the discussion page instead?
Created attachment 162494 [details] [review] Correctly calculate week numbers for calendar - new algorithm Redid the week calculation using the algorithm found here: http://en.wikipedia.org/wiki/Talk:ISO_week_date
Review of attachment 162494 [details] [review]: Looks good overall - some comments, but all are stylistic. The commit message should not only mention the change, but the reason as well, e.g.: Change the algorithm used to calculate week numbers The currently used algorithm returns incorrect results in some corner cases, so replace it with a better one. ::: js/ui/calendar.js @@ +21,3 @@ +function _getCalendarWeekForDate(inDate) { + // Based on the algorithms found here: http://en.wikipedia.org/wiki/Talk:ISO_week_date + let date = new Date(inDate.getFullYear(), inDate.getMonth(), inDate.getDate()); It's not too obvious why you create another date object here, which seems to be a copy of inDate. Apparently you "reset" the time to 00:00:00, but that's not clear from the variable names - I'd propose using "date" as parameter name and midnightDate for the local variable (the latter is only needed when initializing nearestThursday, otherwise using date should be OK) @@ +24,3 @@ + let dayOfWeek = 1 + ((date.getDay() + 6) % 7); // Need to get Monday to be 1 ... Sunday to be 7 + let nearestThursday = new Date(date); + nearestThursday.setDate(nearestThursday.getDate() + (4 - dayOfWeek)); It looks odd (on other days than thursdays of course) to define a variable "nearestThursday" initialized with the current day, then changing its date to something else. Just merge the two lines. @@ -24,3 @@ - let offset = 4 + (7 * Math.floor(sday * (1/5))) - sday; - let firstThursday = new Date(date.getFullYear(), 0, 1 + offset); - let weekOfYear = Math.ceil((date.getTime() - firstThursday.getTime()) / MSECS_IN_WEEK); MSECS_IN_WEEK is no longer needed, so it should be removed above. @@ +29,3 @@ + let diffDate = nearestThursday - jan1st; + let dayNumber = Math.floor(Math.abs(diffDate)/MSECS_IN_DAY); + let weekNumber = Math.floor(dayNumber/7) + 1; Both lines: spaces around '/'
Created attachment 163964 [details] [review] Change the algorithm used to calculate week numbers Sorry for taking so long to tweak the code. I think this new patch should address all your comments.
Comment on attachment 163964 [details] [review] Change the algorithm used to calculate week numbers (In reply to comment #5) > Sorry for taking so long to tweak the code. I think this new patch should > address all your comments. No worries - seems we didn't run into any of those corner cases meanwhile. Patch looks good now, thanks a lot!
Pushed with some minor whitespace changes. Attachment 163964 [details] pushed as ee79579 - Change the algorithm used to calculate week numbers