368066 - print preview chokes when previewing tasks

Status: VERIFIED FIXED

(Calendar :: Printing, defect)

Description

[Daniel Boelzle [:dbo]]

You can run into printing tasks in the preview e.g.
- checking "Taska In View"
- then selecting tasks
- in the print dialog choose "print Selection"
=> exception with empty preview

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: fixed html export

proposed fix

Comment 2

[Stefan Sitter [:ssitter]]

Isn't that the same issue Michael is working on in Bug 336175?

Comment 3

[Daniel Boelzle [:dbo]]

(In reply to comment #2)
> Isn't that the same issue Michael is working on in Bug 336175?

Yes, and it seems Mickey has spent more time on a bigger rework, also fixing Outlook csv. I will set this bug to DUPLICATE to bug 336175 and let Joey decide on the patch.

Thanks (again) for your good overview, Stefan!

Comment 5

[Daniel Boelzle [:dbo]]

Comment on attachment 252621 [details] [diff] [review]
fixed html export

This patch fixes the foremost js exceptions when tasks come into play. Even though there is an open discussion about sorting tasks/events, I would greatly appreciate this getting in quickly to make the situation a little better.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 252621 [details] [diff] [review]
fixed html export

moving review requests around. It's best not to ask me for first-review. It might take quite long before i have time to do them.

Comment 7

[Matthew (lilmatt) Willis]

Comment on attachment 252621 [details] [diff] [review]
fixed html export

>diff -x CVS -a -U 8 -pN -r /cygdrive/d/pim/mozilla/mozilla_ref/calendar/import-export/calHtmlExport.js mozilla/calendar/import-export/calHtmlExport.js
>--- /cygdrive/d/pim/mozilla/mozilla_ref/calendar/import-export/calHtmlExport.js	2007-01-04 10:37:04.455126000 +0100
>+++ mozilla/calendar/import-export/calHtmlExport.js	2007-01-24 17:36:10.061924800 +0100
>@@ -98,62 +98,82 @@ function html_exportToStream(aStream, aC
>     // in e4x. They have to be escaped, which doesn't improve readability
>     html.head.style = ".vevent {border: 1px solid black; padding: 0px; margin-bottom: 10px;}\n";
>     html.head.style += "div.key {font-style: italic; margin-left: 3px;}\n";
>     html.head.style += "div.value {margin-left: 20px;}\n";
>     html.head.style += "abbr {border: none;}\n";
>     html.head.style += ".summarykey {display: none;}\n";
>     html.head.style += "div.summary {background: lightgray; font-weight: bold; margin: 0px; padding: 3px;}\n";
> 
>+    function getStartDate(item) {
>+        return (item instanceof Components.interfaces.calIEvent ? item.startDate : item.entryDate);
>+    }
>+    function getEndDate(item) {
>+        return (item instanceof Components.interfaces.calIEvent ? item.endDate : item.dueDate);
>+    }
>+
These should really be in calUtils.js by now.

>     // Sort aItems
>-    aItems.sort(function (a,b) { return a.startDate.compare(b.startDate); });
>+    function sortFunc(a, b) {
>+        var start_a = getStartDate(a);
>+        if (!start_a)
>+            return -1;
>+        var start_b = getStartDate(b);
>+        if (!start_b)
>+            return 1;
>+        return start_a.compare(start_b);
>+    }
>+    aItems.sort(sortFunc);
Add curly braces to the two if statements

>-    for each (item in aItems) {
>-        try {
>-            item = item.QueryInterface(Components.interfaces.calIEvent);
>-        } catch(e) {
>-            continue;
>-        }
>+    for (var pos = 0; pos < aItems.length; ++pos) {
>+        var item = aItems[pos];
Why are we changing this from a for each loop to a for (var i=0.... style one?

I'd like to know that before granting r+.

Comment 8

[Daniel Boelzle [:dbo]]

(In reply to comment #7)
> (From update of attachment 252621 [details] [diff] [review])
> >+    function getStartDate(item) {
> >+        return (item instanceof Components.interfaces.calIEvent ? item.startDate : item.entryDate);
> >+    }
> >+    function getEndDate(item) {
> >+        return (item instanceof Components.interfaces.calIEvent ? item.endDate : item.dueDate);
> >+    }
> >+
> These should really be in calUtils.js by now.
Can we handle this more liberally or do I need to submit a patch for that?

> >-    for each (item in aItems) {
> >-        try {
> >-            item = item.QueryInterface(Components.interfaces.calIEvent);
> >-        } catch(e) {
> >-            continue;
> >-        }
> >+    for (var pos = 0; pos < aItems.length; ++pos) {
> >+        var item = aItems[pos];
> Why are we changing this from a for each loop to a for (var i=0.... style one?

Because "for [each] in" doesn't define any order while iterating, see eg <http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Statements:for...in#Description>.

Comment 9

[Matthew (lilmatt) Willis]

(In reply to comment #8)

> Can we handle this more liberally or do I need to submit a patch for that?
Just file a follow up and reference it here.

> > Why are we changing this from a for each loop to a for (var i=0.... style one?
> Because "for [each] in" doesn't define any order while iterating, see eg
> <http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Statements:for...in#Description>.

Sounds good enough to me.  r=lilmatt

Comment 10

[Daniel Boelzle [:dbo]]

Do we need r2 and who's gonna do it?

Comment 11

[Daniel Boelzle [:dbo]]

I suppose mvl's queue is stuck... Maybe Clint can do r2?

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 252621 [details] [diff] [review]
fixed html export

r2=mvl

Comment 13

[Daniel Boelzle [:dbo]]

Attachment: adding getStartDate, getEndDate

Comment 14

[Daniel Boelzle [:dbo]]

Reopening to fix only the foremost problems in this bug. Different sorting order etc + fixing the csv export will still be handled in bug 336175.

Comment 15

[Matthew (lilmatt) Willis]

Comment on attachment 253319 [details] [diff] [review]
adding getStartDate, getEndDate

My only comment would to prepend the function names with "cal" so we don't run into any issues with other apps and stuff.

Comment 16

[Daniel Boelzle [:dbo]]

Comment on attachment 253319 [details] [diff] [review]
adding getStartDate, getEndDate

I will change getStartDate/getEndDate to
calGetStartDate/calGetEndDate when/if checking in.

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 253319 [details] [diff] [review]
adding getStartDate, getEndDate

How does just adding functions fix any bug? I don't see any callers.

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #17)
> (From update of attachment 253319 [details] [diff] [review])
> How does just adding functions fix any bug? I don't see any callers.

It's just a follow-up patch about separating calGetStartDate/calGetEndDate out into calUtils.js proposed by Matt. Please see comment #8 and comment #9.

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 253319 [details] [diff] [review]
adding getStartDate, getEndDate

Oh, now i see it. This is supposed to move the functions. Then I think you can remove the original functions.
Looks good otherwise. r2=mvl

Comment 20

[Daniel Boelzle [:dbo]]

patches checked in.

Comment 22

[Andreas Treumann]

Checked in latest nightly build -> task is fixed and verified.

Comment 23

[Lars Wohlfahrt (thetux)]

VERIFIED Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071031 Lightning/0.8pre (2008020119) Thunderbird/2.0.0.9 ID:2007103104