166217 - Print settings on Linux are saved at shutdown but not read at next start

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Roland Mainz]

Print settings on Linux/Unix platforms are saved at shutdown but not read at
next start

Comment 1

[Roland Mainz]

My fault, introduced with bug 147605 ("Printing / Page settings reset themselves
after print (no landscape printing)") ... ;-(

... taking myself...

Comment 2

[Olav Vitters]

isn't this the same as bug 83750?

Comment 3

[Roland Mainz]

[e-mail] wrote:
> isn't this the same as bug 83750?

This bug is a spin-off for Linux since the upcoming fix is for the XUL print
dialog - which is only used on Linux/Unix and OS/2.

Comment 4

[Roland Mainz]

Attachment: Patch for 2002-08-30-08-trunk

Comment 5

[Roland Mainz]

Requesting r=/sr= ...

Comment 6

[Pete Zha]

Comment on attachment 97530 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

>+
This extra new line is not needed.
> //---------------------------------------------------

>+var init_from_selection_once = true;
>+
This variable's name is a little strange. Maybe "firstTime" would be better.
But it's not a problem, just a thinking

r=pete, thanks Roland for this patch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97530 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

sr=bzbarsky

Comment 8

[Pete Zha]

Roland,
I tested this patch, but I find this patch will cause the fix for bug 147605
doesn't work. The setting will be reset every time open the print dialog. I will
do more test later and let you know the result.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97530 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

OK.  I'm rescinding sr till we have a patch that's actually been tested and
works.

Comment 10

[Pete Zha]

Attachment: patch

This patch will fix the problem and will not break the fix for bug 147605.
I just let the function load needed value from the prefs everytime and load
value from printer only when the user change the printer name from the list.
Roland, please take a look and give some suggestion. Thanks!

Comment 11

[Roland Mainz]

Comment on attachment 97575 [details] [diff] [review]
patch

I am not sure whether it's wise to handle InitPrintSettingsFromPrinter() and
InitPrintSettingsFromPrefs() seperately... remember that I was complaining in
bug 147605 about the fact that we have too many init functions without any
protection from doing the wrong things (I still think that we should get rid of
the global |nsIPrintSettings| object and use per-printer objects).

Comment 12

[Pete Zha]

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

Comment 13

[Roland Mainz]

Maybe we should rename |InitPrintSettingsFromPrinter| to
|InitPrintPrefsFromPrinter| and only call it if there are no printer-specific
prefs for this printer yet - that would solve all problems:
- We get printer-specific defaults
- We would fetch the device defaults only _once_
- We would have a clean way to work around the limitations of the global
nsIPrintSettings object 
- We would populate the prefs (currently we're writing into the global
|nsIPrintSettings| object) for this printer once from the device defaults - and
let |InitPrintSettingsFromPrefs| init the print settings object

Comment 14

[Roland Mainz]

Attachment: Patch for 2002-08-30-08-trunk

After some discussion and answering petez's question I had the feeling that
original code before bug 147605 was working correctly except that someone else
was calling a |InitPrintSettingsFrom*|-function some somewhere else.

After looking around I found that mozilla/gfx/src/nsPrintOptionsImpl.cpp is
calling |InitPrintSettingsFromPrinter()| each time we get the global
nsIPrintSettings object througth the print setting service. Removing that code 
(and backing out the "workaround" from bug 147605) seems to fix the problem...

Comment 15

[Roland Mainz]

Comment on attachment 97588 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

Nit: The "can't print landscape"-bug is back, but that's easy to fix...

Comment 16

[Roland Mainz]

Attachment: New patch for 2002-08-30-08-trunk

Tested:
- Print dialog remembers values across prints and browser restarts
- Page setup now works correctly (I can print "portrait" and "landscape")

Comment 17

[Pete Zha]

Comment on attachment 97594 [details] [diff] [review]
New patch for 2002-08-30-08-trunk

I have tested. This patch works well.

Comment 18

[Pete Zha]

r=pete

Comment 19

[Boris Zbarsky [:bzbarsky]]

+  // NOTE: getPRinters sets up the PrintToFile radio buttons

"Printers", not "PRinters"

sr=bzbarsky with that change.

Comment 20

[Roland Mainz]

Attachment: New patch for 2002-08-30-08-trunk to match sr=bz

Comment 21

[Roland Mainz]

Comment on attachment 97741 [details] [diff] [review]
New patch for 2002-08-30-08-trunk to match sr=bz

Carrying over r=petez and sr=bz (per bz's permission via IRC) ...

Comment 22

[Roland Mainz]

rods:
Can we have a moa= from you, please ?

Comment 23

[rods (gone)]

Comment on attachment 97741 [details] [diff] [review]
New patch for 2002-08-30-08-trunk to match sr=bz

This CANNOT be checked in! It will cause a lot of problems for printing on
Windows.

Comment 24

[rods (gone)]

Item #1 there are several duplicates in the list below:
+  var flags = gPrintSettingsInterface.kInitSaveMargins |
+              gPrintSettingsInterface.kInitSaveMargins |
+              gPrintSettingsInterface.kInitSaveOddEvenPages |
+              gPrintSettingsInterface.kInitSaveHeaderLeft |
+              gPrintSettingsInterface.kInitSaveHeaderCenter |
+              gPrintSettingsInterface.kInitSaveHeaderRight |
+              gPrintSettingsInterface.kInitSaveFooterLeft |
+              gPrintSettingsInterface.kInitSaveFooterCenter |
+              gPrintSettingsInterface.kInitSaveFooterRight |
+              gPrintSettingsInterface.kInitSaveBGColors |
+              gPrintSettingsInterface.kInitSaveBGImages |
+              gPrintSettingsInterface.kInitSaveInColor |
+              gPrintSettingsInterface.kInitSaveReversed |
+              gPrintSettingsInterface.kInitSaveOrientation |
+              gPrintSettingsInterface.kInitSaveOddEvenPages;

Item #2
I am not sure why the changes to the nsPrintOptionImpl are needed, but the
Windows platform completely relies on these for printing to work.

Note, that each platform has its own nsPrintOptions obj that is derived from
nsPrintOptionsImpl so you can override any of the methods. So you will want to
over these in those platform implementation.

Summary
The overall approach looks fine.

Comment 25

[Roland Mainz]

rod wrote:
> Item #1 there are several duplicates in the list below:
> +  var flags = gPrintSettingsInterface.kInitSaveMargins |
> +              gPrintSettingsInterface.kInitSaveMargins |
> +              gPrintSettingsInterface.kInitSaveOddEvenPages |
[snip]
> +              gPrintSettingsInterface.kInitSaveOddEvenPages;

OK, I'll fix that...

----

> Item #2
> I am not sure why the changes to the nsPrintOptionImpl are needed, but the
> Windows platform completely relies on these for printing to work.

I thought that |nsPrintOptions::GetGlobalPrintSettings()| returns the object
"as-is", without doing any initalisation or modification and that the matching
platform modules are doing the initalisation for the matching printer they want
to use. Adding an "exception" for the default printer calls for trouble when
they want to use a different printer but don't run the matching initalisation
cycle for it.

Same reason for |nsPrintOptions::InitPrintSettingsFromPrinter()| - there should
be no extra rules for the default printer - all printers should be treated the
same way in the crossplatform code - that's why I removed this code...

Comment 26

[rods (gone)]

I really don't remember what the intent was, your comments sound correct.

I'll try overriding the methods as an experiment. I think think the Mac relies
on this.

Comment 27

[rods (gone)]

Attachment: patch for nsPrintOptionImpl and nsPrintOptionWin

Made minor change to nsPrintOptionImpl from the previous patch and moved the
current impls of the methods in question to nsPrintOptionWin. Tested printing
on Windows, seems to work fine as one would expect for C++ to do its job with
overridden methods.

Comment 28

[Roland Mainz]

OK, I'll file an all-in-one patch later today...

Comment 29

[Roland Mainz]

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

Comment 30

[Roland Mainz]

Attachment: New all-in-one patch for 2002-08-30-08-trunk

New all-in-one patch for Win32 and Unix/Linux, incl. minor cleanup and adding
GUI support for the "printerfeatures" stuff (some widgets are now only enabled
if the printer really supports the matching feature :) ...

Comment 31

[Roland Mainz]

Requesting r=/sr= ...

Comment 32

[Pete Zha]

When testing this bug, I found we don't save color after printing. bug 166826
has been filed to fix this.

Comment 33

[Roland Mainz]

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

Comment 34

[Roland Mainz]

Attachment: New patch for 2002-08-30-08-trunk

I've integrated the patch from bug 166826 ("We don't save print_in_color after
printing.") into this patch...

Comment 35

[Roland Mainz]

rods:
Can you r= this patch, please ?

Comment 36

[rods (gone)]

Comment on attachment 98032 [details] [diff] [review]
New patch for 2002-08-30-08-trunk

r=rods

Comment 37

[Brian Ryner (not reading)]

Comment on attachment 98032 [details] [diff] [review]
New patch for 2002-08-30-08-trunk

>diff -N -r -u original/mozilla/gfx/src/windows/nsPrintOptionsWin.cpp mozilla/gfx/src/windows/nsPrintOptionsWin.cpp
>--- original/mozilla/gfx/src/windows/nsPrintOptionsWin.cpp	Thu Jul 25 20:30:28 2002
>+++ mozilla/gfx/src/windows/nsPrintOptionsWin.cpp	Thu Sep  5 01:17:37 2002
>@@ -39,7 +39,9 @@
> #include "nsPrintOptionsWin.h"
> #include "nsPrintSettingsWin.h"
> 
>-
>+#include "nsGfxCIID.h"
>+#include "nsIServiceManager.h"
>+static NS_DEFINE_IID(kPrinterEnumeratorCID, NS_PRINTER_ENUMERATOR_CID);

Please use the contract id instead ("@mozilla.org/gfx/printerenumerator;1".

>+NS_IMETHODIMP 
>+nsPrintOptionsWin::GetGlobalPrintSettings(nsIPrintSettings * *aGlobalPrintSettings)
>+{
>+  if (!mGlobalPrintSettings) {
>+    CreatePrintSettings(getter_AddRefs(mGlobalPrintSettings));
>+    NS_ASSERTION(mGlobalPrintSettings, "Can't be NULL!");

Why not move the early return if we couldn't get mGlobalPrintSettings up here? 
That way the next block wouldn't need to be indented (as much).

>+    // The very first we should initialize from the default printer

Correct this comment (looks like it should be "The very first time ...."

>+
>+  *aGlobalPrintSettings = mGlobalPrintSettings.get();

No need for this .get().

>+  NS_ADDREF(*aGlobalPrintSettings);
>+
>diff -N -r -u original/mozilla/gfx/src/windows/nsPrintOptionsWin.h mozilla/gfx/src/windows/nsPrintOptionsWin.h
>--- original/mozilla/gfx/src/windows/nsPrintOptionsWin.h	Mon Mar 25 12:55:27 2002
>+++ mozilla/gfx/src/windows/nsPrintOptionsWin.h	Thu Sep  5 01:17:37 2002
>@@ -35,8 +35,13 @@
>   nsPrintOptionsWin();
>   virtual ~nsPrintOptionsWin();
> 
>+protected:
>   NS_IMETHOD CreatePrintSettings(nsIPrintSettings **_retval);
> 
>+  NS_IMETHOD GetGlobalPrintSettings(nsIPrintSettings * *aGlobalPrintSettings);
>+  NS_IMETHOD GetNewPrintSettings(nsIPrintSettings * *aNewPrintSettings);
>+  NS_IMETHOD InitPrintSettingsFromPrinter(const PRUnichar *aPrinterName, nsIPrintSettings *aPrintSettings);
>+
> };
> 
> 

Why are you making these methods, which are part of the nsIPrintSettingsService
interface, protected?

Comment 38

[Pete Zha]

Attachment: patch with bryner's comments

This patch include changes from bryner's comments. Only following problem I
can't reslove.

>Why are you making these methods, which are part of the
nsIPrintSettingsService
>interface, protected?
This question need rods to answer. I just moved out the "protected".

Comment 39

[Scott Davis]

This problem is more than just Linux - I see it on Sun/Solaris as well.

Comment 40

[rods (gone)]

Comment on attachment 98312 [details] [diff] [review]
patch with bryner's comments

I don't have a good reason for making them protected, so they are fine as
public. r=rods

Comment 41

[Brian Ryner (not reading)]

Comment on attachment 98312 [details] [diff] [review]
patch with bryner's comments

sr=bryner

Comment 42

[Pete Zha]

request for OEM branch

Comment 43

[Asa Dotzler [:asa]]

Comment on attachment 98312 [details] [diff] [review]
patch with bryner's comments

a=asa (on behalf of drivers) for checkin to 1.2a (the trunk). Time is short. If
this is going to make 1.2a then it needs to land quickly.

Comment 44

[Pete Zha]

checked in trunk

Comment 45

[Pete Zha]

checked in NETSCAPE_7_0_OEM_BRANCH(a=[e-mail])

Comment 46

[Pete Zha]

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

Comment 47

[Roland Mainz]

This patch caused bug 167894 ("InitPrintSettingsFromPrinter() not called for
Print Preview") which rendered print preview unuseable in some cases... ;-(

Comment 48

[Roland Mainz]

Nominating for 1.0.2-branch...

Comment 49

[Roland Mainz]

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

Comment 50

[Roland Mainz]

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

Comment 51

[Roland Mainz]

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

Comment 52

[R.K.Aa.]

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