275883 - Cannot print calendar on Mozilla Suite, Thunderbird and Sunbird, no print button on Linux

Status: RESOLVED FIXED

(Calendar :: Printing, defect)

Description

[Gregg Reed]

I installed the official Mozilla 1.7.5 with a new profile.  And then installed
calendar_linux041112 extension.  After restarting Mozilla, I am able to open
Calendar and print preview with print buttons available in the window.  If I
upgrade the extension to calendar_linux041217, I can open Calendar but the print
buttons are missing in the preview window.  If I reinstall the older profile, I
will have the print buttons available again.

I repeated this behavior with an optimized Mozilla build where xprint was enabled.

The same calendar_linux041217 extension works fine when installed into Firefox
and Thunderbird, with the print buttons available in the print preview window.

Comment 1

[Matthew (lilmatt) Willis]

*** Bug 277444 has been marked as a duplicate of this bug. ***

Comment 2

[Xavier Vincent]

Print button is also missing in the print preview window using Mozilla 1.7.5
fr-FR + calendar extension 2005011112-cal on Windows 98SE and Windows 2000 (not
tested on Windows XP).

This is rather annoying especially since we're using it for our company's plannings.

OS field should be changed to "All" and bug severity should be changed to "Major".

Comment 3

[Guillaume Perrin]

Print button is also missing using Mozilla 1.7.5 fr-FR with Calendar Extension
2005011112-cal on Windows XP. 

Comment 4

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

*** Bug 281186 has been marked as a duplicate of this bug. ***

Comment 5

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

*** Bug 277938 has been marked as a duplicate of this bug. ***

Comment 6

[Simon Paquet [:sipaq]]

*** Bug 281368 has been marked as a duplicate of this bug. ***

Comment 7

[Simon Paquet [:sipaq]]

*** Bug 278252 has been marked as a duplicate of this bug. ***

Comment 8

[Simon Paquet [:sipaq]]

*** Bug 286792 has been marked as a duplicate of this bug. ***

Comment 9

[EJP]

Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.7.5) Gecko/20041217 + Cal2005011112
I'm seconding Comment#2, unable to print with this combination of
Moz175+Cal2005011112 on all systems in office, this should be major and blocking
any future versions.  We were printing infrequently and didn't really notice at
what point it disappeared, it must have worked in 1.7+20040622.

Comment 10

[Victor Engmark]

(In reply to comment #9)

> I'm seconding Comment#2, unable to print with this combination of
> Moz175+Cal2005011112 on all systems in office, this should be major and
> blocking any future versions.

Ditto, on Sunbird 0.3a2 on Linux.

Comment 11

[Stefan Sitter [:ssitter]]

(In reply to comment #10)
> Ditto, on Sunbird 0.3a2 on Linux.

Hi Victor, what is the specific Sunbird problem? Maybe you are just seeing Bug 325137?

Comment 12

[Victor Engmark]

(In reply to comment #11)
> Hi Victor, what is the specific Sunbird problem? Maybe you are just seeing Bug
> 325137?

Nope, I just don't see any print button in the print preview.

Comment 13

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove

Comment 14

[Matthew (lilmatt) Willis]

(In reply to comment #12)
> Nope, I just don't see any print button in the print preview.

I'm seeing this on Ubuntu. I'm guessing Sunbird thinks "the underlying gfx code does not support multiple devicecontext to be used concurrently (e.g. printing and printpreview at the same time" as described by

http://lxr.mozilla.org/mozilla/source/toolkit/components/printing/content/printPreviewBindings.xml#171


...so printing should happen after you verify the preview looks as you think it should and then you click "Close...".

I say "should" because when you click "Close..." the following error fires:

Error: invalid 'in' operand getBrowser()
Source File: chrome://global/content/printUtils.js
Line 227

We should either add ourselves to the ifdef at line 268 or change the ifdef to ifndef MOZ_PHOENIX.

What this all doesn't address is that when the Print Preview toolbar hides the Print button, it leaves no way to Print. The toolbar shouldn't be expected to know if it's been invoked as a result of "File > Print Preview" as it is on Firefox or "File > Print" as it is on Sunbird.

My suggested solution is to add "Print Preview" to the File menu (except on OSX), and make "Print" bring up the printer selection box, not a preview. This is what Firefox does.

Thoughts?

Comment 15

[Matthew (lilmatt) Willis]

Attachment: in progress patch

It's 1:30am and I don't want to do something dumb and lose this patch so I'm attaching it.

It adds "Print Preview" to non-Mac platforms, and removes the interstitial preview step when you select "Print".

"Print" should now ask you what you want to print, and then immediately take you to the print dialog, rather than showing it to you first.

If any interested folks would like to try this out and see if it works for them, please do.

The function names are now a little screwy because they include "Preview" when they may in fact be just straight up printing, but this patch seemed to touch the smallest amount of stuff possible.

I'm asking for jminta's review more to make sure I'm not on crack and this looks like the right approach. NOT asking for it to land this in the tree as is.

Comment 16

[Joey Minta]

Comment on attachment 230232 [details] [diff] [review]
in progress patch

The one thing I don't like about this approach is that the workflow for printing becomes:
app->preview->app->print, which seems painful and unintuitive.  The user almost never just wants to print-preview, they want to print, and previewing is a way of knowing what they're going to print.

From a usability perspective, it seems the right fix is to overlay the print-preview window to include a print button.  However, I don't know if this is possible to do from a technical standpoint.  So, now it's my turn to ask if I'm on crack.

Comment 17

[Matthew (lilmatt) Willis]

(In reply to comment #16)
> (From update of attachment 230232 [details] [diff] [review] [edit])
> The one thing I don't like about this approach is that the workflow for
> printing becomes:
> app->preview->app->print, which seems painful and unintuitive.  The user almost
> never just wants to print-preview, they want to print, and previewing is a way
> of knowing what they're going to print.

Actually the two workflows I see are:

app -> select what to preview -> preview -> app -> select what to print -> print

and 

app -> select what to print -> print

> From a usability perspective, it seems the right fix is to overlay the
> print-preview window to include a print button.  However, I don't know if this
> is possible to do from a technical standpoint.  So, now it's my turn to ask if
> I'm on crack.

You are. Please put the pipe down. :)

We are doing such an overlay already. The printPreviewToolbar already contains such a button. It is just hidden when Mozilla "senses" that the OS can't handle multiple device contexts for printing, which apparently is the case for many Linux distros.

This may be oversimplifying, but it appears that if you don't have xprint, we (toolkit) hide the Print button.

Again, see
http://lxr.mozilla.org/mozilla/source/toolkit/components/printing/content/printPreviewBindings.xml#171

Comment 18

[Matthew (lilmatt) Willis]

Outcome from iChat videoconference with jminta:

Flow should be:

File > Print > Pick what to print > Preview (opens modal) or Print (default)

Hide Preview for Macs.

I'll work on making that happen later.

Comment 19

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #17)
> We are doing such an overlay already. The printPreviewToolbar already contains
> such a button. It is just hidden when Mozilla "senses" that the OS can't handle
> multiple device contexts for printing, which apparently is the case for many
> Linux distros.

That has nothing to do with the Linux distros. The OS has IMHO nothing to do with it but the mozilla postscript implementation for Unix/Linux is not able to hold more than one postscript object. With switching to cairo-gtk2 all will be different again.

Comment 20

[Matthew (lilmatt) Willis]

Attachment: first cut at fixing printing

This shows you what you're going to be printing before you print.
It also makes printing work on Mac.

It's not complete, in that the preview pane is small, but it's good to get started with.

Comment 21

[Joey Minta]

Comment on attachment 232965 [details] [diff] [review]
first cut at fixing printing

+    var currentView = window.opener.document.getElementById("view-deck").selectedPanel
+    var startDate = currentView.startDay;
+    var endDate = currentView.endDay;
Please use currentView() rather than getElementById, in order to remain sunbird/lightning agnostic.
Also, startDate and endDate vars aren't used for anything here.  i'd say just use currentView.startDay.jsDate where you need it.

+            settings.end = end;
+            //printInitWindow(listener.mEventArray, start, end, bDoPreview);
Don't comment out lines.  just remove.  And 'b' isn't a well-defined moz-prefix.  (I know it's well defined in other styles, but still...)

+function refreshHtml(aTitle,aLayoutCId,aEventList,aStart,aEnd) {
Spaces between arguments please.

+  var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
+                      .getService(Components.interfaces.nsIStringBundleService);
+
+  var props = sbs.createBundle("chrome://calendar/locale/calendar.properties");
calGetString!!!

+  <vbox id="preview-pane">
+
+    <div xmlns="http://www.w3.org/1999/xhtml">
+      <iframe src="about:blank"
+              id="content"
+              style="min-width:400px; min-height:550px; border: 1px grey;"/>
+    </div>
+  </vbox>
This doesn't feel right at all.  Print-preview is always its own window.  We should continue to do that.  We should just offer a way to open that window from the print-settings dialog.

Comment 22

[Matthew (lilmatt) Willis]

Attachment: Screenshot of "first cut"

Comment 23

[Matthew (lilmatt) Willis]

The first cut patch allowed me to print on both Mac and Linux-without-xprint.

I've attached a screenshot. 

Once I've got ui-review, I'll fix jminta's comments and submit again.

Comment 24

[Joey Minta]

As I understand it, this bug is going to fix the Mac bug as well. -> blocking0.3+

Comment 25

[Matthew (lilmatt) Willis]

Attachment: Patch attempting to use canvas to scale the preview.

This doesn't work correctly, and is here more so I don't lose it, and so I can reference it to others when discussing the problem.

It's diff -w, so ignore whitespace issues.

Comment 26

[Matthew (lilmatt) Willis]

Attachment: Second cut (no canvas. this has a full size preview)

jminta's comments were integrated into this patch.  I've cleaned up a lot, and removed the "Print Preview" button since we're (sort of) previewing stuff as it is, at least enough to see that you're getting the events you want.

The preview pane should update each time you change an element.

Comment 27

[Matthew (lilmatt) Willis]

Attachment: Screenshot of "second cut"

Comment 28

[Stefan Sitter [:ssitter]]

(In reply to comment #27)
> Screenshot of "second cut"

Is there still the real print preview available? 
I mean the window where you can check if the margins are ok, if the calendar fits on paper or if you have to change scale factor or orientation, how many pages you are going to print, etc.?

Comment 29

[Matthew (lilmatt) Willis]

Attachment: diff -w version of "Second cut" patch

Due to the big whitespace change, here's a diff -w of the patch for easier reviewing

Comment 30

[Matthew (lilmatt) Willis]

(In reply to comment #28)
> (In reply to comment #27)
> > Screenshot of "second cut"
> 
> Is there still the real print preview available? 
> I mean the window where you can check if the margins are ok, if the calendar
> fits on paper or if you have to change scale factor or orientation, how many
> pages you are going to print, etc.?

It's not available in this patch. It would be preferable to make it available, or make this preview pane truly a preview. I could call PrintUtils.printPreview after each HTML regeneration, so that the toolbar shows up on platforms that it works for, but I've run into repeated "Please wait for the page to finish loading" errors (even when using event listeners) when doing that.

Comment 31

[Matthew (lilmatt) Willis]

Updating whiteboard.

This is now waiting for cal-ui-review.

Comment 32

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

The screenshot here shows a window where the order of action isn't left-to-right. Also, the preview parts is pretty narrow. So we would like to suggest a different layout:

,-----------------------------------------------------------------------------.
|                                   ,-Range to print------------------------. |
| Title  [                       ]  | (o) Events in current view            | |
|                                   | ( ) Selected events                   | |
| Layout [List                [v]]  | ( ) Custom:                           | |
|                                   |   From [01/01/01[v]] To [02/03/04[v]] | |
|                                   `---------------------------------------' |
|                                                                             |
| ,-Preview-----------------------------------------------------------------. |
| |                                                                        ^| |
| |                                                                        #| |
| |                                                                        #| |
| |                                                                        #| |
| |                                                                         | |
| |                                                                         | |
| |                                                                         | |
| |                                                                         | |
| |                                                                         | |
| |                                                                        v| |
| |<###########################################                           > | |
| `-------------------------------------------------------------------------' |
|                                                                             | 
|                                                        [ Cancel ] [ Print ] |
`-----------------------------------------------------------------------------'

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Please post a screenshot along the lines of the ASCII art mvl added, and re-request cal-ui-review at that point.

Comment 34

[Matthew (lilmatt) Willis]

Attachment: Screenshot of suggested layout

Comment 35

[Matthew (lilmatt) Willis]

(In reply to comment #33)
> Please post a screenshot along the lines of the ASCII art mvl added, and
> re-request cal-ui-review at that point.

Attached and rerequesting ui-review.
I'll post xul/js once we have ui+

Comment 36

[Matthew (lilmatt) Willis]

Attachment: Revision of "Print" button

Fixed button position per mvl in IRC

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

OK, so after more discussion with mvl, we're thinking that the way to go is the original proposal (attachment 233107 [details]) flipped left to right, with a bigger preview pane, and with the settings pane made smaller by putting From (date) above To (date).  Possibly/probably with extra border love where appropriate (at the very least around the preview pane).  A screenshot of that would be dandy.

Comment 38

[Matthew (lilmatt) Willis]

Attachment: Screenshot of original layout, but reversed.

Submitting for ui-review

Comment 39

[Matthew (lilmatt) Willis]

Attachment: Month layout, reversed.

Month printout, reversed, per mvl

Comment 40

[Matthew (lilmatt) Willis]

Attachment: Screenshot: Adjustments per dmose via IRC

Requesting ui-review

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

ui-review+ on the screenshot linked to from comment 40.

Comment 42

[Matthew (lilmatt) Willis]

Comment on attachment 233106 [details] [diff] [review]
Second cut (no canvas. this has a full size preview)

removing review

Comment 43

[Matthew (lilmatt) Willis]

Attachment: patch corresponding to approved ui

Comment 44

[Matthew (lilmatt) Willis]

Attachment: patch corresponding to approved ui - w/ title fix

I'm reattaching this with one minor fix. 
The previous patch regressed bug 347790 (Always printing HTMLTitle).

Comment 45

[Stefan Sitter [:ssitter]]

Attachment: Screenshot on Windows 2000

Comment 46

[Stefan Sitter [:ssitter]]

Comment on attachment 234593 [details] [diff] [review]
patch corresponding to approved ui - w/ title fix

>--- calendar/resources/content/calPrintEngine.css 4 Jun 2005 13:12:34 1.3
>+++ /dev/null	1 Jan 1970 00:00:00 -0000
>--- calendar/resources/content/calPrintEngine.js 18 Mar 2006 13:10:56 1.18
>+++ /dev/null	1 Jan 1970 00:00:00 -0000
>--- calendar/resources/content/calPrintEngine.xul 1 Apr 2006 14:38:45 1.6
>+++ /dev/null	1 Jan 1970 00:00:00 -0000

Should we keep this files around? Maybe we need them again when we are going to remove this workaround and put back a real print preview?

>--- calendar/resources/content/printDialog.js	26 Jul 2006 23:01:12 1.17
>+++ calendar/resources/content/printDialog.js	19 Aug 2006 15:29:15 
>@@ -21,102 +21,102 @@
>+    refreshHtml(calGetString("calendar", "Untitled"),
>+                document.getElementById("layout-field").value,

Why not use settings.title and settings.layoutCId here? 

>+                settings.eventList,
>+                settings.start,
>+                settings.end)

Missing ';'

>     var start;
>     var end;

start/end already declared in line 126/127 (below "var filter = ...")

>+        settings.title = document.getElementById("title-field").value;
>+        if (settings.title == "") {
>+            settings.title = calGetString("calendar", "Untitled");
>+        }

You should use "===" to compare with "" or you could use title = title || calGetString(...);

Is it possible to do this common assignment once instead of three times on different places?

>+            settings.end = end;
>+            return settings;
>         },

>+        settings.end = null;
>+        window.close;
>+    }

>@@ -144,34 +144,124 @@
>+    settings.end = end;
>+    return settings;

Should this be "window.close();"?

Can you explain to me why we return settings two times and call window.close one time?

What is the difference between the normal return and the return inside onOperationComplete()?

>+function onDialogChange()
>+{
>+    var settings = getDialogSettings();
>+
>+    refreshHtml(settings.title,
>+                settings.layoutCId,
>+                settings.eventList,
>+                settings.start,
>+                settings.end)

Missing ';' again

>+function refreshHtml(aTitle, aLayoutCId, aEventList, aStart, aEnd)
>+{
>+    // I'd like to use calGetString, but it can't do "formatStringFromName".

Maybe add a new helper method? Might be useful in other places too.

Not in patch but: there are still calls to getElementById("view-deck"). Should be currentView() to make it usable in Lightning too.

--- calendar/resources/content/calendar.js	14 Aug 2006 16:58:33 1.237
+++ calendar/resources/content/calendar.js	19 Aug 2006 15:29:15 
@@ -389,18 +389,18 @@
 
 function calPrint()
 {
-    window.openDialog("chrome://calendar/content/printDialog.xul",
-                      "printdialog","chrome");
+    window.openDialog("chrome://calendar/content/printDialog.xul", "Print",
+                      "centerscreen,chrome,resizeable=yes");
 }

On Windows the dialog is fixed size with this patch. Just use "resizable".

After fixing this the preview iframe does not expand if the dialog is resized. Missing flex? See screenshot.

--- calendar/resources/content/printDialog.xul	8 Aug 2006 12:11:53 1.14
+++ calendar/resources/content/printDialog.xul	19 Aug 2006 15:29:15 
@@ -49,75 +50,104 @@
 <dialog
    id="calendar-new-printwindow"
    title="&calendar.print.window.title;"
    onload="loadCalendarPrintDialog()"
+   onunload="unloadCalendarPrintDialog()"
    buttons="accept,cancel"
-   ondialogaccept="printCalendar()"
+   buttonlabelaccept="&calendar.print.button.label;"
+   buttonaccesskeyaccept="&calendar.print.accesskey;"
+   defaultButton="accept"
+   ondialogaccept="PrintUtils.print();"
    ondialogcancel="return true;"
    persist="screenX screenY"

I would like to use 'persist="screenX screenY height width"' here.

Also there is a second border (splitter?) left of the iframe that looks unnecessary because there already is a border defined.

Comment 47

[Matthew (lilmatt) Willis]

Attachment: Updates per ssitter

(In reply to comment #46)
> >--- calendar/resources/content/calPrintEngine.css 4 Jun 2005 13:12:34 1.3
> >--- calendar/resources/content/calPrintEngine.js 18 Mar 2006 13:10:56 1.18
> >--- calendar/resources/content/calPrintEngine.xul 1 Apr 2006 14:38:45 1.6
> Should we keep this files around? Maybe we need them again when we are going to
> remove this workaround and put back a real print preview?
I've removed them from the patch for the moment. I think we should just cvs rm them and go back to bonsai if we need them.

> >--- calendar/resources/content/printDialog.js	26 Jul 2006 23:01:12 1.17
> >@@ -21,102 +21,102 @@
> >+    refreshHtml(calGetString("calendar", "Untitled"),
> >+                document.getElementById("layout-field").value,
> Why not use settings.title and settings.layoutCId here?
Good point. Done.

> >+                settings.eventList,
> >+                settings.start,
> >+                settings.end)
> Missing ';'
Fixed.

> >     var start;
> >     var end;
> start/end already declared in line 126/127 (below "var filter = ...")
Removed one set and moved them to immediately before they're used.

> >+        settings.title = document.getElementById("title-field").value;
> >+        if (settings.title == "") {
> >+            settings.title = calGetString("calendar", "Untitled");
> >+        }
> You should use "===" to compare with "" or you could use title = title ||
> calGetString(...);
Using the latter now.

> Is it possible to do this common assignment once instead of three times on
> different places?
Yes. Refactored to do so.

> >+        settings.end = null;
> >+        window.close;
> Should this be "window.close();"?
Yes. Fixed.

> Can you explain to me why we return settings two times and call window.close
> one time?
Yes, it was a bug. Now fixed.

> What is the difference between the normal return and the return inside
> onOperationComplete()?
That's now obsolete.

> >+function onDialogChange()
> Missing ';' again
Fixed.

> >+function refreshHtml(aTitle, aLayoutCId, aEventList, aStart, aEnd)
> >+{
> >+    // I'd like to use calGetString, but it can't do "formatStringFromName".
> 
> Maybe add a new helper method? Might be useful in other places too.
It's non-trivial because you can pass it an unlimited number of strings to be substituted in.  Rather than deal with that science project, I did it the "old fashioned way".

> Not in patch but: there are still calls to getElementById("view-deck"). Should
> be currentView() to make it usable in Lightning too.
I caught that too while working on the Lightning printing bug. Fixed.

> --- calendar/resources/content/calendar.js      14 Aug 2006 16:58:33 1.237
>  function calPrint()
>  {
> -    window.openDialog("chrome://calendar/content/printDialog.xul",
> -                      "printdialog","chrome");
> +    window.openDialog("chrome://calendar/content/printDialog.xul", "Print",
> +                      "centerscreen,chrome,resizeable=yes");
>  }
First off, I moved this into calendarUtils.js so Lightning has it too.

> On Windows the dialog is fixed size with this patch. Just use "resizable".
Done.

> After fixing this the preview iframe does not expand if the dialog is resized.
> Missing flex? See screenshot.
Fixed.

> --- calendar/resources/content/printDialog.xul  8 Aug 2006 12:11:53 1.14
> I would like to use 'persist="screenX screenY height width"' here.
Done.

> Also there is a second border (splitter?) left of the iframe that looks
> unnecessary because there already is a border defined.
It's a splitter. It should look a bit better now.

Comment 48

[Stefan Sitter [:ssitter]]

Comment on attachment 234837 [details] [diff] [review]
Updates per ssitter

Looking better. I noticed some things on quick test, see below.
I'll look at the details tomorrow.

>--- calendar/resources/content/calendarUtils.js 31 Jul 2006 18:15:13 	1.27
>+++ calendar/resources/content/calendarUtils.js 21 Aug 2006 20:26:02 
>@@ -522,8 +522,17 @@
>+function calPrint()
>+{
>+    window.openDialog("chrome://calendar/content/printDialog.xul", "Print",
>+                      "centerscreen,chrome,resizeable");
>+}

This must be 'resizable' not 'resizeable'.

>--- calendar/resources/content/printDialog.js 26 Jul 2006 23:01:12 1.17
>+++ calendar/resources/content/printDialog.js 21 Aug 2006 20:26:02 

> function onDatePick() {
>     radioGroupSelectItem("view-field", "custom-range");
>+    onDIalogChange();
> }

You want onDialogChange()

After playing around clicking on the datepicker gave me the following error, but I can't reproduce it right now:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: chrome://calendar/content/printDialog.js :: refreshHtml :: line 192"  data: no]

I also noticed that [Print] does not work the first time for me. The dialog closes but nothing is printed. On the second attemp it worked.

Comment 49

[Matthew (lilmatt) Willis]

(In reply to comment #48)
> (From update of attachment 234837 [details] [diff] [review] [edit])
> Looking better. I noticed some things on quick test, see below.
> I'll look at the details tomorrow.
> 
> >--- calendar/resources/content/calendarUtils.js 31 Jul 2006 18:15:13 	1.27
> >+    window.openDialog("chrome://calendar/content/printDialog.xul", "Print",
> This must be 'resizable' not 'resizeable'.
Stupid English. Fixed. Wow! Look at that! It resizes!

> >--- calendar/resources/content/printDialog.js 26 Jul 2006 23:01:12 1.17
> >+    onDIalogChange();
> You want onDialogChange()
Fixed.

> After playing around clicking on the datepicker gave me the following error,
> but I can't reproduce it right now:
> 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult:
> "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame ::
> chrome://calendar/content/printDialog.js :: refreshHtml :: line 192"  data: no]
I suspect you were "refreshing the dialog too fast", trying to write to the temp file before it was done writing the previous time.

> I also noticed that [Print] does not work the first time for me. The dialog
> closes but nothing is printed. On the second attemp it worked.
Weird. I'm not seeing this. Let me know STR if you can.

Comment 50

[Stefan Sitter [:ssitter]]

Comment on attachment 234837 [details] [diff] [review]
Updates per ssitter

r=ssitter if you fix 'resizeable' and 'onDIalogChange()'

Comment 51

[Joey Minta]

Comment on attachment 234837 [details] [diff] [review]
Updates per ssitter

+    // set the datepickers to the currently selected dates
+    document.getElementById("start-date-picker").value = window.opener.currentView().startDay.jsDate;
+    document.getElementById("end-date-picker").value = window.opener.currentView().endDay.jsDate;
Line wrapping please.

-function printCalendar() {
+function getDialogSettings()
+{
Can you comment briefly what this function does, and more importantly, the structure of the object that it returns?

+    var settings = getDialogSettings();
+
+    refreshHtml(settings.title,
+                settings.layoutCId,
+                settings.eventList,
+                settings.start,
+                settings.end);
Given that every call to refreshHtml looks like this, would it make more sense to have refreshHtml take no args and simply call getDialogSettings() itself?

+        if (gPrintSettings == null) {
+            var useGlobalPrintSettings = getPrefSafe("print.use_global_printsettings","false");
+
+            var pss = Components.classes["@mozilla.org/gfx/printsettings-service;1"]
+                                .getService(Components.interfaces.nsIPrintSettingsService);
+            if (useGlobalPrintSettings) {
+
Don't quote false in your getPrefSafe call.  It'll return something you don't want.
js> var s = "false";
js> if (s) print("foo");
foo


Better yet, per IRC, see if we can just get rid of the getPrintSettings() function, since I don't see any calendar callers.


+  <script type="application/x-javascript" src="chrome://calendar/content/calendar-views.js"/>
I don't think we need to include this file.  If we do, it's really bad.  Since we're calling window.opener, we shouldn't need it.

More review, including the XUL bits after lunch.

Comment 52

[Joey Minta]

Comment on attachment 234837 [details] [diff] [review]
Updates per ssitter

+  <stringbundleset id="stringbundleset">
+    <stringbundle id="bundle_date" src="chrome://calendar/locale/dateFormat.properties"/>
+  </stringbundleset>
+
I don't see where this is used.

+      <!-- Startup -->
+      <groupbox id="settingsGroup">
What does Startup mean?

+        <hbox align="center">
+          <label value="&calendar.print.title.label;"
+                 control="title-field"/>
+          <textbox id="title-field" class="padded" flex="1"
+                   onchange="onDialogChange();"/>
+        </hbox>
+        <separator class="thin"/>
+        <hbox align="center">
+          <label value="&calendar.print.layout.label;"
+                 control="layout-field"/>
           <menulist id="layout-field">
-            <!-- menupopup will be filled in printDialog.js -->
-            <menupopup id="layout-menulist-menupopup"/>
+            <!-- This menupopup will be populated by printDialog.js -->
+            <menupopup id="layout-menulist-menupopup"
+                       oncommand="onDialogChange();"/>
           </menulist>
These should probably be in a <grid> so that Title and Layout are aligned, along with their input fields.  Also, the <menulist> should get some flex.

+      </groupbox>
+
+      <spacer/>
+
+      <groupbox>
My understanding was that <spacer/> without flex or height (width) didn't do anything.

+                <hbox class="field-label-box-class">
+                  <label value="&calendar.print.from.label;"/>
+                </hbox>
As mentioned in IRC, there's something weird with the styling here (and probably with standard-dialog-content too).  It looks like the rules defined for these classes aren't being applied.  It's not clear to me that the rules *should* be being applied, but we shouldn't pretend that they are, if they're not.

I'd like to see how this looks with those changes.

Comment 53

[Matthew (lilmatt) Willis]

Attachment: fixed per jminta

(In reply to comment #51 and comment #52)
> +    document.getElementById("start-date-picker").value =
> window.opener.currentView().startDay.jsDate;
> +    document.getElementById("end-date-picker").value =
> window.opener.currentView().endDay.jsDate;
> Line wrapping please.
Done.

> +function getDialogSettings()
> Can you comment briefly what this function does, and more importantly, the
> structure of the object that it returns?
Done.

> Given that every call to refreshHtml looks like this, would it make more
> sense to have refreshHtml take no args and simply call getDialogSettings()
> itself?
Yes. Done.

> Better yet, per IRC, see if we can just get rid of the getPrintSettings()
Done.

> +  <script type="application/x-javascript"
> src="chrome://calendar/content/calendar-views.js"/>
> I don't think we need to include this file.  If we do, it's really bad.  Since
Removed.

> +    <stringbundle id="bundle_date"
> I don't see where this is used.
Removed.

> +      <!-- Startup -->
> What does Startup mean?
Bad paste. Removed.

> +        <hbox align="center">
> +          <label value="&calendar.print.title.label;"
> These should probably be in a <grid> so that Title and Layout are aligned,
Done.

> My understanding was that <spacer/> without flex or height (width)...
Removed.

> +                <hbox class="field-label-box-class">
> As mentioned in IRC, there's something weird with the styling here (and
> probably with standard-dialog-content too).
Removed both.

> I'd like to see how this looks with those changes.
Screenshot to be attached.

Comment 54

[Matthew (lilmatt) Willis]

Attachment: Screenshot with jminta changes

Comment 55

[Joey Minta]

Comment on attachment 235147 [details] [diff] [review]
fixed per jminta

+var gTempFile = null;
+var gTempUri;
As I read this code, gTempUri is only used internally in one function, so it doesn't need to be a global.  Let's move/rename.

+    // set the datepickers to the currently selected dates
+    document.getElementById("start-date-picker").value =
+            window.opener.currentView().startDay.jsDate;
+    document.getElementById("end-date-picker").value =
+            window.opener.currentView().endDay.jsDate;
How about doing the same thing as below to only resolve window.opener.currentView() once?

+ * @returns a settings object - containing the title, layoutCId, eventList,
+ *                              start, end, and eventList
Why a dash?  Also, you can't say "a settings object" if you haven't defined what such a thing is.  Perhaps something like "@returns, an Object with title, layoutCId, eventList, start, and end properties containing the appropriate values"?  (Also you have eventList twice)

+            var aCurrentView = window.opener.currentView();
+            start = aCurrentView.startDay;
+            end   = aCurrentView.endDay;
eep! The a-prefix is for arguments passed in.

+    gTempFile.createUnique(nsIFile.NORMAL_FILE_TYPE, 0600);
...
+        stream.init(gTempFile, 0x2A, 0600, 0);
Some quick comments about your magic numbers would be nice.

     var start;
     var end;
+    var eventList;
Nit: This is shorter as |var start, end, eventList;|.

         onOperationComplete: function (aCalendar, aStatus, aOperationType, aId, aDateTime) {
-            printInitWindow(listener.mEventArray, start, end); 
While you're here, making this function and onGetResult un-anonymous would rock.  But totally optional.

+  <hbox flex="1">
+    <vbox>
+
+      <groupbox id="settingsGroup">
+        <caption label="&calendar.print.settingsGroup.label;"/>
 
-  <vbox id="standard-dialog-content"  flex="1">
Now it's hard for people to overlay into this window. :-(  Also, if you're removing all of these rules, why not remove the css-include too?  (And maybe file a follow-up for cvs-removing that file)

Comment 56

[Matthew (lilmatt) Willis]

Attachment: more jminta fixes

Comment 57

[Matthew (lilmatt) Willis]

Attachment: Screenshot with dmose changes

Comment 58

[Joey Minta]

Comment on attachment 235161 [details] [diff] [review]
more jminta fixes

+ * Called when a datepicker is finished, and a date was picked.
+ */
Can you re-word?  I don't know what it means for a datepicker to be finished.

+    var previewPane = document.getElementById("content");
+    var previewPaneCW = previewPane.contentWindow;
+    previewPaneCW.location = gTempUri.spec;
If we're not going to use previewPane anywhere, just do document.getElementById("content").contentWindow

+  <hbox id="firstHbox" flex="1">
+    <vbox id="groupboxVbox">
+
+      <groupbox id="settingsGroup">
+        <caption label="&calendar.print.settingsGroup.label;"/>
 
-  <vbox id="standard-dialog-content"  flex="1">
You should trash the standard-dialog-content css rule too then.

+    var gTempUri = ioService.newFileURI(gTempFile); 
No g prefix here please.

+    settings.title = document.getElementById("title-field").value;
+    settings.title = (settings.title || calGetString("calendar", "Untitled"));
This looks a bit weird.  I'd say use a temp-var for the ("title-field").value bit.

r=jminta with that.

Comment 59

[Matthew (lilmatt) Willis]

Patch checked in (with jminta's nits) on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 60

[Matthew (lilmatt) Willis]

*** Bug 268179 has been marked as a duplicate of this bug. ***

Comment 61

[Joey Minta]

In my current build:
Error: gTempUri is not defined
Source File: chrome://calendar/content/printDialog.js
Line: 204

-> REOPENED

Comment 62

[Matthew (lilmatt) Willis]

(In reply to comment #61)
> Error: gTempUri is not defined
> Source File: chrome://calendar/content/printDialog.js
> Line: 204

Typo fix checked in on branch and trunk.

-> FIXED