757902 - Weekly and Monthly print layouts don't work, remove e4x usage (cpg fallout)

Status: RESOLVED FIXED

(Calendar :: Printing, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Thunderbird/15.0a1

The "Weekly Planner" and "Monthly Grid" prints layouts don't work. Most probably due to the usage of e4x.

[Exception... "'[JavaScript Error: "can't wrap XML objects" {file: "resource://calendar/modules/calUtils.jsm -> file:///.../calendar-js/calMonthGridPrinter.js" line: 210}]' when calling method: [calIPrintFormatter::formatToHtml]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://calendar/content/calendar-print-dialog.js :: getEventsAndDialogSettings_response :: line 251"  data: yes]

[Exception... "'[JavaScript Error: "can't wrap XML objects" {file: "resource://calendar/modules/calUtils.jsm -> file:///.../calendar-js/calWeekPrinter.js" line: 194}]' when calling method: [calIPrintFormatter::formatToHtml]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://calendar/content/calendar-print-dialog.js :: getEventsAndDialogSettings_response :: line 251"  data: yes]

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

This is basically a rewrite of the print formatters and html exporter, so make sure to review the whole file, not the diff itself :-) It simplifies many things and makes the design changable. I've kept it almost as fugly as it currently is and the HTML is almost a direct output of what the old formatter did, so please excuse the bad html.

I propose we change the design in another bug and I propose we show this some designer to make him weep and help us ;-)

Comment 2

[Matthew Mecca [:mmecca]]

Comment on attachment 627543 [details] [diff] [review]
Fix - v1

I'm getting the following when opening the print dialog:

Error: File error: Not found = NS_ERROR_FILE_NOT_FOUND
Source file: resource://calendar/modules/calXMLUtils.jsm
Line: 140

Looks like it's happening when trying to parse "chrome://calendar/skin/printing/calHtmlExport.html".

Comment 3

[Stefan Sitter [:ssitter]]

Comment on attachment 627543 [details] [diff] [review]
Fix - v1

Could you please provide an updated patch that can be applied to comm-central or comm-aurora for testing? This one fails to apply.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Problem with the patch was me forgetting to hg add jar.mn. This patch got hit by the license header move. I'll update the patch once I get these builds done.

import-export/jar.mn looks like this:

#filter substitution
calendar.jar:
    skin/calendar/printing/calWeekPrinter.html             (calWeekPrinter.html)
    skin/calendar/printing/calMonthGridPrinter.html        (calMonthGridPrinter.html)
    skin/calendar/printing/calHtmlExport.html              (calHtmlExport.html)

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

How is this one?

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

This time with qrefresh

Comment 7

[Stefan Sitter [:ssitter]]

Comment on attachment 632676 [details] [diff] [review]
Fix - v2

I did not look at the patch, I just tried to use it.

Export or import calendar fails:

Error: '[JavaScript Error: "wildmat is not defined" {file: ".../calendar-js/calIcsImportExport.js" line: 19}]' when calling method: [calIExporter::getFileTypes] = NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS
Source file: chrome://calendar/content/import-export.js Line: 247

Print, Monthly layout, Selected events/tasks fails:

Error: Calendar print dialog:refreshHtml: [Exception... "'[JavaScript Error: "startOfMonth is not defined" {file: ".../calendar-js/calMonthGridPrinter.js" line: 86}]' when calling method: [calIPrintFormatter::formatToHtml]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://calendar/content/calendar-print-dialog.js :: getEventsAndDialogSettings_response :: line 204"  data: yes]
Source file: chrome://calendar/content/calendar-print-dialog.js Line: 220

Print, List layout, many warnings:

Warning: Expected identifier for pseudo-class or pseudo-element but found ' '.  Ruleset ignored due to bad selector.
Source file: about:blank Line: 36

Print, any layout, Custom date range: end date can be set before start date

Comment 8

[Matthew Mecca [:mmecca]]

Comment on attachment 632676 [details] [diff] [review]
Fix - v2

Review of attachment 632676 [details] [diff] [review]:
-----------------------------------------------------------------

Overall the patch looks good. In addition to the issues Stefan pointed out:

::: calendar/base/content/dialogs/calendar-print-dialog.js
@@ +117,3 @@
>          settings.end.day += 1;
> +        settings.start.isDate = false;
> +        settings.end.isDate = false;

If there are any all-day events starting on the day after the current view, those are also included, adding an extra month to the printout

::: calendar/base/public/calIDateTimeFormatter.idl
@@ +76,4 @@
>       */
>      AString formatDateTime(in calIDateTime aDate);
>  
> +

There's an extra blank line introduced here

@@ +106,5 @@
>       */
>      AUTF8String formatInterval(in calIDateTime aStartDate,
>                                 in calIDateTime aEndDate);
>  
> +

and here

::: calendar/import-export/calIcsImportExport.js
@@ +15,5 @@
> +    return [{
> +        QueryInterface: XPCOMUtils.generateQI([Components.interfaces.calIFileType]),
> +        defaultExtension: 'ics',
> +        extensionFilter: '*.ics',
> +        description: cal.calGetString("calendar", 'filterIcs', [wildmat])

wildmat is no longer defined here, it could probably just be replaced with ['*.ics']

::: calendar/import-export/calMonthGridPrinter.js
@@ +39,5 @@
> +        // Make sure to create tables from start to end, if passed
> +        if (aStart && aEnd) {
> +            // Make sure the start date is really a date. Also go back the start
> +            // of the month, since we want to show whole months
> +            let startDate = aStart.startOfMonth;

I'd suggest leaving startDate on the same date as aStart here as in

let startDate = aStart.clone();

otherwise ...

@@ +56,5 @@
> +            probeDate.month++;
> +            probeDate.day = 1;
> +            if (probeDate.startOfWeek.compare(startDate) <= 0) {
> +                startDate = probeDate;
> +            }

... this won't work with the current view and an extra previous month could print. I'd suggest instead something like:

if (probeDate.startOfWeek.compare(startDate) <= 0) {
    startDate = probeDate;
} else {
    startDate = startDate.startOfMonth;
}

@@ +75,5 @@
>          }
>  
> +        for each (let item in aItems) {
> +            // TODO get correct date for item
> +            let boxDate = item[cal.calGetStartDateProp(item)] || item[cal.calGetEndDateProp(item)];

Tasks with a due date but no start date will be included in the item results regardless of whether the due date falls within the print range, so I'd suggest putting those in the "No Date" list section, otherwise we could get a lot of unintended extra months in the printout. This would also be consistent with the month view since we don't show tasks in the view unless they have both a start and due date.

@@ +82,5 @@
>  
> +                if (!(boxDateKey in dayTable)) {
> +                    // Doesn't exist, we need to create a new table for it
> +                    let startOfWeek = boxDate.startOfMonth;
> +                    this.setupMonth(document, startOfMonth, dayTable);

This causes the "startOfMonth is not defined" error, it should be

let startOfMonth = boxDate.startOfMonth;

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I need to take care of this until tomorrow :-/ Sorry for leaving this unfixed for so long!

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Matthew Mecca [:mmecca] from comment #8)
> Comment on attachment 632676 [details] [diff] [review]
> Fix - v2
> 
> Review of attachment 632676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall the patch looks good. In addition to the issues Stefan pointed out:
> 
> ::: calendar/base/content/dialogs/calendar-print-dialog.js
> @@ +117,3 @@
> >          settings.end.day += 1;
> > +        settings.start.isDate = false;
> > +        settings.end.isDate = false;
> 
> If there are any all-day events starting on the day after the current view,
> those are also included, adding an extra month to the printout
Could you give me more details on this? I tried adding an event on August 5th (with start of week sunday), going to month view, then print, monthly grid, current view.

I only get the current month displayed, no extra months. Maybe it went away by fixing your remaining comments.


> ... this won't work with the current view and an extra previous month could
> print. I'd suggest instead something like:
> 
> if (probeDate.startOfWeek.compare(startDate) <= 0) {
>     startDate = probeDate;
> } else {
>     startDate = startDate.startOfMonth;
> }
> 

I added this code as suggested, thanks.


> @@ +75,5 @@
> >          }
> >  
> > +        for each (let item in aItems) {
> > +            // TODO get correct date for item
> > +            let boxDate = item[cal.calGetStartDateProp(item)] || item[cal.calGetEndDateProp(item)];
> 
> Tasks with a due date but no start date will be included in the item results
> regardless of whether the due date falls within the print range, so I'd
> suggest putting those in the "No Date" list section, otherwise we could get
> a lot of unintended extra months in the printout. This would also be
> consistent with the month view since we don't show tasks in the view unless
> they have both a start and due date.

Do you by chance know how this was before? I wouldn't want to change the behavior in this bug.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

With the changes as commented above, I get an extra previous month when setting monday as the first day of the week and selecting current view. Investigating.

Comment 12

[Matthew Mecca [:mmecca]]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> > If there are any all-day events starting on the day after the current view,
> > those are also included, adding an extra month to the printout
> Could you give me more details on this? I tried adding an event on August
> 5th (with start of week sunday), going to month view, then print, monthly
> grid, current view.

Looks like it needs to be a repeating all-day event with an occurrence that falls on the day after the current view.


> > Tasks with a due date but no start date will be included in the item results
> > regardless of whether the due date falls within the print range, so I'd
> > suggest putting those in the "No Date" list section, otherwise we could get
> > a lot of unintended extra months in the printout. This would also be
> > consistent with the month view since we don't show tasks in the view unless
> > they have both a start and due date.
> 
> Do you by chance know how this was before? I wouldn't want to change the
> behavior in this bug.

Before the tasks with a due date and without a start date would show up in the month grid on the due date, but only if the due date falls within the selected print date range.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v3

Ok, I had to take a slightly different approach, but this now works regardless of how the user weekstart is set. I've also gotten rid of all the other errors you showed me. I wonder how v2 even worked for me!

Comment 14

[Stefan Sitter [:ssitter]]

Comment on attachment 643172 [details] [diff] [review]
Fix - v3

Problem 1: 
Menu option Tasks in View is enabled. An event and a task is selected in the main calendar view. Open the Print Preview with any print layout, what to print is set to "Selected events/tasks", and Tasks checkbox is not checked.

Error: Task is shown in the print preview. Every tick/untick of the Tasks checkbox will add an additional entry to the print preview, i.e. you get multiple entries for the same task.

Problem 2: 
Layout: Weekly Planner What: Selected events/tasks - fails with:

Error: Calendar print dialog:refreshHtml: [Exception... "'[JavaScript Error: "aStart is null" {file:[...]/calendar-js/calWeekPrinter.js" line: 40}]' when calling method: [calIPrintFormatter::formatToHtml]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://calendar/content/calendar-print-dialog.js :: getEventsAndDialogSettings_response :: line 204"  data: yes]
Source file: chrome://calendar/content/calendar-print-dialog.js
Line: 220

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v4

This patch fixes the issues you've mentioned. I've decided to change the way I fixed bug 561550 to allow unchecking the events and tasks checkboxes to achieve printing empty month views. Please doublecheck that all cases work. The only "strange" case is when chosing "Selected events & tasks" and then unselecting the Events and Tasks checkboxes. This currently results in an empty printout, but I guess thats ok.

Comment 16

[Matthew Mecca [:mmecca]]

Comment on attachment 643782 [details] [diff] [review]
Fix - v4

A few other issues with the month grid:

- Setting the week start to a day other than Sunday causes an extra blank week row to print on some months; for example set the current view to July 2012, the week start to any day between Wed and Sat.

- Tasks with no due date print with the line-through text decoration whether they are completed or not.

Comment 17

[Stefan Sitter [:ssitter]]

Comment on attachment 643782 [details] [diff] [review]
Fix - v4

Behaves OK according to my short test. But I assume you want to remove the cal.ERROR and cal.WARN debug statements before checkin.

It would be good if we could get the fix ASAP to get more 1.7 beta coverage before the final release and fix non-blocking problems later.

Comment 18

[Matthew Mecca [:mmecca]]

Comment on attachment 643782 [details] [diff] [review]
Fix - v4

Review of attachment 643782 [details] [diff] [review]:
-----------------------------------------------------------------

I agree, I think we can take this as-is and take care of the outstanding issues in follow-up bugs. r=mmecca

::: calendar/import-export/calMonthGridPrinter.js
@@ +38,5 @@
> +
> +        // Make sure to create tables from start to end, if passed
> +        if (aStart && aEnd) {
> +            // Make sure the start date is really a date.
> +            let startDate = aStart && aStart.clone();

Is the "aStart && " necessary since we're already in a "if (aStart && aEnd)" block?

@@ +44,5 @@
> +
> +            // Copy end date, which is exclusive. For our calculations, we will
> +            // only be handling dates and the below code is much cleaner with
> +            // the range being inclusive.
> +            let endDate = aEnd && aEnd.clone();

same here.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

comm-central changeset cd3f24c00e4f with all of your comments fixed. Thanks!

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to releases/comm-aurora changeset 54d1f9cc5d1c

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to releases/comm-beta changeset ca6467b8926e

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'm building 1.7b2 with this fix right now, lets hope I see green trees when I wake up :-)

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

For some reason the beta build is missing the html files. I cannot recreate this on my comm-central build :-(

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

ARGH! It was a problem with my transplant. I've added the missing file in releases/comm-beta changeset 8c7b12e9d892 and need to upload a new beta :-(