Monthly layout: unnecessary additional empty week row printed if last day of month is last day of print range

RESOLVED FIXED in 1.9

Status

Calendar
Printing
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Stefan Sitter, Assigned: Stefan Sitter)

Tracking

({regression})

Lightning 1.7
regression
Dependency tree / graph

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 665092 [details]
screenshot
Created attachment 666909 [details] [diff] [review]
Fix - v1

This should do it, I'd appreciate if you could give it a spin.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #666909 - Flags: review?(ssitter)
(Assignee)

Comment 3

5 years ago
Comment on attachment 666909 [details] [diff] [review]
Fix - v1

Patch is empty.
Attachment #666909 - Flags: review?(ssitter) → review-
Created attachment 666924 [details] [diff] [review]
Fix - v1

Sorry about that
Attachment #666909 - Attachment is obsolete: true
Attachment #666924 - Flags: review?(ssitter)
(Assignee)

Comment 5

5 years ago
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).
Attachment #666924 - Flags: review?(ssitter) → review-
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...
(Assignee)

Comment 7

5 years ago
Out of curiosity: Why was userWeekStart() and userWeekEnd() introduced for the printing code? Can't we just reuse getStartOfWeek() and getEndOfWeek() from calWeekInfoService?
(Assignee)

Comment 8

5 years ago
I'm working on it.
Assignee: philipp → ssitter
Blocks: 757902
(Assignee)

Comment 9

5 years ago
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.
Attachment #666924 - Attachment is obsolete: true
Attachment #668155 - Flags: review?(philipp)
Attachment #668155 - Flags: approval-calendar-aurora?
(Assignee)

Updated

5 years ago
Blocks: 792061
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.
Attachment #668155 - Flags: review?(philipp)
Attachment #668155 - Flags: review+
Attachment #668155 - Flags: approval-calendar-beta+
Attachment #668155 - Flags: approval-calendar-aurora?
Attachment #668155 - Flags: approval-calendar-aurora+
(Assignee)

Comment 11

5 years ago
Pushed to https://hg.mozilla.org/comm-central/rev/c311d81476a4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0
(Assignee)

Comment 12

5 years ago
Pushed to https://hg.mozilla.org/releases/comm-aurora/rev/1f1c1c48c3b3
Target Milestone: 2.0 → 1.9
You need to log in before you can comment on or make changes to this bug.