Closed Bug 351860 Opened 13 years ago Closed 13 years ago

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

Categories

(Calendar :: Printing, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sebo.moz, Assigned: jminta)

Details

Attachments

(3 files, 11 obsolete files)

1.38 KB, patch
mattwillis
: first-review+
mattwillis
: second-review+
Details | Diff | Splinter Review
3.00 KB, patch
ssitter
: first-review-
Details | Diff | Splinter Review
4.39 KB, patch
ferdinand
: first-review+
mattwillis
: second-review+
Details | Diff | Splinter Review
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
Not going to make the 0.3 train.
Target Milestone: Sunbird 0.3 → Sunbird 0.4
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?
Keywords: qawanted
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: qawanted
OS: Windows XP → Windows 2000
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.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #241933 - Flags: second-review?(dmose)
Attachment #241933 - Flags: first-review?
Comment on attachment 241933 [details] [diff] [review]
compare the right days

r=lilmatt
Attachment #241933 - Flags: first-review? → first-review+
Comment on attachment 241933 [details] [diff] [review]
compare the right days

r1/2=lilmatt (with dmose's blessing for me to do the r2)
Attachment #241933 - Flags: second-review?(dmose) → second-review+
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
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
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.
Attached file Fixed js file (obsolete) —
Attached patch diff against "Fixed js file" (obsolete) — Splinter Review
Attachment #257932 - Attachment is obsolete: true
Attached file Fixed the fix (obsolete) —
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.
Attached file Hopefully the last fix (obsolete) —
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.
Attachment #258092 - Attachment is obsolete: true
I changed some tabs to spaces.
Attachment #257992 - Attachment is obsolete: true
Attachment #258280 - Attachment is obsolete: true
Attachment #258435 - Flags: first-review?(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
Attached file fixed start date calculation (obsolete) —
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?
Changed some tabs to spaces and removed whitespace.
Attachment #258435 - Attachment is obsolete: true
Attachment #258568 - Attachment is obsolete: true
Attachment #258570 - Flags: first-review?(ssitter)
Attachment #258435 - Flags: first-review?(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
Attachment #258570 - Flags: first-review?(ssitter) → first-review-
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.
Attachment #258953 - Flags: first-review?(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?
Attachment #258953 - Flags: first-review?(ssitter) → first-review-
(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.
Hi,

hopefully now all exceptions are caught by the code.

Sorry for the inconvenience of multiple tests.
Attachment #258953 - Attachment is obsolete: true
Attachment #259068 - Flags: first-review?(ssitter)
Sorry,

another bug found (nothing being printed when only last day of month was selected).
Attachment #259068 - Attachment is obsolete: true
Attachment #259072 - Flags: first-review?(ssitter)
Attachment #259068 - Flags: first-review?(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-
Attachment #259072 - Flags: first-review?(ssitter) → first-review-
Attached patch Addressed the styling issue (obsolete) — Splinter Review
Stefan,

thank you for your review. I hope I addressed all the styling issues you mentioned so that this could be the last patch. ;-)
Attachment #259072 - Attachment is obsolete: true
Attachment #259107 - Flags: first-review?(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.
Attachment #259107 - Flags: second-review?(lilmatt)
Attachment #259107 - Flags: first-review?(ssitter)
Attachment #259107 - Flags: first-review+
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
Attachment #259107 - Flags: second-review?(lilmatt) → second-review-
(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.
Attached patch Style cleanup (obsolete) — Splinter Review
I hope it is correct to request another first review from Stefan?!
Attachment #259107 - Attachment is obsolete: true
Attachment #259239 - Flags: first-review?(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.
Attachment #259239 - Flags: second-review?(lilmatt)
Attachment #259239 - Flags: first-review?(ssitter)
Attachment #259239 - Flags: first-review+
Attached patch Tabs -> spacesSplinter Review
That is what happens if you use the Midnight Commander inbetween... :-(
Attachment #259239 - Attachment is obsolete: true
Attachment #259250 - Flags: second-review?(lilmatt)
Attachment #259250 - Flags: first-review+
Attachment #259239 - Flags: second-review?(lilmatt)
Not going to make the 0.5 train.
Target Milestone: Sunbird 0.5 → ---
Comment on attachment 259250 [details] [diff] [review]
Tabs -> spaces

r=lilmatt
Attachment #259250 - Flags: second-review?(lilmatt) → second-review+
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
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.