351860 - monthly grid unnecessarily appends second month when first day of week is set to Monday

Status: RESOLVED FIXED

(Calendar :: Printing, defect)

Description

[Sebastian Schwieger]

When printing from month view and selecting "Monthly Grid" + "Events in current view" two months are printed, the actual one and the next one. This is probably happening because 1st of october is in the current view. A check with April 2006 confirmed this: Only one month is printed here, *but* this month has 6 weeks now!

1. set first day of the week to Monday
2. go to Month view
3. choose Print, Monthly Grid, events in current view

Results:
September 06: two Months are printed
April 06: Month has 6 weeks

Comment 1

[Matthew (lilmatt) Willis]

Not going to make the 0.3 train.

Comment 2

[Matthew (lilmatt) Willis]

WFM on
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060909 Calendar/0.3a2+

Both September 2006 and April 2006 print just that month.

Can anyone else reproduce?

Comment 3

[Stefan Sitter [:ssitter]]

Confirmed using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060910 Calendar/0.3a2+ Build-Id 2006091003 and clean profile.

Regarding issue 1:
If 'Start of Week' is set to Sunday only September 2006 is printed. OK.
But if 'Start of Week' is set to Monday September + October 2006 is printed.

Regarding issue 2:
If 'Start of Week' is set to Sunday April 2006 is printed with 6 week rows. OK.
But if 'Start of Week' is set to Monday April 2006 is printed with 6 week rows too. But the last row is unnecessary because it contains only dates from May.

Comment 4

[Joey Minta]

Attachment: compare the right days

We weren't comparing the right days here, so we never properly knew when we had fallen out of the desired range.  This patch makes sure we do that computation correctly.

Comment 5

[Matthew (lilmatt) Willis]

Comment on attachment 241933 [details] [diff] [review]
compare the right days

r=lilmatt

Comment 6

[Matthew (lilmatt) Willis]

Comment on attachment 241933 [details] [diff] [review]
compare the right days

r1/2=lilmatt (with dmose's blessing for me to do the r2)

Comment 7

[Joey Minta]

Patch checked in.

Comment 8

[Stefan Sitter [:ssitter]]

Problem still exists in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061122 Calendar/0.4a1 - Build ID: 2006112207.

Test case 1:
1. First day of week is set to Monday.
2. Switch to month view and navigate to January 2007
3. Open print preview, select 'Monthly Grid' layout 
4. Select 'Events in current view'
--> Preview shows January 2007 and February 2007

Test case 2:
1. First day of week is set to Monday.
2. Switch to month view and navigate to January 2007
3. Open print preview, select 'Monthly Grid' layout
4. Select 'Custom date range' from 01-Jan-2007 to 31-Jan-2007
--> Preview shows December 2006 and January 2007

Comment 9

[Stefan Sitter [:ssitter]]

Retest with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061217 Calendar/0.4a1:

Test case 1 and test case 2 from Comment #8 now both show the same but faulty results listed for test case 1. This was improved by bug 363275.

The issue that an unnecessary month is appended if first day of week is set to Monday still exists. In case of April 2007 there is even one unnecessary month prepended and one unnecessary month appended.

Comment 10

[Ferdinand Grassmann]

I have reproduced this with Sunbird 0.6a1 (20070304), too. This is what I did to overcome this:

In calMonthGridPrinter.js, function monthPrint_format, I changed the lines

    var date = start.clone();
    date.day = 1;

    var body = <body/>

to
	end.day -= weekStart;
	end.normalize();

    var date = start.clone();
    if (date.month != end.month) { date.day = end.day; }
    date.normalize();

    var body = <body/>

This fixed the bug on my system.

Comment 11

[Matthew (lilmatt) Willis]

Ferdinand,
Can you either create a diff containing your changes, or if you can't do that, at least attach your calMonthGridPrinter.js file to this bug.

Thanks

Comment 12

[Ferdinand Grassmann]

Sorry,
I have no diff tool at hand right now, and I see that I missed one line. You also have to change:
    while (date.compare(end) <= 0) {
to:
    while (date.compare(end) < 0) {

I will attach my changed calMonthGridPrinter.js after I commited this comment.

Comment 13

[Ferdinand Grassmann]

Attachment: Fixed js file

Comment 14

[Matthew (lilmatt) Willis]

Attachment: diff against "Fixed js file"

Comment 15

[Ferdinand Grassmann]

Attachment: Fixed the fix

Hi,

I found some other bugs in my code; a (hopefully finally) fixed version is attached. Would you be so kind as to diff this (probably best against the "original" js file) again?

Thanks in advance.

Comment 16

[Ferdinand Grassmann]

Attachment: Hopefully the last fix

Hi there,

in this file I fixed another bug I introduced in my code (end calculation was wrong when day of startdate was >= day of enddate). I hope the end calculation should now be correct.

Again, I am sorry that I cannot provide a diff, but just the complete js file.

Comment 17

[Martin Schröder [:mschroeder]]

Attachment: diff against "Hopefully the last fix"

I changed some tabs to spaces.

Comment 18

[Stefan Sitter [:ssitter]]

Comment on attachment 258435 [details] [diff] [review]
diff against "Hopefully the last fix"

I did a quick test with that patch and it looks promising. However, I still noticed the following failures:

First day of the week is set to Monday:

If Apr 2007 is selected in view the print preview shows Mar + Apr 2007
If Jul 2007 is selected in view the print preview shows Jun + Jul 2007

First day of the week is set to Wednesday:

If Apr 2007 is selected in view the print preview shows Mar + Apr 2007
If May 2007 is selected in view the print preview shows Apr + May 2007
If Jul 2007 is selected in view the print preview shows Jun + Jul 2007
If Oct 2007 is selected in view the print preview shows Sep + Oct 2007

Comment 19

[Ferdinand Grassmann]

Attachment: fixed start date calculation

In the attached version, the calculation of the start date has been fixed (I did not change that until now because I hadn't seen these results until you mentioned it...).

Could somebody be so kind as to diff this again?

Comment 20

[Martin Schröder [:mschroeder]]

Attachment: diff against "fixed start date calculation"

Changed some tabs to spaces and removed whitespace.

Comment 21

[Stefan Sitter [:ssitter]]

Comment on attachment 258570 [details] [diff] [review]
diff against "fixed start date calculation"

For the 'Events in current view' option it seems to work fine, during testing no additional month was appended or prefixed.

But this breaks the 'Custom date range' option. For example if First day of week is set to Monday and the date range is set from 26.02.2007 to 04.03.2007 nothing is displayed. This works with the current implementation.

I also have some difficulties to understand what the code does. Please add some short comments that explain the code or use some describing variables names, e.g. it's hard to tell what testBegin, tBWD, lastDoM, rEnd, lastWD, endw are supposed to mean.

r- based on the issues with Custom date range

Comment 22

[Ferdinand Grassmann]

Attachment: Fix: nothing being printed when only first week of month selected

Hi,

obviously I did not look at this situation. I changed the code, tried to change the variable names to some meaningful strings and tried to add some comments - I hope they are somewhat usefull.

Comment 23

[Stefan Sitter [:ssitter]]

Comment on attachment 258953 [details] [diff] [review]
Fix: nothing being printed when only first week of month selected

Now it fails if the Custom date range contains (or the selected events cover) only days from the last week of the month. For example if First day of
week is set to Monday and you select events from 26.03.2007 to 31.03.2007 nothing is displayed.

On the other side it improves the most common workflow 'Events in current view', so maybe we should take the patch now and follow up the remaining issue with a later one?

Comment 24

[Matthew (lilmatt) Willis]

(In reply to comment #23)
I'm not in favour of regressing custom dates just so we don't print an extra month.  I'd like to get it fixed correctly instead.

Comment 25

[Ferdinand Grassmann]

Attachment: Fix: nothing being printed when only parts of last week of month selected.

Hi,

hopefully now all exceptions are caught by the code.

Sorry for the inconvenience of multiple tests.

Comment 26

[Ferdinand Grassmann]

Attachment: Fix: nothing being printed when only last day of month selected.

Sorry,

another bug found (nothing being printed when only last day of month was selected).

Comment 27

[Stefan Sitter [:ssitter]]

Comment on attachment 259072 [details] [diff] [review]
Fix: nothing being printed when only last day of month selected.

This patch works fine. I only want some code styling nits addresses:

>+    // First, we want to check whether or not the start day is in the same week as
>+    // the beginning of the next month. To do this, we take the start date, add seven
>+    // days and substract the "day of week" value (which has to be corrected in case
>+    // we do not start on Sunday).
>+
>+    // We only need to adjust if start and end are in different months. So first we have
>+    // to adjust the dates for comparison, as the provided end date is exlusive,
>+    // i.e. will not be displayed.

Typo: substract -> subtract, exlusive -> exclusive

Please rewrap the comment so that each line is no longer than 80 chars

>+    if (start.compare(realEnd) <= 0)
>+    {
>+      if (start.month != realEnd.month)
>+      {

This file already uses a 4 space indentation. Please keep 4 space indentation for your code.

Please reformat the if-else blocks according to the coding style used in the calendar sources, that is

    if (...) {
        // do something
    } else {
        // do something else
    }

This also aplies to the code below.

>+        var testBegin = start.clone();
>+        var startWeekday = testBegin.weekday;
>+        if (startWeekday < weekStart) startWeekday += 7;

Please add brackets around the if-block and move to separate line.

>+        testBegin.day += 7 + weekStart - startWeekday;
>+        testBegin.normalize();
>+        if (testBegin.compare(maybeNewStart) > 0)
>+        {
>+          start = maybeNewStart;
>+          date = start.clone();
>+        }
>+      }
>+
>+      // Next, we want to check whether or not the end day is in the same week as
>+      // the end of the previous month. So we have to get the "day of week" value for
>+      // the end of the previous month, adjust it if necessary (when start of week is
>+      // not Sunday) and check if the end day is in the same week.

Please rewrap the comment so that each line is no longer than 80 chars

>+      if (start.month != realEnd.month)
>+      {
>+        var lastDayofpreviousMonth = end.clone();
>+        lastDayofpreviousMonth.day = 0;
>+        lastDayofpreviousMonth.normalize();
>+        var lastDayWeekday = lastDayofpreviousMonth.weekday;
>+        if (lastDayWeekday < weekStart) { lastDayWeekday += 7; }
>+        if (date.month != end.month) { date.day = 1; }
>+        if (lastDayWeekday + end.day - 1 < 7 + weekStart) { date.day = end.day; }
>+        date.normalize(); 

Please reformat the if-blocks as noted above.

>+        // Finally, we have to check whether we adjusted the dates too well so that nothing
>+        // is printed. That happens if you print just one week which has the last day of a
>+        // month in it.

Please rewrap the comment so that each line is no longer than 80 chars

>+        if (date.compare(end) >= 0) { date.day = 1; }
>+        date.normalize(); 
>+      } else {
>+        date.day = 1;
>+        date.normalize();
>+      }
>+    } else { date = realEnd.clone(); }         // If start date is after end date, just print empty month

Please reformat the else-blocks as noted above, move the comment inside the block so that each line is no longer than 80 chars

>-    while (date.compare(end) <= 0) {
>+    while (date.compare(end) < 0) { // <=

What does that comment mean? Currently it looks as it should be removed.

This looks goo but I'd like to see another patch that addresses the style nits, therefore r-

Comment 28

[Ferdinand Grassmann]

Attachment: Addressed the styling issue

Stefan,

thank you for your review. I hope I addressed all the styling issues you mentioned so that this could be the last patch. ;-)

Comment 29

[Stefan Sitter [:ssitter]]

Comment on attachment 259107 [details] [diff] [review]
Addressed the styling issue

Style nit: The { of the if-block should be on the same line as the condition. Let's see if lilmatt has more comments before adding a new patch.
r1=ssitter with that fixed.

Comment 30

[Matthew (lilmatt) Willis]

Comment on attachment 259107 [details] [diff] [review]
Addressed the styling issue

>+++ calMonthGridPrinter.js	2007-03-20 18:46:41.000000000 +0100
>     // Play around with aStart and aEnd to determine the minimal number of
>     // months we can show to still technically meet their requirements.  This
>-    // is most useful when someone printed 'Current View' in the month view. If
>-    // we take the aStart and aEnd literally, we'll print 3 months (because of
>-    // the extra days at the start/end), but we should avoid that.
>+    // is most useful when someone printed 'Current View' in the month view.
>+    // If we take the aStart and aEnd literally, we'll print 3 months (because
>+    // of the extra days at the start/end), but we should avoid that.
Since you're not changing the comment here, except for word wrapping at 80 chars, don't bother. Leave it alone. Changing it simply for formatting makes cvs blame less useful.

>@@ -108,45 +108,88 @@
>     maybeNewStart.day = 1;
>     maybeNewStart.month = start.month+1;
>     maybeNewStart.normalize();
>-    var firstDate = maybeNewStart.startOfWeek;
>-    firstDate.day += weekStart;
>-    firstDate.normalize();
>-    if (firstDate.weekday < weekStart) {
>-        // Go back one week to make sure we display this day
>-        firstDate.day -= 7;
>-        firstDate.normalize();
>-    }
>-    if (firstDate.compare(start) != 1) {
>-        start = maybeNewStart;
>-    }
> 
>-    var maybeNewEnd = end.clone();
>-    maybeNewEnd.day = 1;
>-    maybeNewEnd.month = end.month-1;
>-    maybeNewEnd.normalize();
>-
>-    var lastDate = maybeNewEnd.endOfMonth.endOfWeek;
>-    lastDate.day += weekStart;
>-    lastDate.normalize();
>-    if (lastDate.weekday < weekStart) {
>-        // Go back one week so we don't display any extra days
>-        lastDate.day -= 7;
>-        lastDate.normalize();
>-    }
>-    // aEnd is exclusive, so we can add another day and be safe
>-    lastDate.day += 1;
>-    lastDate.normalize();
>-    if (lastDate.compare(end) != -1) {
>-        end = maybeNewEnd;
>-    }
> 
>+    // First, we want to check whether or not the start day is in the same
>+    // week as the beginning of the next month. To do this, we take the start
>+    // date, add seven days and subtract the "day of week" value (which has
>+    // to be corrected in case we do not start on Sunday).
>+
>+    // We only need to adjust if start and end are in different months. So
>+    // first we have to adjust the dates for comparison, as the provided end
>+    // date is exclusive, i.e. will not be displayed.
> 
>     var date = start.clone();
>-    date.day = 1;
>+    var realEnd = end.clone();
>+    realEnd.day -= 1;
>+    realEnd.normalize();
>+
>+    if (start.compare(realEnd) <= 0)
>+    {
Move { to previous line

>+        if (start.month != realEnd.month)
>+        {
Move { to previous line

>+            var testBegin = start.clone();
>+            var startWeekday = testBegin.weekday;
>+            if (startWeekday < weekStart)
>+            {
Move { to previous line

>+                startWeekday += 7;
>+            }
>+            testBegin.day += 7 + weekStart - startWeekday;
>+            testBegin.normalize();
>+            if (testBegin.compare(maybeNewStart) > 0)
>+            {
Move { to previous line

>+                start = maybeNewStart;
>+                date = start.clone();
>+            }
>+        }
>+
>+        // Next, we want to check whether or not the end day is in the same
>+        // week as the end of the previous month. So we have to get the "day
>+        // of week" value for the end of the previous month, adjust it if
>+        // necessary (when start of week is not Sunday) and check if the end
>+        // day is in the same week.
I personally think the comments are better located next to the checks, as a sort of narration as what's going on in each step.

>+
>+        if (start.month != realEnd.month)
>+        {
Move { to previous line. Also, do we also need to check year in case start and end are both in September, but one's in 2006 and one's in 2007?

>+            var lastDayofpreviousMonth = end.clone();
>+            lastDayofpreviousMonth.day = 0;
>+            lastDayofpreviousMonth.normalize();
>+            var lastDayWeekday = lastDayofpreviousMonth.weekday;
Capitalize this variable like so: lastDayOfPreviousMonth

>+            if (lastDayWeekday < weekStart)
>+            {
Move { to previous line

>+                lastDayWeekday += 7;
You need to normalize() here.

>+            }
>+            if (date.month != end.month)
>+            {
Move { to previous line

>+                date.day = 1;
You need to normalize() here.

>+            }
>+            if (lastDayWeekday + end.day - 1 < 7 + weekStart)
>+            {
Move { to previous line.  Also add some ()s to make this more readable.  Do we need to normalize() here so that comparisons don't get goofy when they cross timezone changes?

>+                date.day = end.day;
>+            }
>+            date.normalize(); 
Trailing space on this line ^^^.

>+
>+            // Finally, we have to check whether we adjusted the dates too
>+            // well so that nothing is printed. That happens if you print just
>+            // one week which has the last day of a month in it.
>+
>+            if (date.compare(end) >= 0)
>+            {
Move { to previous line

>+                date.day = 1;
>+            }
>+            date.normalize(); 
Trailing space on this line ^^^.

>+        } else {
>+            date.day = 1;
>+            date.normalize();
>+        }
>+    } else {
>+         // If start date is after end date, just print empty month
>+         date = realEnd.clone();
>+    }
I suspect we can find some common places to insert date.normalize() rather than adding it after each and every date = date + 1, etc.
> 
>     var body = <body/>
> 
>-    while (date.compare(end) <= 0) {
>+    while (date.compare(end) < 0) {
>         var monthName = calGetString("dateFormat", "month." + (date.month +1)+ ".name");
>         monthName += " " + date.year;
>         body.appendChild(


r- 
Let's get the formatting cleaned up, and determine where and how often we need those normalize()s

Comment 31

[Ferdinand Grassmann]

(In reply to comment #30)
> (From update of attachment 259107 [details] [diff] [review])
> >+        // Next, we want to check whether or not the end day is in the same
> >+        // week as the end of the previous month. So we have to get the "day
> >+        // of week" value for the end of the previous month, adjust it if
> >+        // necessary (when start of week is not Sunday) and check if the end
> >+        // day is in the same week.
> I personally think the comments are better located next to the checks, as a
> sort of narration as what's going on in each step.

Well... I just "imitated" the comments that were in the file... But I now tried to change some of the comments and their position.

> 
> >+
> >+        if (start.month != realEnd.month)
> >+        {
> Move { to previous line. Also, do we also need to check year in case start and
> end are both in September, but one's in 2006 and one's in 2007?

You are right - I changed that.

> >+            if (lastDayWeekday < weekStart)
> >+            {
> Move { to previous line
> 
> >+                lastDayWeekday += 7;
> You need to normalize() here.

I don't think so - lastDayWeekday is a "plain" integer, containing a number from 0 to 6 (or from 0 to 13), but it does not represent an actual date. Or do I understand the concept of normalize() in a wrong way?! I thought it only applies to dates...

> 
> >+            }
> >+            if (date.month != end.month)
> >+            {
> Move { to previous line
> 
> >+                date.day = 1;
> You need to normalize() here.
> 

Technically yes, but as we do not use date. in the next if clause, and we do normalize() after that, I think this could be left out.

> >+            }
> >+            if (lastDayWeekday + end.day - 1 < 7 + weekStart)
> >+            {
> Move { to previous line.  Also add some ()s to make this more readable. 

Well... I added some ()s, but I do not know whether it is more readable that way...

> Do we
> need to normalize() here so that comparisons don't get goofy when they cross
> timezone changes?

Well... Again, as we do not deal with "real" dates here, I do not think so - provided that end is normalized "by definition".

> >+        } else {
> >+            date.day = 1;
> >+            date.normalize();
> >+        }
> >+    } else {
> >+         // If start date is after end date, just print empty month
> >+         date = realEnd.clone();
> >+    }
> I suspect we can find some common places to insert date.normalize() rather than
> adding it after each and every date = date + 1, etc.

I really would like that. But I have to problems with that: If you take the normalize() to the end because you know how the code works now, this might pose a problem in the future if the order gets changed or anything else happens. In the end, this would lead to getting one normalize() instead of the lower to. Furthermore, we definitely need to normalize() once (I think), because we do a date.compare before leaving the if clause - normalize() ist probably needed there, isn't it?

> Let's get the formatting cleaned up, and determine where and how often we need
> those normalize()s

OK. I think we at least need it twice (once before the while loop starts, and once before the date.compare statement). But I would rather not delete the bottom two normalize()s and put one in there right before the while loop starts, as this might be misleading - especially because the last line before that is date = realEnd.clone(), which does not need a normalize() after it (I presume - it is done before).

I will attach a new patch.

Comment 32

[Ferdinand Grassmann]

Attachment: Style cleanup

I hope it is correct to request another first review from Stefan?!

Comment 33

[Stefan Sitter [:ssitter]]

Comment on attachment 259239 [details] [diff] [review]
Style cleanup

From quick test the patch looks good. You added some tabs that should be replaced with spaces before checkin.

Since I already give r+ for the previous patch it's ok to carry forward that review flag unless you make major changes to the patch.

Comment 34

[Ferdinand Grassmann]

Attachment: Tabs -> spaces

That is what happens if you use the Midnight Commander inbetween... :-(

Comment 35

[Matthew (lilmatt) Willis]

Not going to make the 0.5 train.

Comment 36

[Matthew (lilmatt) Willis]

Comment on attachment 259250 [details] [diff] [review]
Tabs -> spaces

r=lilmatt

Comment 37

[Matthew (lilmatt) Willis]

Ignore me.  We can take this for 0.5.  It just needs check in.

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED