Closed Bug 446366 Opened 12 years ago Closed 12 years ago

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

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: rimas, Assigned: berend.cornelius09)

References

Details

Attachments

(4 files, 2 obsolete files)

Attached image screenshot
The header of multiweek view lists wrong days, at least for locales where the week begins on Monday.
OS: Windows XP → All
Hardware: PC → All
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.
Blocks: 444292
So it happens on Lightning, too.
Flags: blocking-calendar0.9?
Summary: Days are offset by one in the header of multiweek view → Header of multiweek view always assumes the week to begin with Sunday, no matter what the actual setting is
Assignee: nobody → Berend.Cornelius
Attached patch patch v. #1 (obsolete) — Splinter Review
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.
Attachment #330583 - Flags: review?(daniel.boelzle)
Attached patch patch v. #2 (obsolete) — Splinter Review
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.
Attachment #330583 - Attachment is obsolete: true
Attachment #330736 - Flags: review?(daniel.boelzle)
Attachment #330583 - Flags: review?(daniel.boelzle)
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
Attachment #330736 - Flags: review?(daniel.boelzle) → review+
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".
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
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. 
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Current win32 hourly builds crash during startup (see e.g. Talkback Incident ID TB48003989E). Adding the new calWeekInfoService.js file seems to fix it.
Attachment #331170 - Flags: review?(philipp)
Attachment #331170 - Flags: review?(philipp) → review+
Comment on attachment 331170 [details] [diff] [review]
[checked in] win32 bustage fix

Bustage fix checked in
Attachment #331170 - Attachment description: win32 bustage fix → [checked in] win32 bustage fix
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.
Attachment #331280 - Flags: review?(daniel.boelzle)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
And thank you, Stefan and Philipp, for reacting so fast with your bustage fix.
Attachment #331280 - Flags: review?(daniel.boelzle) → review+
patch v. #5 checked in on trunk and MOZILLA_1_8_BRANCH
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 447997
Depends on: 447996
Depends on: 448190
Flags: blocking-calendar0.9?
Target Milestone: --- → 0.9
Attachment #331280 - Attachment description: patch v. #5 → [checked in] patch v. #5
Attachment #331085 - Attachment description: patch v. #3 → [checked in] patch v. #3
Attachment #330736 - Attachment is obsolete: true
Checked in lightning build 2008072919 and sunbird 20080729 -> VERIFIED
Status: RESOLVED → VERIFIED
It would be nice to create testcases for the week info service, especially getStartOfWeek/getEndOfWeek.
Flags: in-testsuite?
(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.

You need to log in before you can comment on or make changes to this bug.