Closed Bug 368066 Opened 13 years ago Closed 13 years ago

print preview chokes when previewing tasks

Categories

(Calendar :: Printing, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

Attachments

(2 files)

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
proposed fix
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #252621 - Flags: first-review?(lilmatt)
Isn't that the same issue Michael is working on in Bug 336175?
(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!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 336175
Attachment #252621 - Flags: first-review?(lilmatt) → first-review?(jminta)
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.
Attachment #252621 - Flags: first-review?(jminta) → first-review?(mvl)
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.
Attachment #252621 - Flags: second-review?(mvl)
Attachment #252621 - Flags: first-review?(mvl)
Attachment #252621 - Flags: first-review?(lilmatt)
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+.
(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>.
(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

Attachment #252621 - Flags: first-review?(lilmatt) → first-review+
Do we need r2 and who's gonna do it?
I suppose mvl's queue is stuck... Maybe Clint can do r2?
Comment on attachment 252621 [details] [diff] [review]
fixed html export

r2=mvl
Attachment #252621 - Flags: second-review?(mvl) → second-review+
Attachment #253319 - Flags: first-review?(lilmatt)
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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.
Attachment #253319 - Flags: first-review?(lilmatt) → first-review+
Comment on attachment 253319 [details] [diff] [review]
adding getStartDate, getEndDate

I will change getStartDate/getEndDate to
calGetStartDate/calGetEndDate when/if checking in.
Attachment #253319 - Flags: second-review?(mvl)
Comment on attachment 253319 [details] [diff] [review]
adding getStartDate, getEndDate

How does just adding functions fix any bug? I don't see any callers.
(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 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
Attachment #253319 - Flags: second-review?(mvl) → second-review+
patches checked in.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Duplicate of this bug: 374413
Checked in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.