Last Comment Bug 794585 - Monthly layout: unnecessary additional empty week row printed if last day of month is last day of print range
: Monthly layout: unnecessary additional empty week row printed if last day of ...
: regression
Product: Calendar
Classification: Client Software
Component: Printing (show other bugs)
: Lightning 1.7
: All All
: -- minor (vote)
: 1.9
Assigned To: Stefan Sitter
Depends on:
Blocks: 757902 792061
  Show dependency treegraph
Reported: 2012-09-26 12:41 PDT by Stefan Sitter
Modified: 2012-10-08 10:52 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

screenshot (125.36 KB, image/png)
2012-09-26 12:43 PDT, Stefan Sitter
no flags Details
Fix - v1 (71 bytes, patch)
2012-10-02 02:25 PDT, Philipp Kewisch [:Fallen]
ssitter: review-
Details | Diff | Splinter Review
Fix - v1 (563 bytes, patch)
2012-10-02 04:14 PDT, Philipp Kewisch [:Fallen]
ssitter: review-
Details | Diff | Splinter Review
fix for monthly and weekly print (13.29 KB, patch)
2012-10-04 13:46 PDT, Stefan Sitter
philipp: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
Details | Diff | Splinter Review

Description Stefan Sitter 2012-09-26 12:41:52 PDT
An unnecessary additional empty week is printed in Monthly layout if last day of month is last day of print range. See attached screenshot. The monthly calendar view on the left is correct, the monthly print output on the right contains the unnecessary additional empty week row.
Comment 1 Stefan Sitter 2012-09-26 12:43:24 PDT
Created attachment 665092 [details]
Comment 2 Philipp Kewisch [:Fallen] 2012-10-02 02:25:47 PDT
Created attachment 666909 [details] [diff] [review]
Fix - v1

This should do it, I'd appreciate if you could give it a spin.
Comment 3 Stefan Sitter 2012-10-02 03:00:39 PDT
Comment on attachment 666909 [details] [diff] [review]
Fix - v1

Patch is empty.
Comment 4 Philipp Kewisch [:Fallen] 2012-10-02 04:14:10 PDT
Created attachment 666924 [details] [diff] [review]
Fix - v1

Sorry about that
Comment 5 Stefan Sitter 2012-10-02 05:10:22 PDT
Comment on attachment 666924 [details] [diff] [review]
Fix - v1

From a first test at least two problems:

1) Weekly print layout seems completely broken with the patch, shows only blank page and the following error:

> Error: 'TypeError: parentNode is undefined' when calling method:[calIPrintFormatter::formatToHtml] = NS_ERROR_XPC_JS_THREW_JS_OBJECT
> Source file: chrome://calendar/content/calendar-print-dialog.js Line: 208

2) Monthly print layout testcase fails:

First day of week is Sunday (default). Calendar view is set to month September and shows 6 weeks (26.08.2012 - 06.10.2012).

Without patch: Print output in monthly format shows the very same -> OK.

With patch: Print output doesn't contain the last day of September anymore, shows only 5 weeks (26.08.2012 - 29.09.2012). In addition a second calendar is printed for October that contains 5 more weeks (30.09.2012 - 03.11.2012).
Comment 6 Philipp Kewisch [:Fallen] 2012-10-02 05:35:28 PDT
I don't understand :-( parentNode is the weekContainer, which is a html element on the template. Why would changing the weekEnd (which is evaluated after getting the element) remove the weekContainer!?

I'm sorry, but I'll have to take care of this after the release...
Comment 7 Stefan Sitter 2012-10-02 07:56:19 PDT
Out of curiosity: Why was userWeekStart() and userWeekEnd() introduced for the printing code? Can't we just reuse getStartOfWeek() and getEndOfWeek() from calWeekInfoService?
Comment 8 Stefan Sitter 2012-10-04 11:55:54 PDT
I'm working on it.
Comment 9 Stefan Sitter 2012-10-04 13:46:42 PDT
Created attachment 668155 [details] [diff] [review]
fix for monthly and weekly print

Patch uses the existing functions to get first and last day of the week and the removes the duplicates that were added with Bug 757902.

Patch was tested mostly for Sunday and Monday as first day of week, so far it looks good. 

The TypeError in Comment 5 happened only if Monday was selected as first day of week. I think it was caused by the usage of calIDateTime::startOfWeek that always returns the Sunday of the week. I assume this somehow messed up the created document.

It seems that fixing this error fixed the behavior described in Bug 792061 too.
Comment 10 Philipp Kewisch [:Fallen] 2012-10-08 09:20:13 PDT
Comment on attachment 668155 [details] [diff] [review]
fix for monthly and weekly print

Looks good, I've also done some simple tests and it seems to work. Approval for aurora, if you push this after the merge (which is today, or was today, not sure) then also approval for beta.
Comment 11 Stefan Sitter 2012-10-08 10:51:59 PDT
Pushed to
Comment 12 Stefan Sitter 2012-10-08 10:52:44 PDT
Pushed to

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