420050 - Changing the print orientation requires a restart of TB

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Boying Lu]

====================
Steps to re-create:-
====================

1. Open thunderbird
2. Goto File -> Page Setup
3. Change the orientaiton format (e.g. from potrait to landscape in this case) & click OK
4. Select an email and print it
5. You will see it prints in the original orientaion. (e.g. portrait in this case)
6. Restart thunderbird and print the email again and you will see it prints in the newly selected orientation (e.g. landscape in this case)
7. Changing back to the original orientation (e.g. portrait in this case) requires another restart of the application and so on...

Comment 1

[Boying Lu]

Attachment: patch

Comment 2

[Boying Lu]

Attachment: only affect TB

Comment 3

[[e-mail]]

Comment on attachment 306447 [details] [diff] [review]
only affect TB

Ian, you touched printing last ;-)

Does this problem affect addressbook too?

Comment 4

[Boying Lu]

Comment on attachment 306447 [details] [diff] [review]
only affect TB

The bug also affect address book. But the patch doesn't work for address book.  I'll make a new patch

Comment 5

[Boying Lu]

Attachment: [checked in] fixed in both e-mail and address book

Comment 6

[Ian Neal]

Comment on attachment 309076 [details] [diff] [review]
[checked in] fixed in both e-mail and address book

This is fine, SeaMonkey will also need to be patched though. It would be good to see if we can get this and the SM patch onto the branch too.

Comment 7

[[e-mail]]

Comment on attachment 309076 [details] [diff] [review]
[checked in] fixed in both e-mail and address book

Two questions, which you may want to fix now or leave for a followup bug:

1. Do we need to keep the print settings in a global any more?

2. If not, do we actually need to access the print settings here or should we do it in msgPrintEngine.js?

Comment 8

[Reed Loden [:reed]]

Checking in mail/components/addrbook/content/addressbook.js;
/cvsroot/mozilla/mail/components/addrbook/content/addressbook.js,v  <--  addressbook.js
new revision: 1.46; previous revision: 1.45
done
Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.190; previous revision: 1.189
done

Comment 9

[Boying Lu]

Attachment: new patch

The new patch is based on Neil's comment

Comment 10

[Ian Neal]

Comment on attachment 311520 [details] [diff] [review]
new patch

>Index: mail/components/addrbook/content/addressbook.js
>===================================================================
>@@ -365,13 +364,12 @@
>      }
>   }
> 
>-  gPrintSettings = PrintUtils.getPrintSettings();
> 
Nit: Remove the extra blank line, only need one.

>@@ -410,12 +408,11 @@
>   var abURIArr = uri.split("://");
>   var printUrl = "addbook://" + abURIArr[0] + "/" + abURIArr[1] + "?action=print"
> 
>-  gPrintSettings = PrintUtils.getPrintSettings();
> 
Nit: Remove the extra blank line, only need one.

>Index: mailnews/base/resources/content/msgPrintEngine.js
>===================================================================

>-    if (window.arguments.length > 5) {
>-      printEngine.setMsgType(window.arguments[5]);
>+    if (window.arguments.length > 4) {
>+      printEngine.setMsgType(window.arguments[4]);
>+      // printEngine.setMsgType(window.arguments[5]);
Nit: Remove the commented out line.

r- as with this change to the arguments sent to msgPrintEngine, you will completely break printing from mailnews and addressbook in SeaMonkey. Lines to patch in /mailnews/addrbook/resources/content/addressbook.js and /mailnews/base/resources/content/mailWindowOverlay.js should be identical to TB equivalents - if you need help/testing on SM let me know.

Comment 11

[Boying Lu]

Thanks a lot.  I'll make a new patch 

Comment 12

[Boying Lu]

Attachment: patch for both thunderbird and seamonkey

Comment 13

[Ian Neal]

Comment on attachment 311949 [details] [diff] [review]
patch for both thunderbird and seamonkey

>Index: mail/components/addrbook/content/addressbook.js
>===================================================================
>@@ -365,13 +364,12 @@
>      }
>   }
> 
>-  gPrintSettings = PrintUtils.getPrintSettings();
> 
You still need to remove the extra blank line here, you only need the one.

>   printEngineWindow = window.openDialog("chrome://messenger/content/msgPrintEngine.xul",
>                                          "",
>                                          "chrome,dialog=no,all",
>                                           totalCard, selectionArray, statusFeedback,
>-                                          gPrintSettings, doPrintPreview, msgType);
>+                                          doPrintPreview, msgType);
> 
>   return;
> }
>@@ -410,12 +408,11 @@
>   var abURIArr = uri.split("://");
>   var printUrl = "addbook://" + abURIArr[0] + "/" + abURIArr[1] + "?action=print"
> 
>-  gPrintSettings = PrintUtils.getPrintSettings();
> 
You still need to remove the extra blank line here, you only need the one.

>   printEngineWindow = window.openDialog("chrome://messenger/content/msgPrintEngine.xul",
>                     "",
>                     "chrome,dialog=no,all",
>-                    1, [printUrl], statusFeedback, gPrintSettings, doPrintPreview, msgType);
>+                    1, [printUrl], statusFeedback, doPrintPreview, msgType);
> 
>   return;
> }

r=me with those changes.

Comment 14

[Boying Lu]

Attachment: patch with empty lines removed

Comment 15

[[e-mail]]

Comment on attachment 312661 [details] [diff] [review]
patch with empty lines removed

>+    printSettings = PrintUtils.getPrintSettings();
>+    if (printSettings) {
>+      printSettings.isCancelled = false;
>+    }
Nit: This is possibly not the ideal place to put this code block.

Comment 16

[Ian Neal]

(In reply to comment #15)
> (From update of attachment 312661 [details] [diff] [review])
> >+    printSettings = PrintUtils.getPrintSettings();
> >+    if (printSettings) {
> >+      printSettings.isCancelled = false;
> >+    }
> Nit: This is possibly not the ideal place to put this code block.
> 

Boying Lu are you going to look at this before checkin?

Comment 17

[Boying Lu]

ok i'll take a look at this

Comment 18

[Boying Lu]

Attachment: new patch based on Neil's comments

Comment 19

[Ian Neal]

(In reply to comment #18)
> Created an attachment (id=316193) [details]
> new patch based on Neil's comments
> 

Don't forget the SeaMonkey part of the patch...

Comment 20

[Boying Lu]

Attachment: [checked in] patch for both thunderbird and seamonkey

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 309076 [details] [diff] [review]
[checked in] fixed in both e-mail and address book

Note, this patch was already checked in by reed on 2008-03-18 12:39.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 311949 [details] [diff] [review]
patch for both thunderbird and seamonkey

I'm fairly sure this one is obsolete.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 316343 [details] [diff] [review]
[checked in] patch for both thunderbird and seamonkey

Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.196; previous revision: 1.195
done
Checking in mail/components/addrbook/content/addressbook.js;
/cvsroot/mozilla/mail/components/addrbook/content/addressbook.js,v  <--  addressbook.js
new revision: 1.47; previous revision: 1.46
done
Checking in mailnews/base/resources/content/msgPrintEngine.js;
/cvsroot/mozilla/mailnews/base/resources/content/msgPrintEngine.js,v  <--  msgPrintEngine.js
new revision: 1.24; previous revision: 1.23
done
Checking in mailnews/base/resources/content/mailWindowOverlay.js;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.267; previous revision: 1.266
done
Checking in mailnews/addrbook/resources/content/addressbook.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.js,v  <--  addressbook.js
new revision: 1.137; previous revision: 1.136