447996 - Month View doesn't display the full month anymore

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17pre) Gecko/20080724 Calendar/0.9pre

Steps to Reproduce:
1. Start Sunbird with clean profile
2. Switch to Month View and check the view
3. Set first day of the week from Sunday to Monday
4. Switch to Month View and check the view again

Actual Results:
After step 2. the entire month is displayed (e.g. July 2008 view shows 29-Jun to 02-Aug).
After step 4. NOT the entire month is displayed (e.g. July 2008 view shows 30-Jun to 27-Jul). One week is missing.

Comment 1

[Stefan Sitter [:ssitter]]

Regression range: Works in Sunbird 0.9pre (2008072320)
                  Fails in Sunbird 0.9pre (2008072420)

Comment 2

[Berend Cornelius]

Daniel, I debugged into it and found that the attribute "endOfWeek" for the day 31. July" returns 27. July. 
See calWeekInfoService.js, line 143, function "getEndOfWeek()".
called from "setDateRange" in calendar-month-view.xml.

Could you check this?

Comment 3

[Daniel Boelzle [:dbo]]

Berend, I cannot confirm this. Dumping out the variables using toString() gives me:
aDate: 2008/07/31 00:00:00 Europe/Paris isDate=1
endOfWeek: 2008/08/02 00:00:00 Europe/Paris isDate=1

Comment 4

[Andrea Guglielmi]

Hi,
with the build currently installed (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17pre) Gecko/20080731 Calendar/0.9pre) if I select "Week starts on Sunday" month view works (today the first day displayed is 27 July, last 6 September). If I change the option and select "Week starts on Monday" month view doesn't work anymore (first day displayed is 28 July the last one is 24 August).

Comment 5

[Andrea Guglielmi]

I just tried and it works just when "Week starts on Sunday" is selected, doesn't work with any other week day.

Comment 6

[Markus Adrario [:Taraman]]

ALMOST works with Sunday.
The 31st August is not shown, but in my opinion should be included in the Monthview.

Comment 7

[bugzilla_mozilla-ark_fmfm]

Attachment: Month view working fine with Sunday set as beginning of the week

Markus, August 31 is shown for me. I added a screenshot to the attachments.

Comment 8

[bugzilla_mozilla-ark_fmfm]

Comment on attachment 332237 [details]
Month view working fine with Sunday set as beginning of the week

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.17pre) Gecko/20080804 Calendar/0.9pre

Comment 9

[Markus Adrario [:Taraman]]

Oh sh..
I still had Multiweek view selected, which is my workaround right now... hehe.
I can confirm your screenshots.
Thinderbird Portable Version 2.0.0.16 (20080708) / Win XP / Lightning 0.9 Pre

Comment 10

[Stefan Sitter [:ssitter]]

I think this change in attachment 331085 [details] [diff] [review] in Bug 446366 causes it:

+++ base/content/calendar-month-view.xml	24 Jul 2008 12:14:32 -0000
-      this.mStartDate = aStartDate.startOfWeek;
-      this.mEndDate = aEndDate.endOfWeek;
-      this.mStartDate.day += this.mWeekStartOffset;
-      this.mEndDate.day += this.mWeekStartOffset;
+      this.mStartDate = getWeekInfoService().getStartOfWeek(aStartDate)
+      this.mEndDate = getWeekInfoService().getEndOfWeek(aEndDate);

The new function adjustPrefWeekStart() will decrease the date and therefore returns a different date value as the original code used to calculate:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/calendar/base/src/calWeekInfoService.js&rev=1.2&mark=154-155#147

Comment 11

[Stefan Sitter [:ssitter]]

Just tried it, removing the -7 line linked above seems to fix the month view. But after that the week view is broken because it needs the -7 line.

That's the punishment for unifying non-identical code into one function as done in Bug 446366.

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: fix + cleanup work - v1

I am not sure about that Stefan. To me it seems that just the calculation in getStartOfWeek()/getEndOfWeek() is buggy, but the client code consolidation looks OK.

I've fixed that in this patch and did overwork the week title service to be in better shape, corrected some minor stuff/indentations etc.

Since this whole area is quite sensible to changes, I'd like to have some prestesting of the patch, setting qawanted.

Comment 13

[Daniel Boelzle [:dbo]]

Attachment: fix + cleanup work - v1, debitrot

Comment 14

[Daniel Boelzle [:dbo]]

Attachment: isolated fix

Comment 15

[Ilja Sekler]

I've tried the patch https://bugzilla.mozilla.org/attachment.cgi?id=332924 with 
the 1.8 branch on Linux, checkout start: Sa 9. Aug 22:26:47 CEST 2008 and I see a bunch of problems in the Month View a Sunbird build without the patch doesn't have (the first day of week: Monday):

June 2008: an extraneous previous week (2008-04-19 - 2008-04-25) is shown, the view lacks the last week; clicking on the forward arrow in the Month View or "Go" -> "Next Month" switches only the Minimonth to July, not the entire view; "Go" -> "Previous Month" goes to April, the Minimonth shows May.

August 2008: the last week is not shown, the same applies to November 2008.

February 2009: can't switch to March 2009, the Month View shows an unnecessary previous week; the same applies to March 2009.

May, June, July, August, September, October 2009 are OK, November 2009 is broken again.

No errors in the Error Console.

The rest works as advertised.

Comment 16

[Stefan Sitter [:ssitter]]

Comment on attachment 332924 [details] [diff] [review]
fix + cleanup work - v1, debitrot

With the patch applied it seems that there exists more issues as before. I can confirm the observation from comment #15 that some months miss the last week and some show an previous week. A regression is that if I switch to Week or Multiweek View and press [Today] button the header switches from e.g. "August 10 - September 6, 2008" to "10.08.2008 16:50 - 06.09.2008 16:50".

Looking at the months that fails it seems that the issues exists with months that start or end at a day prior to the week start day, e.g. Sunday (0) if the week start is set to Monday (1).

Is it possible to ensure that getStartOfWeek() and getEndOfWeek() work correctly e.g. via unit test?

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

I'm pretty sure its possible to create such unit tests, I've requested in-testsuite on bug 446366 which moved/added the interface.

Comment 18

[Stefan Sitter [:ssitter]]

Ok, I played around with some dump() statements and it seems that the month view already passes the wrong dates to getStartOfWeek() resp. getEndOfWeek()

first day of week is Sunday, June:
===================================
+++ getStartOfWeek date_in  = 2008-6-1  day = 0
--- getStartOfWeek date_out = 2008-6-1  day = 0

+++ getEndOfWeek date_in    = 2008-6-30 day = 1
--- getEndOfWeek date_out   = 2008-7-5  day = 6

first day of week is Sunday, July:
===================================
+++ getStartOfWeek date_in  = 2008-7-1  day = 2
--- getStartOfWeek date_out = 2008-6-29 day = 0

+++ getEndOfWeek date_in    = 2008-7-31 day = 4
--- getEndOfWeek date_out   = 2008-8-2  day = 6

first day of week is Sunday, August:
===================================
+++ getStartOfWeek date_in  = 2008-8-1  day = 5
--- getStartOfWeek date_out = 2008-7-27 day = 0

+++ getEndOfWeek date_in    = 2008-8-31 day = 0
--- getEndOfWeek date_out   = 2008-9-6 day = 6


first day of week is Monday, June:
===================================
+++ getStartOfWeek date_in  = 2008-5-25 day = 0  // WRONG should be 06-01
--- getStartOfWeek date_out = 2008-5-19 day = 1

+++ getEndOfWeek date_in    = 2008-6-30 day = 1
--- getEndOfWeek date_out   = 2008-7-6  day = 0

first day of week is Monday, July:
===================================
+++ getStartOfWeek date_in  = 2008-7-1  day = 2
--- getStartOfWeek date_out = 2008-6-30 day = 1

+++ getEndOfWeek date_in    = 2008-7-31 day = 4
--- getEndOfWeek date_out   = 2008-8-3  day = 0

first day of week is Monday, August:
===================================
+++ getStartOfWeek date_in  = 2008-8-1  day = 5
--- getStartOfWeek date_out = 2008-7-28 day = 1

+++ getEndOfWeek date_in    = 2008-8-24 day = 0  // WRONG should be 08-31
--- getEndOfWeek date_out   = 2008-8-24 day = 0

Comment 19

[Daniel Boelzle [:dbo]]

Attachment: fix + cleanup work - v2

+ cleaned up month view's showDate() code

Comment 20

[Stefan Sitter [:ssitter]]

Comment on attachment 333206 [details] [diff] [review]
fix + cleanup work - v2

Patch seems to work fine. I'd almost give r+ but

>+++ calendar/base/src/calWeekInfoService.js	11 Aug 2008 11:27:20 -0000
>+        const SUNDAY = 0; // xxx todo  comments

What's left to do? What comments?

>+    getStartOfWeek: function(aDate) {
>+        var date = aDate.clone();
>+        var offset = (getPrefSafe("calendar.week.start", 0) - aDate.weekday);
>+        if (offset > 0) {

Maybe we could use SUNDAY too for better readability?

I still see the issue with the week and multiweek header mentioned in Comment #16. 
I don't see it in the nightly build therefore I assume it's caused by this patch.

Comment 21

[Daniel Boelzle [:dbo]]

Attachment: fix + cleanup work - v3

+ another small fix in getStartOfWeek()
+ SUNDAY documentation todo has been a leftover => removed

thanks for the thorough testing!

Comment 22

[Stefan Sitter [:ssitter]]

Comment on attachment 333292 [details] [diff] [review]
fix + cleanup work - v3

I hope everything works now, r=ssitter

Comment 23

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Comment 24

[Andreas Treumann]

Checked with lighning build 2008081403 and sunbird 20080813 -> VERIFIED