Closed Bug 361057 Opened 13 years ago Closed 13 years ago

Cannot print WCAP calendars

Categories

(Calendar :: Provider: WCAP, defect)

All
Windows XP
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Lightning 0.5

People

(Reporter: stephan.schaefer, Unassigned)

Details

Attachments

(1 file)

Using the Calendar->Print Calendar... UI in Lightning events from a WCAP calendar cannot be printed. The preview just remains empty without any output to the JS console.
Events from the Home calendar print fine. However, as soon as a WCAP calendar is visible at the same time the events from the Home calendar are missing as well.
Problem is that the current print preview code needs to take care about asynchronous results.
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #246606 - Flags: first-review?(lilmatt)
However I cannot get a print preview for "monthly grid". This seems to be unrelated to the fixed stuff in the patch (because it does not work for the synchronous storage provider either). different bug that is already known?
(In reply to comment #2)
> However I cannot get a print preview for "monthly grid". This seems to be
> unrelated to the fixed stuff in the patch (because it does not work for the
> synchronous storage provider either). different bug that is already known?

I have not seen such problems using storage calendar and monthly print layout during testing, neither with Sunbird or Lightning.
I checked this bug with the latest inhouse build from Daniel.

My test results:

1. I have the same problem with the 'monthly grid' layout. The preview is empty, and also the printed document. This happens with the home clanedar and the wcap caldendar at all operating systems (tested at windows, linux (Suse 10.1), Solaris-Sparc)

2.1 calendar printing at windows

2.1.1 home calendar

Layout 'list' -> preview ok, print ok

Layout 'monthly grid' -> preview empty, print empty

Layout 'weekly planner' -> preview ok, print ok

2.1.2 wcap calendar

layout 'list' -> preview ok, select 'list' in the drop down list box that this entry get the focus, push the print button -> print ok

layout 'list' -> preview ok, push the print button directly, whitout getting focus on the 'list' entry -> error message: 'The page was replached while you trying to print. Please try again.' 

the same happens with the 'weekly planner' layout.

2.2 calendar printing at UNIX (Solaris-Sparc and Suse 10.1)

no differences between home- and wcap calendar.

layout 'list' -> preview ok, select 'list' in the drop down list box that this entry get the focus, push the print button -> error message: 'You cannot print while in print previw'

layout 'list' -> preview ok, push the print button directly, whitout getting focus on the 'list' entry -> error message: 'The page was replached while you trying to print. Please try again.'


the same happens with the 'weekly planner' layout.
After some additional testing I get the 'monthly grid' layout. But only when the calendar view shows the 'month view'. Switching the calendar view to week or day view the 'monthly grid' print preview is empty. 
Comment on attachment 246606 [details] [diff] [review]
receiving dialog settings via callback function

>+++ mozilla/calendar/resources/content/printDialog.js	2006-11-26 14:43:40.891582400 +0100

>+    var tempTitle = document.getElementById("title-field").value;
>+    settings.title = (tempTitle || calGetString("calendar", "Untitled"));
>+    settings.layoutCId = document.getElementById("layout-field").value;
>+    settings.start = null;
>+    settings.end = null;
>+    settings.eventList = null;
>+    
Trailing spaces on this line. ^^

>         case 'custom':
>-            start = jsDateToDateTime(document.getElementById("start-date-picker").value);
>-            end   = jsDateToDateTime(document.getElementById("end-date-picker").value);
>+            settings.start = jsDateToDateTime(
>+                document.getElementById("start-date-picker").value);
>+            settings.end   = jsDateToDateTime(
>+                document.getElementById("end-date-picker").value);
Wrap each of these by moving the jsDateToDateTime( to the second line as well.  It should help readability.

>-    if (!eventList) {
>+    
>+    if (settings.eventList) {
>+        receiverFunc(settings);
>+    }
>+    else {
Prevailing style is } else {

>+        var listener = {
>+            onOperationComplete:
>+            function (aCalendar, aStatus, aOperationType, aId, aDateTime) {
>+                // xxx todo: just swallow errors here?
>+                receiverFunc(settings);
>+            },
>+            onGetResult:
>+            function (aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>+                settings.eventList = settings.eventList.concat(aItems);
>+            }
Please make each of these functions unanonymous.

>+        };
>+        window.opener.getCompositeCalendar().getItems(
>+            Components.interfaces.calICalendar.ITEM_FILTER_TYPE_EVENT | 
Trailing space on this line. ^^

>+            Components.interfaces.calICalendar.ITEM_FILTER_CLASS_OCCURRENCES,
>+            0, settings.start, settings.end, listener);
>     }
> }
> 
> /**
>  * Looks at the selections the user has made (start date, layout, etc.), and
>  * updates the HTML in the iframe accordingly. This is also called when a
>  * dialog UI element has changed, since we'll want to refresh the preview.
>  */
> function refreshHtml()
> {
>-    var settings = getDialogSettings();
>-
>-    // I'd like to use calGetString, but it can't do "formatStringFromName".
>-    var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>-                        .getService(Components.interfaces.nsIStringBundleService);
>-
>-    var props = sbs.createBundle("chrome://calendar/locale/calendar.properties");
>-    document.title = props.formatStringFromName("PrintPreviewWindowTitle",
>-                                                [settings.title], 1);
>-
>-    var printformatter = Components.classes[settings.layoutCId]
>-                                   .createInstance(Components.interfaces.calIPrintFormatter);
>-
>-    // Fail-safe check to not init twice, to prevent leaking files
>-    if (gTempFile) {
>-        gTempFile.remove(false);
>-    }
>-    const nsIFile = Components.interfaces.nsIFile;
>-    var dirService = Components.classes["@mozilla.org/file/directory_service;1"]
>-                               .getService(Components.interfaces.nsIProperties);
>-    gTempFile = dirService.get("TmpD", nsIFile);
>-    gTempFile.append("calendarPrint.html");
>-    gTempFile.createUnique(nsIFile.NORMAL_FILE_TYPE, 0600); // 0600 = -rw-------
>-    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>-                              .getService(Components.interfaces.nsIIOService);
>-    var tempUri = ioService.newFileURI(gTempFile); 
>-
>-    var stream = Components.classes["@mozilla.org/network/file-output-stream;1"]
>-                           .createInstance(Components.interfaces.nsIFileOutputStream);
>-
>-    try {
>-        // 0x2A = PR_TRUNCATE, PR_CREATE_FILE, PR_WRONLY
>-        // 0600 = -rw-------
>-        stream.init(gTempFile, 0x2A, 0600, 0);
>-        printformatter.formatToHtml(stream, settings.start, settings.end,
>-                                    settings.eventList.length,
>-                                    settings.eventList, settings.title);
>-        stream.close();
>-    } catch (e) {
>-        dump("printDialog::refreshHtml:"+e+"\n");
>-    }
>-
>-    document.getElementById("content").contentWindow.location = tempUri.spec;
>+    getDialogSettings(
>+        function(settings) {
Please make this function ^^ unanonymous.

>+            // I'd like to use calGetString,
>+            // but it can't do "formatStringFromName".
How about "calGetString can't do 'formatStringFromName'."

>+            var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                      .getService(Components.interfaces.nsIStringBundleService);
Align the dots please.

>+            var props = sbs.createBundle(
>+                "chrome://calendar/locale/calendar.properties");
Break the line after the = instead.

>+            document.title = props.formatStringFromName(
>+                "PrintPreviewWindowTitle", [settings.title], 1);
Same here. Break the line after the = instead.

>+            
Trailing blank spaces on this line. ^^

>+            var printformatter =
>+                Components.classes[settings.layoutCId]
>+                .createInstance(Components.interfaces.calIPrintFormatter);
Align the dots please.

>+            
Trailing blank spaces on this line. ^^

>+            // Fail-safe check to not init twice, to prevent leaking files
>+            if (gTempFile) {
>+                gTempFile.remove(false);
>+            }
>+            const nsIFile = Components.interfaces.nsIFile;
>+            var dirService =
>+                Components.classes["@mozilla.org/file/directory_service;1"]
>+                .getService(Components.interfaces.nsIProperties);
Align the dots please.

>+            gTempFile = dirService.get("TmpD", nsIFile);
>+            gTempFile.append("calendarPrint.html");
>+            gTempFile.createUnique(
>+                nsIFile.NORMAL_FILE_TYPE, 0600); // 0600 = -rw-------
Don't wrap this line. Just put the comment above this line.

>+            var ioService =
>+                Components.classes["@mozilla.org/network/io-service;1"]
>+                .getService(Components.interfaces.nsIIOService);
Align the dots please.

>+            var tempUri = ioService.newFileURI(gTempFile); 
>+            
Trailing blank spaces on this line. ^^

>+            var stream =
>+                Components.classes["@mozilla.org/network/file-output-stream;1"]
>+                .createInstance(Components.interfaces.nsIFileOutputStream);
Align the dots please.

>+            
Trailing blank spaces on this line. ^^

>+            try {
>+                // 0x2A = PR_TRUNCATE, PR_CREATE_FILE, PR_WRONLY
>+                // 0600 = -rw-------
>+                stream.init(gTempFile, 0x2A, 0600, 0);
>+                printformatter.formatToHtml(
>+                    stream, settings.start, settings.end,
>+                    settings.eventList.length,
>+                    settings.eventList, settings.title);
Align this as follows instead:

                printformatter.formatToHtml(stream,
                                            settings.start,
                                            settings.end,
                                            settings.eventList.length,
                                            settings.eventList,
                                            settings.title);

>+                stream.close();
>+            } catch (e) {
>+                dump("printDialog::refreshHtml:"+e+"\n");
Please add spaces around the + signs for additional readability.

>+            }
>+            
Trailing blank spaces on this line. ^^

>+            document.getElementById("content").contentWindow.location =
>+                tempUri.spec;
>+        } );
> }
Put the ); on a new line indented in 4 spaces.


These are entirely style nits.  r=lilmatt with them fixed.
Attachment #246606 - Flags: first-review?(lilmatt) → first-review+
This patch is entirely black voodoo magic to me. You really need to add a ton of comments (here and in the patch) about why those changes makes printing work on async calendars.

And a readability comment:
> function refreshHtml()
> {
>+    getDialogSettings(
>+        function(settings) {

Make that inner function a top-level function. Then you don't need to reindent everything, making the patch much smaller. And i think it will increase the readability of the outer function.
(In reply to comment #7)
> This patch is entirely black voodoo magic to me. You really need to add a ton
> of comments (here and in the patch) about why those changes makes printing work
> on async calendars.
> 

The core problem with the existing code is that the settings object is returned and evaluated even before (async) providers fire their onOperationComplete. So what this patch fixes is: fire the passed "receiverFunc" in onOperationComplete().
The rest of the patch just takes the existing bits as is (with some simplification, though) and brings it into 80 chars shape (which BTW I would welcome for other files, too).

> And a readability comment:
> > function refreshHtml()
> > {
> >+    getDialogSettings(
> >+        function(settings) {
> 
> Make that inner function a top-level function. Then you don't need to reindent
> everything, making the patch much smaller. And i think it will increase the
> readability of the outer function.

IMO the existing code relates to refreshHtml(), so I would leave it in that scope. The alternative would be a function like refreshHtml_response(settings), leaving an almost empty refreshHtml().
Though, if you insist this has to be split into two top-level functions, I will do so...
(In reply to comment #4)

Though I think the attached patch fixes the foremost problem related to async/wcap providers, I think Andreas has mentioned further valid problems for both storage and wcap provider.

Especially
- Unix printing, comment #4, 2.2 ('The page was replached while you trying to print. Please try again.' with repsect to focus on list box or not
- comment #5 (month view needs to be selected to print monthly grid)
worry me.

Printing experts welcome... Matt?
>+            gTempFile.createUnique(
>+                nsIFile.NORMAL_FILE_TYPE, 0600); // 0600 = -rw-------

If you re-indent, please don't create silly linebreaks like this. the 80char limit is a guide (and an outdated one, if you ask me), not a holy rule. Just put the above on one line. Same goes for a lot of other lines.
The example is extra silly, because the comment should be on the line above, not at the end of the line.
(In reply to comment #10)
> >+            gTempFile.createUnique(
> >+                nsIFile.NORMAL_FILE_TYPE, 0600); // 0600 = -rw-------
> 
> If you re-indent, please don't create silly linebreaks like this. the 80char
> limit is a guide (and an outdated one, if you ask me), not a holy rule. Just
> put the above on one line. Same goes for a lot of other lines.

Despite any wholy rules 80 chars files just read much better on my 80 chars emacs.

> The example is extra silly, because the comment should be on the line above,
> not at the end of the line.

But please let's stop this. I don't want to discuss line breaks and existing comments. I have no time for this.
=> I won't add any line breaks to existing code that I re-indent.
Now i see why i got confused: getDialogSettings() is misnamed: it also gets the events for the given settings. Should we rename it or split it?
(In reply to comment #12)
> Now i see why i got confused: getDialogSettings() is misnamed: it also gets the
> events for the given settings. Should we rename it or split it?
> 

Yes, the code probably benefits from refactoring, but for a short term solution, this was beyond the scope of the fix.
(In reply to comment #13)
> Yes, the code probably benefits from refactoring, but for a short term
> solution, this was beyond the scope of the fix.

Agreed.  The original code that this whole refreshHtml and getDialogSettings business replaced was even worse, calling unknown xpfe and toolkit printing and printsettings functions.  Incremental fixes are still fixes.

If we're replacing the getDialogSettings line to add receiverFunc, feel free to rename it to getEventsAndDialogSettings or something. :)

Andreas' comment #4 looks like it may be related to bug 351957.  In either case, as we spoke about in the meeting, his issues should be split out to separate bugs.
checked in on both HEAD and BRANCH.
Andreas, please verify and split up further bugs, if necessary.
Assignee: daniel.boelzle → andreas.treumann
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Why did this land without second-review?
I have had a further look (on Windows): I can reproduce the "Page has been replaced while printing bug" when entering a different print title.

It is because the title-field's onchange causes refreshHtml(). The thing is that onchange is fired once I hold down left mouse button on the dialog's accept/cancel buttons. For async providers, this is actually too late, because the printing will proceed shortly after, not waiting for the async result creating the file to be printed.
[Side-Note for a successfull print: change the title, then hold down left mouse button on "Print" without actually relasing it, wait for the async response being completed, then release to print => works.]
Another problem I am facing is that the temp file is removed when the dialog is unloaded: can we assure that printing buffers the file? I think this is fragile.

I reopen this bug, because my findings seem to relate to async providers only.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: andreas.treumann → nobody
Status: REOPENED → NEW
(In reply to comment #18)
> I have had a further look (on Windows): I can reproduce the "Page has been
> replaced while printing bug" when entering a different print title.

See Bug 351957
(In reply to comment #17)
> Why did this land without second-review?
> 

Sorry, I wasn't aware that this fix needs a second review. I thought Matt's review was sufficient, as he is the expert in that field.
fixed the async provider stuff here, further fixes continued in bug 351957.
Status: NEW → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.