446366 - Header of multiweek view always assumes the week to begin with Sunday, no matter what the actual setting is

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Rimas Kudelis [use needinfo to ping]]

Attachment: screenshot

The header of multiweek view lists wrong days, at least for locales where the week begins on Monday.

Comment 1

[Stefan Sitter [:ssitter]]

I can confirm this misbehavior using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17pre) Gecko/20080720 Calendar/0.9pre.

This was already reported in bug 444292 comment #38 but was ignored. The displayed week range is always considered to start with Sunday.

Comment 2

[Rimas Kudelis [use needinfo to ping]]

So it happens on Lightning, too.

Comment 3

[Berend Cornelius]

Attachment: patch v. #1

this should fix the problem as far as I could see. Currently the methods "startOfWeek()" and "endOfWeek()" available at a datetime object always return a week day based on Sunday as the start of the week. My introduced functions "getStartOfWeek()" and "getEndOfWeek()" that consider the preference settings "calendar.week.start" are probably not the wisest of all options but for the time they are sufficient IMHO.

Comment 4

[Berend Cornelius]

Attachment: patch v. #2

After I added the first patch I thought that the "WeekTitleService.js is a more appropriate place for "getStartOfWeek()" and "getEndOfWee()" than calUtils is. If this new patch finds the approval of Daniel I would suggest to rename that corresponding interface to "calWeekFormatter" or something like that.

Comment 5

[Daniel Boelzle [:dbo]]

Comment on attachment 330736 [details] [diff] [review]
patch v. #2

>-  // the start/end of the current object's week
>+  // Return the day of the week of the given time. Sunday is 1
Add a trailing point.

>Index: base/public/calIWeekTitleService.idl
We should probably rename this service to calIWeekInfoService.idl (and give it a new uuid).

>+/**
>+ * gets the first day of a week of a passed day under consideration 
Start with uppercase "Gets...".

>+ * of the preference setting "calendar.week.start"
>+ *
>+ * @param aDate     The dateTime to get get the start of the week for
>+ * @return          a dateTime-object denoting the first day of the week
dto

>+ */
>+calIDateTime getStartOfWeek( in calIDateTime dateTime );
>+
>+
>+/**
>+ * gets the last day of a week of a passed day under consideration
dto.

>+ * of the preference setting "calendar.week.start"
>+ *
>+ * @param aDate     The dateTime to get get the last day of the week for
>+ * @return          a dateTime-object denoting the last day of the week
dto

>+ */
>+calIDateTime getEndOfWeek( in calIDateTime dateTime );
>+

>-    var weekFormatter = Components.classes["@mozilla.org/calendar/weektitle-service;1"]
>-                                  .getService(Components.interfaces.calIWeekTitleService);
>+    var weekFormatter = getWeekFormatter();
should call the service abbrev helper getWeekInfoService() then.

r=dbo

Comment 6

[Stefan Sitter [:ssitter]]

Berend, does your patch also fixes the following issue? If the first day of the week is set to Monday the displayed Calendar Weeks are wrong. For example it displays "Calendar Weeks 29-33" but expected is "Calendar Weeks 30-33".

Comment 7

[Berend Cornelius]

Attachment: [checked in] patch v. #3

modified the recent patch and introduced a new Service calIWeekInfoService according to Daniel's suggestion in comment #5
patch v. #3 checked in on trunk and MOZILLA_1_8_BRANCH

->FIXED

Comment 8

[Berend Cornelius]

in reply to comment #6
>Berend, does your patch also fixes the following issue? If the first day of the
>week is set to Monday the displayed Calendar Weeks are wrong. For example it
>displays "Calendar Weeks 29-33" but expected is "Calendar Weeks 30-33".

Yes I verified this. 

Comment 9

[Stefan Sitter [:ssitter]]

Attachment: [checked in] win32 bustage fix

Current win32 hourly builds crash during startup (see e.g. Talkback Incident ID TB48003989E). Adding the new calWeekInfoService.js file seems to fix it.

Comment 10

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 331170 [details] [diff] [review]
[checked in] win32 bustage fix

Bustage fix checked in

Comment 11

[Berend Cornelius]

Attachment: [checked in] patch v. #5

As Andreas found out the week view displays a wrong range (see bug 430382 comment #37. As that is a regression of this issue I add the patch here.

Comment 12

[Berend Cornelius]

And thank you, Stefan and Philipp, for reacting so fast with your bustage fix.

Comment 13

[Berend Cornelius]

patch v. #5 checked in on trunk and MOZILLA_1_8_BRANCH

Comment 14

[Andreas Treumann]

Checked in lightning build 2008072919 and sunbird 20080729 -> VERIFIED

Comment 15

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

It would be nice to create testcases for the week info service, especially getStartOfWeek/getEndOfWeek.

Comment 16

[Martin Schröder [:mschroeder]]

(In reply to comment #15)
> It would be nice to create testcases for the week info service, especially
> getStartOfWeek/getEndOfWeek.

I started writing some unit tests for the weekinfoservice.