158110 - [FIX]Impl PrintPreview for Mail and Addrbook

Status: VERIFIED FIXED

(MailNews Core :: Printing, defect)

Description

[rods (gone)]

Comment 1

[rods (gone)]

Attachment: patch

This includes the fix from Bug 157508
It also factors out some of the JS printing code into a new stand-alone file
printing.js

Comment 2

[rods (gone)]

Attachment: patch v2

This fixes a small problem with printing.

Comment 3

[rods (gone)]

Comment on attachment 92213 [details] [diff] [review]
patch v2

new patch is coming

Comment 4

[rods (gone)]

Attachment: patch v3

This patch does the following:
1) Breaks out the printing/pp js code from browser.js and puts it into a new
file printing.js (Benefit: isolates the code and enables it to be used from
mail news via the PP toolbar)

2) Adds to method to the internal interface nsIContentViewerFile for printing
for enabling the print dialog to be parented to a different DOMWindow. Includes
minor changes to nsDocumentViewer to also support this. (benefit: the Print
Dialog no loger fails behind the mail window if the mail window is clicked on.
In otherwords, the mail window is no modal to the print dialog, justlike the
browser)

3) Enables AddressBook to Print Preview a card or the entire addrbook in a
separate window.

4) Enables the Print Preview of Mail message in a separate window

5) Adds "Print Preview" to the File menu for "View Source"

6) Adds the "Preparing..." progress dialog for the printing of the addrbook and
mail.
---------------------------------------------------------------
Overview for PP
Currently mail/ab creates a hidden window to load the generated content into
for printing. In order for PP to work, we show this "hidden" window and add the
PP toolbar. From the stand-alone window the user can adjust any of the Page
Setup options and then print. Upon hitting the "Close" button, the entire
window is closed and destroyed.

The printing JS code in the xpfe/browser need to be broken out into its own
file so it could be more easily accessed via mailnews JS.

Note: The AddrBook's File menu now has four options:
Print Card
Print Preview Card
Print AddrBook
Print Preview AddrBook
(Not sure how clean this is from a UI stand point)

Comment 5

[Alex Bishop]

> Note: The AddrBook's File menu now has four options:
> Print Card
> Print Preview Card
> Print AddrBook
> Print Preview AddrBook
> (Not sure how clean this is from a UI stand point)

It might be an idea to add separators in, so you get:

--------------------------
Print Card
Print Preview Card
--------------------------
Print Address Book
Print Preview Address Book
--------------------------

It may also be clearer if the phrasing of the Print Preview items is 'Print
Preview of Card' and 'Print Preview of Address Book'. Should probably get some
UE input for that.

Comment 6

[timeless]

imo there should only be two print items

print <currently selected object>
print preview

(and of course, as always, print preview should not even exist on MacOSX or BeOS)

Comment 7

[jglick]

>imo there should only be two print items

>print <currently selected object>
>print preview

I agree. It would be nice the the print items were context sensitive.

Comment 8

[rods (gone)]

Please don't side track this bug, if you want the features of comments #6 and #7
then open a new bug. This bug isn't about that.

Comment 9

[(not reading, please use [e-mail] instead)]

I commend rod on tackling such a beast of a bug.  (print preview to mail /
addressbook is no small undertaking.)

I'm not a module owner for a lot of the files rod changes, so we should get some
other reviewers to look at this.

but here's some comments:

1) 
       <menuseparator/>
-      <!--Page setup gets overlaid here from platformGlobalOverlay.xul / -->
+      <menuitem id="printSetupMenuItem" label="&printSetupCmd.label;"
accesskey="&printSetupCmd.accesskey;" command="cmd_printSetup"/>
       <menuitem id="printMenuItem" key="key_print" label="&printCmd.label;"
accesskey="&printCmd.accesskey;" observes="cmd_print"/>
+      <menuitem id="printPreviewMenuItem" label="&printPreviewCmd.label;"
accesskey="&printPreviewCmd.accesskey;" observes="cmd_printpreview"/>
     </menupopup>
   </menu>
 

+function setPPTitle(title)
+{
+    var gBrandBundle = document.getElementById("bundle_brand");
+    if (gBrandBundle) {
+      title += " - " + gBrandBundle.getString("brandShortName");

instead of formatting this string in our js, we should be using the string
bundle code to format.

2)
 
   <!-- The main display frame -->
-  <iframe id="printengine" style="height: 0px" flex="1" name="printengine"
type="content" src="about:blank" />
+  <browser id="content" type="content-primary" name="content" src="about:blank"
flex="1"/>

are there any security issues by switching from iframe to browser?

3)

+const char* kPrintingPromptService =
"@mozilla.org/embedcomp/printingprompt-service;1";

isn't there already a #define for this?

4) 

+        printf("******* domWindow.get(%p) %s mMsgDOMWin.get(%p)\n",
domWindow.get(), 
+                (domWindow.get() != mMsgDOMWin.get()?"!=":"=="), mMsgDOMWin.get());
+          printf("******* BAILING OUT\n");
+      printf("******* Closing Progress Dialog\n");
+      printf("******* FirePrintEvent For PP\n");

these printfs should be removed, commented out, or wrapped with #ifdef DEBUG_rods

5)

+    nsCOMPtr<nsIPref> prefs (do_GetService(NS_PREF_CONTRACTID));
+    if (prefs) 
+    {
+      prefs->GetBoolPref("print.show_print_progress", &showProgressDialog);
+    }

nsIPref is obsolete, according the idl file.  Use nsIPrefService instead

see http://lxr.mozilla.org/mozilla/source/modules/libpref/public/nsIPref.idl#49

6)


+      else
+      {
+        // Tell the user we started printing...
+        nsAutoString keyStr;
+        keyStr.AssignWithConversion(kMsgKeys[mMsgInx]);
+        PRUnichar *msg = GetString(keyStr.get());
+        SetStatusMessage( msg );
+        CRTFREEIF(msg)
+      }

I think you can do:

+        PRUnichar *msg = GetString(NS_ConvertASCIItoUCS2(kMsgKeys[mMsgInx]));
+        SetStatusMessage( msg );
+        CRTFREEIF(msg)

or better yet, make this an array of PRUnichar * consts.

+  const char* kMsgKeys[] = {"PrintingMessage",  "PrintPreviewMessage",
+                            "PrintingCard",     "PrintPreviewCard",
+                            "PrintingAddrBook", "PrintPreviewAddrBook"};

7)

+  *aWebBrowserPrint = mWebBrowserPrint.get();
+  NS_ADDREF(*aWebBrowserPrint);

you could do:

+  NS_ADDREF(*aWebBrowserPrint = mWebBrowserPrint);

8)

+<script type="application/x-javascript"
src="chrome://communicator/content/printing.js"/>
 <script type="application/x-javascript"
src="chrome://messenger/content/accountUtils.js"/>
 <script type="application/x-javascript"
src="chrome://messenger/content/widgetglue.js"/>
 <script type="application/x-javascript"
src="chrome://messenger/content/mail-offline.js"/>
@@ -225,7 +226,7 @@
           <menuitem label="&sendNowCmd.label;"
accesskey="&sendNowCmd.accesskey;" key="key_send" command="cmd_sendNow"
id="menu-item-send-now"/>
           <menuitem label="&sendLaterCmd.label;"
accesskey="&sendLaterCmd.accesskey;" key="key_sendLater" command="cmd_sendLater"/>
           <menuseparator/>
-          <!--Page setup gets overlaid here from platformGlobalOverlay.xul / -->
+          <menuitem id="printSetupMenuItem" label="&printSetupCmd.label;"
accesskey="&printSetupCmd.accesskey;" command="Browser:PrintSetup"/>
           <menuitem id="printMenuItem" label="&printCmd.label;"
accesskey="&printCmd.accesskey;" key="key_print" command="cmd_print"/>
         </menupopup>
       </menu>

Is "Print Setup" the same as "Page setup"?  Is that something XP, or just on
certain platforms?
Since it used to come from platformglobaloverlay, it makes me think it was per
platform.
if all platforms have it now, do we still need this overlay?

9)

make this alert a dump, as we'd never want the user to see that.

+    if (printSettings == null) {
+      alert("PrintSettings arg is null!");
+      return false;
+    }
+

10)
     
     onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress,
aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress)
     {
+      if (switchUI) 
+      {
+        dialog.tempLabel.setAttribute("hidden", "true");
+        dialog.progress.setAttribute("hidden", "false");
+        dialog.cancel.setAttribute("disabled", "false");
+
+        var progressLabel = getString("progress");
+        if (progressLabel == "") {
+          progressLabel = "Progress:"; // better than nothing
+        }
+        switchUI = false;
+      }

This is not localized.  looking at
mozilla/xpfe/global/resources/content/printProgress.xul
it looks like you have to add

<data id="dialog.strings.progress">&progress;</data>

It looks like progress entity already lives in printProgress.dtd

There's a note in printProgress.xul  "XXX-TODO:   convert to use string bundles."

we should log a bug on that, if there isn't one already.


11)  interCaps, so this should be printWithParent()
 
+  [noscript] void PrintWithParent(in nsIDOMWindowInternal aParentWin,
+                                  in nsIPrintSettings aThePrintSettings,
+                                  in nsIWebProgressListener aWPListener);
+
+};

Comment 10

[rods (gone)]

#1) Not sure about the XUL part is there an example of using the "brand bundle"
to do this?

2) Not sure, sgehani? is this ok?

3) There is not one in any public header.

4) Fixed.

5) Fixed.

6) Fixed using NS_ConvertASCIItoUCS2, sure how define a static array of
PRUnichar strings

7) Fixed.

8) What about this, sgehani?

9) Fixed.

10) It is localized, but a bug should be filed. (sgehani?)

11) Fixed.

Comment 11

[Samir Gehani]

> 1) Not sure about the XUL part is there an example of using the "brand bundle"
> to do this?

I believe Seth is alluding to formatStringFromName() [1][2].

> 2) Not sure, sgehani? is this ok?

There are a bunch of properties [3] that a browser has but I don't think this
matters as far as content from the network is concerned because they won't have
access to these properties unless the scripts with access are in chrome or have
been granted UniversalXPConnect privileges by the privilege manager (irrelevant
cause they have access to the entire machine in this state).

> 8) What about this, sgehani?

I believe print setup is page setup and that there is now one cross platform
dialog.  But, I would prefer to defer to Bill Law since he implemented the page
setup front-end.  CC'ing Bill.

> 10) It is localized, but a bug should be filed. (sgehani?)

This is indeed localized but the way to do this would be to use a
<stringbundle/>.  See [4] for an example.

___________________________
[1] nsIStringBundle::formatStringFromName,
<http://lxr.mozilla.org/mozilla/source/intl/strres/public/nsIStringBundle.idl#67>

[2] Example usage of formatStringFromName,
<http://lxr.mozilla.org/mozilla/source/xpfe/components/updates/src/nsUpdateNotifier.js#232>

[3] browser properties,
<http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/browser.xml#183>

[4] <stringbundle/> usage in xpinstall status dialog,
<http://lxr.mozilla.org/seamonkey/source/xpinstall/res/content/xpistatus.xul>
<http://lxr.mozilla.org/seamonkey/source/xpinstall/res/content/xpistatus.js>

Comment 12

[rods (gone)]

Attachment: patch v4

I did a little research on item #8, and platformGlobalOverlay.xul no longer
exists and it couldn't get it from that xul file anyway. So we are fine with
that.

I have fixed all of issues from above. This I hope is the final patch.

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 98893 [details] [diff] [review]
patch v4

1)  based on your last comment, shouldn't we remove this comment:

	 <!--Page setup gets overlaid here from platformGlobalOverlay.xul / -->

2)  won't this throw the end user into venkman (the js debugger) if it's
installed?

debug("Exception getting bundle " + aURI + ": " + ex);

we should use dump, or comment it out.

3)

-  <iframe id="printengine" style="height: 0px" flex="1" name="printengine"
type="content" src="about:blank" />
+  <browser id="content" type="content-primary" name="content"
src="about:blank" flex="1"/>

are there any security issues here?  do we want to add these attributes:

disablehistory="true" disablesecurity="true"

do we also want to disable js?	Is that conditional on if the user allows js in
mailnews?

element.docShell.allowJavascript = false;

we should talk to mstoltz.

4) no need to initialize rv here:

+  nsresult rv = NS_ERROR_FAILURE;

5)  \ No newline at end of file

add the newlines, or HPUX will break.

Comment 14

[jag (Peter Annema)]

> -  <iframe id="printengine" style="height: 0px" flex="1" name="printengine"
>            type="content" src="about:blank" />
> +  <browser id="content" type="content-primary" name="content"
>             src="about:blank" flex="1"/>
>
> are there any security issues here?  do we want to add these attributes:
>
> disablehistory="true" disablesecurity="true"

You'll probably want those two for a print preview "browser". disabledhistory
makes it not set session and global history on itself (used for navigation,
which I doubt you're interested in for print preview), disablesecurity makes it
not hook up a security UI state object. It's for tracking a document's security
state and showing that to the user, throwing a dialog in their face every now
and then. I doubt we'll want to repeat that process for print preview, but I'll
CC kaie, he might have some better input on that.

Comment 15

[Kai Engert (:KaiE:)]

Jag's suggestion sounds fine. If the print preview window *only* shows a print
preview and gets closed afterwards, and if the print preview window does not
show a lock icon in the lower right corner, then disabling security tracking is
a good idea.

Comment 16

[(not reading, please use [e-mail] instead)]

about disabling js, rods tells me that we do that here:

http://lxr.mozilla.org/mozilla/source/content/base/src/nsPrintEngine.cpp#1123

for PP and print.

Comment 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 98893 [details] [diff] [review]
patch v4

sr=sspitzer.  rod tells me he's fixed the minor nits in his local tree.

Comment 18

[David :Bienvenu]

+	      if ( GetNumSelectedMessages() == 1)
+        {
+          if (gDBView)
+          {
+             gDBView.getCommandStatus
(nsMsgViewCommandType.cmdRequiringMsgBody, enabled, checkStatus);
+              return enabled.value;
+          }
+        }
how about combining the if's: if (GetNumSelectedMessages() == 1 && gDBView)
{
}
else
return false;

Here, you don't need CRTFREEIF
+          if (msg) 
+          {
+            mPrintProgressParams->SetDocTitle(msg);
+            CRTFREEIF(msg)
+          }

you can just use nsCRT::free because it checks for null, and you don't 
want/need to null out msg, since it's a local var.

same thing here:
+        PRUnichar *msg = GetString(NS_ConvertASCIItoUCS2(kMsgKeys[mMsgInx]).get
());
+        SetStatusMessage( msg );
+        CRTFREEIF(msg)
+      }

fix those nits, and r=bienvenu

Comment 19

[David :Bienvenu]

Comment on attachment 98893 [details] [diff] [review]
patch v4

r=bienvenu, modulo the comments above

Comment 20

[rods (gone)]

checked in, fixed

Comment 21

[hacker formerly known as [e-mail]]

This checkin is causing the static builds to break.  Global symbols must be
unique across the tree.  At a glance, both definitions of the symbol are not
used outside of their respective files and should be declared static.

c++ -o mozilla-bin ...

../../dist/lib/components/libmailnews.a(nsMsgPrintEngine.o)(.data+0x0): multiple
definition of `kPrintingPromptService'
../../dist/lib/components/libgkcontent.a(nsPrintEngine.o)(.data+0x0): first
defined here
collect2: ld returned 1 exit status

Comment 22

[[e-mail]]

Comment on attachment 98893 [details] [diff] [review]
patch v4

>Index: mailnews/base/resources/content/mailWindowOverlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.xul,v
>retrieving revision 1.207
>diff -u -r1.207 mailWindowOverlay.xul
>--- mailnews/base/resources/content/mailWindowOverlay.xul	14 Aug 2002 22:12:14 -0000	1.207
>+++ mailnews/base/resources/content/mailWindowOverlay.xul	12 Sep 2002 16:14:32 -0000
>@@ -225,6 +227,7 @@
>   <command id="button_mark"/>
>   <command id="button_getNewMessages"/>
>   <command id="button_print"/>
>+  <command id="button_printpreview"/>
>   <command id="button_next"/>
>   <command id="button_file"/>
>   <command id="cmd_delete"/>

An oversight?

Comment 23

[[e-mail]]

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

Comment 24

[esther]

Using trunk builds 20021121 on  winxp, macosx and linux the following results
for Print Preview in UI for Mail and Address Book:
Winxp:   OK
Print Preview menu items listed for Address Book Card and Book, Mail Msg menu
items in Msg Pane, Stand Alone and Print Preview all are present.
Print Preview basic functionality for all mentioned above=OK.
Print page source=OK

Linux:  Not OK for basic functionality. Page Source doesn't print
Print Preview menu items listed for Address Book Card and Book, Mail Msg menu
items in Msg Pane, Stand Alone and Print Preview all are present=OK.
Print Preview basic functionality for all menu items mentioned above=there are
no buttons within the Print Preview window to Print, Page Setup, Scale or change
orientation, the only button available is to Close. Not OK 
Print Page source= OK

Mac OSX: Not OK for basic functionality.
Print Preview menu items listed for Address Book Card and Book, Mail Msg menu
items in Msg Pane, Stand Alone and Print Preview all are present=OK
Print Preview basic functionality for all mentioned above=Print Preview window
blank=Not OK.
Print Page Source=OK
If this bug is just for UI adding the menu items, I can verify it as fixed and
then look for or log new bugs for functionality bugs.
If this bug is for adding menu items and UI correctness of Print Preview window
then I need to reopen this. 

Comment 25

[esther]

correction to above comment: Linux page source does print if the message is
simple. It didn't print for me with a mail message that had images and links,
but neither did the mail message itself.  I'll investigate and log a separate
bug for that.

Comment 26

[esther]

Using trunk builds 20021202 on winxp, mac OSX10.2.2 and linux this is fixed.
Verified.